Skip to content

Commit

Permalink
fix(package): check dirtiness of path fields in manifest
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
weihanglo committed Dec 24, 2024
1 parent a991a7d commit 863e1f4
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 3 deletions.
39 changes: 38 additions & 1 deletion src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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,
Expand All @@ -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()
Expand Down Expand Up @@ -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<Vec<PathBuf>> {
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,
Expand Down
9 changes: 7 additions & 2 deletions tests/testsuite/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit 863e1f4

Please sign in to comment.