From a991a7dd9847ae31c9f7bce97de9411579254453 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Sun, 22 Dec 2024 03:39:02 -0500 Subject: [PATCH 1/2] test(package): show corner cases of vcs dirtiness check This is a test showing corner cases that dirty files outside the package being packaging actually made the `.crate` file dirty. However, `cargo package` and `.cargo_vcs_info.json` didn't capture it. --- tests/testsuite/package.rs | 112 +++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index e349f744ce8..405cd8bbeca 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -1308,6 +1308,118 @@ to proceed despite this and include the uncommitted changes, pass the `--allow-d ); } +#[cargo_test] +fn dirty_file_outside_pkg_root_considered_dirty() { + if !symlink_supported() { + return; + } + let main_outside_pkg_root = paths::root().join("main.rs"); + let (p, repo) = git::new_repo("foo", |p| { + p.file( + "Cargo.toml", + r#" + [workspace] + members = ["isengard"] + resolver = "2" + [workspace.package] + edition = "2015" + "#, + ) + .file("lib.rs", r#"compile_error!("you shall not pass")"#) + .file("LICENSE", "before") + .file("README.md", "before") + .file( + "isengard/Cargo.toml", + r#" + [package] + name = "isengard" + edition.workspace = true + homepage = "saruman" + description = "saruman" + license-file = "../LICENSE" + "#, + ) + .symlink("lib.rs", "isengard/src/lib.rs") + .symlink("README.md", "isengard/README.md") + .file(&main_outside_pkg_root, "fn main() {}") + .symlink(&main_outside_pkg_root, "isengard/src/main.rs") + }); + git::commit(&repo); + + // Changing files outside pkg root under situations below should be treated + // as dirty. `cargo package` is expected to fail on VCS stastus check. + // + // * Changes in files outside package root that source files symlink to + p.change_file("README.md", "after"); + p.change_file("lib.rs", "pub fn after() {}"); + // * Changes in files outside pkg root that `license-file`/`readme` point to + p.change_file("LICENSE", "after"); + // * When workspace inheritance is involved and changed + p.change_file( + "Cargo.toml", + r#" + [workspace] + members = ["isengard"] + resolver = "2" + [workspace.package] + edition = "2021" + "#, + ); + // Changes in files outside git workdir won't affect vcs status check + p.change_file( + &main_outside_pkg_root, + r#"fn main() { eprintln!("after"); }"#, + ); + + // Ensure dirty files be reported. + p.cargo("package --workspace --no-verify") + .with_stderr_data(str![[r#" +[PACKAGING] isengard v0.0.0 ([ROOT]/foo/isengard) +[PACKAGED] 8 files, [FILE_SIZE]B ([FILE_SIZE]B compressed) + +"#]]) + .run(); + + p.cargo("package --workspace --no-verify --allow-dirty") + .with_stderr_data(str![[r#" +[PACKAGING] isengard v0.0.0 ([ROOT]/foo/isengard) +[PACKAGED] 8 files, [FILE_SIZE]B ([FILE_SIZE]B compressed) + +"#]]) + .run(); + + let cargo_toml = str![[r##" +... +[package] +edition = "2021" +... + +"##]]; + + let f = File::open(&p.root().join("target/package/isengard-0.0.0.crate")).unwrap(); + validate_crate_contents( + f, + "isengard-0.0.0.crate", + &[ + ".cargo_vcs_info.json", + "Cargo.toml", + "Cargo.toml.orig", + "src/lib.rs", + "src/main.rs", + "Cargo.lock", + "LICENSE", + "README.md", + ], + [ + ("src/lib.rs", str!["pub fn after() {}"]), + ("src/main.rs", str![r#"fn main() { eprintln!("after"); }"#]), + ("README.md", str!["after"]), + ("LICENSE", str!["after"]), + ("Cargo.toml", cargo_toml), + ], + ); +} + #[cargo_test] fn issue_13695_allow_dirty_vcs_info() { let p = project() From 863e1f40deebd27fbfac3d292327bd066905a297 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Sun, 22 Dec 2024 03:31:11 -0500 Subject: [PATCH 2/2] fix(package): check dirtiness of path fields in manifest This adds a special case for `package.{readme,license-file}` to Git VCS status check. If they were specified with paths outside the current package root, but still under git workdir, Cargo checks git status of those files to determine if they were dirty. We don't need to take care of other fields with path values because * `PathSource` only list files under the package root. Things like `target.path` works for `cargo build`, but won't be included in `.crate` file from `cargo publish`. * The only exceptions are `package.readme`/`package.license-file`. Cargo would copy files over if they are outside package root. --- src/cargo/ops/cargo_package.rs | 39 +++++++++++++++++++++++++++++++++- tests/testsuite/package.rs | 9 ++++++-- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index b54a16d2c9b..9c0b1d6ac43 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -796,7 +796,7 @@ fn check_repo_state( .and_then(|p| p.to_str()) .unwrap_or("") .replace("\\", "/"); - let Some(git) = git(gctx, src_files, &repo, &opts)? else { + let Some(git) = git(p, gctx, src_files, &repo, &opts)? else { // If the git repo lacks essensial field like `sha1`, and since this field exists from the beginning, // then don't generate the corresponding file in order to maintain consistency with past behavior. return Ok(None); @@ -805,6 +805,7 @@ fn check_repo_state( return Ok(Some(VcsInfo { git, path_in_vcs })); fn git( + pkg: &Package, gctx: &GlobalContext, src_files: &[PathBuf], repo: &git2::Repository, @@ -828,6 +829,7 @@ fn check_repo_state( let mut dirty_src_files: Vec<_> = src_files .iter() .filter(|src_file| dirty_files.iter().any(|path| src_file.starts_with(path))) + .chain(dirty_metadata_paths(pkg, repo)?.iter()) .map(|path| { pathdiff::diff_paths(path, cwd) .as_ref() @@ -860,6 +862,41 @@ fn check_repo_state( } } + /// Checks whether files at paths specified in `package.readme` and + /// `package.license-file` have been modified. + /// + /// This is required because those paths may link to a file outside the + /// current package root, but still under the git workdir, affecting the + /// final packaged `.crate` file. + fn dirty_metadata_paths(pkg: &Package, repo: &git2::Repository) -> CargoResult> { + let mut dirty_files = Vec::new(); + let workdir = repo.workdir().unwrap(); + let root = pkg.root(); + let meta = pkg.manifest().metadata(); + for path in [&meta.license_file, &meta.readme] { + let Some(path) = path.as_deref().map(Path::new) else { + continue; + }; + let abs_path = paths::normalize_path(&root.join(path)); + if paths::strip_prefix_canonical(abs_path.as_path(), root).is_ok() { + // Inside package root. Don't bother checking git status. + continue; + } + if let Ok(rel_path) = paths::strip_prefix_canonical(abs_path.as_path(), workdir) { + // Outside package root but under git workdir, + if repo.status_file(&rel_path)? != git2::Status::CURRENT { + dirty_files.push(if abs_path.is_symlink() { + // For symlinks, shows paths to symlink sources + workdir.join(rel_path) + } else { + abs_path + }); + } + } + } + Ok(dirty_files) + } + // Helper to collect dirty statuses for a single repo. fn collect_statuses( repo: &git2::Repository, diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 405cd8bbeca..b418513eace 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -1373,9 +1373,14 @@ fn dirty_file_outside_pkg_root_considered_dirty() { // Ensure dirty files be reported. p.cargo("package --workspace --no-verify") + .with_status(101) .with_stderr_data(str![[r#" -[PACKAGING] isengard v0.0.0 ([ROOT]/foo/isengard) -[PACKAGED] 8 files, [FILE_SIZE]B ([FILE_SIZE]B compressed) +[ERROR] 2 files in the working directory contain changes that were not yet committed into git: + +LICENSE +README.md + +to proceed despite this and include the uncommitted changes, pass the `--allow-dirty` flag "#]]) .run();