Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fingerprint out of root targets #15205

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fingerprint out of root targets #15205

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions src/cargo/sources/path.rs
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for helping with this issue!

As you've said, this is hacky. Some high-level feedback:

  • PathSource::list_file (which then calls _list_files) is used in computing fingerprints for build scripts and cargo doc, as well as determining what is going to be packaged into cargo package. During packaging, any file outside the package root will not be packed into the resulting .crate file. Cargo even emits a warning when the situation is detected. With this change, cargo-package starts including those files again. This is undesired, not even mention that where should their path be in the .crate tarball.
  • How do this hack know what really needs to track? It simply looks at the directory the target source file is in and assume it always at the root directory with all other relevant files. Yeah it cannot handle #[path] we already knew, but it also doesn't respect git directory and include/exclude options. It may accidentally include more files they necessary.

To me I still think depinfo from rustdoc is a more sustainable approach. We might want to put more effort on that route than sorting out a hack.

Some other alternatives are on user side, like breaking them into sub packages for reasonable reuse.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It is linked in #12266 (comment), which also calls out a relevant Cargo issue #9931.

Original file line number Diff line number Diff line change
Expand Up @@ -622,15 +622,29 @@ fn _list_files(pkg: &Package, gctx: &GlobalContext) -> CargoResult<Vec<PathEntry

ignore_should_package(relative_path, is_dir)
};

// Attempt Git-prepopulate only if no `include` (see rust-lang/cargo#4135).
if no_include_option {
if let Some(repo) = git_repo {
return list_files_gix(pkg, &repo, &filter, gctx);
let mut ret = match git_repo {
Some(repo) if no_include_option => {
// Attempt Git-prepopulate only if no `include` (see rust-lang/cargo#4135).
list_files_gix(pkg, &repo, &filter, gctx)?
}
_ => {
let mut ret = Vec::new();
list_files_walk(pkg.root(), &mut ret, true, &filter, gctx)?;
ret
}
};
for target in pkg.manifest().targets() {
let Some(path) = target.src_path().path() else {
continue;
};
let path = path.canonicalize()?;
if !path.starts_with(pkg.root()) {
// The target "root" is not included in the package root
// Unwrapping is ok because we know path is to a file
let target_root = path.parent().unwrap();
list_files_walk(target_root, &mut ret, false, &(|_, _| true), gctx)?;
}
}
let mut ret = Vec::new();
list_files_walk(pkg.root(), &mut ret, true, &filter, gctx)?;
Ok(ret)
}

Expand Down
Loading