Moved manifest metadata tracking from fingerprint to dep info#14973
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
2165205 to
44931d7
Compare
readme, license, license-file, keywords, and categories to fingerprint6c04032 to
43c1913
Compare
| let metadata = pkg.manifest().metadata(); | ||
| for (key, value) in metadata.emitted_env_vars() { | ||
| cmd.env(key, value); | ||
| } |
There was a problem hiding this comment.
nit: could we move this to line 376 so env vars are added in a similar order as before?
It doesn't really matter much though. Just I am accustomed to the current order when debugging rustc invocations in cargo --verbose mode.
|
Huh! Didn't realize you pushed more updates. |
| cmd.env("CARGO_MANIFEST_DIR", pkg.root()) | ||
| .env("CARGO_MANIFEST_PATH", pkg.manifest_path()) | ||
| .env("CARGO_PKG_VERSION_MAJOR", &pkg.version().major.to_string()) | ||
| .env("CARGO_PKG_VERSION_MINOR", &pkg.version().minor.to_string()) | ||
| .env("CARGO_PKG_VERSION_PATCH", &pkg.version().patch.to_string()) | ||
| .env("CARGO_PKG_VERSION_PRE", pkg.version().pre.as_str()) | ||
| .env("CARGO_PKG_VERSION", &pkg.version().to_string()) | ||
| .env("CARGO_PKG_NAME", &*pkg.name()) |
There was a problem hiding this comment.
I would probably keep these untouched, and work on metatata for now.
They are either already tracked by other means (PackageId), or still under discussion (#8693).
There was a problem hiding this comment.
I mean. We can revisit them later, after we figure out manifest metdata part. Doesn't need to do everything in one PR :)
| match value { | ||
| Cow::Borrowed(value) => cmd.env(key, value), | ||
| Cow::Owned(ref value) => cmd.env(key, value.as_str()), | ||
| }; |
There was a problem hiding this comment.
nit:
| match value { | |
| Cow::Borrowed(value) => cmd.env(key, value), | |
| Cow::Owned(ref value) => cmd.env(key, value.as_str()), | |
| }; | |
| cmd.env(key, value.as_ref()); |
3c3901f to
6323304
Compare
This change moves the manifest metadata track to dep-info files with the goal of reduce unneeded rebuilds when metadata is changed as well fixing issues where builds are not retrigged due to metadata changes when they should (ie. rust-lang#14154)
6323304 to
3d7b154
Compare
|
@rustbot label -A-git |
weihanglo
left a comment
There was a problem hiding this comment.
Thank you for working on this with us!
Update cargo 6 commits in 652623b779c88fe44afede28bf7f1c9c07812511..c86f4b3a1b153218e6e50861214b0b4b4e695f23 2024-12-20 15:44:42 +0000 to 2024-12-24 17:49:48 +0000 - fix(package): check dirtiness of path fields in manifest (rust-lang/cargo#14966) - test: make path arguments more generic and flexible (rust-lang/cargo#14979) - Moved manifest metadata tracking from fingerprint to dep info (rust-lang/cargo#14973) - fix: assure possibly blocking non-files (like FIFOs) won't be picked up for publishing. (rust-lang/cargo#14977) - simplify SourceID Hash (rust-lang/cargo#14800) - upgrade `gix` to the latest release 0.69. (rust-lang/cargo#14975)
### What does this PR try to resolve? This is basically my PR feedback for #14973. ### How should we test and review this PR? ### Additional information
What does this PR try to resolve?
Fixes #14154
Added some of the missing package metadata to the fingerprint so that rebuilds are triggered correctly.Also updated the test to prevent a future regression.Moved the manifest metadata tracking to use dep info in favor of fingerprint so that rebuilds are only triggered if the metadata is actually used.