Skip to content
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

Implement a file-path remapping feature in support of debuginfo and reproducible builds #41508

Merged
merged 5 commits into from Apr 28, 2017

Conversation

@michaelwoerister
Copy link
Contributor

michaelwoerister commented Apr 24, 2017

This PR adds the -Zremap-path-prefix-from/-Zremap-path-prefix-to commandline option pair and is a more general implementation of #41419. As opposed to the previous attempt, this implementation should enable reproducible builds regardless of the working directory of the compiler.

This implementation of the feature is more general in the sense that the re-mapping will affect all paths the compiler emits, including the ones in error messages.

r? @alexcrichton

FilePathMapping::new(
self.debugging_opts.remap_path_prefix_from.iter().zip(
self.debugging_opts.remap_path_prefix_to.iter()
).map(|(src, dst)| (src.clone(), dst.clone())).collect()

This comment has been minimized.

Copy link
@nagisa

nagisa Apr 24, 2017

Contributor

This seems like a potentially confusing CLUI to me. At this point I would simply make -Z remap-path take two arguments… obviously not something our argument parser can do.

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Apr 24, 2017

Author Contributor

Yeah, I know, it's not perfect. There's been quite some discussion about the CLUI in the initial issue (#38322). This seemed to be an acceptable compromise. It's an unstable feature at the moment. If our parser had some improvements before this is stabilized, we could certainly revisit this.

This comment has been minimized.

Copy link
@gistofj

gistofj Apr 27, 2017

Given that the values are paths, and most file systems do not like colon ':' in path names, can't a single argument be bifurcated on a colon? Example: -Z remap-path="original_path_from:preferred_path_to".

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 24, 2017

Looks great to me! Was it intentional to remove the same-number-arguments validation? (checking that the same number of -to as -from was supplied)

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Apr 24, 2017

Was it intentional to remove the same-number-arguments validation?

Good catch! I'll add it back in tomorrow.

@arielb1 arielb1 added the relnotes label Apr 25, 2017
@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Apr 25, 2017

@rust-lang/docs, is there a place where a not-yet-stable feature like this can be documented?

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Apr 25, 2017

@frewsxcv

This comment has been minimized.

Copy link
Member

frewsxcv commented Apr 25, 2017

@rust-lang/docs, is there a place where a not-yet-stable feature like this can be documented?

Yep! Just add a new file to this directory and link to it in the 'compiler flags' section of the summary file.

@frewsxcv

This comment has been minimized.

Copy link
Member

frewsxcv commented Apr 25, 2017

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Apr 26, 2017

OK, I added some documentation to the unstable book and made the codegen test check more things.
r? @alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 26, 2017

☔️ The latest upstream changes (presumably #41504) made this pull request unmergeable. Please resolve the merge conflicts.

@michaelwoerister michaelwoerister force-pushed the michaelwoerister:generic-path-remapping branch from 5b20acc to 7be6f22 Apr 26, 2017
@michaelwoerister michaelwoerister force-pushed the michaelwoerister:generic-path-remapping branch from 7be6f22 to 4dbd8a3 Apr 26, 2017
Copy link
Member

alexcrichton left a comment

just a few nits, otherwise r=me

@@ -2,6 +2,7 @@

- [Compiler flags](compiler-flags.md)
- [linker_flavor](compiler-flags/linker-flavor.md)
- [linker_flavor](compiler-flags/remap-path-prefix.md)

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Apr 26, 2017

Member

s/linker_flavor/remap_path_prefix/

@@ -0,0 +1,37 @@
# `remap-path-prefix`

The tracking issue for this feature is: None

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Apr 26, 2017

Member

Mind opening a tracking issue for this?

@michaelwoerister michaelwoerister force-pushed the michaelwoerister:generic-path-remapping branch from 4dbd8a3 to ab9691d Apr 26, 2017
@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Apr 26, 2017

@bors r=alexcrichton

Nits fixed, I think.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 26, 2017

📌 Commit ab9691d has been approved by alexcrichton

@jsgf

This comment has been minimized.

Copy link
Contributor

jsgf commented Apr 26, 2017

Thanks for turning this around @michaelwoerister! I hope to try this out soon.

BTW, assuming this gets stabilized in more or less the current form, what would the stable command-line options look like?

```text
rustc -Zremap-path-prefix-from="/home/foo/my-project/src" -Zremap-path-prefix-to="/sources/my-project"
```

This comment has been minimized.

Copy link
@jsgf

jsgf Apr 26, 2017

Contributor

It's unclear from this how the remapping is actually operating. It should also answer:

  • Is it delimited by path element boundaries, or is it operating on literal strings?
  • Is there any normalization performed before/after doing the transformation? (Redundant / elimination?)
  • Are the input paths absolute, relative, or whatever was provided to the compiler?
  • In this example, if the input path were /home/foo/my-project/src-orig, would this remap to /sources/my-project-orig? Should there be a trailing / to prevent this if its undesirable?

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Apr 26, 2017

Author Contributor

Good points. I'll provide some more documentation in a follow-up PR. The implementation as it is does as little as possible: (1) Take each string as it is provided to the compiler, (2) don't do any normalization, (3) do a simple string-level prefix replacement.


When the options are given multiple times, the nth `-from` will be matched up
with the nth `-to` and they can appear anywhere on the commandline. Mappings
specified later on the line will take precedence over earlier ones.

This comment has been minimized.

Copy link
@jsgf

jsgf Apr 26, 2017

Contributor

What does

Mappings specified later on the line will take precedence over earlier ones.

actually mean? Do you mean that if the -Zremap-path-prefix-from is identical it is replaced? Or does it more generally apply, such that a more specific path overrides a more general one?

For example, what's the mapping for:

rustc -Zremap-path-prefix-from=my-project/src -Zremap-path-prefix-to=/sources/my-project \
      -Zremap-path-prefix-from=my-project/src/vendored/otherproject -Zremap-path-prefix-to=/sources/otherproject

when applied to my-project/src/vendored/otherproject/src/lib.rs?

In other words, does the ordering of the -from options matter when multiple mappings could apply? Or is there some other way to disambiguate multiple matching mappings?

Is it guaranteed that at most a single mapping can be applied, or is there some notion of repeated application of mappings until nothing matches?

This comment has been minimized.

Copy link
@jsgf

jsgf Apr 26, 2017

Contributor

I wonder if defining the matching as:

  1. later remap-path-prefix-from replaces a previous identical one
  2. mappings are matched from longest from prefix to shortest

would be better.

I think this would make it deterministically always select more specific mappings over more general ones, rather than relying purely on ordering.

This comment has been minimized.

Copy link
@infinity0

infinity0 Apr 26, 2017

Contributor

That would differ from the current behaviour in the example a/b -> y, a -> x. I'm not sure which is better, but without a concrete argument that the new one is better I'd suggest to stick with the current behaviour which is simple to describe and implement. The other behaviour would require a trie-like data structure or other more complex thing.

I'm trying to standardise this behaviour across multiple compilers and GCC is already doing something very similar based on the ordering of command-line flags (and I have a patch pending to make it exactly match the behaviour being proposed in this PR). Changing this in the suggested way would make the spec for this standard even more complex. :(

In real-world cases I don't think the issue will crop up, in rustc or anywhere else - it's unlikely that a child process inheriting a prefix-map from a parent process would want to add a less-specific mapping for a higher-level directory.

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Apr 26, 2017

Author Contributor

The implementation is very simple: Walk all from/to pairs as provided on the commandline, starting at the last, and just stop at the first match.

In my opinion, relying only on ordering is a good approach, since the rules are clear and without surprises.

This comment has been minimized.

Copy link
@jsgf

jsgf Apr 26, 2017

Contributor

The current behaviour allows you to specify useless mappings (by putting more general before more specific in the reverse ordering), which might be inadvertent or unexpected.

I don't think it would require very fancy structures; I think you could implement it pretty simply using BTreeMap<String, String> instead of Vec<String, String>. It would be nice to have an algorithm that can have more efficient implementations (ie, not linear search), to cope with the case where there are lots of mappings (but that's definitely not a requirement for the first implementation).

This comment has been minimized.

Copy link
@jsgf

jsgf Apr 26, 2017

Contributor

@michaelwoerister

In my opinion, relying only on ordering is a good approach, since the rules are clear and without surprises

I think that's fine - I think the wording could do with clarification (ie, I managed to misinterpret what it meant until I looked at the source).

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Apr 26, 2017

Author Contributor

Noted. I'll provide an update in a subsequent PR and make sure to ping the usual suspects for review.

This comment has been minimized.

Copy link
@jmesmon

jmesmon Apr 26, 2017

Contributor

Using the ordering of flags for the lookup allows easy overriding of previous flags without needing to have a way to remove items from an accumulated list of flags (ex: presuming a variable in some higher level build env is accumulating RUST_FLAGS, using ordering means there is no need to parse all the accumulated flags to figure out which ones to remove when adding a higher priority remapping).

This comment has been minimized.

Copy link
@jsgf

jsgf Apr 26, 2017

Contributor

Well, that might be useful to have anyway, since an identity mapping is semantically different from no mapping at all.

@jmesmon

This comment has been minimized.

Copy link
Contributor

jmesmon commented Apr 26, 2017

As this remaps all paths, will this also resolve #40552 ? ie: does it affect the expansion of file!()?

@jsgf

This comment has been minimized.

Copy link
Contributor

jsgf commented Apr 26, 2017

@jmesmon - I believe that's the intent.

@aidanhs

This comment has been minimized.

Copy link
Member

aidanhs commented Apr 27, 2017

(failure due to appveyor network issues - https://appveyor.statuspage.io/incidents/06gzq846jl9x)

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Apr 27, 2017

@aidanhs OK, thanks for the heads up!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 27, 2017

💔 Test failed - status-appveyor

@TimNN

This comment has been minimized.

Copy link
Contributor

TimNN commented Apr 27, 2017

Looks like a legitimate failure:

[01:07:46] stdout:
[01:07:46] ------------------------------------------
[01:07:46] 
[01:07:46] ------------------------------------------
[01:07:46] stderr:
[01:07:46] ------------------------------------------
[01:07:46] C:\projects\rust\src/test\codegen\remap_path_prefix\main.rs:19:11: error: expected string not found in input
[01:07:46] // CHECK: internal constant [34 x i8] c"/the/src/remap_path_prefix/main.rs"
[01:07:46]           ^
[01:07:46] C:\projects\rust\build\x86_64-pc-windows-gnu\test\codegen\remap_path_prefix\main.ll:1:1: note: scanning from here
[01:07:46] ; ModuleID = 'main.cgu-0.rs'
[01:07:46] ^
[01:07:46] C:\projects\rust\build\x86_64-pc-windows-gnu\test\codegen\remap_path_prefix\main.ll:9:10: note: possible intended match here
[01:07:46] @str.0 = internal constant [34 x i8] c"/the/src\5Cremap_path_prefix\5Cmain.rs"
[01:07:46]          ^
[01:07:46] 
[01:07:46] ------------------------------------------
[01:07:46] 
[01:07:46] thread '[codegen] codegen\remap_path_prefix\main.rs' panicked at 'explicit panic', src\tools\compiletest\src\runtest.rs:2630
[01:07:46] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:07:46] 
[01:07:46] 
[01:07:46] failures:
[01:07:46]     [codegen] codegen\remap_path_prefix\main.rs
[01:07:46] 
[01:07:46] test result: FAILED. 34 passed; 1 failed; 5 ignored; 0 measured
[01:07:46] 
[01:07:46] thread 'main' panicked at 'Some tests failed', src\tools\compiletest\src\main.rs:329
@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Apr 27, 2017

Oh yeah, paths look different on Windows...

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Apr 27, 2017

Actual failure:

stdout:
------------------------------------------
------------------------------------------
stderr:
------------------------------------------
C:\projects\rust\src/test\codegen\remap_path_prefix\main.rs:19:11: error: expected string not found in input
// CHECK: internal constant [34 x i8] c"/the/src/remap_path_prefix/main.rs"
          ^
C:\projects\rust\build\x86_64-pc-windows-gnu\test\codegen\remap_path_prefix\main.ll:1:1: note: scanning from here
; ModuleID = 'main.cgu-0.rs'
^
C:\projects\rust\build\x86_64-pc-windows-gnu\test\codegen\remap_path_prefix\main.ll:9:10: note: possible intended match here
@str.0 = internal constant [34 x i8] c"/the/src\5Cremap_path_prefix\5Cmain.rs"
         ^
------------------------------------------
thread '[codegen] codegen\remap_path_prefix\main.rs' panicked at 'explicit panic', src\tools\compiletest\src\runtest.rs:2630
note: Run with `RUST_BACKTRACE=1` for a backtrace.
@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Apr 27, 2017

I guess I'll have to provide two versions of the test case, one for Unix- and one for Windows-style paths. Or is there a more elegant way?

@TimNN

This comment has been minimized.

Copy link
Contributor

TimNN commented Apr 27, 2017

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Apr 27, 2017

I could make compiletest expand {{path-sep}} to the platform specific path separator...

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Apr 27, 2017

@TimNN Thanks for the tip! But the problem is already caused by the arguments passed to the compiler, I think.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Apr 27, 2017

But the problem is already caused by the arguments passed to the compiler, I think.

More specifically, the test case tells the compiler to replace something Unix-specific here:

-Zremap-path-prefix-from={{src-base}}/remap_path_prefix/auxiliary

But on Windows the compiler will see the equivalent of

{{src-base}}\remap_path_prefix\auxiliary
            ^                 ^             

😭

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 27, 2017

@michaelwoerister I'd also consider it ok to ignore the test on windows, it seems like we may not benefit much from the extra coverage there

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Apr 28, 2017

@bors r=alexcrichton

OK, since we are only doing string replacement there should be nothing platform-specific in there atm, so I
disabled the test on Windows. Thanks, @alexcrichton!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 28, 2017

📌 Commit 8ea050d has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 28, 2017

⌛️ Testing commit 8ea050d with merge 9339127...

bors added a commit that referenced this pull request Apr 28, 2017
…xcrichton

Implement a file-path remapping feature in support of debuginfo and reproducible builds

This PR adds the `-Zremap-path-prefix-from`/`-Zremap-path-prefix-to` commandline option pair and is a more general implementation of #41419. As opposed to the previous attempt, this implementation should enable reproducible builds regardless of the working directory of the compiler.

This implementation of the feature is more general in the sense that the re-mapping will affect *all* paths the compiler emits, including the ones in error messages.

r? @alexcrichton
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 28, 2017

💔 Test failed - status-travis

@TimNN

This comment has been minimized.

Copy link
Contributor

TimNN commented Apr 28, 2017

@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 28, 2017

⌛️ Testing commit 8ea050d with merge 2971d49...

bors added a commit that referenced this pull request Apr 28, 2017
…xcrichton

Implement a file-path remapping feature in support of debuginfo and reproducible builds

This PR adds the `-Zremap-path-prefix-from`/`-Zremap-path-prefix-to` commandline option pair and is a more general implementation of #41419. As opposed to the previous attempt, this implementation should enable reproducible builds regardless of the working directory of the compiler.

This implementation of the feature is more general in the sense that the re-mapping will affect *all* paths the compiler emits, including the ones in error messages.

r? @alexcrichton
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 28, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 2971d49 to master...

@bors bors merged commit 8ea050d into rust-lang:master Apr 28, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
kevinmehall added a commit to kevinmehall/rust-peg that referenced this pull request Apr 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.