Translate the virtual `/rustc/$hash` prefix back to a real directory. #70642

eddyb · 2020-04-01T01:19:01Z

Closes #53486 and fixes #53081, by undoing the remapping to /rustc/$hash on the fly, when appropriate (e.g. our testsuites, or user crates that depend on libstd), but not during the Rust build itself (as that could leak the absolute build directory into the artifacts, breaking deterministic builds).

Tested locally by setting remap-debuginfo = true in config.toml, which without these changes, was causing 56 tests to fail (see #53081 (comment) for more details).

cc @Mark-Simulacrum @alexcrichton @ehuss

rust-highfive · 2020-04-01T01:19:05Z

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

eddyb · 2020-04-01T01:35:34Z

@bors try @rust-timer queue (this might need further optimization)

rust-timer · 2020-04-01T01:35:36Z

Awaiting bors try build completion

bors · 2020-04-01T01:35:47Z

⌛ Trying commit 88c0d0e475bc5a086fe2669ae53cb04ad95a2eb9 with merge 72a8ca0ab60ce346eb5a4495a56e408a9dd715bc...

src/bootstrap/lib.rs

estebank · 2020-04-01T02:26:44Z

src/etc/generate-deriving-span-tests.py

-// FIXME: missing sysroot spans (#53081)
-// ignore-i586-unknown-linux-gnu
-// ignore-i586-unknown-linux-musl


bors · 2020-04-01T03:53:57Z

☀️ Try build successful - checks-azure
Build commit: 72a8ca0ab60ce346eb5a4495a56e408a9dd715bc (72a8ca0ab60ce346eb5a4495a56e408a9dd715bc)

rust-timer · 2020-04-01T03:53:59Z

Queued 72a8ca0ab60ce346eb5a4495a56e408a9dd715bc with parent 58dd1ce, future comparison URL.

RalfJung · 2020-04-01T07:29:00Z

This is great. :)
Is there a good way to grep for tests that might also have had this problem, but failed to mention 53081 in the FIXME? (I assume you grepped for 53081.) Some time ago I tried to make them all carry this FIXME, but more not properly annotated tests could have crept in.

"ignore on 32bit targets" was a common work-around for this before we figured out which exact targets are affected, whatever the // ignore clause for that is. Also all of these have $SRC_DIR in their stderr file, I suppose.

rust-timer · 2020-04-01T09:25:50Z

Finished benchmarking try commit 72a8ca0ab60ce346eb5a4495a56e408a9dd715bc, comparison URL.

eddyb · 2020-04-01T11:40:23Z

@RalfJung 57 files mention $SRC_DIR, out of which 56 are the tests that were failing before this PR and the last one is the implementation in src/tools/compiletest/src/runtest.rs.

varkor · 2020-04-01T12:56:39Z

r? @Mark-Simulacrum

src/librustc_session/session.rs

ehuss · 2020-04-01T19:27:41Z

There were some concerns noted here about how suggestions are exposed for cross-crate sources. Is that still a concern? Does rustc know not to show suggestions for things in the standard library? I've been trying to think of some way to concoct an example, but haven't succeeded.

eddyb · 2020-04-01T20:18:13Z

@ehuss It only points to the standard library to note some things, like trait/type definitions, IME, most diagnostic logic check is_imported though, especially suggestions and lints.

You can search the test suite for $SRC_DIR, or just look at the diff of this PR, those are all the tests where libstd shows up.
You'll get far more though, if you enable -Z macro-backtrace (which most tests don't).

Ideally we'd have a way to check inside the compiler if the sources are user-editable, and only emit suggestions then, does rustup make rust-src and Cargo its ~/.cargo/cache, read-only?
That'd be a nice way to hint to rustc to never emit suggestions for those files (i.e. we could gate this inside the diagnostic code itself, instead of in every place where they're emitted).

Kind of hard to test though, unless we made some test files read-only through git?

Bonus: if a suggestion arises while building Rust itself, then it would be fine for to point to the standard library, in some situations (we'd also get this if Cargo hinted workespaces to rustc directly).

src/bootstrap/compile.rs

Mark-Simulacrum · 2020-04-01T20:37:51Z

@bors try

I think this looks good to land. The concern over linking to std from the sysroot seems largely fine to leave for a future discussion -- I believe it's true that this PR might make things worse (showing stuff from std as well) but it doesn't really change our decision making at all, so orthogonal to some extent.

I want to check that dist artifacts aren't suddenly full of sources though (and that dist doesn't break). I don't think it will, as we only copy dylibs and rlibs and such so I think we should be fine. But good to check.

Ideally we'd probably check that rustup can install rust-src tarball as well, but that's harder as we don't ship it from the try builder I think? I'll look into that separately. (That is, if nothing changed in layout then we're probably fine).

bors · 2020-04-01T20:38:02Z

⌛ Trying commit aba2085c320dce63d45d31257a4194ddebf1d90b with merge 5da172dc1b9c7a124a51904b96c20a79b5587292...

bors · 2020-04-01T20:38:17Z

⌛ Trying commit aba2085c320dce63d45d31257a4194ddebf1d90b with merge 40f2b445cd1c6c4e72d663d23c6d1bef2c674ad4...

estebank · 2020-04-01T22:58:40Z

Ideally we'd have a way to check inside the compiler if the sources are user-editable, and only emit suggestions then

Suggestions generally lean towards the assumption that the user can only change the usage of an existing API, as we assume that for example a trait's item signatures are correct while the impl might be wrong. This is not universal, but common enough that there should be few real issues with us suggesting users to change code in the stdlib.

bors · 2020-04-01T23:04:03Z

☀️ Try build successful - checks-azure
Build commit: 40f2b445cd1c6c4e72d663d23c6d1bef2c674ad4 (40f2b445cd1c6c4e72d663d23c6d1bef2c674ad4)

jonas-schievink · 2020-04-02T17:35:13Z

@bors p=1

jonas-schievink · 2020-04-02T21:15:47Z

@bors p=2

Seems like this could fix #70025, so let's try to land it quickly

bors · 2020-04-02T22:25:09Z

⌛ Testing commit 8deff18 with merge 3701be0c48cbc2246d6083a56603384cc9a4a422...

Centril · 2020-04-02T22:32:44Z

@bors retry yielding

bors · 2020-04-03T01:22:45Z

⌛ Testing commit 8deff18 with merge c53e4b3...

bors · 2020-04-03T04:18:23Z

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing c53e4b3 to master...

@eddyb

Rust bootstrap sysroot now has src in the same place as rust-src So we can remove a special hack. I checked this locally to confirm it works. Cc rust-lang/rust#70642 @eddyb

pnkfelix · 2020-05-26T18:28:49Z

We may need to revert this PR if I cannot find a way to fix #70924 soonish...

…s-issue-70924, r=eddyb Track devirtualized filenames Split payload of FileName::Real to track both real and virtualized paths. (Such splits arise from metadata refs into libstd; the virtualized paths look like `/rustc/1.45.0/src/libstd/io/cursor.rs` rather than `/Users/felixklock/Dev/Mozilla/rust.git/src/libstd/io/cursor.rs`) This way, we can emit the virtual name into things like the like the StableSourceFileId (as was done back before PR rust-lang#70642) that ends up in incremental build artifacts, while still using the devirtualized file path when we want to access the file. Fix rust-lang#70924

…cs, r=estebank Path remapping: Make behavior of diagnostics output dependent on presence of --remap-path-prefix. This PR fixes a regression (rust-lang#87745) with `--remap-path-prefix` where the flag stopped causing diagnostic messages to be remapped as well. The regression was introduced in rust-lang#83813 where we erroneously assumed that remapping of diagnostic messages was not desired anymore (because rust-lang#70642 partially undid that functionality with nobody objecting). The issue is fixed by making `--remap-path-prefix` remap diagnostic messages again, including for paths that have been remapped in upstream crates (e.g. the standard library). This means that "sysroot-localization" (implemented in rust-lang#70642) is also disabled if `rustc` is invoked with `--remap-path-prefix`. The assumption is that once someone starts explicitly remapping paths they also don't want paths to their local Rust installation in their build output. In the future we might want to give more fine-grained control over this behavior via compiler flags (see rust-lang/rfcs#3127 for a related RFC). For now this PR is intended as a regression fix. This PR is an alternative to rust-lang#88191, which makes diagnostic messages be remapped unconditionally. That approach, however, would effectively revert rust-lang#70642. Fixes rust-lang#87745. cc `@cbeuw` r? `@ghost`

…cs, r=estebank Path remapping: Make behavior of diagnostics output dependent on presence of --remap-path-prefix. This PR fixes a regression (rust-lang#87745) with `--remap-path-prefix` where the flag stopped causing diagnostic messages to be remapped as well. The regression was introduced in rust-lang#83813 where we erroneously assumed that remapping of diagnostic messages was not desired anymore (because rust-lang#70642 partially undid that functionality with nobody objecting). The issue is fixed by making `--remap-path-prefix` remap diagnostic messages again, including for paths that have been remapped in upstream crates (e.g. the standard library). This means that "sysroot-localization" (implemented in rust-lang#70642) is also disabled if `rustc` is invoked with `--remap-path-prefix`. The assumption is that once someone starts explicitly remapping paths they also don't want paths to their local Rust installation in their build output. In the future we might want to give more fine-grained control over this behavior via compiler flags (see rust-lang/rfcs#3127 for a related RFC). For now this PR is intended as a regression fix. This PR is an alternative to rust-lang#88191, which makes diagnostic messages be remapped unconditionally. That approach, however, would effectively revert rust-lang#70642. Fixes rust-lang#87745. cc ``@cbeuw`` r? ``@ghost``

…cs, r=estebank Path remapping: Make behavior of diagnostics output dependent on presence of --remap-path-prefix. This PR fixes a regression (rust-lang#87745) with `--remap-path-prefix` where the flag stopped causing diagnostic messages to be remapped as well. The regression was introduced in rust-lang#83813 where we erroneously assumed that remapping of diagnostic messages was not desired anymore (because rust-lang#70642 partially undid that functionality with nobody objecting). The issue is fixed by making `--remap-path-prefix` remap diagnostic messages again, including for paths that have been remapped in upstream crates (e.g. the standard library). This means that "sysroot-localization" (implemented in rust-lang#70642) is also disabled if `rustc` is invoked with `--remap-path-prefix`. The assumption is that once someone starts explicitly remapping paths they also don't want paths to their local Rust installation in their build output. In the future we might want to give more fine-grained control over this behavior via compiler flags (see rust-lang/rfcs#3127 for a related RFC). For now this PR is intended as a regression fix. This PR is an alternative to rust-lang#88191, which makes diagnostic messages be remapped unconditionally. That approach, however, would effectively revert rust-lang#70642. Fixes rust-lang#87745. cc `@cbeuw` r? `@ghost`

…, r=estebank Path remapping: Make behavior of diagnostics output dependent on presence of --remap-path-prefix. This PR fixes a regression (rust-lang#87745) with `--remap-path-prefix` where the flag stopped causing diagnostic messages to be remapped as well. The regression was introduced in rust-lang#83813 where we erroneously assumed that remapping of diagnostic messages was not desired anymore (because rust-lang#70642 partially undid that functionality with nobody objecting). The issue is fixed by making `--remap-path-prefix` remap diagnostic messages again, including for paths that have been remapped in upstream crates (e.g. the standard library). This means that "sysroot-localization" (implemented in rust-lang#70642) is also disabled if `rustc` is invoked with `--remap-path-prefix`. The assumption is that once someone starts explicitly remapping paths they also don't want paths to their local Rust installation in their build output. In the future we might want to give more fine-grained control over this behavior via compiler flags (see rust-lang/rfcs#3127 for a related RFC). For now this PR is intended as a regression fix. This PR is an alternative to rust-lang#88191, which makes diagnostic messages be remapped unconditionally. That approach, however, would effectively revert rust-lang#70642. Fixes rust-lang#87745. cc `@cbeuw` r? `@ghost`

rust-highfive assigned varkor Apr 1, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 1, 2020

estebank reviewed Apr 1, 2020

View reviewed changes

src/bootstrap/lib.rs Show resolved Hide resolved

estebank reviewed Apr 1, 2020

View reviewed changes

rust-highfive assigned Mark-Simulacrum and unassigned varkor Apr 1, 2020

Mark-Simulacrum reviewed Apr 1, 2020

View reviewed changes

src/librustc_session/session.rs Outdated Show resolved Hide resolved

eddyb force-pushed the remap-sysroot-src branch from 88c0d0e to aba2085 Compare April 1, 2020 20:29

Mark-Simulacrum reviewed Apr 1, 2020

View reviewed changes

src/bootstrap/compile.rs Show resolved Hide resolved

eddyb mentioned this pull request Apr 1, 2020

Rustdoc doesn't show src button for many functions in the rustc docs #70025

Closed

jonas-schievink mentioned this pull request Apr 2, 2020

Hide task_context when lowering body #70640

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 3, 2020

bors merged commit c53e4b3 into rust-lang:master Apr 3, 2020

eddyb deleted the remap-sysroot-src branch April 3, 2020 09:42

RalfJung mentioned this pull request Apr 4, 2020

Rust bootstrap sysroot now has src in the same place as rust-src rust-lang/miri#1300

Merged

ollie27 mentioned this pull request Apr 5, 2020

rustdoc's treatment of cross-crate ("imported") files is suboptimal. #70757

Open

mibac138 mentioned this pull request May 5, 2020

Error message on Windows missing "required by" hint #66860

Closed

arlosi mentioned this pull request May 11, 2020

'rustc' panicked at 'failed to lookup SourceFile in new context' #70924

Closed

pnkfelix mentioned this pull request May 30, 2020

Track devirtualized filenames #72767

Merged

nagisa mentioned this pull request Jun 9, 2020

Binaries end up containing path to the rust-src component despite --remap-path-prefix #73167

Closed

ehuss mentioned this pull request Aug 11, 2020

remap-debuginfo causes some rustc tests to fail #63339

Closed

michaelwoerister mentioned this pull request Apr 7, 2021

Fix --remap-path-prefix not correctly remapping rust-src component paths and unify handling of path mapping with virtualized paths #83813

Merged

michaelwoerister mentioned this pull request Aug 19, 2021

--remap-path-prefix no longer works for compiler messages #87745

Closed

michaelwoerister mentioned this pull request Aug 26, 2021

Path remapping: Make behavior of diagnostics output dependent on presence of --remap-path-prefix. #88363

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Translate the virtual `/rustc/$hash` prefix back to a real directory. #70642

Translate the virtual `/rustc/$hash` prefix back to a real directory. #70642

eddyb commented Apr 1, 2020

rust-highfive commented Apr 1, 2020

eddyb commented Apr 1, 2020

rust-timer commented Apr 1, 2020

bors commented Apr 1, 2020

estebank Apr 1, 2020

bors commented Apr 1, 2020

rust-timer commented Apr 1, 2020

RalfJung commented Apr 1, 2020

rust-timer commented Apr 1, 2020

eddyb commented Apr 1, 2020

varkor commented Apr 1, 2020

ehuss commented Apr 1, 2020

eddyb commented Apr 1, 2020 •

edited

Loading

Mark-Simulacrum commented Apr 1, 2020 •

edited

Loading

bors commented Apr 1, 2020

bors commented Apr 1, 2020

estebank commented Apr 1, 2020

bors commented Apr 1, 2020

jonas-schievink commented Apr 2, 2020

jonas-schievink commented Apr 2, 2020

bors commented Apr 2, 2020

Centril commented Apr 2, 2020

bors commented Apr 3, 2020

bors commented Apr 3, 2020

pnkfelix commented May 26, 2020

Translate the virtual /rustc/$hash prefix back to a real directory. #70642

Translate the virtual /rustc/$hash prefix back to a real directory. #70642

Conversation

eddyb commented Apr 1, 2020

rust-highfive commented Apr 1, 2020

eddyb commented Apr 1, 2020

rust-timer commented Apr 1, 2020

bors commented Apr 1, 2020

estebank Apr 1, 2020

Choose a reason for hiding this comment

bors commented Apr 1, 2020

rust-timer commented Apr 1, 2020

RalfJung commented Apr 1, 2020

rust-timer commented Apr 1, 2020

eddyb commented Apr 1, 2020

varkor commented Apr 1, 2020

ehuss commented Apr 1, 2020

eddyb commented Apr 1, 2020 • edited Loading

Mark-Simulacrum commented Apr 1, 2020 • edited Loading

bors commented Apr 1, 2020

bors commented Apr 1, 2020

estebank commented Apr 1, 2020

bors commented Apr 1, 2020

jonas-schievink commented Apr 2, 2020

jonas-schievink commented Apr 2, 2020

bors commented Apr 2, 2020

Centril commented Apr 2, 2020

bors commented Apr 3, 2020

bors commented Apr 3, 2020

pnkfelix commented May 26, 2020

Translate the virtual `/rustc/$hash` prefix back to a real directory. #70642

Translate the virtual `/rustc/$hash` prefix back to a real directory. #70642

eddyb commented Apr 1, 2020 •

edited

Loading

Mark-Simulacrum commented Apr 1, 2020 •

edited

Loading