Skip to content
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

Open
Manishearth opened this issue Sep 8, 2021 · 42 comments
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@Manishearth
Copy link
Member

Manishearth commented Sep 8, 2021

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 run cargo <foo> -p packagename from the workspace root.

This means that the diagnostics also assume you are at the workspace root:

[10:17:15] मanishearth@manishearth-glaptop ~/dev/icu4x/components/calendar ^_^ 
$ carg<sup>1</sup>o check
   Compiling icu_calendar v0.3.0 (/home/manishearth/dev/icu4x/components/calendar)
warning: missing documentation for the crate
  --> components/calendar/src/lib.rs:5:1
   |
5  | / #![cfg_attr(not(any(test, feature = "std")), no_std)]
6  | |

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 on foo

See also

@Manishearth Manishearth added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Sep 8, 2021
@ehuss ehuss added the A-diagnostics Area: Error and warning messages generated by Cargo itself. label Sep 11, 2021
@ehuss
Copy link
Contributor

ehuss commented Sep 11, 2021

Can you clarify, I don't think this is specific to workspaces? That is, if you go to the src directory and run cargo build, you get the same effect where the paths displayed are not relative to the current directory?

@Manishearth
Copy link
Member Author

Yeah, you're right, this isn't actually related to workspaces

@Manishearth Manishearth changed the title Cargo should ask rustc to print appropriate relative paths when being run from a non-workspace root Cargo should ask rustc to print appropriate relative paths when being run from a non-root folder Sep 12, 2021
@Manishearth
Copy link
Member Author

We could use -Zremap-cwd-prefix here: rust-lang/rust#87320

@danakj
Copy link

danakj commented Sep 14, 2021

I'm not sure remap-cwd-prefix helps, but perhaps an example, to make sure I am understanding.

crate root: /home/foo/mycrate
current working dir: /home/foo/mycrate/somedir

What -Zremap-cwd-prefix=. would do is remap any paths in rustc of the form /home/foo/mycrate/somedir/blah to ./blah.

But it would not do anything with other paths rooted under /home/foo/mycrate. You would need to --remap-path-prefix=$CRATE_ROOT:$REL_PATH_FROM_CWD_TO_CRATE_ROOT to get what I think this is looking for.

@Manishearth
Copy link
Member Author

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

@danakj
Copy link

danakj commented Sep 14, 2021

Right. It's good for build caching if you're replacing something variable (an abs path) with something fixed (.) which is the point of it really. But for this scenario I think you don't want to touch the debuginfo, as you say.

@bjorn3
Copy link
Member

bjorn3 commented Sep 14, 2021

It would be nice if cargo could tell rustc where it is being invoked from, so that rustc may print appropriate paths.

Say you do

$ cargo build -p crate1
$ cd crate1 && cargo build

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 cargo build running in the background at the workspace root and then from a terminal run cargo build in the directory of the specific crate you are working on.

@Manishearth
Copy link
Member Author

Telling rustc where it is invoked from would require rustc to be run twice in this scenario

Why so? The flag is excluded from build caching.

@bjorn3
Copy link
Member

bjorn3 commented Sep 14, 2021

But then the diagnostics would be outdated when you switch directory as cargo caches the old diagnostics.

@Manishearth
Copy link
Member Author

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)

@epage epage changed the title Cargo should ask rustc to print appropriate relative paths when being run from a non-root folder Cargo should print appropriate relative paths when being run from a non-root folder Oct 19, 2023
@RalfJung
Copy link
Member

RalfJung commented Oct 19, 2023

This behavior is particularly confusing for cases like rustc's x.py, which invoke cargo on a whole bunch of crates across multiple workspaces. When the build failure is e.g. in codegen_cranelift, then the error path is relative to compiler/rustc_codegen_cranelift, making it quite hard to interpret for a human -- and impossible to interpret for RA.

@epage
Copy link
Contributor

epage commented Oct 19, 2023

@RalfJung does the caller of cargo have a consistent CWD, using --manifest-path, or does it change the CWD?

Wondering if rewriting the diagnostic paths to be relative to cargo's CWD would be more useful for your case or using absolute paths as requested in #5450

@RalfJung
Copy link
Member

I don't know for sure what x.py does but I think it's using --manifest-path. And it's under our control so if the requirement becomes "you need to use --manifest-path to get the errors reported relative to the directory of your choice", then that's okay and we can work with that.

@RalfJung
Copy link
Member

Wondering if rewriting the diagnostic paths to be relative to cargo's CWD would be more useful for your case or using absolute paths as requested in #5450

Relative paths to CWD seem best, users invoking x.py will probably not want to see absolute paths. (The issue affects human consumers as well, not just RA.)

@RalfJung
Copy link
Member

Looking at the bootstrap code, it does seem to use --manifest-path indeed:

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

@grothesque
Copy link

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.

@weihanglo
Copy link
Member

I don't know how common "I need my build cache to survive moving to a different location" really is

Some CI services do assign random path to a project, and reuse build artifacts. This one #10915 is related to CARGO_HOME but the idea applies.

@RalfJung
Copy link
Member

RalfJung commented Aug 6, 2024

Maybe we can for now have a nightly-only flag that makes cargo use an absolute path when invoking rustc, and have x.py set that? That would at least provide some sort of remedy for rust-lang/rust#128726 until someone can figure out a proper solution. (It would still take a beta compiler bump until x.py can actually use that flag, which is bad enough, but there's nothing we can do about that.)

@bjorn3
Copy link
Member

bjorn3 commented Aug 6, 2024

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.

@RalfJung
Copy link
Member

RalfJung commented Aug 6, 2024

@bjorn3 or maybe cargo could just have a flag telling it which cwd to use.

@weihanglo
Copy link
Member

weihanglo commented Aug 6, 2024

add a flag to rustc that sets a different folder to use as the root relative to which diagnostics are printed.

How does this revert #4788

Or we could have a flag to make cargo invoke rustc N directories up from where it would normally invoke it.

Can file! macros and debuginfo be expanded to the desired value when rustc is invoked from a different cwd? That being said, it will be an opt-in feature so people should take care of themselves.

@RalfJung
Copy link
Member

RalfJung commented Aug 7, 2024

How does this revert #4788

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?

Can file! macros and debuginfo be expanded to the desired value when rustc is invoked from a different cwd? That being said, it will be an opt-in feature so people should take care of themselves.

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.

@bjorn3
Copy link
Member

bjorn3 commented Aug 7, 2024

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.

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.

If you pass an absolute path as source file for the crate root to rustc, diagnostics and file!() will use absolute paths. If you pass a relative path as source file, it will resolve relative to the current working directory. Debuginfo will store the current working directory of rustc in the DW_AT_comp_dir attribute and use relative/absolute paths in the same cases as file!() outside of this.

@RalfJung
Copy link
Member

RalfJung commented Aug 7, 2024

If you pass an absolute path as source file for the crate root to rustc, diagnostics and file!() will use absolute paths. If you pass a relative path as source file, it will resolve relative to the current working directory.

Ah I mixed up file! and include! so what I said made no sense oops.

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.

Basically, around here, instead of stripping ws_root we'd strip that user-defined path (that defaults to the workspace root and must be a prefix of the workspace root). This is already used to determine the cache key if I understand correctly (though maybe that code changed quite a bit since 2017).

@weihanglo @epage does that sound like a design worth experimenting with?

Does anyone here want to try to implement this? :)

@epage
Copy link
Contributor

epage commented Aug 7, 2024

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.

@RalfJung
Copy link
Member

RalfJung commented Aug 7, 2024

Could you fully summarize the proposal to avoid misunderstanding each other in which parts of this thread are relevant?

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 file!()) should be printed. I am not sure what a good name for such a flag could be, maybe -Zroot-path or so? The path must be a prefix of the workspace root. The effect of this flag is to use that directory as the current working directory when invoking rustc. -Zroot-path can itself be relative or absolute; it is interpreted relative to the cwd that cargo is invoked in (similar to other paths, like --manifest-path).

# Imagine there is a Cargo.toml in `library/`
$ cargo build --manifest-path=library/Cargo.toml
error[E0425]: cannot find function `handle_errors` in this scope
   --> alloc/src/raw_vec.rs:177:25
# Note the path in the error is relative to `library/`.
$ cargo build -Zroot-path=. --manifest-path=library/Cargo.toml
error[E0425]: cannot find function `handle_errors` in this scope
   --> library/alloc/src/raw_vec.rs:177:25
# Now the path is relative to `.`.

(I don't know what cargo's usual approach to nightly-only flags is, hence I am taking the -Zflag convention from rustc.)

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 x.py which can just always set -Zroot-path to the root of the current rustc checkout, rather than having to count how many directories deep the workspace is that it is currently building.

Also, for myself at least, this is the bottom of the priority list.

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.

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.

Damn, that's a lot. Thanks for all your hard work and good luck with the regression! 💛 🍀

A cosmetic issue that we've been able to live with for a while is low on my priority list.

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.

@Alexendoo
Copy link
Member

I opened a potential workaround for this in Clippy - rust-lang/rust-clippy#13232

It passes --remap-path-prefix "=dir" to the relevant packages

@epage
Copy link
Contributor

epage commented Aug 7, 2024

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.

I'm even talking from a design discussion perspective.

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.

by "we've been able to live with", I meant the rust community, not rustc development.

@epage
Copy link
Contributor

epage commented Aug 7, 2024

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 file!()) should be printed. I am not sure what a good name for such a flag could be, maybe -Zroot-path or so? The path must be a prefix of the workspace root. The effect of this flag is to use that directory as the current working directory when invoking rustc. -Zroot-path can itself be relative or absolute; it is interpreted relative to the cwd that cargo is invoked in (similar to other paths, like --manifest-path).

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.

@RalfJung
Copy link
Member

RalfJung commented Aug 7, 2024

@Alexendoo that's an interesting approach! Maybe we should try this in x.py as well. Not sure which other side-effects it may have though.

by "we've been able to live with", I meant the rust community, not rustc development.

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.

@RalfJung
Copy link
Member

RalfJung commented Aug 9, 2024

That trick based on --remap-path-prefix also works for Miri. :)

@RalfJung
Copy link
Member

RalfJung commented Aug 25, 2024

Sadly --remap-path-prefix does not seem to be an option for rustc itself, so we're back to needing some sort of support from cargo here to fix the problem. Developing the standard library is quite annoying right now since one can't get proper diagnostics in vscode.

So we're back to likely needing something like this.

@RalfJung
Copy link
Member

So we're back to likely needing #9887 (comment).

I wrote a PR implementing that: #14752

bors added a commit that referenced this issue Oct 31, 2024
add unstable -Zroot-dir flag to configure the path from which rustc should be invoked

This implements the proposal described [here](#9887 (comment)): 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. :)
@epage
Copy link
Contributor

epage commented Oct 31, 2024

To carry over some thoughts from #14752 for what I could see being stabilized some day.

Cargo should work well for people out of the box. Giving them a path that can be used where they are at is a big help for that. This means in my ideal scenario, the output being relative to the current directory should b the default. Some have talked about rerendering rustc's output. This gets into problems with paths embedded in text. Another approach is to have a flag to pass to rustc that would adjust how it shows paths to users (e.g. yet another field in the message format). In both cases, this falls short in terms of file! and tracebacks. The incompleteness / inconsistency might be a blocker. We'll see.

EDIT: The above would have a problem with warnings replay, see #9887 (comment)

If it is a blocker or if we then find people still have justifiable enough use cases for it to be further customized, I lean against a CLI flag. Rustc is a lower level mechanism piece and I don't have as strong opinions on its flags but Cargo is the main user interface (in addition to rust-analyzer) that people work with. The more we add to the CLI, the harder it is to find what people are looking for. Overriding this is unlikely to be justifiable enough to be on the CLI. This could be in the config. While people normally think of that as static via .cargo/config.toml, its also dynamic through env variables and --config. Depending on the use cases we uncover, we might even consider some kind of templating scheme to refer to common locations, like home or workspace root. The question would then be whether we do this shallowly like the default or deeper like #14752. The problem with the latter is it affects caching but it gets the result everywhere.

@RalfJung
Copy link
Member

RalfJung commented Nov 1, 2024

Another approach is to have a flag to pass to rustc that would adjust how it shows paths to users (e.g. yet another field in the message format). In both cases, this falls short in terms of file! and tracebacks. The incompleteness / inconsistency might be a blocker. We'll see.

This also gets into problems with caching. After all, the original issue that's causing all this trouble is #4788; we could trivially achieve the goal of having paths relative to the working dir by invoking rustc from that working dir (i.e., by reverting #4788). But that will lead to a full rebuild when the entire project with its target dir is moved to a different location.

If it is a blocker or if we then find people still have justifiable enough use cases for it to be further customized, I lean against a CLI flag. Rustc is a lower level mechanism piece and I don't have as strong opinions on its flags but Cargo is the main user interface (in addition to rust-analyzer) that people work with. The more we add to the CLI, the harder it is to find what people are looking for. Overriding this is unlikely to be justifiable enough to be on the CLI. This could be in the config.

At least for the usecase of cargo being invoked from some surrounding build system, like rustc bootstrap or Miri's ./miri, a CLI flag seems like the best solution. I agree that for direct use by the user it is less suited. But that's not really what the flag is intended for.

@epage
Copy link
Contributor

epage commented Nov 4, 2024

Another approach is to have a flag to pass to rustc that would adjust how it shows paths to users (e.g. yet another field in the message format). In both cases, this falls short in terms of file! and tracebacks. The incompleteness / inconsistency might be a blocker. We'll see.

This also gets into problems with caching. After all, the original issue that's causing all this trouble is #4788; we could trivially achieve the goal of having paths relative to the working dir by invoking rustc from that working dir (i.e., by reverting #4788). But that will lead to a full rebuild when the entire project with its target dir is moved to a different location.

I assumed it wouldn't have a problem with caching so long as we didn't fingerprint the argument; it is just cosmetic. However, one place that I can think of that would be affected by this is warnings replay. Cargo records the warnings from rustc and replays them if its reusing an existing intermediate build artifact.

@epage
Copy link
Contributor

epage commented Nov 4, 2024

At least for the usecase of cargo being invoked from some surrounding build system, like rustc bootstrap or Miri's ./miri, a CLI flag seems like the best solution. I agree that for direct use by the user it is less suited. But that's not really what the flag is intended for.

Could you explain why? I have my guesses but its important for us to consider the full set of requirements and circumstances rather than just "someone has a need".

@RalfJung
Copy link
Member

RalfJung commented Nov 4, 2024

Could you explain why?

For which part of my comment?

A CLI flag seems best since these are anyway scripts invoking cargo, so having the script add one more CLI flag is trivial. Having the script write a config.toml file somewhere is a lot more tricky and risks messing up the user's state.

@epage
Copy link
Contributor

epage commented Nov 4, 2024

Assuming we make the output relative to CWD, why would an external build tool need to customize it further?

Having the script write a config.toml file somewhere is a lot more tricky and risks messing up the user's state.

There are also environment variables and the --config flag.

@RalfJung
Copy link
Member

RalfJung commented Nov 4, 2024

Assuming we make the output relative to CWD, why would an external build tool need to customize it further?

Sure, if that's a possibility that would also suffice for these tools.

There are also environment variables and the --config flag.

Okay, it wasn't clear to me that this would automatically be implied with a config file option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests

9 participants