fix(package): report lockfile / workspace manifest is dirty #15276

weihanglo · 2025-03-07T04:43:59Z

What does this PR try to resolve?

Workspace manifest and lockfile might not be under the package root,
but workspace inheritance and outdated lockfile entries
may still affect what is packaged into the .crate. tarball.
We take a conservative action that if any of them is dirty,
then all workspace members are considered dirty.

Fixes #14967

How should we test and review this PR?

Workspace manifest case is simple.

Lockfile requires a bit more works.
It is allowed to be missing, and can be generated during packaging.
We skip checking when it is missing in both workdir and git index,
otherwise cargo will fail with git2 not found error.

Workspace manifest might not be under the package root, but workspace inheritance may still affect what is packaged into the `.crate.` tarball. We take a conservative action that if the workspace manifest is dirty, then all workspace members are considered dirty.

rustbot · 2025-03-07T04:44:03Z

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

weihanglo · 2025-03-07T05:34:30Z

tests/testsuite/package.rs

+    // Now check if it is considered dirty when removed.
+    p.cargo("clean").run();
+    fs::remove_file(p.root().join("Cargo.lock")).unwrap();
+    p.cargo("package --workspace --no-verify")


Might be some path normalization issues on Windows.

thread 'package::dirty_ws_lockfile_dirty' panicked at tests\testsuite\package.rs:1553:10: test failed running `D:\a\cargo\cargo\target\debug\cargo.exe package --workspace --no-verify` error: process exited with code 0 (expected 101) --- stdout --- stderr Packaging isengard v0.0.0 (D:\a\cargo\cargo\target\tmp\cit\t2239\foo\isengard) Updating `dummy-registry` index Packaged 5 files, 1.5KiB (986B compressed)

Fixed by https://github.com/rust-lang/cargo/compare/4c0e42dac2b05d52b52ef2435a9a25d73dd92c5e..f0907fca16a84101d69fc53272a955e8bb53d9fe.

It was failing because removed files can't be canonicalized, so when comparing path prefix with a canonicalized workdir it will never pass.

Lockfile might not be under the package root, but its entries may be outdated and get packaged into the `.crate` tarball. We take a conservative action that if the lockfile is dirty, then all workspace members are considered dirty.

epage · 2025-03-07T14:32:13Z

src/cargo/ops/cargo_package/vcs.rs

    for rel_path in src_files
        .iter()
        .filter(|p| p.is_symlink_or_under_symlink())
        .map(|p| p.as_ref().as_path())
        .chain(metadata_paths.iter().map(AsRef::as_ref))
        .chain([ws.root_manifest()])
+        .chain(lockfile_path.as_deref().into_iter())


I'm trying to think through how this PR might be disruptive to people's workflows and the most likely case I can come ip with is people being sloppy with version bumps and publishing which can run into #5979.

Update cargo 22 commits in 2622e844bc1e2e6123e54e94e4706f7b6195ce3d..ab1463d632528e39daf35f263e10c14cbe590ce8 2025-02-28 12:33:57 +0000 to 2025-03-08 01:45:05 +0000 - test: redact host target when comparing CARGO_ENV path (rust-lang/cargo#15279) - feat: add completions for install --path (rust-lang/cargo#15266) - fix(package): report lockfile / workspace manifest is dirty (rust-lang/cargo#15276) - feat(tree): Add `--depth public` behind `-Zunstable-options` (rust-lang/cargo#15243) - Don't use `$CARGO_BUILD_TARGET` in `cargo metadata` (rust-lang/cargo#15271) - feat: show extra build description from bootstrap (rust-lang/cargo#15269) - Upgrade to `rustc-stable-hash v0.1.2` (rust-lang/cargo#15268) - fix: Respect --frozen everywhere --offline or --locked is accepted (rust-lang/cargo#15263) - feat(tree): Color the output (rust-lang/cargo#15242) - fix(vendor): dont remove non-cached source (rust-lang/cargo#15260) - docs: lockfile is always included since 1.84 (rust-lang/cargo#15257) - Remove `Cargo.toml` from `package.include` in example (rust-lang/cargo#15253) - Small cleanup: remove unneeded result (rust-lang/cargo#15256) - Fix typo in build-scripts.md (rust-lang/cargo#15254) - chore(deps): update rust crate pulldown-cmark to 0.13.0 (rust-lang/cargo#15250) - chore(deps): update compatible (rust-lang/cargo#15249) - feat(cli): forward bash completions of third party subcommands (rust-lang/cargo#15247) - feat: add completions for `--lockfile-path` (rust-lang/cargo#15238) - fix: reset $CARGO if the running program is real `cargo[.exe]` (rust-lang/cargo#15208) - Get all members as `available targets` even though default-members was specified. (rust-lang/cargo#15199) - refactor: control byte display precision with std::fmt options (rust-lang/cargo#15246) - fix(package): Ensure we can package directories ending with '.rs' (rust-lang/cargo#15240)

### What does this PR try to resolve? Fixes #15339 Lockfile might be gitignore'd, So it never shows up in `git status`. However, `cargo packag` vcs checks actually performs `git status --untracked --ignored`. It is a bit confusing that lockfile is ignored but still counts as dirty from the report of `cargo package`. There are some more nuances: We check lockfile's VCS status if the lockfile is ouside the current package root. That means a non-workspace Cargo package will not have this VCS check. I don't think it is good that we have diverged behaviors Hence this revert. We can always re-evaluate later, as we've reserved rooms for doing more dirty checks. #14967 (comment) ### How should we test and review this PR? See if the revert is what we want now. Or we should do a more thorough check for both workspace and non-workspace cases. ### Additional information This is a revert of f0907fc from #15276

# Rationale for this change Semantic release updates `Cargo.toml` during the release process. Our release process publish script has failed since we upgraded to Rust version 1.87. There does not appear to be any other indication of people experiencing this online, but we suspect adding the `--allow-dirty` flag to the `cargo publish` call will allow us to work around the issue. Re-running the release script in Github's Actions UI has succeeded in 3 of the 4 failing jobs. One job has not succeeded: https://github.com/spaceandtimefdn/sxt-proof-of-sql/actions/runs/15476798627/job/43576240647. This issue is likely related to rust-lang/cargo#15276. # What changes are included in this PR? - `--allow-dirty` is added to the publish script # Are these changes tested? No, I can only think to run the release process to test this?

weihanglo added 5 commits March 6, 2025 23:18

refactor(package): rename variable

af1e77c

refactor(package): move path_in_vcs closer to where it is used

4244b64

refactor(package): use format_arg! to avoid allocations

2375b19

test(package): dirty lockfile isn't considered dirty but should

d9908b1

rustbot assigned ehuss Mar 7, 2025

rustbot added A-git Area: anything dealing with git Command-package S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 7, 2025

weihanglo mentioned this pull request Mar 7, 2025

cargo package VCS dirty check corner cases #14967

Open

4 tasks

weihanglo marked this pull request as draft March 7, 2025 05:33

weihanglo commented Mar 7, 2025

View reviewed changes

fix(package): report if the lockfile is dirty

f0907fc

Lockfile might not be under the package root, but its entries may be outdated and get packaged into the `.crate` tarball. We take a conservative action that if the lockfile is dirty, then all workspace members are considered dirty.

weihanglo force-pushed the dirty-lock branch from 4c0e42d to f0907fc Compare March 7, 2025 13:45

weihanglo marked this pull request as ready for review March 7, 2025 13:48

epage reviewed Mar 7, 2025

View reviewed changes

epage approved these changes Mar 7, 2025

View reviewed changes

epage added this pull request to the merge queue Mar 7, 2025

Merged via the queue into rust-lang:master with commit a9e1a34 Mar 7, 2025
21 checks passed

weihanglo deleted the dirty-lock branch March 7, 2025 15:08

weihanglo mentioned this pull request Mar 8, 2025

Update cargo rust-lang/rust#138200

Merged

rustbot added this to the 1.87.0 milestone Mar 10, 2025

DaniPopes mentioned this pull request Mar 21, 2025

Dirty Cargo.lock file reported on package/publish even if in .gitignore #15339

Closed

epage mentioned this pull request Mar 24, 2025

fix: revert the behavior checking lockfile's VCS status #15341

Merged

jacobtrombetta mentioned this pull request Jun 5, 2025

ci: add allow dirty flag to crate publishing script spaceandtimefdn/sxt-proof-of-sql#883

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(package): report lockfile / workspace manifest is dirty #15276

fix(package): report lockfile / workspace manifest is dirty #15276

Uh oh!

weihanglo commented Mar 7, 2025

Uh oh!

rustbot commented Mar 7, 2025

Uh oh!

weihanglo Mar 7, 2025

Uh oh!

weihanglo Mar 7, 2025

Uh oh!

epage Mar 7, 2025

Uh oh!

Uh oh!

Uh oh!

fix(package): report lockfile / workspace manifest is dirty #15276

fix(package): report lockfile / workspace manifest is dirty #15276

Uh oh!

Conversation

weihanglo commented Mar 7, 2025

What does this PR try to resolve?

How should we test and review this PR?

Uh oh!

rustbot commented Mar 7, 2025

Uh oh!

weihanglo Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

weihanglo Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

epage Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!