refactor: use Path::push to construct remap-path-prefix #14908

weihanglo · 2024-12-09T00:08:33Z

What does this PR try to resolve?

It creates paths with correct separators for different systems.

How should we test and review this PR?

Try out -Ztrim-paths on Windows. Run cargo build --verbose and see if slashes in --remap-path-prefix use the system's path separators.

cargo-features = ["trim-paths"]

[package]
name = "foo"

[profile.release]trim-paths = true

Additional information

rustbot · 2024-12-09T00:08:38Z

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

weihanglo · 2024-12-09T00:26:07Z

src/cargo/core/compiler/mod.rs

@@ -1313,7 +1313,11 @@ fn sysroot_remap(build_runner: &BuildRunner<'_, '_>, unit: &Unit) -> OsString {
    let sysroot = &build_runner.bcx.target_data.info(unit.kind).sysroot;
    let mut remap = OsString::from("--remap-path-prefix=");
    remap.push(sysroot);
-    remap.push("/lib/rustlib/src/rust"); // See also `detect_sysroot_src_path()`.
+    // See also `detect_sysroot_src_path()`
+    remap.push("lib");


Oops I forgot it is a OsString

src/cargo/core/compiler/mod.rs

It creates paths with correct separators for different systems.

ehuss · 2024-12-09T13:57:10Z

Can you say more about what this is fixing? Is path remapping just not working on Windows for sysroot paths? Do we not have a test that would catch that?

weihanglo · 2024-12-09T14:14:36Z

Can you say more about what this is fixing? Is path remapping just not working on Windows for sysroot paths? Do we not have a test that would catch that?

We don't have any test for windows now, though path remapping works on Windows.
I just got paranoid about it is not using the correct \ separator and am afraid of #13919 happening if rustc --print sysroot starts printing one.

ehuss

Ok, I'll go ahead and merge. I just wanted to comment on a few things:

It would be good to eventually have some Windows tests.
Without trim-paths, diagnostic paths look like C:\Users\eric\.rustup\toolchains\nightly-aarch64-pc-windows-msvc\lib/rustlib/src/rust\library\core\src\hash\mod.rs. With trim-paths (with or without this PR), they get remapped to /rustc/4d669fb34e7db6f3825d01e4c59b7996f0531431\library\core\src\hash\mod.rs. Is rustc maybe doing some backslash/forwardslash translation?
I don't see rustlib absolute paths in the .exe with trim-paths turned off. I'm not sure if there are scenarios where that might happen?
I do see absolute paths in the .pdb file. With or without this PR, they are still there, so I assume Windows debug stripping is either not implemented, or broken, or fundamentally not feasible. They look to always use backslashes. For example, C:\Users\eric\.rustup\toolchains\nightly-aarch64-pc-windows-msvc\lib\rustlib\aarch64-pc-windows-msvc\lib\libstd-42ec610576778e0a.rlib.

weihanglo · 2024-12-09T18:40:09Z

I do see absolute paths in the .pdb file. With or without this PR, they are still there

Yes I noticed it yesterday as well and posted a comment: rust-lang/rust#111540 (comment).

The entire stuff on Windows is still a new thing to me. Sorry off the top of my head I don't have any useful answers for you now :(

I'll add them to unresolved part respectively

Update cargo 18 commits in 20a443231846b81c7b909691ec3f15eb173f2b18..7847c03965260b5dcc8d93218d6af295a717abb6 2024-12-06 21:56:56 +0000 to 2024-12-13 18:06:39 +0000 - fix(base): Support bases in patches in virtual manifests (rust-lang/cargo#14931) - fix(resolver): Report invalid index entries (rust-lang/cargo#14927) - feat: Implement `--depth workspace` for `cargo tree` command (rust-lang/cargo#14928) - fix(resolver): In errors, show rejected versions over alt versions (rust-lang/cargo#14923) - fix: emit_serialized_unit_graph uses the configured shell (rust-lang/cargo#14926) - fix(script): Don't override the release profile (rust-lang/cargo#14925) - feature(SourceId): use stable hash from rustc-stable-hash (rust-lang/cargo#14917) - fix(resolver): Don't report all versions as rejected (rust-lang/cargo#14921) - fix(resolver): Report unmatched versions, rather than saying no package (rust-lang/cargo#14897) - fix(build-rs): Implicitly report rerun-if-env-changed for input (rust-lang/cargo#14911) - a faster hash for ActivationsKey (rust-lang/cargo#14915) - feat(build-script): Pass CARGO_CFG_FEATURE (rust-lang/cargo#14902) - fix(build-rs): Correctly refer to the item in assert (rust-lang/cargo#14913) - chore: update auto-label to include build-rs crate (rust-lang/cargo#14912) - refactor: use Path::push to construct remap-path-prefix (rust-lang/cargo#14908) - feat(build-rs): Add the 'error' directive (rust-lang/cargo#14910) - fix(build-std): determine root crates by target spec `std:bool` (rust-lang/cargo#14899) - SemVer: Add section on RPIT capturing (rust-lang/cargo#14849)

Update cargo 19 commits in 20a443231846b81c7b909691ec3f15eb173f2b18..769f622e12db0001431d8ae36d1093fb8727c5d9 2024-12-06 21:56:56 +0000 to 2024-12-14 04:27:35 +0000 - test(build-std): dont require rustup (rust-lang/cargo#14933) - fix(base): Support bases in patches in virtual manifests (rust-lang/cargo#14931) - fix(resolver): Report invalid index entries (rust-lang/cargo#14927) - feat: Implement `--depth workspace` for `cargo tree` command (rust-lang/cargo#14928) - fix(resolver): In errors, show rejected versions over alt versions (rust-lang/cargo#14923) - fix: emit_serialized_unit_graph uses the configured shell (rust-lang/cargo#14926) - fix(script): Don't override the release profile (rust-lang/cargo#14925) - feature(SourceId): use stable hash from rustc-stable-hash (rust-lang/cargo#14917) - fix(resolver): Don't report all versions as rejected (rust-lang/cargo#14921) - fix(resolver): Report unmatched versions, rather than saying no package (rust-lang/cargo#14897) - fix(build-rs): Implicitly report rerun-if-env-changed for input (rust-lang/cargo#14911) - a faster hash for ActivationsKey (rust-lang/cargo#14915) - feat(build-script): Pass CARGO_CFG_FEATURE (rust-lang/cargo#14902) - fix(build-rs): Correctly refer to the item in assert (rust-lang/cargo#14913) - chore: update auto-label to include build-rs crate (rust-lang/cargo#14912) - refactor: use Path::push to construct remap-path-prefix (rust-lang/cargo#14908) - feat(build-rs): Add the 'error' directive (rust-lang/cargo#14910) - fix(build-std): determine root crates by target spec `std:bool` (rust-lang/cargo#14899) - SemVer: Add section on RPIT capturing (rust-lang/cargo#14849)

rustbot assigned ehuss Dec 9, 2024

rustbot added A-build-execution Area: anything dealing with executing the compiler S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2024

weihanglo commented Dec 9, 2024

View reviewed changes

Muscraft reviewed Dec 9, 2024

View reviewed changes

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

weihanglo force-pushed the remap branch from 1cdf37a to 4159cfa Compare December 9, 2024 02:30

Muscraft reviewed Dec 9, 2024

View reviewed changes

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

refactor: use Path::push to construct remap-path-prefix

4661f9b

It creates paths with correct separators for different systems.

weihanglo force-pushed the remap branch from 4159cfa to 4661f9b Compare December 9, 2024 04:35

ehuss approved these changes Dec 9, 2024

View reviewed changes

ehuss added this pull request to the merge queue Dec 9, 2024

weihanglo added the Z-trim-paths Nightly: path sanitization label Dec 9, 2024

weihanglo mentioned this pull request Dec 9, 2024

Tracking Issue for trim-paths RFC 3127 #12137

Open

8 tasks

Merged via the queue into rust-lang:master with commit 2f74b54 Dec 9, 2024
20 checks passed

weihanglo mentioned this pull request Dec 9, 2024

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

Open

11 tasks

weihanglo deleted the remap branch December 9, 2024 19:09

weihanglo mentioned this pull request Dec 13, 2024

Update cargo rust-lang/rust#134278

Merged

rustbot added this to the 1.85.0 milestone Dec 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use Path::push to construct remap-path-prefix #14908

refactor: use Path::push to construct remap-path-prefix #14908

weihanglo commented Dec 9, 2024 •

edited

Loading

rustbot commented Dec 9, 2024

weihanglo Dec 9, 2024

ehuss commented Dec 9, 2024

weihanglo commented Dec 9, 2024

ehuss left a comment

weihanglo commented Dec 9, 2024

refactor: use Path::push to construct remap-path-prefix #14908

refactor: use Path::push to construct remap-path-prefix #14908

Conversation

weihanglo commented Dec 9, 2024 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Dec 9, 2024

weihanglo Dec 9, 2024

Choose a reason for hiding this comment

ehuss commented Dec 9, 2024

weihanglo commented Dec 9, 2024

ehuss left a comment

Choose a reason for hiding this comment

weihanglo commented Dec 9, 2024

weihanglo commented Dec 9, 2024 •

edited

Loading