-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
Prefer remapping the relative library/ and compiler/ directories
#150110
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.
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
Conversation
This is done to avoid leaking the relative paths to the standard library after the overall of filenames. Noted that the paths were already leaking before, but to a lesser extent since the paths embedded in the distributed `rlib` were absolute. In general Cargo compiles workspace members with relative paths, so it's better anyway to remap the relative path. cf https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/remapping.20of.20the.20standard.20library/near/564093571
|
@bors try |
Prefer remapping the relative `library/` and `compiler/` directories
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, let give this a try at least. If this proves problematic, we can revert.
|
@bors r+ rollup=never |
Prefer remapping the relative `library/` and `compiler/` directories This is done to avoid leaking the relative paths to the standard library after the overall of filenames in #149709. Noted that the paths were already leaking before, but to a lesser extent since most (but not all) the paths embedded in the distributed `rlib` were absolute. In general Cargo compiles workspace members with relative paths, so it's better anyway to remap the relative path. In addition to our tests I have manually confirmed that it also works as expected for the printed diagnostics paths. cf. https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/remapping.20of.20the.20standard.20library/near/564093571
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
|
Huh. |
|
Those tests failures are interesting, they show the remapped paths, despite I have manually checked and the above prefix change works as expected for imported files (ie files described in Instead they come from the compiled artifact it-self, and more specifically the debuginfo, which is retrived by the backtraces and shown on those tests. (yes, even in the The reason this wasn't an issue before is because were only producing the relative paths, which are not remapped before this PR. As for why those tests didn't fail on PR CI, I have no idea. In any-case we well need a mechanism that supplants the current This is even complicated by the fact depending on whenever the compiler/std was compiled with remapping on or off changes the kind of paths we get, as with remapping on we will get absolute paths (starting with Given that all of those test already had custom normalization for those backtrace paths, I have opted to add a simple |
|
@bors try jobs=dist-x86_64-linux |
This comment has been minimized.
This comment has been minimized.
Prefer remapping the relative `library/` and `compiler/` directories try-job: dist-x86_64-linux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... yeah. The extra test normalizations don't feel great, but not end of the world.
|
@bors r+ |
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 0f0d850 (parent) -> 1d8f9c5 (this PR) Test differencesNo test diffs found Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 1d8f9c548d4d8845951f1d927caac1620e86353d --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (1d8f9c5): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.6%, secondary 0.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.1%, secondary 2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.1%, secondary 0.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 479.055s -> 480.26s (0.25%) |
This is done to avoid leaking the relative paths to the standard library after the overall of filenames in #149709.
Noted that the paths were already leaking before, but to a lesser extent since most (but not all) the paths embedded in the distributed
rlibwere absolute.In general Cargo compiles workspace members with relative paths, so it's better anyway to remap the relative path.
In addition to our tests I have manually confirmed that it also works as expected for the printed diagnostics paths.
cf. https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/remapping.20of.20the.20standard.20library/near/564093571
r? @jieyouxu