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 (
|
rustbot
added
A-rebuild-detection
ranger-ross
force-pushed
the
fix-metadata-not-retriggering-builds
branch
from
2165205 to
44931d7
Compare
rustbot
added
A-build-execution
ranger-ross
changed the title
readme, license, license-file, keywords, and categories to fingerprint
ranger-ross
force-pushed
the
fix-metadata-not-retriggering-builds
branch
2 times, most recently
from
6c04032 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()); |
ranger-ross
force-pushed
the
fix-metadata-not-retriggering-builds
branch
2 times, most recently
from
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)
ranger-ross
force-pushed
the
fix-metadata-not-retriggering-builds
branch
from
6323304 to
3d7b154
Compare
|
@rustbot label -A-git |
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?
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.