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

Tracking issue for Path Prefix Remapping #41555

Closed
michaelwoerister opened this Issue Apr 26, 2017 · 41 comments

Comments

Projects
None yet
@michaelwoerister
Contributor

michaelwoerister commented Apr 26, 2017

This feature was originally requested in #38322. The PR that implements this in its unstable form is #41508. This will eventually become stable if the testing phase goes well.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 8, 2017

Contributor

What's the next step to stabilize this? Looks like it probably won't be stable for 1.20?

Contributor

brson commented Jul 8, 2017

What's the next step to stabilize this? Looks like it probably won't be stable for 1.20?

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Jul 10, 2017

Contributor

Good question. I did not heard any complaints about the way things currently work -- which I interpret as everybody being happy :).

Some of the people initially interested in the feature were @jmesmon, @infinity0, @sanxiyn, and @jsgf. Do you have any feedback?

Other than that, two open topics are:

  • The documentation (currently in the unstable book) needs a revamp. @jsgf found it confusing, IIRC.
  • The exact form of the CLI has seen some discussion but there was no clear agreement on what would be best. The current form is -Zremap-path-prefix-from=_, -Zremap-path-prefix-to=_. The could be transformed to
    • -Cremap-path-prefix-from=_, -Cremap-path-prefix-to=_ or
    • --remap-path-prefix-from=_, --remap-path-prefix-to=_ or
    • something else altogether.

I personally would be fine with either of the first two options.

cc @rust-lang/dev-tools

Contributor

michaelwoerister commented Jul 10, 2017

Good question. I did not heard any complaints about the way things currently work -- which I interpret as everybody being happy :).

Some of the people initially interested in the feature were @jmesmon, @infinity0, @sanxiyn, and @jsgf. Do you have any feedback?

Other than that, two open topics are:

  • The documentation (currently in the unstable book) needs a revamp. @jsgf found it confusing, IIRC.
  • The exact form of the CLI has seen some discussion but there was no clear agreement on what would be best. The current form is -Zremap-path-prefix-from=_, -Zremap-path-prefix-to=_. The could be transformed to
    • -Cremap-path-prefix-from=_, -Cremap-path-prefix-to=_ or
    • --remap-path-prefix-from=_, --remap-path-prefix-to=_ or
    • something else altogether.

I personally would be fine with either of the first two options.

cc @rust-lang/dev-tools

@infinity0

This comment has been minimized.

Show comment
Hide comment
@infinity0

infinity0 Jul 10, 2017

Contributor

I haven't tested this yet but I'm still interested. When I get a chance, I'll test it with 1.19 in Debian and see if we get a reproducible build.

Contributor

infinity0 commented Jul 10, 2017

I haven't tested this yet but I'm still interested. When I get a chance, I'll test it with 1.19 in Debian and see if we get a reproducible build.

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Jul 10, 2017

Contributor

We use it in clippy for making our tests' stderr output OS independent. Works like a charm: https://github.com/Manishearth/rust-clippy/blob/master/tests/examples.rs#L29

Contributor

oli-obk commented Jul 10, 2017

We use it in clippy for making our tests' stderr output OS independent. Works like a charm: https://github.com/Manishearth/rust-clippy/blob/master/tests/examples.rs#L29

@infinity0

This comment has been minimized.

Show comment
Hide comment
@infinity0

infinity0 Jul 10, 2017

Contributor

Is the duplication of the path with / and \ really necessary? I thought the stripping algorithm worked on whole-path components, so that trailing directory separators shouldn't be necessary.

Contributor

infinity0 commented Jul 10, 2017

Is the duplication of the path with / and \ really necessary? I thought the stripping algorithm worked on whole-path components, so that trailing directory separators shouldn't be necessary.

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Jul 10, 2017

Contributor

Maybe it changed. But I had to implement it this way, otherwise it wouldn't work

Contributor

oli-obk commented Jul 10, 2017

Maybe it changed. But I had to implement it this way, otherwise it wouldn't work

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Jul 10, 2017

Contributor

I thought the stripping algorithm worked on whole-path components, so that trailing directory separators shouldn't be necessary.

The current implementation works on plain strings. It does not try to interpret them as paths.

Contributor

michaelwoerister commented Jul 10, 2017

I thought the stripping algorithm worked on whole-path components, so that trailing directory separators shouldn't be necessary.

The current implementation works on plain strings. It does not try to interpret them as paths.

@infinity0

This comment has been minimized.

Show comment
Hide comment
@infinity0

infinity0 Jul 10, 2017

Contributor

Oh right. I was going by the code at the bottom of this older comment, I didn't notice you had gone back to plain string matching. What was the reason? I know GCC also do plain string matching, but I actually prefer path-level matching because it would be easier to use. This cross-platform duplication issue is a further example in that area.

Contributor

infinity0 commented Jul 10, 2017

Oh right. I was going by the code at the bottom of this older comment, I didn't notice you had gone back to plain string matching. What was the reason? I know GCC also do plain string matching, but I actually prefer path-level matching because it would be easier to use. This cross-platform duplication issue is a further example in that area.

@jsgf

This comment has been minimized.

Show comment
Hide comment
@jsgf

jsgf Jul 13, 2017

Contributor

@brson, @michaelwoerister I just hacked it up in buck and it all looks good. Once it's stable (at least, we have decided on a stable command line option), I can do a real diff.

Either -Cremap-path-prefix or --remap-path-prefix is fine by me.

Contributor

jsgf commented Jul 13, 2017

@brson, @michaelwoerister I just hacked it up in buck and it all looks good. Once it's stable (at least, we have decided on a stable command line option), I can do a real diff.

Either -Cremap-path-prefix or --remap-path-prefix is fine by me.

@jsgf

This comment has been minimized.

Show comment
Hide comment
@jsgf

jsgf Jul 17, 2017

Contributor

@michaelwoerister Do you think it's possible to stabilize this in the 1.21 timeframe?

Contributor

jsgf commented Jul 17, 2017

@michaelwoerister Do you think it's possible to stabilize this in the 1.21 timeframe?

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Jul 18, 2017

Contributor

@infinity0 I'm divided on whether path matching should be string- or component-based. Component-based is a bit more ergonomic in the cross-platform case. I'm not sure though if it can be somewhat less predictable in corner cases (e.g. strange Windows path prefixes).

@jsgf That should be doable.

I'm nominating this for the next @rust-lang/dev-tools meeting to discuss string matching and which form the CLI should take -- and then we can let the appropriate subteams vote on it.

@rust-lang/core: Which teams need to sign off on stabilizing this? Quick recap: This feature allows to remap file paths (in output messages and output artifacts) to be remapped. This is necessary for reproducible builds and helps with custom debugging setups.

Contributor

michaelwoerister commented Jul 18, 2017

@infinity0 I'm divided on whether path matching should be string- or component-based. Component-based is a bit more ergonomic in the cross-platform case. I'm not sure though if it can be somewhat less predictable in corner cases (e.g. strange Windows path prefixes).

@jsgf That should be doable.

I'm nominating this for the next @rust-lang/dev-tools meeting to discuss string matching and which form the CLI should take -- and then we can let the appropriate subteams vote on it.

@rust-lang/core: Which teams need to sign off on stabilizing this? Quick recap: This feature allows to remap file paths (in output messages and output artifacts) to be remapped. This is necessary for reproducible builds and helps with custom debugging setups.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 18, 2017

Member

@michaelwoerister @rust-lang/dev-tools sounds good to me as a team for signing off!

Member

alexcrichton commented Jul 18, 2017

@michaelwoerister @rust-lang/dev-tools sounds good to me as a team for signing off!

@jmesmon

This comment has been minimized.

Show comment
Hide comment
@jmesmon

jmesmon Jul 18, 2017

Contributor

On matching mechanism: an alternate to paths & strings is to use regexes. Strings works fine for me though.

Contributor

jmesmon commented Jul 18, 2017

On matching mechanism: an alternate to paths & strings is to use regexes. Strings works fine for me though.

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Aug 8, 2017

Contributor

This has been discussed in the @rust-lang/dev-tools meeting and the conclusion was that we want to stabilize in the following form:

  • This becomes a single, non--C flag: --remap-path-prefix
  • We change parameter passing to the GCC way: the option takes a single string argument of the form from=to. This means that the new prefix (the to part) cannot contain an equals sign, but otherwise this has the advantage of being shorter than the current unstable form and is close to prior art.
  • The option will only be shown in the extended help output of rustc (i.e. when --help -v is passed).

@rfcbot fcp merge

Contributor

michaelwoerister commented Aug 8, 2017

This has been discussed in the @rust-lang/dev-tools meeting and the conclusion was that we want to stabilize in the following form:

  • This becomes a single, non--C flag: --remap-path-prefix
  • We change parameter passing to the GCC way: the option takes a single string argument of the form from=to. This means that the new prefix (the to part) cannot contain an equals sign, but otherwise this has the advantage of being shorter than the current unstable form and is close to prior art.
  • The option will only be shown in the extended help output of rustc (i.e. when --help -v is passed).

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Aug 8, 2017

Team member @michaelwoerister has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Aug 8, 2017

Team member @michaelwoerister has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@jonathandturner

This comment has been minimized.

Show comment
Hide comment
@jonathandturner

jonathandturner Aug 8, 2017

Contributor

@rfcbot reviewed

Contributor

jonathandturner commented Aug 8, 2017

@rfcbot reviewed

@infinity0

This comment has been minimized.

Show comment
Hide comment
@infinity0

infinity0 Aug 8, 2017

Contributor

We change parameter passing to the GCC way: the option takes a single string argument of the form from=to. This means that the new prefix (the to part) cannot contain an equals sign, but otherwise this has the advantage of being shorter than the current unstable form and is close to prior art.

So in other words you would split on the final =, right? I prefer that to the current GCC behaviour, which splits on the initial = so that the from part cannot contain an equals sign.

Contributor

infinity0 commented Aug 8, 2017

We change parameter passing to the GCC way: the option takes a single string argument of the form from=to. This means that the new prefix (the to part) cannot contain an equals sign, but otherwise this has the advantage of being shorter than the current unstable form and is close to prior art.

So in other words you would split on the final =, right? I prefer that to the current GCC behaviour, which splits on the initial = so that the from part cannot contain an equals sign.

bors added a commit that referenced this issue Oct 5, 2017

Auto merge of #44940 - philipc:remap-path, r=michaelwoerister
Don't use remapped path when loading modules and include files

Fixes bug reported in #41555 (comment).

cc @michaelwoerister
@jsgf

This comment has been minimized.

Show comment
Hide comment
@jsgf

jsgf Oct 6, 2017

Contributor

Does this need release notes? Is this where that gets tagged?

Contributor

jsgf commented Oct 6, 2017

Does this need release notes? Is this where that gets tagged?

@johnthagen

This comment has been minimized.

Show comment
Hide comment
@johnthagen

johnthagen Oct 7, 2017

Contributor

I can confirm that #44940 fixed the compile issues I was seeing when using this feature and a simple reqwest application on macOS and Windows.

Thoughts on privacy

I think this feature possesses the mechanics to more broadly address privacy issues for closed-source code. This may be out of scope for the current issue, but I wanted to bring it up now since this feature hasn't been fully implemented yet.

The current reality for closed source code is that any time a panic!() is used, the module name, line number, and column number will be leaked into the binary to support unwinding. This is true even if panic = 'abort' is specified. The root issue is that panic!() calls file!(). This could leak private, perhaps proprietary or sensitive, implementation details that the company may not want to be included in released binaries.

Example

Let's say a company creates a video game with some hidden content. If the source for that module included a panic!() (and note, even things like unreachable!() call panic!()), the module name could be leaked as a string:

src/hidden_dlc/extra_boss_fight/victors_betrayal.rs

Using this feature, the company now has a manual way to suppress this at compile time (cool!):

RUSTFLAGS=-Zremap-path-prefix-from=src/hidden_dlc/extra_boss_fight/victors_betrayal.rs -Zremap-path-prefix-to=

But note that this is very manual (need a -from/-to for each module), and you'd need to make sure you didn't forget modules.

Straw Man Proposal

If rustc let you suppress all uses of file!() at compile time, you could allow such companies to keep these sensitive implementation details out of the binaries. This also helps save on binary size if these strings are not needed.

RUSTFLAGS=-Zremap-path-prefix-from=* -Zremap-path-prefix-to=
Contributor

johnthagen commented Oct 7, 2017

I can confirm that #44940 fixed the compile issues I was seeing when using this feature and a simple reqwest application on macOS and Windows.

Thoughts on privacy

I think this feature possesses the mechanics to more broadly address privacy issues for closed-source code. This may be out of scope for the current issue, but I wanted to bring it up now since this feature hasn't been fully implemented yet.

The current reality for closed source code is that any time a panic!() is used, the module name, line number, and column number will be leaked into the binary to support unwinding. This is true even if panic = 'abort' is specified. The root issue is that panic!() calls file!(). This could leak private, perhaps proprietary or sensitive, implementation details that the company may not want to be included in released binaries.

Example

Let's say a company creates a video game with some hidden content. If the source for that module included a panic!() (and note, even things like unreachable!() call panic!()), the module name could be leaked as a string:

src/hidden_dlc/extra_boss_fight/victors_betrayal.rs

Using this feature, the company now has a manual way to suppress this at compile time (cool!):

RUSTFLAGS=-Zremap-path-prefix-from=src/hidden_dlc/extra_boss_fight/victors_betrayal.rs -Zremap-path-prefix-to=

But note that this is very manual (need a -from/-to for each module), and you'd need to make sure you didn't forget modules.

Straw Man Proposal

If rustc let you suppress all uses of file!() at compile time, you could allow such companies to keep these sensitive implementation details out of the binaries. This also helps save on binary size if these strings are not needed.

RUSTFLAGS=-Zremap-path-prefix-from=* -Zremap-path-prefix-to=
@luser

This comment has been minimized.

Show comment
Hide comment
@luser

luser Feb 2, 2018

Contributor

What's the current status of this? It looks like it's been approved for stabilization, but the plan is to change the option to a single --remap-path-prefix. Does that change just need to be made, and then this can ship on stable?

Contributor

luser commented Feb 2, 2018

What's the current status of this? It looks like it's been approved for stabilization, but the plan is to change the option to a single --remap-path-prefix. Does that change just need to be made, and then this can ship on stable?

@jsgf

This comment has been minimized.

Show comment
Hide comment
@jsgf

jsgf Feb 14, 2018

Contributor

I've been using the -Zremap-path-prefix- form in our build environment (I failed to guess when they'd stabilize) for a few months now, and it's been working well.

Contributor

jsgf commented Feb 14, 2018

I've been using the -Zremap-path-prefix- form in our build environment (I failed to guess when they'd stabilize) for a few months now, and it's been working well.

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Feb 15, 2018

Contributor

Yes, it's just waiting to be implemented. I've just been busy with other things. I'm glad to take PRs though.

Contributor

michaelwoerister commented Feb 15, 2018

Yes, it's just waiting to be implemented. I've just been busy with other things. I'm glad to take PRs though.

@jsgf

This comment has been minimized.

Show comment
Hide comment
@jsgf

jsgf Feb 18, 2018

Contributor

OK, I'm looking at it now.

Contributor

jsgf commented Feb 18, 2018

OK, I'm looking at it now.

@kpcyrd

This comment has been minimized.

Show comment
Hide comment
@kpcyrd

kpcyrd Feb 19, 2018

As a minor note, it would be interesting to have both $HOME and $PWD remapped to standard values for release builds. This is currently the only thing that I have to adjust manually for reproducible builds (besides #47135).

kpcyrd commented Feb 19, 2018

As a minor note, it would be interesting to have both $HOME and $PWD remapped to standard values for release builds. This is currently the only thing that I have to adjust manually for reproducible builds (besides #47135).

@jsgf

This comment has been minimized.

Show comment
Hide comment
@jsgf

jsgf Feb 20, 2018

Contributor

They would only affect the build if you're explicitly referencing them with env!("HOME"), right? You could just do --remap-path-prefix=$PWD=./.

Contributor

jsgf commented Feb 20, 2018

They would only affect the build if you're explicitly referencing them with env!("HOME"), right? You could just do --remap-path-prefix=$PWD=./.

jsgf added a commit to jsgf/rust that referenced this issue Feb 20, 2018

Implement --remap-path-prefix
Remove experimental -Zremap-path-prefix-from/to, and replace it with
the stabilized --remap-path-prefix=from=to variant.

This is an implementation for issue of rust-lang#41555.

jsgf added a commit to jsgf/rust that referenced this issue Feb 20, 2018

Implement --remap-path-prefix
Remove experimental -Zremap-path-prefix-from/to, and replace it with
the stabilized --remap-path-prefix=from=to variant.

This is an implementation for issue of rust-lang#41555.

jsgf added a commit to jsgf/rust that referenced this issue Feb 22, 2018

Implement --remap-path-prefix
Remove experimental -Zremap-path-prefix-from/to, and replace it with
the stabilized --remap-path-prefix=from=to variant.

This is an implementation for issue of rust-lang#41555.

Manishearth added a commit to Manishearth/rust that referenced this issue Feb 28, 2018

Rollup merge of rust-lang#48359 - jsgf:remap-path-prefix, r=sanxiyn
Implement --remap-path-prefix

Remove experimental -Zremap-path-prefix-from/to, and replace it with
the stabilized --remap-path-prefix=from=to variant.

Implementation for rust-lang#41555
@kpcyrd

This comment has been minimized.

Show comment
Hide comment
@kpcyrd

kpcyrd May 9, 2018

@jsgf the binary includes my $CARGO_HOME into the binary by default, if I don't have that variable set it defaults to $HOME/.cargo. :)

I think my project root and my explicit or implicit cargo home should be remapped.

kpcyrd commented May 9, 2018

@jsgf the binary includes my $CARGO_HOME into the binary by default, if I don't have that variable set it defaults to $HOME/.cargo. :)

I think my project root and my explicit or implicit cargo home should be remapped.

@jsgf

This comment has been minimized.

Show comment
Hide comment
@jsgf

jsgf May 9, 2018

Contributor

@kpcyrd Well I guess that's an issue to be addressed in cargo.

Contributor

jsgf commented May 9, 2018

@kpcyrd Well I guess that's an issue to be addressed in cargo.

@kpcyrd

This comment has been minimized.

Show comment
Hide comment
@kpcyrd

kpcyrd May 10, 2018

@jsgf you're right, I've opened an issue for that: rust-lang/cargo#5505

kpcyrd commented May 10, 2018

@jsgf you're right, I've opened an issue for that: rust-lang/cargo#5505

@Aaronepower

This comment has been minimized.

Show comment
Hide comment
@Aaronepower

Aaronepower Jul 13, 2018

Contributor

Closing as this was stabilised in 1.26.0 and there doesn't seem to be any leftover features to be implemented.

Contributor

Aaronepower commented Jul 13, 2018

Closing as this was stabilised in 1.26.0 and there doesn't seem to be any leftover features to be implemented.

tylerwhall added a commit to tylerwhall/meta-rust that referenced this issue Jul 22, 2018

rust-common: fix remap-path-prefix for 1.26
Before the flag was stabilized, it required two arguments of the form
"from=<from_path>" and "to=<to_path>" respectively. The stabilized
version uses one argument of the form "<from_path>=<to_path>".

Unfortunately the old format is still parsed successfully, but results
in attempting to replace the literal paths "from" and "to".

rust-lang/rust#41555 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment