Implement RFC 3127 sysroot path handling changes #118149
Open
+132
−52
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? @b-naber (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
A-testsuite
This comment has been minimized.
This comment has been minimized.
cbeuw
commented
Nov 21, 2023
src/tools/compiletest/src/header.rs
Outdated
Comment on lines
814
to
819
| if value.contains(RUST_SRC_BASE) { | ||
| let src = config.sysroot_base.join("lib/rustlib/src/rust"); | ||
| let canonical = canonicalize(&src).unwrap(); | ||
| value = value.replace(RUST_SRC_BASE, &canonical.to_string_lossy()); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is required, rather than just having a {{sysroot-base}} template, because /home/me/rust/build/<target>/stage1/lib/rustlib/src/rust is a symlink to /home/me/rust/. When /rust/$hash is mapped back to a real path, we resolve symlink by one step:
rust/compiler/rustc_session/src/config.rs
Lines 2838 to 2850 in 2f8d81f
/home/me/rust/.../foo.rs. If we were to have a --remap-path-prefix={{sysroot-base}}=remapped in the test header, nothing will get remapped because it becomes --remap-path-prefix=/home/me/rust/build/<target>/stage1/=remapped, which doesn't match /home/me/rust/
cbeuw
force-pushed
the
rfc3127-sysroot
branch
from
4780f7d
to
9c1614f
Compare
This comment has been minimized.
This comment has been minimized.
cbeuw
force-pushed
the
rfc3127-sysroot
branch
from
9c1614f
to
332abda
Compare
This comment has been minimized.
This comment has been minimized.
b-naber
reviewed
Dec 11, 2023
| virtual_name.display(), | ||
| new_path.display(), | ||
| ); | ||
| let new_name = rustc_span::RealFileName::LocalPath(new_path); |
There was a problem hiding this comment.
I don't understand this. The part of the RFC you quote says that the virtual path should only be replaced with the real path if rust-src is found. It's not obvious to me how this check is implemented here. Can you elaborate, please?
There was a problem hiding this comment.
rust-src needs to be found here
rust/compiler/rustc_session/src/config.rs
Lines 2885 to 2901 in 21cce21
for real_rust_source_base_dir to be Some, then checked by if let Some(real_dir) = &sess.opts.real_rust_source_base_dir above.
cbeuw
force-pushed
the
rfc3127-sysroot
branch
2 times, most recently
from
a306c76
to
100af45
Compare
cbeuw
commented
Dec 11, 2023
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
cbeuw
force-pushed
the
rfc3127-sysroot
branch
from
e59d24f
to
b7eeb3c
Compare
|
Looks good to me now. Thanks for the PR! @bors r+ |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 16, 2023
Implement RFC 3127 sysroot path handling changes Fix rust-lang#105907 Fix rust-lang#85463 Implement parts of rust-lang#111540 Right now, backtraces into sysroot always shows /rustc/$hash in diagnostics, e.g. ``` thread 'main' panicked at 'hello world', map-panic.rs:2:50 stack backtrace: 0: std::panicking::begin_panic at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/panicking.rs:616:12 1: map_panic::main::{{closure}} at ./map-panic.rs:2:50 2: core::option::Option<T>::map at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/option.rs:929:29 3: map_panic::main at ./map-panic.rs:2:30 4: core::ops::function::FnOnce::call_once at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ops/function.rs:248:5 note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. ``` [RFC 3127 said](https://rust-lang.github.io/rfcs/3127-trim-paths.html#changing-handling-of-sysroot-path-in-rustc) > We want to change this behaviour such that, when rust-src source files can be discovered, the virtual path is discarded and therefore the local path will be embedded, unless there is a --remap-path-prefix that causes this local path to be remapped in the usual way. This PR implements this behaviour. When `rust-src` is present at compile time, rustc replaces /rustc/$hash with a real path into local rust-src with best effort. To sanitise this, users must explicitly supply `--remap-path-prefix=<path to rust-src>=foo`.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
Looks like some legitimate issues. |
rustbot
added
S-waiting-on-author
|
Ughhh test suite normalisation shennenigans. On Windows, besides resolving links, |
|
Should be fixed now |
|
Can you change this yaml file to include the machine, which failed in the bors run, in CI so that we can make sure this is really fixed before starting a new bors run? I think all you have to do is add this: to the pr jobs here. Could be wrong though, haven't done this in a while. |
rustbot
added
the
T-infra
|
Don't think I can change the CI jobs:
https://github.com/orgs/community/discussions/27084#discussioncomment-3254544 I have tested this on my local MSVC environment anyway |
cbeuw
force-pushed
the
rfc3127-sysroot
branch
from
27e216a
to
0cba7c8
Compare
|
hm I know for a fact that I have added test runs to CI in the past without having any special privileges on this repo, but anyway if you have tested it locally it should be good enough. @bors r+ rollup=iffy |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 16, 2023
Implement RFC 3127 sysroot path handling changes Fix rust-lang#105907 Fix rust-lang#85463 Implement parts of rust-lang#111540 Right now, backtraces into sysroot always shows /rustc/$hash in diagnostics, e.g. ``` thread 'main' panicked at 'hello world', map-panic.rs:2:50 stack backtrace: 0: std::panicking::begin_panic at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/panicking.rs:616:12 1: map_panic::main::{{closure}} at ./map-panic.rs:2:50 2: core::option::Option<T>::map at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/option.rs:929:29 3: map_panic::main at ./map-panic.rs:2:30 4: core::ops::function::FnOnce::call_once at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ops/function.rs:248:5 note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. ``` [RFC 3127 said](https://rust-lang.github.io/rfcs/3127-trim-paths.html#changing-handling-of-sysroot-path-in-rustc) > We want to change this behaviour such that, when rust-src source files can be discovered, the virtual path is discarded and therefore the local path will be embedded, unless there is a --remap-path-prefix that causes this local path to be remapped in the usual way. This PR implements this behaviour. When `rust-src` is present at compile time, rustc replaces /rustc/$hash with a real path into local rust-src with best effort. To sanitise this, users must explicitly supply `--remap-path-prefix=<path to rust-src>=foo`.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
(I had to synchronize the queue) |
bors
added
S-waiting-on-author
|
@cbeuw any updates on this? thanks |
|
Sorry I put this off because I couldn't reproduce or figure out why specifically |
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.
Fix #105907
Fix #85463
Implement parts of #111540
Right now, backtraces into sysroot always shows /rustc/$hash in diagnostics, e.g.
RFC 3127 said
This PR implements this behaviour. When
rust-srcis present at compile time, rustc replaces /rustc/$hash with a real path into local rust-src with best effort. To sanitise this, users must explicitly supply--remap-path-prefix=<path to rust-src>=foo.