Skip to content

fix(sdist): regenerate Cargo.lock when workspace members are removed - #3192

Open
ckhordiasma wants to merge 10 commits into
PyO3:mainfrom
ckhordiasma:fix/sdist-cargolock
Open

ckhordiasma wants to merge 10 commits into
PyO3:mainfrom
ckhordiasma:fix/sdist-cargolock

Conversation

@ckhordiasma

@ckhordiasma ckhordiasma commented May 19, 2026

Copy link
Copy Markdown

Summary

Fixes #2609.

When maturin builds an sdist from a workspace, rewrite_cargo_toml strips workspace members
not needed by the package. The Cargo.lock was copied verbatim and still referenced those
removed crates, so cargo build --locked inside the sdist would fail.

After all sdist entries are assembled in the virtual writer, materialize them to a temp
directory, run cargo update --workspace to reconcile the lockfile, and replace the tracker
entry with the regenerated Cargo.lock.

Changes

  • Add materialize_to and replace_bytes helpers on VirtualWriter<SDistWriter>
  • Add regenerate_cargo_lock step after add_workspace_manifest in the sdist builder
  • Use cargo update --workspace instead of cargo generate-lockfile to preserve existing dependency pins
  • Only trigger lockfile regeneration when workspace members are actually stripped (not for any multi-member workspace)
  • Propagate --offline flag to the cargo subprocess
  • Degrade gracefully when cargo update --workspace fails (e.g. missing .cargo/config.toml for private registries in the temp dir) — warn and keep the original lockfile instead of failing the build
  • Use --manifest-path explicitly instead of relying on current_dir + cargo's upward search

Review feedback addressed

Per review from @messense:

  1. cargo generate-lockfile re-resolves to latest → replaced with cargo update --workspace which preserves existing pins. Test pins itoa to 1.0.1 with --precise and verifies it survives.
  2. Trigger condition too coarse → now compares original workspace member count against kept members (main crate + same-workspace path deps). Test uses a [patch.crates-io] with a relative path that breaks in the temp dir, proving unnecessary regeneration is skipped.
  3. Offline / cargo config → propagates --offline; degrades to warning + original lockfile on failure. Test uses a [patch] relative path to trigger failure and verifies the build succeeds via graceful degradation.
  4. Smaller points--manifest-path used explicitly; error message spacing fixed; test asserts devtool/rand are absent and pinned versions are preserved.

Test plan

  • cargo test sdist_workspace_removed_members_cargo_lock — stripped members are pruned, pins preserved, --frozen succeeds
  • cargo test sdist_workspace_all_members_kept_skips_regeneration — no unnecessary regeneration when all members are path deps
  • cargo test sdist_workspace_lockfile_regen_graceful_degradation — build succeeds with warning when cargo update fails
  • All sdist tests pass
  • cargo fmt clean

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes sdists produced from Cargo workspaces where rewrite_cargo_toml removes unneeded workspace members but the copied Cargo.lock still references them, causing cargo build --locked / cargo metadata --frozen to fail inside the unpacked sdist.

Changes:

  • Add VirtualWriter<SDistWriter>::materialize_to and replace_bytes helpers to support post-processing tracked sdist entries.
  • After assembling workspace-related sdist entries, materialize the sdist to a temp dir, run cargo generate-lockfile, and replace the sdist’s Cargo.lock with the regenerated version.
  • Add a regression test that constructs a two-member workspace, builds an sdist for one member, unpacks it, and asserts cargo metadata --frozen succeeds.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
tests/run/sdist.rs Adds regression coverage ensuring workspace-member sdists have a usable Cargo.lock.
src/source_distribution/mod.rs Adds regenerate_cargo_lock step in the sdist build pipeline to reconcile lockfiles after workspace-member stripping.
src/module_writer/virtual_writer.rs Adds helpers to materialize tracked sdist entries to disk and replace a tracked file’s bytes in-memory.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/source_distribution/mod.rs Outdated
Comment thread src/module_writer/virtual_writer.rs
Comment thread src/module_writer/virtual_writer.rs
Comment thread src/module_writer/virtual_writer.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

Comment thread src/module_writer/virtual_writer.rs
Comment thread src/source_distribution/mod.rs
Comment thread tests/run/sdist.rs Outdated
@messense

Copy link
Copy Markdown
Member

Thanks for the PR — the problem is real and reconciling the lockfile against the rewritten manifests is the right general approach. A few things need to change before this can merge though:

1. cargo generate-lockfile re-resolves everything to latest

Per the cargo docs, if the lockfile already exists it "will be rebuilt with the latest available version of every package" — it's effectively a full cargo update. So this doesn't just prune the stale entries: every third-party pin in the sdist's Cargo.lock silently floats to whatever is newest at sdist-build time. That breaks the reproducibility guarantee shipping a lockfile is meant to provide, contradicts a user-supplied --locked, and makes sdists non-reproducible day to day.

Please use cargo update --workspace instead — it conservatively preserves existing pins while re-resolving the graph, which drops the unreachable entries. (Running plain cargo metadata without --frozen for its minimal lockfile sync would also work.)

2. Trigger condition is too coarse

workspace_members.len() > 1 fires even when no member is actually stripped (all siblings are path deps that stay in the sdist). Combined with point 1, that rewrites previously-correct lockfiles. The rewrite code in cargo_toml_rewrite.rs already knows which members were dropped — plumbing that through (or checking whether the lockfile references packages absent from the sdist) would make this a no-op in the common case.

3. Offline / cargo config

This adds a hard network dependency to sdist builds for multi-member workspaces. At minimum, propagate ctx.project.cargo_options.offline as --offline. Also note the temp dir has no .cargo/config.toml, so source replacement / private registries won't be visible to the regeneration run — worth a comment, and possibly degrading to a warning + original lockfile on failure rather than failing the whole build.

Smaller points

  • Pass --manifest-path explicitly instead of relying on current_dir + cargo's upward search (matches cargo_package_file_list in the same file).
  • Missing space in the error message: `{}`(exit status: .
  • Test: please add an assertion that a surviving pinned version is preserved (e.g. cargo update --precise something in the original lockfile, then compare) — that's the test that distinguishes "pruned" from "re-resolved", and it would have caught point 1. Asserting devtool/rand are absent from the regenerated lockfile would also be more direct than relying only on --frozen's exit status.

The VirtualWriter helpers themselves look good — replace_bytes preserving entry metadata and materialize_to reusing default_permission are both right.

@ckhordiasma
ckhordiasma force-pushed the fix/sdist-cargolock branch 2 times, most recently from c257afa to 6cbf023 Compare August 18, 2026 18:36
@ckhordiasma

Copy link
Copy Markdown
Author

@messense thanks for the review comments, I believe I have addressed them in the most recent commits.

(I'm seeing a CI failure that doesn't appear to be related to changes in this PR, maybe it's a flake? - not sure how to rerun checks)

ckhordiasma and others added 10 commits September 16, 2026 15:20
Fixes PyO3#2609.

When maturin builds an sdist from a workspace, `rewrite_cargo_toml`
strips workspace members not needed by the package. The Cargo.lock was
copied verbatim and still referenced those removed crates, so
`cargo build --locked` inside the sdist would fail.

After all sdist entries are assembled, materialize them to a temp
directory, run `cargo generate-lockfile` to reconcile the lockfile,
and replace the tracker entry with the regenerated Cargo.lock.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Exercises the fix for PyO3#2609: builds an sdist from a workspace member
while a sibling member (`devtool`, with a unique `rand` dependency)
is stripped, then asserts `cargo metadata --frozen` succeeds in the
unpacked sdist.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use `workspace_members.len() > 1` instead of path-inequality heuristic
  so workspace-root packages are also covered.
- Make `replace_bytes` return `Result` and error if the target entry
  does not exist in the tracker.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Set file permissions based on the tracked executable flag after
writing each entry, matching the existing default_permission pattern.
Unix-only via #[cfg(unix)], consistent with the rest of the codebase.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Preserve original executable flag and source path when replacing
  a tracker entry in replace_bytes.
- Include stdout and working directory in cargo generate-lockfile
  error messages.
- Use output() instead of status() in test for better failure
  diagnostics.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
`cargo generate-lockfile` re-resolves all dependencies to their latest
versions, which silently floats third-party pins and breaks the
reproducibility guarantee that shipping a Cargo.lock is meant to
provide. Replace with `cargo update --workspace` which conservatively
preserves existing pins while pruning entries for removed workspace
members.

The test now pins itoa to 1.0.1 with `--precise` and verifies the pin
survives in the sdist lockfile. It also asserts that devtool-only
packages (devtool, rand) are absent from the regenerated lockfile.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tripped

The previous condition `workspace_members.len() > 1` fired for any
multi-member workspace, even when all members are path dependencies
that remain in the sdist. This triggered unnecessary cargo invocations
that could fail when the materialized temp dir lacks `.cargo/config.toml`
or when [patch] sections reference relative paths outside the sdist.

Now the caller computes whether members were actually stripped by
comparing the original workspace member count against the members
kept in the sdist (main crate + path deps in the same workspace root),
and only invokes regeneration when the counts differ.

The test uses a [patch.crates-io] section with a relative path that
resolves in the original workspace but not in the materialized temp
dir, making the unnecessary regeneration observable as a hard failure.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ion failure

The temp dir used for Cargo.lock regeneration has no .cargo/config.toml,
so source replacement and private registries are invisible to the cargo
subprocess. When cargo update --workspace fails (e.g. a [patch] relative
path doesn't resolve, or a private registry is unreachable), degrade
gracefully by warning and keeping the original lockfile rather than
failing the entire sdist build.

Also propagate the --offline flag from cargo options to the cargo update
subprocess, so offline sdist builds behave consistently.

The test creates a workspace with stripped members and a [patch.crates-io]
section using a relative path that resolves in the original workspace but
not in the materialized temp dir, verifying that the build succeeds via
graceful degradation rather than failing.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ution

Adapt to upstream API change where build_source_distribution() now
returns BuiltWheel instead of a (PathBuf, _) tuple.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pass --manifest-path to `cargo update --workspace` instead of relying
on current_dir + cargo's upward search, matching the pattern used by
cargo_package_file_list in the same file.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sdist generated from workspace contains outdated Cargo.lock

3 participants