-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cargo should print appropriate relative paths when being run from a non-root folder #9887
Comments
Can you clarify, I don't think this is specific to workspaces? That is, if you go to the |
Yeah, you're right, this isn't actually related to workspaces |
We could use -Zremap-cwd-prefix here: rust-lang/rust#87320 |
I'm not sure remap-cwd-prefix helps, but perhaps an example, to make sure I am understanding. crate root: /home/foo/mycrate What But it would not do anything with other paths rooted under /home/foo/mycrate. You would need to |
Yeah it doesn't, i just realized: remap-cwd-prefix affects debuginfo, which is not good for build caching remap-path-prefix has the same problem |
Right. It's good for build caching if you're replacing something variable (an abs path) with something fixed ( |
Say you do
Telling rustc where it is invoked from would require rustc to be run twice in this scenario. This is not a theoretical scenario as this exact thing will happen if you have the workspace open in your editor with |
Why so? The flag is excluded from build caching. |
But then the diagnostics would be outdated when you switch directory as cargo caches the old diagnostics. |
hmmm, interesting point that's only for successful builds, though. I wonder if there's something that can be done here (Perhaps cargo should be operating on the json output for cached messages) |
This behavior is particularly confusing for cases like rustc's |
I don't know for sure what |
Relative paths to CWD seem best, users invoking |
Looking at the bootstrap code, it does seem to use
So if cargo could "just" print the error messages with files relative to the CWD it was invoked in, rather than relative to the root of the workspace it works in, that would be great. :) |
I'd like to second this. The reasonable thing to expect is that compilers and build tools report errors relative to the directory they were invoked from. This issue is still breaking any tool that doesn't have special provisions for cargo. I have trouble to understand why #4788 was accepted in the first place and not reverted despite warnings (#4998). Correct behavior was traded in for an optimization of the build process. How cargo works internally should not matter for how error messages are displayed. |
This now affects the standard library in the rustc repo, see rust-lang/rust#128726: cargo reports errors as
omitting the leading |
Seems like cargo does not have any understanding of there being paths in the error messages that it renders. So there's only really two options:
However, in both cases we'd effectively revert #4788. I don't know how common "I need my build cache to survive moving to a different location" really is; the stated motivation of CI doesn't apply to CI services I used AFAIK -- those always use the same path (inside some container). Or maybe we can make rustc emit a placeholder, like |
Some CI services do assign random path to a project, and reuse build artifacts. This one #10915 is related to |
Maybe we can for now have a nightly-only flag that makes cargo use an absolute path when invoking rustc, and have |
Or we could have a flag to make cargo invoke rustc N directories up from where it would normally invoke it. So for example this flag could be set to 1 for library/ and 2 for compiler/rustc_codegen_cranelift to invoke rustc in the root of the workspace. I think we could even implement this in the rustc wrapper used by bootstrap without needing cargo changes. |
@bjorn3 or maybe cargo could just have a flag telling it which cwd to use. |
How does this revert #4788
Can |
Wouldn't we hit this problem because rustc output depends on that flag so either changing the flag invalidates the cache, or moving the cache leads to output with outdated paths?
Doesn't rustc already resolve all of these relative to the file they occur in, not the rustc cwd? After all, before #4788 things also worked. |
I think the value of the flag should be required to be a prefix of the workspace root. Then the cache key can be the path of the workspace root relative to the value given to the flag. This will avoid invalidation when moving the directory pointed to by the flag, while ensuring that the cache is invalidated when moving the workspace relative to the directory pointed to by the flag.
If you pass an absolute path as source file for the crate root to rustc, diagnostics and |
Ah I mixed up
Basically, around here, instead of stripping @weihanglo @epage does that sound like a design worth experimenting with? Does anyone here want to try to implement this? :) |
Could you fully summarize the proposal to avoid misunderstanding each other in which parts of this thread are relevant? Also, for myself at least, this is the bottom of the priority list. We have a stable-to-stable regression for almost a week without progress, pressure for a beta backport for Edition 2024, and all of the work I've had to put off because of Edition 2024, vacation, or emergencies. A cosmetic issue that we've been able to live with for a while is low on my priority list. |
The proposal would be to add a new nightly-only flag to cargo that sets the directory relative to which paths in diagnostics (and all other paths emitted by rustc, like
(I don't know what cargo's usual approach to nightly-only flags is, hence I am taking the A prior proposal was to have a flag that sets the number of directories that should be popped from the workspace root to compute this root path, but to me the variant that just sets the path seems easier to understand. It is probably also easier to use for
To be clear, I put the request for implementers in a separate paragraph because I didn't want it to be part of the ping address cargo maintainers. :) I just want to be sure that if someone shows up with a patch, the maintainers are okay with the general direction this is taking.
Damn, that's a lot. Thanks for all your hard work and good luck with the regression! 💛 🍀
Note however that the issue recently got a lot worse due to a change in how rustc is organized into workspaces, see rust-lang/rust#128726: it used to only affect bootstrap and the alternative codegen backends, now it affects the entire standard library. Anyone using vscode to work on rustc no longer gets errors and warnings properly displayed in the library sources. I also would call this more than just a "cosmetic" issue, since it breaks fundamental IDE functionality. |
I opened a potential workaround for this in Clippy - rust-lang/rust-clippy#13232 It passes |
I'm even talking from a design discussion perspective.
by "we've been able to live with", I meant the rust community, not rustc development. |
If this is not intended for the path to stabilization and is a "short term" hack to improve the rustc experience, the bar is significantly lower. I would prefer such a short term solution to not be too invasive. |
@Alexendoo that's an interesting approach! Maybe we should try this in
So far rustc devs could also live with it, but that's much less true now. Whether the Rust community can live with the Rust compiler having a poor developer experience, I leave for others to decide. I would hope that, since cargo and rustc are sister projects under the Rust project umbrella, the sub-projects are willing to talk and listen to each other and consider themselves as part of the same larger whole. |
That trick based on |
Sadly So we're back to likely needing something like this. |
Currently, if you run
cargo
commands from a place that is not the root (workspace root for workspace projects), it does (almost1) the same thing as if you had runcargo <foo> -p packagename
from the workspace root.This means that the diagnostics also assume you are at the workspace root:
It would be nice if cargo could tell rustc where it is being invoked from, so that rustc may print appropriate paths. A lot of IDEs and terminals have the ability to click on paths in compiler output to open files; and it's frustrating that this only works if you
cargo build -p package
from the workspace root.1 I believe there are some slight differences as to which dependencies get built when you call
cargo build -p foo
from the root vs a folder that depends onfoo
See also
The text was updated successfully, but these errors were encountered: