tests: extend remap-path-prefix-std to all stdlib rlibs#156570
tests: extend remap-path-prefix-std to all stdlib rlibs#156570paradoxicalguy wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
4c83da0 to
6a3fc8b
Compare
|
@bors try jobs=dist-x86_64-linux |
This comment has been minimized.
This comment has been minimized.
tests: extend remap-path-prefix-std to all stdlib rlibs try-job: dist-x86_64-linux
|
@bors r+ rollup |
…lean, r=Urgau tests: extend remap-path-prefix-std to all stdlib rlibs ### problem: the current `remap-path-prefix-std` run-make test validates path remapping for `libstd.rlib`, which leaves the rest of the distributed `stdlib` artifacts unchecked. ### fix: generalizing the test to go over over all distributed `.rlib` files found in the target `sysroot libdir` instead of checking only the `libstd`. ### result: improves reproducibility coverage for distributed `stdlib` artifacts. r? @Urgau
|
This pull request was unapproved. This PR was contained in a rollup (#156584), which was unapproved. |
This comment has been minimized.
This comment has been minimized.
94d97b7 to
afa7f77
Compare
|
Let's do a quick try run to be sure. @bors try jobs=dist-i586-gnu-i586-i686-musl |
tests: extend remap-path-prefix-std to all stdlib rlibs try-job: dist-i586-gnu-i586-i686-musl
This comment has been minimized.
This comment has been minimized.
|
💔 Test for e47907c failed: CI. Failed job:
|
tests: extend remap-path-prefix-std to all stdlib rlibs
|
✌️ @paradoxicalguy, you can now perform try builds on this pull request! Warning You used the legacy format of the delegate command. You can now post |
|
yay thank you! |
|
@bors try jobs=dist-i586-gnu-i586-i686-musl |
This comment has been minimized.
This comment has been minimized.
tests: extend remap-path-prefix-std to all stdlib rlibs try-job: dist-i586-gnu-i586-i686-musl
|
💔 Test for 6b1fa23 failed: CI. Failed job:
|
|
|
@bors try jobs=dist-i586-gnu-i586-i686-musl |
This comment has been minimized.
This comment has been minimized.
tests: extend remap-path-prefix-std to all stdlib rlibs try-job: dist-i586-gnu-i586-i686-musl
|
💔 Test for b3b0fe2 failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
…king properly via prior test
|
cc @tgross35 |
|
@bors try jobs=dist-i586-gnu-i586-i686-musl |
This comment has been minimized.
This comment has been minimized.
tests: extend remap-path-prefix-std to all stdlib rlibs try-job: dist-i586-gnu-i586-i686-musl
| let commit_hash = debuginfo_map | ||
| .split('\t') | ||
| .next() | ||
| .and_then(|first| first.split("/rustc/").nth(1)) | ||
| .and_then(|s| s.split('/').next()) | ||
| .map(String::from); |
There was a problem hiding this comment.
Why do we do this extraction? Looking at #t-compiler > Checking reproducibility of rustc @ 💬 I feel like we could just apply all remapping like we do for llvm, no?
rust/compiler/rustc_llvm/build.rs
Lines 276 to 283 in b26c8ef
There was a problem hiding this comment.
wait i'll test this one rn, thanks
There was a problem hiding this comment.
tested this: switched to -ffile-prefix-map, looping over all RUSTC_DEBUGINFO_MAP entries like rustc_llvm's build.rs does.
added one extra mapping for the crate's own manifest dir since it's not covered by the existing entries.
removed the old line-538 mapping since it was conflicting with this.
verified via llvm-dwarfdump: all comp_dir entries now show /rustc/<hash>, no leaked paths.
There was a problem hiding this comment.
I'm surprised the crate own manifest dir is still necessary, but oh well, if it works.
Ping me when you push.
There was a problem hiding this comment.
mb it doesnt need a strict manifest dir
removing it gives: DW_AT_comp_dir ("/rustc/08f62523b1c3262cc2f0288a3369b760ab4bda45/library/compiler-builtins/compiler-builtins").
instead of the shorter DW_AT_comp_dir ("/rustc/08f62523b1c3262cc2f0288a3369b760ab4bda45").
no path leak tho. happy to keep code or drop it
There was a problem hiding this comment.
Well, the first DW_AT_comp_dir is more inline with what I would expect, to be honest. I would say, let's drop it, that makes the code simpler and clearer.
There was a problem hiding this comment.
sure, i'll open a separate pr first
|
why bootstrap doesn't just thread this through CFLAGS/CXXFLAGS instead of the custom |
|
That's a very good point, I didn't think of that. Turns out we already even have such remapping flags set in Lines 1323 to 1332 in 9f36de7 rust/src/bootstrap/src/core/builder/cargo.rs Line 433 in 9f36de7 I wonder why it's not working then, could it be because we only set it for the target? |
|
mark was asking here and found it, |
|
i tried it with while reverting the final result gives: the remap-path-prefix-std run-make test also passes with just the bootstrap change locally. |
View all comments
problem:
the current
remap-path-prefix-stdrun-make test validates path remapping forlibstd.rlib, which leaves the rest of the distributedstdlibartifacts unchecked.fix:
generalizing the test to go over over all distributed
.rlibfiles found in the targetsysroot libdirinstead of checking only thelibstd.result:
improves reproducibility coverage for distributed
stdlibartifacts.r? @Urgau