feat: implement RFC 3127 `-Ztrim-paths` #12625

weihanglo · 2023-09-06T15:06:55Z

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-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.
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 SourceId hash 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" and trim-paths = "none" but not trim-paths = ["all", "none"].
Support trim-paths = true/false.
Default value for dev and release profile.
Set CARGO_TRIM_PATHS environment 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-prefix works with rustdoc invocations (which rustdoc need an update)

rustbot · 2023-09-06T15:07:01Z

r? @ehuss

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

src/cargo/core/compiler/mod.rs

src/doc/src/reference/unstable.md

bors · 2023-09-09T02:45:09Z

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

ehuss · 2023-09-09T20:13:40Z

Do you want a review on this while it is in draft status?

weihanglo · 2023-09-12T13:53:31Z

I would wait for rustc side being reviewed.

bors · 2023-10-04T17:24:12Z

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

weihanglo · 2023-10-25T02:19:40Z

src/cargo/core/compiler/mod.rs

+        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);


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?

What's the correct remap prefix for sysroot

rustc --print=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.

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

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?

The desired behavior is mentioned in the RFC

https://rust-lang.github.io/rfcs/3127-trim-paths.html#handling-sysroot-paths

https://rust-lang.github.io/rfcs/3127-trim-paths.html#changing-handling-of-sysroot-path-in-rustc

To follow the RFC, yes, we might want to disable it in bootstrap.

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.

weihanglo · 2023-10-25T02:31:33Z

src/cargo/core/compiler/mod.rs

+        let mut remap = OsString::from("--remap-path-prefix=");
+        // Remapped to path relative to workspace root:
+        //
+        // * path dependencies under workspace root directory


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.

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.

weihanglo · 2023-10-25T03:15:37Z

tests/testsuite/profile_trim_paths.rs

+[RUNNING] [..]rustc [..]-Zremap-path-scope=diagnostics --remap-path-prefix=[..]/lib/rustlib/src/rust=/rustc/[..] --remap-path-prefix=[CWD]= [..]",
+        )
+        .run();
+}


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?

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.

weihanglo · 2023-10-25T03:30:39Z

This is ready for review. Not 100% complete but good for shipping as a nightly feature.

ehuss · 2023-10-30T18:32:15Z

src/cargo/core/compiler/mod.rs

+    if let Some(trim_paths) = trim_paths {
+        trim_paths_args(cmd, cx, unit, &trim_paths)?;
+    }


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.)

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'

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.

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.

ehuss · 2023-10-30T18:57:25Z

tests/testsuite/profile_trim_paths.rs

+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",


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.

Thanks for calling this out, and replaced this with with_stderr_line_without.

epage · 2023-10-30T20:13:15Z

src/cargo/util/toml/mod.rs

+    }
+}
+
+impl TomlTrimPaths {


nit: when i reorganized this file, I put all non-trait impl blocks first

Oh how I missed that. Fixed!

is there any rustfmt or clippy rules to enforce this?

epage

Besides anything Eric brings up, I'm good with merging this. Thanks for moving this along!

The rustc commit hash is required as we'll need it to remap sysroot path to `/rustc/<commit-hash>`, as rustc bootstrap does. See <https://github.com/rust-lang/rust/blob/c2ef3516/src/bootstrap/src/lib.rs#L1113-L1116>. This is optional as the compiler it may be built without a Git repository.

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.

ehuss · 2023-10-31T02:21:43Z

I haven't done a thorough review, but overall looks good to me.

@bors r=epage

bors · 2023-10-31T02:21:45Z

📌 Commit 63cef2c has been approved by epage

It is now in the queue for this repository.

bors · 2023-10-31T02:23:03Z

⌛ Testing commit 63cef2c with merge 4aee12c...

bors · 2023-10-31T03:18:37Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 4aee12c to master...

Update cargo 7 commits in 708383d620e183a9ece69b8fe930c411d83dee27..b4d18d4bd3db6d872892f6c87c51a02999b80802 2023-10-27 21:09:26 +0000 to 2023-10-31 18:19:10 +0000 - refactor(toml): Cleanup noticed on the way to rust-lang/cargo#12801 (rust-lang/cargo#12902) - feat(trim-paths): set env `CARGO_TRIM_PATHS` for build scripts (rust-lang/cargo#12900) - feat: implement RFC 3127 `-Ztrim-paths` (rust-lang/cargo#12625) - docs: clarify config to use vendored source is printed to stdout (rust-lang/cargo#12893) - Improve the margin calculation for the search command's UI (rust-lang/cargo#12890) - Add new packages to [workspace.members] automatically (rust-lang/cargo#12779) - refactor(toml): Decouple parsing from interning system (rust-lang/cargo#12881) r? ghost

rustbot assigned ehuss Sep 6, 2023

weihanglo added the Z-trim-paths Nightly: path sanitization label Sep 6, 2023

weihanglo force-pushed the rfc3127 branch 2 times, most recently from 5764df8 to 42377d2 Compare September 6, 2023 15:24

weihanglo mentioned this pull request Sep 6, 2023

Implement rustc part of RFC 3127 trim-paths rust-lang/rust#115214

Merged

Urgau reviewed Sep 6, 2023

View reviewed changes

src/cargo/core/compiler/mod.rs Outdated Show resolved Hide resolved

src/doc/src/reference/unstable.md Show resolved Hide resolved

weihanglo force-pushed the rfc3127 branch from 42377d2 to 7558706 Compare September 8, 2023 13:40

weihanglo force-pushed the rfc3127 branch from 7558706 to 8926897 Compare September 12, 2023 13:52

weihanglo force-pushed the rfc3127 branch 2 times, most recently from 6a63947 to b37054d Compare October 25, 2023 03:02

weihanglo commented Oct 25, 2023

View reviewed changes

weihanglo changed the title ~~[DRAFT] feat: implement RFC 3127~~ feat: implement RFC 3127 -Ztrim-paths Oct 25, 2023

weihanglo marked this pull request as ready for review October 25, 2023 03:29

weihanglo force-pushed the rfc3127 branch 4 times, most recently from 8862b05 to ae2a95e Compare October 25, 2023 21:05

weihanglo force-pushed the rfc3127 branch from c3449be to 739acb8 Compare October 29, 2023 03:34

ehuss reviewed Oct 30, 2023

View reviewed changes

weihanglo force-pushed the rfc3127 branch from 739acb8 to e281181 Compare October 30, 2023 19:55

epage reviewed Oct 30, 2023

View reviewed changes

epage approved these changes Oct 30, 2023

View reviewed changes

weihanglo added 6 commits October 30, 2023 16:38

feat(trim-paths): parsing in mainfest and config

4d29af1

test(trim-paths): parsing in mainfest and config

08c5e35

feat(trim-paths): default value for release profile

fd893b2

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.

test(trim-paths): verify current behavior with -Ztrim-paths

557fc4f

feat(trim-paths): rustc invocation integration

63cef2c

weihanglo force-pushed the rfc3127 branch from e281181 to 63cef2c Compare October 30, 2023 20:39

ehuss mentioned this pull request Oct 30, 2023

Tracking Issue for trim-paths RFC 3127 rust-lang/rust#111540

Open

9 tasks

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 Oct 31, 2023

bors merged commit 4aee12c into rust-lang:master Oct 31, 2023
20 checks passed

bors mentioned this pull request Oct 31, 2023

Add cache garbage collection #12634

Merged

weihanglo deleted the rfc3127 branch October 31, 2023 04:44

This was referenced Oct 31, 2023

feat(trim-paths): set env CARGO_TRIM_PATHS for build scripts #12900

Merged

Tracking Issue for trim-paths RFC 3127 #12137

Open

Update cargo rust-lang/rust#117462

Merged

ehuss added this to the 1.75.0 milestone Nov 6, 2023

johnthagen mentioned this pull request Nov 7, 2023

Local file path strings embedded in binary johnthagen/min-sized-rust#56

Open

RalfJung mentioned this pull request Nov 12, 2023

cargo miri setup no longer works for locally installed toolchains rust-lang/miri#3160

Closed

jieyouxu mentioned this pull request Aug 14, 2024

Tracking Issue for Reproducible Build bugs and challenges rust-lang/rust#129080

Open

81 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement RFC 3127 `-Ztrim-paths` #12625

feat: implement RFC 3127 `-Ztrim-paths` #12625

weihanglo commented Sep 6, 2023 •

edited

Loading

rustbot commented Sep 6, 2023

bors commented Sep 9, 2023

ehuss commented Sep 9, 2023

weihanglo commented Sep 12, 2023

bors commented Oct 4, 2023

weihanglo Oct 25, 2023

Urgau Oct 25, 2023

weihanglo Oct 25, 2023

Urgau Oct 25, 2023

weihanglo Oct 25, 2023

weihanglo Oct 26, 2023

weihanglo Oct 25, 2023 •

edited

Loading

epage Oct 26, 2023

weihanglo Oct 25, 2023

weihanglo Oct 26, 2023

weihanglo commented Oct 25, 2023

ehuss Oct 30, 2023

weihanglo Oct 30, 2023

ehuss Oct 30, 2023

Urgau Oct 30, 2023 •

edited

Loading

ehuss Oct 30, 2023

weihanglo Oct 30, 2023

epage Oct 30, 2023

weihanglo Oct 30, 2023

epage left a comment

ehuss commented Oct 31, 2023

bors commented Oct 31, 2023

bors commented Oct 31, 2023

bors commented Oct 31, 2023

feat: implement RFC 3127 -Ztrim-paths #12625

feat: implement RFC 3127 -Ztrim-paths #12625

Conversation

weihanglo commented Sep 6, 2023 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Things I am uncertain

Things to decide (but can postpone to follow-up PRs)

Additional information

rustbot commented Sep 6, 2023

bors commented Sep 9, 2023

ehuss commented Sep 9, 2023

weihanglo commented Sep 12, 2023

bors commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weihanglo Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weihanglo commented Oct 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Urgau Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage left a comment

Choose a reason for hiding this comment

ehuss commented Oct 31, 2023

bors commented Oct 31, 2023

bors commented Oct 31, 2023

bors commented Oct 31, 2023

feat: implement RFC 3127 `-Ztrim-paths` #12625

feat: implement RFC 3127 `-Ztrim-paths` #12625

weihanglo commented Sep 6, 2023 •

edited

Loading

weihanglo Oct 25, 2023 •

edited

Loading

Urgau Oct 30, 2023 •

edited

Loading