Skip to content

Commit c86f4b3

Browse files
authored
fix(package): check dirtiness of path fields in manifest (#14966)
### What does this PR try to resolve? 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. ### How should we test and review this PR? While this doesn't fix ever case listed in #14967, it at least fixes one of them.
2 parents 4945803 + 863e1f4 commit c86f4b3

File tree

2 files changed

+155
-1
lines changed

2 files changed

+155
-1
lines changed

src/cargo/ops/cargo_package.rs

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -796,7 +796,7 @@ fn check_repo_state(
796796
.and_then(|p| p.to_str())
797797
.unwrap_or("")
798798
.replace("\\", "/");
799-
let Some(git) = git(gctx, src_files, &repo, &opts)? else {
799+
let Some(git) = git(p, gctx, src_files, &repo, &opts)? else {
800800
// If the git repo lacks essensial field like `sha1`, and since this field exists from the beginning,
801801
// then don't generate the corresponding file in order to maintain consistency with past behavior.
802802
return Ok(None);
@@ -805,6 +805,7 @@ fn check_repo_state(
805805
return Ok(Some(VcsInfo { git, path_in_vcs }));
806806

807807
fn git(
808+
pkg: &Package,
808809
gctx: &GlobalContext,
809810
src_files: &[PathBuf],
810811
repo: &git2::Repository,
@@ -828,6 +829,7 @@ fn check_repo_state(
828829
let mut dirty_src_files: Vec<_> = src_files
829830
.iter()
830831
.filter(|src_file| dirty_files.iter().any(|path| src_file.starts_with(path)))
832+
.chain(dirty_metadata_paths(pkg, repo)?.iter())
831833
.map(|path| {
832834
pathdiff::diff_paths(path, cwd)
833835
.as_ref()
@@ -860,6 +862,41 @@ fn check_repo_state(
860862
}
861863
}
862864

865+
/// Checks whether files at paths specified in `package.readme` and
866+
/// `package.license-file` have been modified.
867+
///
868+
/// This is required because those paths may link to a file outside the
869+
/// current package root, but still under the git workdir, affecting the
870+
/// final packaged `.crate` file.
871+
fn dirty_metadata_paths(pkg: &Package, repo: &git2::Repository) -> CargoResult<Vec<PathBuf>> {
872+
let mut dirty_files = Vec::new();
873+
let workdir = repo.workdir().unwrap();
874+
let root = pkg.root();
875+
let meta = pkg.manifest().metadata();
876+
for path in [&meta.license_file, &meta.readme] {
877+
let Some(path) = path.as_deref().map(Path::new) else {
878+
continue;
879+
};
880+
let abs_path = paths::normalize_path(&root.join(path));
881+
if paths::strip_prefix_canonical(abs_path.as_path(), root).is_ok() {
882+
// Inside package root. Don't bother checking git status.
883+
continue;
884+
}
885+
if let Ok(rel_path) = paths::strip_prefix_canonical(abs_path.as_path(), workdir) {
886+
// Outside package root but under git workdir,
887+
if repo.status_file(&rel_path)? != git2::Status::CURRENT {
888+
dirty_files.push(if abs_path.is_symlink() {
889+
// For symlinks, shows paths to symlink sources
890+
workdir.join(rel_path)
891+
} else {
892+
abs_path
893+
});
894+
}
895+
}
896+
}
897+
Ok(dirty_files)
898+
}
899+
863900
// Helper to collect dirty statuses for a single repo.
864901
fn collect_statuses(
865902
repo: &git2::Repository,

tests/testsuite/package.rs

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1308,6 +1308,123 @@ to proceed despite this and include the uncommitted changes, pass the `--allow-d
13081308
);
13091309
}
13101310

1311+
#[cargo_test]
1312+
fn dirty_file_outside_pkg_root_considered_dirty() {
1313+
if !symlink_supported() {
1314+
return;
1315+
}
1316+
let main_outside_pkg_root = paths::root().join("main.rs");
1317+
let (p, repo) = git::new_repo("foo", |p| {
1318+
p.file(
1319+
"Cargo.toml",
1320+
r#"
1321+
[workspace]
1322+
members = ["isengard"]
1323+
resolver = "2"
1324+
[workspace.package]
1325+
edition = "2015"
1326+
"#,
1327+
)
1328+
.file("lib.rs", r#"compile_error!("you shall not pass")"#)
1329+
.file("LICENSE", "before")
1330+
.file("README.md", "before")
1331+
.file(
1332+
"isengard/Cargo.toml",
1333+
r#"
1334+
[package]
1335+
name = "isengard"
1336+
edition.workspace = true
1337+
homepage = "saruman"
1338+
description = "saruman"
1339+
license-file = "../LICENSE"
1340+
"#,
1341+
)
1342+
.symlink("lib.rs", "isengard/src/lib.rs")
1343+
.symlink("README.md", "isengard/README.md")
1344+
.file(&main_outside_pkg_root, "fn main() {}")
1345+
.symlink(&main_outside_pkg_root, "isengard/src/main.rs")
1346+
});
1347+
git::commit(&repo);
1348+
1349+
// Changing files outside pkg root under situations below should be treated
1350+
// as dirty. `cargo package` is expected to fail on VCS stastus check.
1351+
//
1352+
// * Changes in files outside package root that source files symlink to
1353+
p.change_file("README.md", "after");
1354+
p.change_file("lib.rs", "pub fn after() {}");
1355+
// * Changes in files outside pkg root that `license-file`/`readme` point to
1356+
p.change_file("LICENSE", "after");
1357+
// * When workspace inheritance is involved and changed
1358+
p.change_file(
1359+
"Cargo.toml",
1360+
r#"
1361+
[workspace]
1362+
members = ["isengard"]
1363+
resolver = "2"
1364+
[workspace.package]
1365+
edition = "2021"
1366+
"#,
1367+
);
1368+
// Changes in files outside git workdir won't affect vcs status check
1369+
p.change_file(
1370+
&main_outside_pkg_root,
1371+
r#"fn main() { eprintln!("after"); }"#,
1372+
);
1373+
1374+
// Ensure dirty files be reported.
1375+
p.cargo("package --workspace --no-verify")
1376+
.with_status(101)
1377+
.with_stderr_data(str![[r#"
1378+
[ERROR] 2 files in the working directory contain changes that were not yet committed into git:
1379+
1380+
LICENSE
1381+
README.md
1382+
1383+
to proceed despite this and include the uncommitted changes, pass the `--allow-dirty` flag
1384+
1385+
"#]])
1386+
.run();
1387+
1388+
p.cargo("package --workspace --no-verify --allow-dirty")
1389+
.with_stderr_data(str![[r#"
1390+
[PACKAGING] isengard v0.0.0 ([ROOT]/foo/isengard)
1391+
[PACKAGED] 8 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
1392+
1393+
"#]])
1394+
.run();
1395+
1396+
let cargo_toml = str![[r##"
1397+
...
1398+
[package]
1399+
edition = "2021"
1400+
...
1401+
1402+
"##]];
1403+
1404+
let f = File::open(&p.root().join("target/package/isengard-0.0.0.crate")).unwrap();
1405+
validate_crate_contents(
1406+
f,
1407+
"isengard-0.0.0.crate",
1408+
&[
1409+
".cargo_vcs_info.json",
1410+
"Cargo.toml",
1411+
"Cargo.toml.orig",
1412+
"src/lib.rs",
1413+
"src/main.rs",
1414+
"Cargo.lock",
1415+
"LICENSE",
1416+
"README.md",
1417+
],
1418+
[
1419+
("src/lib.rs", str!["pub fn after() {}"]),
1420+
("src/main.rs", str![r#"fn main() { eprintln!("after"); }"#]),
1421+
("README.md", str!["after"]),
1422+
("LICENSE", str!["after"]),
1423+
("Cargo.toml", cargo_toml),
1424+
],
1425+
);
1426+
}
1427+
13111428
#[cargo_test]
13121429
fn issue_13695_allow_dirty_vcs_info() {
13131430
let p = project()

0 commit comments

Comments
 (0)