Introduce -Z remap-cwd-prefix switch #87320

danakj · 2021-07-20T20:43:53Z

This switch remaps any absolute paths rooted under the current
working directory to a new value. This includes remapping the
debug info in DW_AT_comp_dir and DW_AT_decl_file.

Importantly, this flag does not require passing the current working
directory to the compiler, such that the command line can be
run on any machine (with the same input files) and produce the
same results. This is critical property for debugging compiler
issues that crop up on remote machines.

This is based on adetaylor's dbc4ae7

Major Change Proposal: rust-lang/compiler-team#450
Discussed on #38322. Would resolve issue #87325.

rust-highfive · 2021-07-20T20:43:56Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

Please see the contribution instructions for more information.

petrochenkov · 2021-07-20T21:00:47Z

r? @michaelwoerister

jsgf · 2021-07-20T21:42:24Z

Why --debug-...? --remap-path-prefix applies to all the places where paths are emitted, such as diagnostic messages, not just debug info. Maybe something like --remap-path-cwd <path> would be a better name?

jsgf · 2021-07-20T21:49:10Z

compiler/rustc_session/src/config.rs

+            .into_iter()
+            .map(|(i, s)| (i, RemapType::CompilationDir(s))),
+    );
+    positions.sort_by(|a, b| a.0.cmp(&b.0));


I assume the intent here is to make sure the relative ordering of remap-path-prefix and debug-compilation-dir options is preserved. But I wonder how much that matters? Presumably the remap-path-prefix option won't contain the cwd as a prefix, since avoiding that is the whole point of debug-compilation-dir.

Also will there ever be more than one instance of debug-compilation-dir? What does it mean to map the cwd multiple times?

That's right. I don't know that it matters much, I don't expect that most people would be mixing these, but it made for an easy way to make something well-defined, rather than one command-line argument having precedence over the other.

It would be slightly simpler code to not maintain the ordering, but I considered it a net win to have something more easily explained. In this case --debug-compilation-dir acts exactly like an alias for remap-path-prefix=$(pwd)=. Anything else seems to require more explanation in the docs, and more risk of an arbitrary change breaking some edge case in the future.

danakj · 2021-07-20T22:07:12Z

Why --debug-...? --remap-path-prefix applies to all the places where paths are emitted, such as diagnostic messages, not just debug info. Maybe something like --remap-path-cwd would be a better name?

I mentioned it on here: #38322 (comment)

I was just copying the name from Clang, but I am not tied to it. Happy to rename to whatever you prefer. If you like --remap-path-cwd (I like it too) then I'll use that, just let me know.

jsgf · 2021-07-20T23:11:50Z

I was just copying the name from Clang, but I am not tied to it. Happy to rename to whatever you prefer. If you like --remap-path-cwd (I like it too) then I'll use that, just let me know.

Yeah, I think consistency with rustc's existing options is more important than being consistent with clang's.

jsgf · 2021-07-20T23:20:37Z

compiler/rustc_session/src/config.rs

+            RemapType::CompilationDir(to) => match std::env::current_dir() {
+                Err(_) => early_error(
+                    error_format,
+                    "Unable to determine current directory for \
+                    --debug-compilation-dir",
+                ),
+                Ok(from) => (PathBuf::from(from), PathBuf::from(to)),


This only matches the precise cwd, not a trailing slash, so you're required to map to a non-empty path. So /home/me/src/foo.rs with a cwd of /home/me can only be mapped to ./src/foo.rs. But if you match against /home/me/ then you can choose to map it to '' giving src/foo.rs which is a bit cleaner. But that means you need to take the current path separator into account.

This implementation also has the pitfall that it will remap /home/meat/src/bar.rs to at/src/bar.rs since --remap-path-prefix works strictly on strings without regard to path separators.

I think there's an opportunity to discuss a more pathname-aware prefix-replacement algorithm, but there are a lot of edge cases to take into account (the current algorithm, for all its faults, is very simple to explain).

It’s cleaner without the path separator but there’s no less functionality with it, right?

If you wanted to specify a different prefix you can with something like “/foo/.” which would translate “/home/me/src/file.rs” into “/foo/./src/file.rs”. It’s not how you’d normally write it by hand, but it functions just fine, and is universally deterministic. Is it worth it to you to add extra code in here to avoid some dots in command lines?

This implementation also has the pitfall that it will remap /home/meat/src/bar.rs to at/src/bar.rs

Yes, true. However as this path will be rooted at the current working directory, all paths have the same prefix, so this shouldn’t actually come up, right?

Is there something we need to change here with the preexisting flag? Or could that be considered separately? The way this new flag is defined, as an alias for —remap-path-prefix, would allow for the existing flag to evolve and both command line flags could change in tandem.

You wouldn't be able to map a path to src/foo.rs with no prefix, and random intermixing of ``, ./ or even `././` gets messy esp if you're trying to be careful because of exact string matching. But no, not the end of the world.

Edit: it's implemented in terms of Path::strip_prefix, which ignores spurious ./ foo//bar, etc, so its more robust than plain string matching. It also won't be confused by partial components in the prefix.

rust/compiler/rustc_span/src/source_map.rs

Line 1026 in 7db08ee

if let Ok(rest) = path.strip_prefix(from) {

michaelwoerister · 2021-07-21T12:25:07Z

Thanks for the PR, @danakj! Since this would introduce a new stable compiler flag, would you mind opening a major change proposal for it? It looks like this needs some careful design work. I also recommend to word the MCP such that it proposes adding an unstable flag (i.e. -Zdebug-compilation-dir) first so that we have the chance to iterate on the feature before making it stable.

danakj · 2021-07-21T15:33:42Z

Thanks @michaelwoerister for pointing me to the MFP process, I was not aware of it. I will come back with that later today.

michaelwoerister · 2021-07-23T06:57:05Z

Side note: This feature (or something that encompasses it's functionality) might also be a good step towards improving the privacy situation around absolute paths (see for example #64839).

cbeuw · 2021-07-26T22:01:21Z

There is a related RFC: rust-lang/rfcs#3127, which utilises Cargo to supply different --remap-path-prefix flags to cover CWD as well as some other absolute paths.

michaelwoerister · 2021-08-19T08:47:03Z

@cbeuw, am I correct in assuming that the functionality implemented in this PR would not interfere with rust-lang/rfcs#3127?

michaelwoerister · 2021-08-19T09:40:04Z

src/doc/rustc/src/command-line-arguments.md

+same execution will work on all machines, regardless of build environment.
+
+This is an unstable option. Use `-Z remap-cwd-prefix=val` to specify a value.
+


As long as this option is unstable it needs to be documented under https://github.com/rust-lang/rust/tree/master/src/doc/unstable-book/src/compiler-flags rather than here.

@danakj, would you mind moving this piece of documentation to the "unstable book" (as mentioned above)? Otherwise it would show up in the regular documentation which is only meant for stable features.

Yes absolutely. I’ll be back at work next week and will do so then. Thanks very much!

I've moved the documentation over.

I believe that #87325 needs a tracking bug label attached to it, as well as an unstable label? But I am unable to do that. I thought of filing a new tracking but, but then I can't add the other labels it has.

I went ahead and opened #89434 as a dedicated tracking issue.

michaelwoerister · 2021-08-19T09:46:01Z

I just seconded the MCP and I think we should move forward with implementing this behind a -Z flag.

I'd be interested to hear any feedback (and especially objections) from @jsgf, @cbeuw, and @the8472.

jsgf · 2021-08-20T18:55:18Z

I have no concerns about an option which is effectively equivalent to --remap-path-prefix $(pwd)/=.

I think it's useful to have a late-binding cwd remapping for the cases where one doesn't know what the working directory will be at command-line construction time and don't want to interpose a shell to do backtick (or equiv) expansion.

bors · 2021-09-15T15:16:34Z

✌️ @danakj can now approve this pull request

Mark-Simulacrum · 2021-09-15T15:20:18Z

bors try is likely to be useless as-is. It doesn't run any tests, only builds a dist build for x86_64-unknown-linux-gnu, which likely works OK if the CI builders are passing.

You can enable a subset of the builders an r+ will trigger by editing this section and adding the builders above that (by copying them down) and then run ./x.py run src/tools/expand-yaml-anchors to generate the actual CI configuration. After that, bors try will run the selected targets.

Please limit this to just a few CI builders at a time.

This disables the remap_cwd_bin test which is failing on windows, and adds a test for --remap-path-prefix making a bin crate instead, to see if it will fail the same way.

danakj · 2021-09-15T15:50:32Z

@bors try

bors · 2021-09-15T15:50:40Z

⌛ Trying commit 4933be9 with merge 1e88e879f6e3d88c9b77998b1cac615454a9e10e...

bors · 2021-09-15T16:51:23Z

💔 Test failed - checks-actions

danakj · 2021-09-15T17:32:14Z

I disabled my new bin target test, and added one for --remap-path-prefix instead, and we can see that it fails as well. So the failure is not related to this PR, but there's an existing bug. I will add both tests (disabled) here, and file a bug.

These tests fail on Windows, as the build is not deterministic there for bin targets. Issue rust-lang#88982 is filed for this problem.

danakj · 2021-09-15T17:45:31Z

Bug is filed at #88982. I kept both tests in, and disabled them here for now.

Since that one test was all that went wrong, I would think this is ready to try again.

@rustbot ready

danakj · 2021-09-15T17:58:49Z

Also, thank you Manish and Mark for your assistance in navigating the maze of Rust CI and PR submission :) I really appreciate the help!

Manishearth · 2021-09-15T18:07:20Z

@bors r=michaelwoerister

(new test looks fine)

I'm going to let this stay in rollup=iffy mode but I might include it in my own rollups. The advantage of rollup=iffy is that you get priority over most of the other PRs for non-rollups so if there are still issues you'll find out quickly

(queue is a bit backed up rn though)

bors · 2021-09-15T18:07:21Z

📌 Commit c011828 has been approved by michaelwoerister

bors · 2021-09-15T18:07:22Z

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

…arth Rollup of 8 pull requests Successful merges: - rust-lang#87320 (Introduce -Z remap-cwd-prefix switch) - rust-lang#88690 (Accept `m!{ .. }.method()` and `m!{ .. }?` statements. ) - rust-lang#88775 (Revert anon union parsing) - rust-lang#88841 (feat(rustc_typeck): suggest removing bad parens in `(recv.method)()`) - rust-lang#88907 (Highlight the `const fn` if error happened because of a bound on the impl block) - rust-lang#88915 (`Wrapping<T>` has the same layout and ABI as `T`) - rust-lang#88933 (Remove implementation of `min_align_of` intrinsic) - rust-lang#88951 (Update books) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned petrochenkov Jul 20, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2021

rust-highfive assigned michaelwoerister and unassigned petrochenkov Jul 20, 2021

This comment has been minimized.

Sign in to view

jsgf reviewed Jul 20, 2021

View reviewed changes

danakj mentioned this pull request Jul 20, 2021

Reproducible outputs without build-environment-specific command lines #87325

Closed

jsgf reviewed Jul 20, 2021

View reviewed changes

danakj force-pushed the debug-compilation-dir branch from 26f4afe to a1bbe73 Compare July 22, 2021 18:56

danakj changed the title ~~Introduce --debug-compilation-dir switch~~ Introduce -Z remap-cwd-prefix switch Jul 22, 2021

danakj mentioned this pull request Jul 27, 2021

Reproducible command line + determinism rust-lang/compiler-team#450

Closed

3 tasks

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021

michaelwoerister reviewed Aug 19, 2021

View reviewed changes

jsgf mentioned this pull request Aug 19, 2021

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

Closed

inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021

michaelwoerister added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 30, 2021

Verify bin crates are not deterministic on Windows

4933be9

This disables the remap_cwd_bin test which is failing on windows, and adds a test for --remap-path-prefix making a bin crate instead, to see if it will fail the same way.

This comment has been minimized.

Sign in to view

danakj mentioned this pull request Sep 15, 2021

Windows builds are not deterministic with --crate-type=bin and path-prefix remapping #88982

Open

Disable both reproducible-build tests for crate-type=bin

c011828

These tests fail on Windows, as the build is not deterministic there for bin targets. Issue rust-lang#88982 is filed for this problem.

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 15, 2021

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 15, 2021

Manishearth mentioned this pull request Sep 15, 2021

Rollup of 8 pull requests #88992

Merged

bors merged commit 84646e9 into rust-lang:master Sep 16, 2021

rustbot added this to the 1.57.0 milestone Sep 16, 2021

ChrisDenton mentioned this pull request Sep 30, 2021

Rust binaries contain path strings that expose system username when using any dependency #89410

Closed

ehuss mentioned this pull request Oct 1, 2021

Tracking Issue for -Z remap-cwd-prefix #89434

Open

2 tasks

HCastano mentioned this pull request Sep 7, 2022

Add information for reproducible builds to metadata use-ink/cargo-contract#525

Closed

5 tasks

This was referenced Jul 10, 2024

Build failure: espanso (aarch64-darwin) NixOS/nixpkgs#274769

Closed

rust-lldb finds wrong source map (from build directory) NixOS/nixpkgs#282900

Open

jieyouxu mentioned this pull request Aug 14, 2024

Tracking Issue for Reproducible Build bugs and challenges #129080

Open

81 tasks

		same execution will work on all machines, regardless of build environment.

		This is an unstable option. Use `-Z remap-cwd-prefix=val` to specify a value.

Introduce -Z remap-cwd-prefix switch #87320

Introduce -Z remap-cwd-prefix switch #87320

Conversation

danakj commented Jul 20, 2021 • edited Loading

rust-highfive commented Jul 20, 2021

petrochenkov commented Jul 20, 2021

This comment has been minimized.

jsgf commented Jul 20, 2021

jsgf Jul 20, 2021

Choose a reason for hiding this comment

danakj Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

danakj commented Jul 20, 2021

jsgf commented Jul 20, 2021

jsgf Jul 20, 2021

Choose a reason for hiding this comment

danakj Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

jsgf Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

michaelwoerister commented Jul 21, 2021

danakj commented Jul 21, 2021

michaelwoerister commented Jul 23, 2021

cbeuw commented Jul 26, 2021 • edited Loading

michaelwoerister commented Aug 19, 2021

michaelwoerister Aug 19, 2021

Choose a reason for hiding this comment

michaelwoerister Aug 30, 2021

Choose a reason for hiding this comment

danakj Aug 30, 2021

Choose a reason for hiding this comment

danakj Sep 7, 2021

Choose a reason for hiding this comment

ehuss Oct 1, 2021

Choose a reason for hiding this comment

michaelwoerister commented Aug 19, 2021

jsgf commented Aug 20, 2021

bors commented Sep 15, 2021

Mark-Simulacrum commented Sep 15, 2021

danakj commented Sep 15, 2021

bors commented Sep 15, 2021

bors commented Sep 15, 2021

This comment has been minimized.

danakj commented Sep 15, 2021

danakj commented Sep 15, 2021

danakj commented Sep 15, 2021

Manishearth commented Sep 15, 2021

bors commented Sep 15, 2021

bors commented Sep 15, 2021

danakj commented Jul 20, 2021 •

edited

Loading

danakj Jul 20, 2021 •

edited

Loading

danakj Jul 21, 2021 •

edited

Loading

jsgf Jul 21, 2021 •

edited

Loading

cbeuw commented Jul 26, 2021 •

edited

Loading