add unstable -Zroot-dir flag to configure the path from which rustc should be invoked #14752

RalfJung · 2024-10-30T21:42:02Z

This implements the proposal described here: we add a new flag, for now called -Zroot-dir, that configures the directory relative to which rustc is given the crate root filenames to build. (Files outside this directory are passed absolutely.)

This is necessary to be able to fix (no github don't close that issue yet) rust-lang/rust#128726: in multi-workspace repositories that use scripts to manage a whole bunch of cargo invocations, currently the output cargo+rustc produce is often hard or even impossible to interpret for both human and machine consumption. This is because directories in the output are always relative to the workspace root, but when cargo is invoked many times for different workspaces, it is quite unclear what the workspace root is for the invocation that failed.

So I suggest we should have a new flag that the build script in such a repo can set to the consistent "root dir" that the user would recognize as such (e.g., the root of the rustc source tree), and all paths emitted by cargo and rustc should be relative to that directory.

I don't know all the places that cargo itself emits paths (if any), but this PR changes the way we invoke rustc to honor the new flag, so all paths emitted by rustc will be relative to the -Zroot-dir.

See rust-lang/rust#132390 for the changes needed in rustc bootstrap to wire this up; together, that suffices to finally properly show errors in RA for all parts of the rustc src tree. :)

rustbot · 2024-10-30T21:42:07Z

r? @weihanglo

rustbot has assigned @weihanglo.
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

RalfJung · 2024-10-30T21:44:08Z

@epage indicated here that they'd be okay with unstable experimentation, so I hope this is sufficient from a procedural standpoint.

epage · 2024-10-30T21:54:28Z

@epage indicated #9887 (comment) that they'd be okay with unstable experimentation, so I hope this is sufficient from a procedural standpoint.

Something that isn't clear from the PR that was in my post is that I specifically called out a feature that is intended to be unstable only. Granted, I worry this will take the pressure off of fixing this for everyone, rather than a rustc-only fix.

src/cargo/core/features.rs

tests/testsuite/directory.rs

RalfJung · 2024-10-30T22:04:44Z

Something that isn't clear from the PR that was in my post is that I specifically called out a feature that is intended to be unstable only.

I think this will eventually be needed on stable: there's clearly a missing feature in cargo here (letting users control the relative paths emitted during the build, a feature that got removed in #4788). However, the most pressing need is fixing rust-lang/rust#128726 so that one can finally get proper RA feedback for rustc library work again (I am holding off writing certain PRs as it's just annoyingly painful to work on the library right now) -- and an unstable-only flag is enough for that. As you can see the patch is fairly simple, so it's hard to imagine a better solution for the immediate problem rustc has. If the alternative is having cargo rewrite rustc output to adjust the base paths are relative too (which will need a lot closer coordination between rustc and cargo than we have today to identify which part of the output is a path), then that will definitely be a lot more work; it also fundamentally alters the relationship between rustc and cargo in terms of how output is handled so I'm not convinced it is actually a better solution.

But anyway, if we have to label this a short-term unstable hack to make it land, I am fine with that. I want to get back to rustc development again, this patch is just meant to make that possible. You asked it to be "not be too invasive"; given the size of the patch I hope it qualifies.

Granted, I worry this will take the pressure off of fixing this for everyone, rather than a rustc-only fix.

I'm afraid I don't understand this sentence. Which part does "rather than" refer to?

RalfJung · 2024-10-31T06:30:09Z

bikeshed the name of the flag

I have now painted this in the -Zroot-dir color. That works best with the current flag description, I think:

"Set the root directory relative to which paths are printed (defaults to workspace root)"

RalfJung · 2024-10-31T07:06:54Z

Seems like the path prefix stripping is not working on Windows... not quite sure how to debug this, without access to a Windows machine. :/

RalfJung · 2024-10-31T07:17:00Z

oops I had forgotten to remove the canonicalize... let's hope that is enough for Windows.

epage · 2024-10-31T16:29:02Z

I think this will eventually be needed on stable: there's clearly a missing feature in cargo here

Something will eventually be needed in stable.

We need to find a way to improve the default of what gets rendered
If we make it further configurable from that default, it likely should live in config somehow (templated variable like we want to do with build/target dirs?)

…should be invoked

RalfJung · 2024-10-31T20:54:34Z

You seem to have a larger design space in mind here than I realized -- is that spelled out anywhere? #9887 doesn't have a lot of concrete ideas, the only ones I could find are

what is being implemented here
have cargo translate the output so rustc is always invoked from the workspace root but diagnostics are printed relative to the working dir cargo is invoked from

Anyway, as I said I am fine with this being unstable without a clear path to stabilization. But I think we have to do something to make rustc development more pleasant, and this is the most actionable path forward I can see. (Having cargo translate paths is quite complicated to get right and has some open design questions itself, like how cargo should know about paths embedded in the human-readable part of a diagnostic.)

I have done some git history surgery and I think it now has the commit structure you asked for. I also wrote the corresponding rustc patch and confirmed that this actually works -- I am finally seeing library errors in my IDE again. 🎉
So this should be ready for review.

epage · 2024-10-31T21:04:10Z

src/cargo/core/features.rs

@@ -783,6 +784,7 @@ unstable_cli_options!(
    profile_rustflags: bool = ("Enable the `rustflags` option in profiles in .cargo/config.toml file"),
    public_dependency: bool = ("Respect a dependency's `public` field in Cargo.toml to control public/private dependencies"),
    publish_timeout: bool = ("Enable the `publish.timeout` key in .cargo/config.toml file"),
+    root_dir: Option<PathBuf> = ("Set the root directory relative to which paths are printed (defaults to workspace root)"),


I'm sorry, I forgot to call out that we need an entry in https://doc.rust-lang.org/cargo/reference/unstable.html

You can find it in https://github.com/rust-lang/cargo/blob/master/src/doc/src/reference/unstable.md

I did my best at guessing the format and organization here, please have a look.

epage · 2024-10-31T21:08:07Z

You seem to have a larger design space in mind here than I realized -- is that spelled out anywhere? #9887 doesn't have a lot of concrete ideas, the only ones I could find are

Not really. I think it would be great for us to always show diagnostics relative to the current working directory. How we do that is less important (re-render diagnostics, add a flag to rustc to re-parent paths, etc).

If there remain use cases after that, then we can explore something else. People generally bring up a CLI for this but I think config is more relevant as I'd like us to reserve stuff for the CLI that is commonly used. The templating part is an idea I came up with on the fly as config can be as static or dynamic as the user wants and having a template helps for if the user wants something specific relative to something else.

epage · 2024-10-31T21:15:16Z

Thanks!

@bors r+

bors · 2024-10-31T21:15:19Z

📌 Commit 0f80faf has been approved by epage

It is now in the queue for this repository.

bors · 2024-10-31T21:16:38Z

⌛ Testing commit 0f80faf with merge 40d804b...

bors · 2024-10-31T21:51:09Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 40d804b to master...

Update cargo 18 commits in e75214ea4936d2f2c909a71a1237042cc0e14b07..0310497822a7a673a330a5dd068b7aaa579a265e 2024-10-25 16:34:32 +0000 to 2024-11-01 19:27:56 +0000 - Add more metadata to `rustc_fingerprint` (rust-lang/cargo#14761) - test(rustfix): switch to a simpler case for dedup-suggestions (rust-lang/cargo#14765) - chore(deps): update rust crate security-framework to v3 (rust-lang/cargo#14766) - chore(deps): update rust crate gix to 0.67.0 (rust-lang/cargo#14762) - fix(util): Respect all `..`s in `normalize_path` (rust-lang/cargo#14750) - test(doc): Resolve flaky test (rust-lang/cargo#14760) - refactor(test): Remove dead 'expect_stdout_contains_n' check (rust-lang/cargo#14759) - add unstable -Zroot-dir flag to configure the path from which rustc should be invoked (rust-lang/cargo#14752) - docs(resolver): Further v3 prep (rust-lang/cargo#14753) - fix: track version in fingerprint dep-info files (rust-lang/cargo#14751) - test: Remove unused msrv-policy (rust-lang/cargo#14748) - download targeted transitive deps of with artifact deps' target platform (rust-lang/cargo#14723) - Remove requirement for --target when invoking Cargo with -Zbuild-std (rust-lang/cargo#14317) - docs(fingerprint): document the encoding of Cargo's depinfo (rust-lang/cargo#14745) - Allow build scripts to report error messages through `cargo::error` (rust-lang/cargo#14743) - fix(publish): Downgrade version-exists error to warning on dry-run (rust-lang/cargo#14742) - fix: clean up for deprecated and removed commands (rust-lang/cargo#14739) - Deprecate `cargo verify-project` (rust-lang/cargo#14736)

rustbot assigned weihanglo Oct 30, 2024

rustbot added A-unstable Area: nightly unstable support A-workspaces Area: workspaces S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2024

RalfJung force-pushed the rustc-root-path branch from 146cc8a to a3932ea Compare October 30, 2024 21:43

RalfJung mentioned this pull request Oct 30, 2024

Cargo should print appropriate relative paths when being run from a non-root folder #9887

Open

RalfJung force-pushed the rustc-root-path branch from a3932ea to cdd0060 Compare October 30, 2024 21:53

RalfJung commented Oct 30, 2024

View reviewed changes

src/cargo/core/features.rs Outdated Show resolved Hide resolved

epage reviewed Oct 30, 2024

View reviewed changes

src/cargo/core/features.rs Outdated Show resolved Hide resolved

epage reviewed Oct 30, 2024

View reviewed changes

tests/testsuite/directory.rs Outdated Show resolved Hide resolved

epage reviewed Oct 30, 2024

View reviewed changes

tests/testsuite/directory.rs Show resolved Hide resolved

RalfJung force-pushed the rustc-root-path branch from cdd0060 to d21bf01 Compare October 31, 2024 06:25

RalfJung force-pushed the rustc-root-path branch 2 times, most recently from ad2636d to d488912 Compare October 31, 2024 06:48

This was referenced Oct 31, 2024

bootstrap: show diagnostics relative to rustc src dir rust-lang/rust#132390

Open

Rust-analyzer + rustc repo: build failures in the standard library are not shown in the editor any more rust-lang/rust#128726

Open

RalfJung force-pushed the rustc-root-path branch 2 times, most recently from dbe5d5c to 2072be5 Compare October 31, 2024 07:09

RalfJung force-pushed the rustc-root-path branch from 2072be5 to e452dd6 Compare October 31, 2024 07:17

RalfJung changed the title ~~add unstable -Zroot-path flag to configure the path from which rustc should be invoked~~ add unstable -Zroot-dir flag to configure the path from which rustc should be invoked Oct 31, 2024

RalfJung force-pushed the rustc-root-path branch from e452dd6 to 3cf582b Compare October 31, 2024 08:37

add tests in preparation for root-dir flag that changes their behavior

573ff00

add unstable -Zroot-path flag to configure the path from which rustc …

c2be327

…should be invoked

RalfJung force-pushed the rustc-root-path branch from 3cf582b to c2be327 Compare October 31, 2024 20:36

epage reviewed Oct 31, 2024

View reviewed changes

add unstable feature docs

0f80faf

rustbot added the A-documenting-cargo-itself Area: Cargo's documentation label Oct 31, 2024

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, 2024

bors merged commit 40d804b into rust-lang:master Oct 31, 2024
20 checks passed

This was referenced Oct 31, 2024

Lint against unexpected cfgs in [target.'cfg(...)'] #14581

Open

Implement RFC3695: Allow boolean literals as cfg predicates #14649

Open

RalfJung deleted the rustc-root-path branch November 1, 2024 06:19

weihanglo mentioned this pull request Nov 1, 2024

Update cargo rust-lang/rust#132475

Merged

rustbot added this to the 1.84.0 milestone Nov 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add unstable -Zroot-dir flag to configure the path from which rustc should be invoked #14752

add unstable -Zroot-dir flag to configure the path from which rustc should be invoked #14752

RalfJung commented Oct 30, 2024 •

edited

Loading

rustbot commented Oct 30, 2024

RalfJung commented Oct 30, 2024

epage commented Oct 30, 2024

RalfJung commented Oct 30, 2024 •

edited

Loading

RalfJung commented Oct 31, 2024

RalfJung commented Oct 31, 2024

RalfJung commented Oct 31, 2024

epage commented Oct 31, 2024

RalfJung commented Oct 31, 2024 •

edited

Loading

epage Oct 31, 2024

RalfJung Oct 31, 2024

epage commented Oct 31, 2024

epage commented Oct 31, 2024

bors commented Oct 31, 2024

bors commented Oct 31, 2024

bors commented Oct 31, 2024

add unstable -Zroot-dir flag to configure the path from which rustc should be invoked #14752

add unstable -Zroot-dir flag to configure the path from which rustc should be invoked #14752

Conversation

RalfJung commented Oct 30, 2024 • edited Loading

rustbot commented Oct 30, 2024

RalfJung commented Oct 30, 2024

epage commented Oct 30, 2024

RalfJung commented Oct 30, 2024 • edited Loading

RalfJung commented Oct 31, 2024

RalfJung commented Oct 31, 2024

RalfJung commented Oct 31, 2024

epage commented Oct 31, 2024

RalfJung commented Oct 31, 2024 • edited Loading

epage Oct 31, 2024

Choose a reason for hiding this comment

RalfJung Oct 31, 2024

Choose a reason for hiding this comment

epage commented Oct 31, 2024

epage commented Oct 31, 2024

bors commented Oct 31, 2024

bors commented Oct 31, 2024

bors commented Oct 31, 2024

RalfJung commented Oct 30, 2024 •

edited

Loading

RalfJung commented Oct 30, 2024 •

edited

Loading

RalfJung commented Oct 31, 2024 •

edited

Loading