feat: implement RFC 3127 -Ztrim-paths by weihanglo · Pull Request #12625 · rust-lang/cargo (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation72 Commits8 Checks0 Files changed
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
What does this PR try to resolve?
Implement RFC 3127 trim paths on Cargo side. The counterpart of the compiler is in rust-lang/rust#115214. The tracking issue for Cargo is #12137.
How should we test and review this PR?
By commits. I would recommend reading the doc first to get the an overview on this.
Things I am uncertain
- The correct path prefix for sysroot remap. See feat: implement RFC 3127 -Ztrim-paths #12625 (comment).
- @Urgau has pointed out the remapping of sysroot has already been done by rustc bootstrap here. Changing that may require
-Ztrim-pathsbeing stabilized, and may leak CI details into all pre-built std, core…. This is something requiring to collaborate withT-bootstrap. We'll postpone it for this pull request.
- @Urgau has pointed out the remapping of sysroot has already been done by rustc bootstrap here. Changing that may require
- Do we want to verify the final artifact is actually sanitized? See feat: implement RFC 3127 -Ztrim-paths #12625 (comment)
- Current we're satisfied with testing only on Linux.
- For other platforms we verify Cargo passing in correct arguments to rustc.
Things to decide (but can postpone to follow-up PRs)
- Is remapping dependencies to
<pkg>-<version>good enough for all source kinds?- How to tell if a package is from Git or registry? IDE may wants that information?
- Should we use
SourceIdhash or something instead?
- The remap of workspace was not clear in the RFC. This PR choses to remap from workspace root instead. See feat: implement RFC 3127 -Ztrim-paths #12625 (comment)
Additional information
Things to do:
- Support
trim-paths = "all"andtrim-paths = "none"but nottrim-paths = ["all", "none"]. - Support
trim-paths = true/false. - Default value for
devandreleaseprofile. - Set
CARGO_TRIM_PATHSenvironment variable for build scripts. (feat(trim-paths): set env CARGO_TRIM_PATHS for build scripts #12900) - Fingerprint should take trim-paths settings into account. (already done here, and this test demonstrates fingerprint gets dirty)
- Make
--remap-path-prefixworks withrustdocinvocations (whichrustdocneed an update)
r? @ehuss
(rustbot has picked a reviewer for you, use r? to override)
Do you want a review on this while it is in draft status?
I would wait for rustc side being reviewed.
| let commit_hash = &cx.bcx.rustc().commit_hash; |
|---|
| let mut remap = OsString::from("--remap-path-prefix="); |
| // TODO(trim-paths): what is the correct prefix for sysroot? does rustup affect this? |
| remap.push(sysroot); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the correct remap prefix for sysroot, and how to setup a test case for it?
I've checked rustup, Debian, and Fedora. All of them put the rust-src under [sysroot]/lib/rustlib/src/rust. From what I can tell, -Zbuild-std is also hardcoded with this path.
@Urgau do you have any insight?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the correct remap prefix for sysroot
and how to setup a test case for it?
Well you could have a Rust fn main() { panic!("something"); } with RUST_BACKTRACE=1 and see the resulting backtrace and assert that it doesn't contain the un-remapped sysroot.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip!
Unfortunately I've tried panic! backtrace and it has already remapped even without -Ztrim-paths 😞.
0: std::panicking::begin_panic
at /rustc/cd674d61790607dfb6faa9d754bd3adfa13aea7c/library/std/src/panicking.rs:638:12
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, the mapping is done by bootstrap here, I wonder if we should remove/disable it since Cargo will now do it.
Or maybe instead of disabling it you could do the inverse and demap in debug?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me and Urgau had a chat on this. Urgau pointed out changing the bootstrap this may require -Ztrim-paths being stabilized, and may leak CI details into all pre-built std, core…. This is something requiring to collaborate with T-bootstrap. We'll postpone it for this pull request.
| let mut remap = OsString::from("--remap-path-prefix="); |
|---|
| // Remapped to path relative to workspace root: |
| // |
| // * path dependencies under workspace root directory |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RFC only mentions about mapping the current package, which doesn't take workspaces into account:
For the the current package (where the current working directory is in), from the the absolute path of the package root to empty string. For other packages, from the absolute path of the package root to
[package name]-[package version].
Here we expand the RFC a bit: always remap from the workspace root to empty string. When dealing with compilations, Cargo always handle the workspace as a whole instead of a single member. We might not want an exception for -Ztrim-paths.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, the main question of whether to be relative to workspace or package root is if there'd be ambiguity. Since we have name+version, I'm guessing not.
| [RUNNING] [..]rustc [..]-Zremap-path-scope=diagnostics --remap-path-prefix=[..]/lib/rustlib/src/rust=/rustc/[..] --remap-path-prefix=[CWD]= [..]", |
|---|
| ) |
| .run(); |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to set up a smoke test against debug symbol, so that we can make sure they are sanitized. I can follow what rustc does by using objdump. Is there any better way to verify it? What about cross-platform tests?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ended up using readelf. Now we have covered Linux for object option. I think that's pretty acceptable at this stage. We can add more tests for macOS and Windows later.
[DRAFT] feat: implement RFC 3127 feat: implement RFC 3127 -Ztrim-paths
This is ready for review. Not 100% complete but good for shipping as a nightly feature.
Comment on lines +1033 to +1035
| if let Some(trim_paths) = trim_paths { |
|---|
| trim_paths_args(cmd, cx, unit, &trim_paths)?; |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also be included in rustdoc to handle the diagnostics stripping (and same with doctests)?
(Though I still don't 100% understand the purpose of diagnostic remapping.)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is such an option for rustdoc. Did I miss something?
Maybe this could be put in "unresolved questions" in RFC 3127 tracking issue.
$ rustdoc +nightly -vV rustdoc 1.75.0-nightly (608e9682f 2023-10-29) binary: rustdoc commit-hash: 608e9682f0a6482903de8d3332770104a0ad943c commit-date: 2023-10-29 host: aarch64-apple-darwin release: 1.75.0-nightly LLVM version: 17.0.3
$ rustc --remap-path-prefix error: Argument to option 'remap-path-prefix' missing
$ rustdoc --remap-path-prefix error: Unrecognized option: 'remap-path-prefix'
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, leaving that as an open issue sounds good. The rustdoc argument parser would just need to be updated.
Though, before that I would like to better understand why the diagnostic option exists and when someone would want to turn it on. I can't really follow the discussion from rust-lang/rust#87745 and how that relates to cargo. If we end up not stabilizing "diagnostic" as an option, then I don't have any motivation to do that.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the diagnostics option I think it's related to build-systems that create VFS (Virtual File System) or temporary location when building, throwing the all diagnostics path to the wrong location.
Comment on lines 399 to 405
| warning: unused variable: `unused` |
|---|
| --> bar-0.0.1/src/lib.rs:1:18 |
| | |
| 1 | pub fn f() { let unused = 0; } |
| | ^^^^^^ help: if this is intentional, prefix it with an underscore: `_unused` |
| | |
| = note: `#[warn(unused_variables)]` on by default", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests can't match on the compiler's output exactly, since it changes too often, and makes it impossible to change (since it would require simultaneously changing both rust-lang/rust and rust-lang/cargo).
I think you can do something like with_stderr_contains("[..]bar-0.0.1/src/lib.rs:1[..]") to just check the important part.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for calling this out, and replaced this with with_stderr_line_without.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: when i reorganized this file, I put all non-trait impl blocks first
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh how I missed that. Fixed!
is there any rustfmt or clippy rules to enforce this?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides anything Eric brings up, I'm good with merging this. Thanks for moving this along!
trim-paths is shown as disabled as default in Debug impl.
Although this doesn't reflect the correct default for -Ztrim-pthas,
this is no critical as it is only for debugging,
and it's a bit tricky to make it more correct.
11 tasks
I haven't done a thorough review, but overall looks good to me.
@bors r=epage
📌 Commit 63cef2c has been approved by epage
It is now in the queue for this repository.
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
This was referenced
Oct 31, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request