cargo package VCS dirty check corner cases

[weihanglo]

Problem

During the investigation of #14955 and #14962, we found more corner cases that cargo package doesn't consider dirty. You don't even need to pass --allow-dirty flag to explicitly allow it. .cargo_vcs_info.json shows no dirty at all. However, the actual packaged .crate file contains dirty files.

Let's consider this virtual workspace:

./
├── foo/
│   ├── src/
│   ├── Cargo.toml
│   └── README.md -> ../README.md
├── Cargo.toml
├── LICENSE
└── README.md

# foo/Cargo.toml
[package]
name = "foo"
edition.workspace = true
license-file = "../LICENSE" # link to parent LICENSE

# Cargo.toml
[workspace]
members = ["foo"]
[workspace.package]
edition = "2015"

There are at least three corner cases:

• Symlinks — if you change README.md at workspace root level, when packaging it would be included. However, since PathSource::list_files list symlink as ordinary file. When doing dirty check, the naive path prefix check is not able to find the symlink source file dirty.
  • Fixed by fix(package): check dirtiness of symlinks source files #14981
• package.license-file and package.readme — they are copied during cargo package. PathSource::list_files doesn't even report them because they are not under the directory of the package.
  • Fixed by fix(package): check dirtiness of path fields in manifest #14966
• Workspace inheritance — like changing workspace.package.edition to 2021 would actually make foo's Cargo.toml dirty, but the current dirty status check doesn't capture that.

Steps

See #14966

Possible Solution(s)

No response

Notes

No response

Version

cargo 1.85.0-nightly (769f622e1 2024-12-14)

9c17646

[epage]

Options

• We manually patch up each case
• We check if the whole repo is dirty, instead of only the package

I re-implement this check in cargo-release so I catch it before the release process, not just publish. It check if the workspace is clean (instead it should check if the whole repo is clean). I implemented it this way because (1) its simpler and (2) people can hook into cargo-release and do their own thing so I can't exhaustively know all the files.

If we check the whole repo, potential broken workflows

• An interactive user has a file edited for local development purposes that is unrelated to publishing
  • As its interactive, we have fewer guarantees.
• A programmatic user (script) manually releases multiple packages in a workspace (so works on stable) and has to make files dirty for the release process of some but not all and so they selectively pass in --allow-dirty
  • I would assume that if they need to edit one, they need to edit all so unsure how likely this is to happen

[weihanglo]

BTW the doc says this:

cargo/src/doc/src/commands/cargo-package.md

Lines 60 to 61 in 9c17646

	`dirty` indicates that the Git worktree was dirty when the package
	was built.

and this:

cargo/src/doc/src/commands/cargo-package.md

Line 94 in 9c17646

<dd class="option-desc">Allow working directories with uncommitted VCS changes to be packaged.</dd>

So it seems to look at the repository as a whole when it was designed, but #2781 made it check only files from PathSource::list_files.

#2063 has some discussion around this. Haven't read through all of them.

[epage]

Looking through that, it seems there was a larger problem being dealt with and there was some discussion of details but not too much on this point (except one person not wanting cargo to be opinionated).

[weihanglo]

Regarding potential solutions to fixing workspace inheritance, there are a couple of ideas

1. If workspace Cargo.toml is dirty, we consider the member's Cargo.toml dirty.
   • May have lots of false positives.
   • Easy to implement and maintain.
2. If workspace Cargo.toml is dirty, we check whether member's Cargot.toml has any inheritannce fields. If any, consider dirty.
   • Have less false positive. Irrelevant changes may still fail the dirtiness check.
   • May need to hardcode a list of inheritable fields. Easy to get out of sync.
3. Swap the workspace Cargo.toml from worktree version to index version. Do a member manifest normalization against it. And then compare two normalized manifests
   • More precise.
   • Due to the limitation of Cargo internals, we need to refactor a lot of code to make Cargo accept in-memory manifests and workspace loading. Perhaps something like refactor: factor out Config from core::features #13278 and Expose manifest.rs as a standalone crate #4614 (comment).
   • If the index version of workspace manifest is not compatible with the worktree workspace layout, for example missing some members, the manifest loading won't work.
4. Like the previous one, but do publish-normalization manifests for both worktree and index.
   • The most precise option.
   • Has the same drawback as the previous one.