fix(trim-paths): custom workspace-relative member paths remap - #17366
fix(trim-paths): custom workspace-relative member paths remap#17366weihanglo wants to merge 3 commits into
Conversation
|
r? @epage rustbot has assigned @epage. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
I don't particularly like this, but seems like inevitable as we already opened the |
I'm not certain about that. I would expect users who distribute binaries and libraries to only want remapped paths, not a mix of absolute remapped paths but relative paths are not. |
|
For example if I distribute a Rust software with Like is the case with |
|
I see. So it is not just an extra prefix that a symbolication pipeline or symbol server can handle with one more entry. It is a dual mode (relative paths vs. virtual prefix) that consumers need to deal with separately. My original idea of keeping it relative is for an easier debugging experience that people mostly debug their own application code. Without a virtual prefix for local there is nothing to configure in the debugger. Anyway, valid point. Let's record it as an unresolved question and ask users during the call for testing. |
| (from, to) | ||
| } | ||
|
|
||
| /// Custom remap prefix for workspace members in rustc bootstrap not at workspace root. |
There was a problem hiding this comment.
I'm finding this PR confusing. This is for rustc bootstrap's special mode but I'm not seeing how the new code path ties to it.
I suspect this is too general, making that relationship unclear.
I also feel like the descriptions here and the PR assume enough knowledge that the intended effect is unclear.
There was a problem hiding this comment.
Thanks for the review! Updated the PR description with more contexts. Let me know if it is good enough.
For this feature, I intentionally made it a bit more general, as it may become an (unstable) config for people to customize it. See Urague's comment above #17366 (comment).
This is effectively `library/=<to>/library` and `compiler/=<to>/compiler` in rustc bootstrap, when `__CARGO_RUSTC_BOOTSTRAP_WS_REMAP` is set. This isn't needed for normal cases because relative member paths are exactly what users want. See <rust-lang/rust#161049 (comment)>
What does this PR try to resolve?
This adds the missing rustc bootstrap custom remap prefix
for packages relative to the working directory.
Cargo's workspace remap rule was pretty simple:
everything under the working directory1 becomes relative to the working directory
(like
strip_prefix(root)).For supporting rustc bootstrap remaps (#17309),
we added specific env var
__CARGO_RUSTC_BOOTSTRAP_WS_REMAPto set custom prefix for workspace remap rule.
That covers absolute paths starting with `
Unfortunately,
it didn't cover paths already relative to the working directory.
The reason is that
Cargo already passes workspace member paths relative to the working directory.
For example,
the path rustc saw was already relative like
library/std/src/lib.rs.The path never matches the remap prefix
<ws-root>so cannot be remapped by Cargo's
-Ztrim-paths.Below are incorrect and expected diagnostics:
This PR extends
__CARGO_RUSTC_BOOTSTRAP_WS_REMAPto additionally add a remap rule for everything relative to workspace.
The new remap rule joisn the prefix with the relative path,
so
library/std/src/lib.rsbecomes/<custom-prefix>/library/std/src/lib.rs.Full comparsion:
-Ztrim-pathsbefore this-Ztrim-pathsafter this<root>->/rustc/<sha><root>->/rustc/<sha><root>→/rustc/<sha>library/->/rustc/<sha>/library/library/->library/(no remap at all)library/->/rustc/<sha>/librarySee rust-lang/rust#161049 (comment)
Part of #17309
How to test and review this PR?
cc @Urgau
🤖 LLM disclosure: Used for verifying it actually works with rustc bootstrap, which has been signed off on rust-lang/rust side (rust-lang/rust#161049).
Footnotes
by default it is Cargo workspace root but rustc bootstrap use
-Zroot-dirto set the working directory always to the repo root (or checkout out, whatever you like, it is the directory that has the x.py) ↩