From cc3b6cb5c50c5811468177d8ea87060ae1bacacc Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Thu, 19 Dec 2024 12:25:14 -0500 Subject: [PATCH 1/3] fix(package): sort dirty file report To make error message more deterministic --- src/cargo/ops/cargo_package.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index bc619c1b86a..2c7b6eb44cf 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -824,7 +824,7 @@ fn check_repo_state( // Find the intersection of dirty in git, and the src_files that would // be packaged. This is a lazy n^2 check, but seems fine with // thousands of files. - let dirty_src_files: Vec = src_files + let mut dirty_src_files: Vec<_> = src_files .iter() .filter(|src_file| dirty_files.iter().any(|path| src_file.starts_with(path))) .map(|path| { @@ -847,6 +847,7 @@ fn check_repo_state( dirty, })) } else { + dirty_src_files.sort_unstable(); anyhow::bail!( "{} files in the working directory contain changes that were \ not yet committed into git:\n\n{}\n\n\ From ecc8e37f401d87d62ce8b1d688ac8b9841aeed6d Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Thu, 19 Dec 2024 10:40:46 -0500 Subject: [PATCH 2/3] test(package): track vcs status on workspace member granularity --- tests/testsuite/package.rs | 138 +++++++++++++++++++++++++++++++++++++ 1 file changed, 138 insertions(+) diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 5a27cc34920..d505ae3d6dc 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -1156,6 +1156,144 @@ src/lib.rs .run(); } +#[cargo_test] +fn vcs_status_check_for_each_workspace_member() { + // Cargo checks VCS status separately for each workspace member. + // This ensure one file changed in a package won't affect the other. + // Since the dirty bit in .cargo_vcs_info.json is just for advisory purpose, + // We may change the meaning of it in the future. + let (p, repo) = git::new_repo("foo", |p| { + p.file( + "Cargo.toml", + r#" + [workspace] + members = ["isengard", "mordor"] + "#, + ) + .file("hobbit", "...") + .file( + "isengard/Cargo.toml", + r#" + [package] + name = "isengard" + edition = "2015" + homepage = "saruman" + description = "saruman" + license = "MIT" + "#, + ) + .file("isengard/src/lib.rs", "") + .file( + "mordor/Cargo.toml", + r#" + [package] + name = "mordor" + edition = "2015" + homepage = "sauron" + description = "sauron" + license = "MIT" + "#, + ) + .file("mordor/src/lib.rs", "") + }); + git::commit(&repo); + + p.change_file( + "Cargo.toml", + r#" + [workspace] + members = ["isengard", "mordor"] + [workspace.package] + edition = "2021" + "#, + ); + // Dirty file outside won't affect packaging. + p.change_file("hobbit", "changed!"); + p.change_file("mordor/src/lib.rs", "changed!"); + p.change_file("mordor/src/main.rs", "fn main() {}"); + + // Ensure dirty files be reported only for one affected package. + p.cargo("package --workspace --no-verify") + .with_status(101) + .with_stderr_data(str![[r#" +[PACKAGING] isengard v0.0.0 ([ROOT]/foo/isengard) +[PACKAGED] 5 files, [FILE_SIZE]B ([FILE_SIZE]B compressed) +[ERROR] 2 files in the working directory contain changes that were not yet committed into git: + +src/lib.rs +src/main.rs + +to proceed despite this and include the uncommitted changes, pass the `--allow-dirty` flag + +"#]]) + .run(); + + // Ensure only dirty package be recorded as dirty. + p.cargo("package --workspace --no-verify --allow-dirty") + .with_stderr_data(str![[r#" +[PACKAGING] isengard v0.0.0 ([ROOT]/foo/isengard) +[PACKAGED] 5 files, [FILE_SIZE]B ([FILE_SIZE]B compressed) +[PACKAGING] mordor v0.0.0 ([ROOT]/foo/mordor) +[PACKAGED] 6 files, [FILE_SIZE]B ([FILE_SIZE]B compressed) + +"#]]) + .run(); + + 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", + "Cargo.lock", + ], + [( + ".cargo_vcs_info.json", + // No change within `isengard/`, so not dirty at all. + str![[r#" +{ + "git": { + "sha1": "[..]" + }, + "path_in_vcs": "isengard" +} +"#]] + .is_json(), + )], + ); + + let f = File::open(&p.root().join("target/package/mordor-0.0.0.crate")).unwrap(); + validate_crate_contents( + f, + "mordor-0.0.0.crate", + &[ + ".cargo_vcs_info.json", + "Cargo.toml", + "Cargo.toml.orig", + "src/lib.rs", + "src/main.rs", + "Cargo.lock", + ], + [( + ".cargo_vcs_info.json", + // Dirty bit is recorded. + str![[r#" +{ + "git": { + "dirty": true, + "sha1": "[..]" + }, + "path_in_vcs": "mordor" +} +"#]] + .is_json(), + )], + ); +} + #[cargo_test] fn issue_13695_allow_dirty_vcs_info() { let p = project() From 982eb2de3a445cb7c2c5ca8e4b24f55b875df216 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Thu, 19 Dec 2024 11:13:00 -0500 Subject: [PATCH 3/3] fix(package): show dirty filepaths relative to git workdir --- src/cargo/ops/cargo_package.rs | 6 +++--- tests/testsuite/package.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 2c7b6eb44cf..b12a72db579 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(p, src_files, &repo, &opts)? else { + let Some(git) = git(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,7 +805,6 @@ fn check_repo_state( return Ok(Some(VcsInfo { git, path_in_vcs })); fn git( - pkg: &Package, src_files: &[PathBuf], repo: &git2::Repository, opts: &PackageOpts<'_>, @@ -824,11 +823,12 @@ fn check_repo_state( // Find the intersection of dirty in git, and the src_files that would // be packaged. This is a lazy n^2 check, but seems fine with // thousands of files. + let workdir = repo.workdir().unwrap(); let mut dirty_src_files: Vec<_> = src_files .iter() .filter(|src_file| dirty_files.iter().any(|path| src_file.starts_with(path))) .map(|path| { - path.strip_prefix(pkg.root()) + path.strip_prefix(workdir) .unwrap_or(path) .display() .to_string() diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index d505ae3d6dc..75d21ffb2ce 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -1220,8 +1220,8 @@ fn vcs_status_check_for_each_workspace_member() { [PACKAGED] 5 files, [FILE_SIZE]B ([FILE_SIZE]B compressed) [ERROR] 2 files in the working directory contain changes that were not yet committed into git: -src/lib.rs -src/main.rs +mordor/src/lib.rs +mordor/src/main.rs to proceed despite this and include the uncommitted changes, pass the `--allow-dirty` flag