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

Using --manifest-path, the filenames in error messages are relative to the manifest, not the current working directory #11007

Closed
RalfJung opened this issue Aug 19, 2022 · 11 comments
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. C-bug Category: bug

Comments

@RalfJung
Copy link
Member

RalfJung commented Aug 19, 2022

Problem

When using --manifest-path, the filenames in error messages are relative to the manifest given, not the directory cargo is invoked from.

Steps

  1. For example in Miri, introduce a build failure in cargo-miri/src/main.rs.
  2. Run cargo check --manifest-path cargo-miri/Cargo.toml
  3. Notice the error refers to src/main.rs:21:17

Possible Solution(s)

Cargo could instruct rustc to print the paths relative to a given directory, or it could adjust the paths itself.

Notes

This also affects the rustc repo, where we have 3 workspaces currently (the main one, bootstrap, and codegen_cranelift). Errors in codegen_cranelift show paths that are quite hard to interpret since they are relative to src/compiler/rustc_codegen_cranelift, which is hard to users to even realize.

Version

No response

@RalfJung RalfJung added the C-bug Category: bug label Aug 19, 2022
@weihanglo weihanglo added the A-diagnostics Area: Error and warning messages generated by Cargo itself. label Aug 22, 2022
bors added a commit to rust-lang/rust-analyzer that referenced this issue Oct 19, 2022
Implement invocation strategy config

Fixes #10793

This allows to change how we run build scripts (and `checkOnSave`), exposing two configs:
- `once`: run the specified command once in the project root (the working dir of the server)
- `per_workspace`: run the specified command per workspace in the corresponding workspace

This also applies to `checkOnSave` likewise, though `once_in_root` is useless there currently, due to rust-lang/cargo#11007
bors added a commit to rust-lang/rust-analyzer that referenced this issue Oct 19, 2022
Implement invocation strategy config

Fixes #10793

This allows to change how we run build scripts (and `checkOnSave`), exposing two configs:
- `once`: run the specified command once in the project root (the working dir of the server)
- `per_workspace`: run the specified command per workspace in the corresponding workspace

This also applies to `checkOnSave` likewise, though `once_in_root` is useless there currently, due to rust-lang/cargo#11007
@weihanglo
Copy link
Member

Some other issues around relative path in error messages:

Basically, this is by design that always be relative to the workspace root. Most projects have a workspace at the root of the repository, and most IDE also depends on workspace when building Cargo projects.

Maybe we do need a -Zdiagnose-with-full-path, so that either IDE or anyone's specialized tool can take advantage of absolute path without ambiguity.

In the meanwhile, could you try if it helps with --remap-path-prefix or -Zremap-cwd-prefix? For example,

RUSTFLAGS='--remap-path-prefix cargo-miri=' cargo check --manifest-path cargo-miri/Cargo.toml

Not ideal I know. Sorry about that :(

@RalfJung
Copy link
Member Author

RalfJung commented Oct 25, 2022

Basically, this is by design that always be relative to the workspace root. Most projects have a workspace at the root of the repository, and most IDE also depends on workspace when building Cargo projects.

I am not talking about workspace projects though. I am talking about using --manifest-path to build a different workspace.

If you read the discussion at rust-lang/rust-analyzer#10793 and the other backlinks above, you will see that the current behavior is quite painful for IDEs in multi-workspace repos.

I can try RUSTFLAGS but then the problem is that all builds inside and outside the IDE need to be somehow configured to use the same flags.

@weihanglo
Copy link
Member

Sorry for not being more clear 🙇🏾

By workspace root I mean the root path of either a Cargo workspace or a non-workspace Cargo package. (Just realized VS Code also has its own workspace definition 😅).

Let's look into miri script in rust-lang/miri and compiler/rustc_codegen_cranelift in rust-lang/rust. Here are what I found1 (You may already know 😆):

  • Since rust-lang/miri is not a Cargo workspace, when invoking cargo check --manifest-path ./cargo-miri, rustc invocations will be resolved relatively to ./cargo-miri. My impression is that if you run cargo check --mainfest-path src/tools/miri/cargo-miri/Cargo.toml, you'll get diagnostics from rust-lang/rust root.
  • compiler/rustc_codegen_cranelift is excluded from rustc workspace, so any rustc invocations will be resolved from its own package root. So is src/bootstrap.

Changing the behaviour of --manifest-path is definitely a breaking change. Would it help if Cargo has a flag to forced print absolute paths, such as what #5450 and #11043 propose?

Footnotes

  1. FYI, I use cargo locate-project --workspace to interactively check whether a package resides in a workspace.

@RalfJung
Copy link
Member Author

The situation with these multi-workspace (or multi-crate-no-workspace) git repos is that they come with a single script that does all the building (./miri or ./x.py). Right now when ./miri or ./x.py prints a path it might be relative to whatever (different errors can even be relative to different paths), which is already ambiguous for people consuming their output and a big problem for tools, such as IDEs, consuming their output.

@jyn514
Copy link
Member

jyn514 commented Jun 19, 2023

  • My impression is that if you run cargo check --mainfest-path src/tools/miri/cargo-miri/Cargo.toml, you'll get diagnostics from rust-lang/rust root.

This is not the case. Bootstrap runs all cargo processes from the rust-lang/rust root but still gets the truncated file paths.

@jyn514
Copy link
Member

jyn514 commented Jun 19, 2023

here is a way to reproduce the issue:

$ git apply
diff --git a/src/bootstrap/lib.rs b/src/bootstrap/lib.rs
index d7e77aeb338..6a046b0995f 100644
--- a/src/bootstrap/lib.rs
+++ b/src/bootstrap/lib.rs
@@ -117,7 +117,7 @@ pub unsafe fn setup(_build: &mut crate::Build) {}
 
 /// Extra --check-cfg to add when building
 /// (Mode restriction, config name, config values (if any))
-const EXTRA_CHECK_CFGS: &[(Option<Mode>, &'static str, Option<&[&'static str]>)] = &[
+const EXTRA_CHEK_CFGS: &[(Option<Mode>, &'static str, Option<&[&'static str]>)] = &[
     (None, "bootstrap", None),
     (Some(Mode::Rustc), "parallel_compiler", None),
     (Some(Mode::ToolRustc), "parallel_compiler", None),
$ cargo check -q --manifest-path src/bootstrap/Cargo.toml  --message-format json | jq 'select(.reason = 
"compiler-message") | select((.message.spans | length) != 0) | { package_id, manifest_path, file_name: .
message.spans[0].file_name }'
{
  "package_id": "bootstrap 0.0.0 (path+file:///home/jyn/src/rust/src/bootstrap)",
  "manifest_path": "/home/jyn/src/rust/src/bootstrap/Cargo.toml",
  "file_name": "builder.rs"
}

@jyn514
Copy link
Member

jyn514 commented Jun 19, 2023

I think this is the same problem as #9427.

@jyn514
Copy link
Member

jyn514 commented Jun 19, 2023

Would it help if Cargo has a flag to forced print absolute paths, such as what #5450 and #11043 propose?

yes, this would help. i'm looking into adding it as an unstable flag to rustc now, but putting it in cargo would be fine too.

jyn514 added a commit to jyn514/rust that referenced this issue Jun 19, 2023
It's unfortunate that this hack is necessary. A short summary of the background here:
- Rust-Analyzer uses `x check --json-output` to show red underlines in the editor.
- There are several different Cargo workspaces in rust-lang/rust, including src/bootstrap and src/tools/miri.
- Cargo runs each invocation of rustc relative to the *workspace*, not to the current working directory of cargo itself.
- Rustc prints file paths relative to its current working directory. As a result, it would print things like `config.rs:43:14` instead of `src/bootstrap/config.rs`.

This adds a new flag to rustc to print the files as an absolute path instead of a relative path. I went with this approach instead of one of the wrapping tools for the following reasons:
1. Cargo considers changing the working directory to be a breaking change:
   rust-lang/cargo#11007 (comment). They have open feature
   requests for adding a flag to print absolute paths, but none have been implemented.
2. Bootstrap would potentially be able to parse and rewrite the paths that cargo emits, but I don't
   want to hard-code Cargo's JSON output format in both bootstrap.py and rustbuild; unlike rustbuild,
   bootstrap.py can't use `cargo_metadata` as a library.
3. Rust-Analyzer could potentially rewrite this output, but that wouldn't fix ctrl+click on the relative path when someone runs `cargo check`.

In addition to adding and using the new flag, this also adds `local_rebuild` detection to bootstrap.py, so that I could test the change.

Before:
```
error[E0425]: cannot find value `MIRI_DEFAULT_ARGS` in crate `miri`
   --> src/bin/miri.rs:269:33
    |
269 |         args.splice(1..1, miri::MIRI_DEFAULT_ARGS.iter().map(ToString::to_string));
    |                                 ^^^^^^^^^^^^^^^^^ help: a constant with a similar name exists: `MIRI_DEFLT_ARGS`
    |
   ::: src/lib.rs:128:1
    |
128 | pub const MIRI_DEFLT_ARGS: &[&str] = &[
    | ---------------------------------- similarly named constant `MIRI_DEFLT_ARGS` defined here
```

After:
```
error[E0425]: cannot find value `MIRI_DEFAULT_ARGS` in crate `miri`
   --> /home/jyn/src/rust/src/tools/miri/src/bin/miri.rs:269:33
    |
269 |         args.splice(1..1, miri::MIRI_DEFAULT_ARGS.iter().map(ToString::to_string));
    |                                 ^^^^^^^^^^^^^^^^^ help: a constant with a similar name exists: `MIRI_DEFLT_ARGS`
    |
   ::: /home/jyn/src/rust/src/tools/miri/src/lib.rs:128:1
    |
128 | pub const MIRI_DEFLT_ARGS: &[&str] = &[
    | ---------------------------------- similarly named constant `MIRI_DEFLT_ARGS` defined here
```
@jyn514
Copy link
Member

jyn514 commented Jun 25, 2023

  • My impression is that if you run cargo check --mainfest-path src/tools/miri/cargo-miri/Cargo.toml, you'll get diagnostics from rust-lang/rust root.

This is not the case. Bootstrap runs all cargo processes from the rust-lang/rust root but still gets the truncated file paths.

/// The path that we pass to rustc is actually fairly important because it will
/// show up in error messages (important for readability), debug information
/// (important for caching), etc. As a result we need to be pretty careful how we
/// actually invoke rustc.
///
/// In general users don't expect `cargo build` to cause rebuilds if you change
/// directories. That could be if you just change directories in the package or
/// if you literally move the whole package wholesale to a new directory. As a
/// result we mostly don't factor in `cwd` to this calculation. Instead we try to
/// track the workspace as much as possible and we update the current directory
/// of rustc/rustdoc where appropriate.
///
/// The first returned value here is the argument to pass to rustc, and the
/// second is the cwd that rustc should operate in.
pub fn path_args(ws: &Workspace<'_>, unit: &Unit) -> (PathBuf, PathBuf) {
let ws_root = ws.root();
let src = match unit.target.src_path() {
TargetSourcePath::Path(path) => path.to_path_buf(),
TargetSourcePath::Metabuild => unit.pkg.manifest().metabuild_path(ws.target_dir()),
};
assert!(src.is_absolute());
if unit.pkg.package_id().source_id().is_path() {
if let Ok(path) = src.strip_prefix(ws_root) {
return (path.to_path_buf(), ws_root.to_path_buf());
}
}
(src, unit.pkg.root().to_path_buf())
}

(note that miri is in a separate workspace, so this effectively strips src/tools/miri from the start of each diagnostic)

@RalfJung
Copy link
Member Author

FWIW, Miri is not a separate workspace, but the cranelift backend is.

@epage
Copy link
Contributor

epage commented Oct 19, 2023

#9887 appears to ask for the same thing (shift messages to be relative to CWD), so closing in favor of that

@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Oct 19, 2023
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-bug Category: bug
Projects
None yet
Development

No branches or pull requests

4 participants