fix(fingerprint): Don't throwaway the cache on RUSTFLAGS changes #14830
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @weihanglo rustbot has assigned @weihanglo. Use |
rustbot
added
A-layout
epage
commented
Nov 16, 2024
|
epage
force-pushed
the
rustflags2
branch
2 times, most recently
from
f709c73 to
2af218d
Compare
|
FWIW, I think this should resolve an issue in the Hubris build system. Currently we bypass Cargo's dirtiness checks for building deps in certain circumstances (to get maximum reuse of objects despite small controlled changes to Haven't tested it yet though. |
epage
force-pushed
the
rustflags2
branch
2 times, most recently
from
26ced7e to
536ae6e
Compare
weihanglo
reviewed
Nov 25, 2024
| dep_c_extra_filename_hashes.hash(&mut c_extra_filename_hasher); | ||
| // Avoid trashing the caches on RUSTFLAGS changing via `c_extra_filename` | ||
| // | ||
| // Limited to `c_extra_filename` to help with reproducible build / PGO issues. |
There was a problem hiding this comment.
This should (probably) avoid the PGO problems, since the symbols won't change. However, this could still cause problems with reproducible builds if the rlib filename somehow leaks into the resulting binary (I don't have a reason to think it does, but it seems risky).
I believe this does because the filenames of object files show up as the codegen unit names in dwarf debug info (and probably PDB?). I suspect that everything else in the binary will be the same, but builds differing only in extra-filename are likely to still produce different binaries.
Have we checked if it doesn't affect PGO?
We would like to check if anything is leaking rlib filename in debug info files.
There was a problem hiding this comment.
Any idea how to go about that? I have no experience with PGO and we have no tests focusing on it.
There was a problem hiding this comment.
I'll do some experiments this week and see if we can have tests written down!
There was a problem hiding this comment.
The test passes with this PR for me
There was a problem hiding this comment.
In the meeting notes, it says we were going to move forward with the hack where we conditionally add RUSTFLAGS to -Cextra-filename
There was a problem hiding this comment.
Thanks. I am forgetting things more often recently…
Given that, this PR is ready to go!
There was a problem hiding this comment.
I removed this from the queue as I'd like to add that hack before this gets merged so we don't accidentally break people.
There was a problem hiding this comment.
Will move forward with ed's hack.
I am confused by the meeting note. I assumed Ed's hack is this PR. The “hack” you are going to add is #6966, no?
There was a problem hiding this comment.
#6966 filters out --remap-path-prefix from being hashed.
The hack I proposed is that we skip hashing all RUSTFLAGS if something that looks like --remap-path-prefix is present.
epage
force-pushed
the
rustflags2
branch
2 times, most recently
from
16ce931 to
c762927
Compare
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 26, 2024
### What does this PR try to resolve? This is a regression test to prevent issues like #7416. The test only run on Linux, as other platforms have different requirements for PGO, or emit different PGO function missing warnings. ### How should we test and review this PR? Not sure how brittle it is. We can optionally run it only on Cargo's CI? cc #14830
epage
force-pushed
the
rustflags2
branch
3 times, most recently
from
6f03f7f to
1b5bc3a
Compare
weihanglo
previously approved these changes
Dec 5, 2024
| dep_c_extra_filename_hashes.hash(&mut c_extra_filename_hasher); | ||
| // Avoid trashing the caches on RUSTFLAGS changing via `c_extra_filename` | ||
| // | ||
| // Limited to `c_extra_filename` to help with reproducible build / PGO issues. |
There was a problem hiding this comment.
Thanks. I am forgetting things more often recently…
Given that, this PR is ready to go!
I almost overlooked these
…e hashes For now, they should result in the same data.
epage
force-pushed
the
rustflags2
branch
3 times, most recently
from
a0a0122 to
0559b75
Compare
epage
force-pushed
the
rustflags2
branch
2 times, most recently
from
7a1b3d3 to
29a267a
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Doesn't seem relevant. Re-queue it. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 7, 2024
Update cargo 6 commits in 05f54fdc34310f458033af8a63ce1d699fae8bf6..20a443231846b81c7b909691ec3f15eb173f2b18 2024-12-03 03:14:12 +0000 to 2024-12-06 21:56:56 +0000 - fix(fingerprint): Don't throwaway the cache on RUSTFLAGS changes (rust-lang/cargo#14830) - fix(build-rs)!: Remove meaningless 'cargo_cfg_debug_assertions' (rust-lang/cargo#14901) - docs(fingerprint): cargo-rustc extra flags do not affect the metadata (rust-lang/cargo#14898) - fix(add): Don't select yanked versions when normalizing names (rust-lang/cargo#14895) - fix(fix): Migrate workspace dependencies (rust-lang/cargo#14890) - test(build-std): make mock-std closer to real world (rust-lang/cargo#14896)
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 8, 2024
Update cargo 6 commits in 05f54fdc34310f458033af8a63ce1d699fae8bf6..20a443231846b81c7b909691ec3f15eb173f2b18 2024-12-03 03:14:12 +0000 to 2024-12-06 21:56:56 +0000 - fix(fingerprint): Don't throwaway the cache on RUSTFLAGS changes (rust-lang/cargo#14830) - fix(build-rs)!: Remove meaningless 'cargo_cfg_debug_assertions' (rust-lang/cargo#14901) - docs(fingerprint): cargo-rustc extra flags do not affect the metadata (rust-lang/cargo#14898) - fix(add): Don't select yanked versions when normalizing names (rust-lang/cargo#14895) - fix(fix): Migrate workspace dependencies (rust-lang/cargo#14890) - test(build-std): make mock-std closer to real world (rust-lang/cargo#14896)
github-actions bot
pushed a commit
to rust-lang/miri
that referenced
this pull request
Dec 8, 2024
Update cargo 6 commits in 05f54fdc34310f458033af8a63ce1d699fae8bf6..20a443231846b81c7b909691ec3f15eb173f2b18 2024-12-03 03:14:12 +0000 to 2024-12-06 21:56:56 +0000 - fix(fingerprint): Don't throwaway the cache on RUSTFLAGS changes (rust-lang/cargo#14830) - fix(build-rs)!: Remove meaningless 'cargo_cfg_debug_assertions' (rust-lang/cargo#14901) - docs(fingerprint): cargo-rustc extra flags do not affect the metadata (rust-lang/cargo#14898) - fix(add): Don't select yanked versions when normalizing names (rust-lang/cargo#14895) - fix(fix): Migrate workspace dependencies (rust-lang/cargo#14890) - test(build-std): make mock-std closer to real world (rust-lang/cargo#14896)
lnicola
pushed a commit
to lnicola/rust-analyzer
that referenced
this pull request
Dec 11, 2024
Update cargo 6 commits in 05f54fdc34310f458033af8a63ce1d699fae8bf6..20a443231846b81c7b909691ec3f15eb173f2b18 2024-12-03 03:14:12 +0000 to 2024-12-06 21:56:56 +0000 - fix(fingerprint): Don't throwaway the cache on RUSTFLAGS changes (rust-lang/cargo#14830) - fix(build-rs)!: Remove meaningless 'cargo_cfg_debug_assertions' (rust-lang/cargo#14901) - docs(fingerprint): cargo-rustc extra flags do not affect the metadata (rust-lang/cargo#14898) - fix(add): Don't select yanked versions when normalizing names (rust-lang/cargo#14895) - fix(fix): Migrate workspace dependencies (rust-lang/cargo#14890) - test(build-std): make mock-std closer to real world (rust-lang/cargo#14896)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR try to resolve?
Fixes #8716
This splits
-C metdataand-C extra-filenameand addsRUSTFLAGSto-C extra-filenamein the hope that we can still make PGO and reproducible builds work while caching artifacts per RUSTFLAGS used.How should we test and review this PR?
Additional information