overrideCommand with a relative path (./miri/./x.py) does not work very well, and that is very hard to debug

[RalfJung]

I am using the following workspace settings for working on Miri with vscode:

// Place your settings in this file to overwrite default and user settings.
{
    "rust-analyzer.rustfmt.extraArgs": [
        "+nightly"
    ],
    "rust-analyzer.checkOnSave.overrideCommand": [
        "./miri",
        "check",
        "--message-format=json",
    ],
    "rust-analyzer.linkedProjects": [
        "./Cargo.toml",
        "./cargo-miri/Cargo.toml"
    ],
    //"rust-analyzer.rustcSource": "discover"
}

Whenever I save a file, an error pops up saying

cargo check failed: No such file or directory (os error 2)

That's all it says. I have no idea which file it is talking about, or what is even going wrong -- the cargo check seems to be working fine, I am getting the errors I am expected highlighted as they should. The only sign that anything going wrong is this error message, and the error is not giving any details that would be helpful in determining what is happening.

[bjorn3]

The stderr of cargo is read but never presented to the user: https://github.com/rust-analyzer/rust-analyzer/blob/d1e756e05aab5410f6176fce26bf021453708b9b/crates/flycheck/src/lib.rs#L292

[RalfJung]

Also, running ./miri check --message-format=json does not show any such error. So if this error is coming from cargo, the question is what RA is doing that it invokes cargo in a way that causes that error.

This is also a regression, the same configuration used to work fine a few months ago.

[RalfJung]

This excerpt from the log might be interesting though it does not tell me much

[INFO rust_analyzer::main_loop] handle_event(FetchWorkspace(Report("metadata")))
[INFO rust_analyzer::reload] did fetch workspaces [Ok(Cargo { root: Some("miri"), n_packages: 57, sysroot: true, n_rustc_compiler_crates: 0, n_rustc_cfg: 67, n_cfg_overrides: 1 }), Ok(Cargo { root: Some("cargo-miri"), n_packages: 53, sysroot: true, n_rustc_compiler_crates: 0, n_rustc_cfg: 67, n_cfg_overrides: 1 })]
[INFO rust_analyzer::main_loop] handle_event(FetchWorkspace(End([Ok(Cargo { root: Some("miri"), n_packages: 57, sysroot: true, n_rustc_compiler_crates: 0, n_rustc_cfg: 67, n_cfg_overrides: 1 }), Ok(Cargo { root: Some("cargo-miri"), n_packages: 53, sysroot: true, n_rustc_compiler_crates: 0, n_rustc_cfg: 67, n_cfg_overrides: 1 })])))
[INFO rust_analyzer::reload] will switch workspaces
[INFO rust_analyzer::main_loop] handle_event(PrimeCaches(Begin))
[INFO rust_analyzer::main_loop] handle_event(Response(Response { id: RequestId(I32(12)), result: None, error: None }))
[INFO rust_analyzer::main_loop] handle_event(Response(Response { id: RequestId(I32(13)), result: None, error: None }))
[INFO rust_analyzer::main_loop] handle_event(Response(Response { id: RequestId(I32(14)), result: None, error: None }))
[INFO rust_analyzer::main_loop] handle_event(Request(Request { id: RequestId(I32(9)), method: "textDocument/semanticTokens/full", params: Object({"textDocument": Object({"uri": String("file:///home/r/src/rust/miri/tests/run-pass/portable-simd.rs")})}) }))
[INFO flycheck] restart flycheck "./miri" "check" "--message-format=json"
[INFO flycheck] restart flycheck "./miri" "check" "--message-format=json"
[INFO rust_analyzer::main_loop] handle_event(Progress { id: 1, progress: DidStart })
[ERROR flycheck] Flycheck failed to run the following command: "./miri" "check" "--message-format=json"
[INFO rust_analyzer::main_loop] handle_event(Progress { id: 1, progress: DidFinish(Err(Os { code: 2, kind: NotFound, message: "No such file or directory" })) })
[INFO rust_analyzer::main_loop] handle_event(Response(Response { id: RequestId(I32(15)), result: None, error: None }))
[INFO rust_analyzer::main_loop] handle_event(Response(Response { id: RequestId(I32(16)), result: None, error: None }))
[INFO rust_analyzer::main_loop] handle_event(Progress { id: 0, progress: DidFinish(Ok(())) })
[INFO rust_analyzer::main_loop] handle_event(Request(Request { id: RequestId(I32(10)), method: "textDocument/codeLens", params: Object({"textDocument": Object({"uri": String("file:///home/r/src/rust/miri/tests/run-pass/portable-simd.rs")})}) }))
[INFO rust_analyzer::main_loop] handle_event(Response { id: RequestId(I32(10)), error: None })
[INFO rust_analyzer::global_state] handled textDocument/codeLens - (10) in 104.40µs

Could it be the case that it starts ./miri ... twice, one of them works and gives me the in-editor warnings I expect, and the other fails with the error it shows?

[bjorn3]

It sees two workspaces: One for miri (at .) and one for cargo-miri (at ./cargo-miri) and then tries to run ./miri check --message-format=json within the directory of both workspaces. The first succeeds, but the second fails as it needs to be ../miri in that case.

[RalfJung]

That is new behavior though I think? We had two workspaces for a while. I don't remember exactly why but I do remember that making them one workspace did not work the way I wanted it to.

./miri check --message-format=json actually builds both of them.

[RalfJung]

I confirmed @bjorn3's theory by creating a dummy script cargo-miri/miri that does nothing -- then the error goes away.

Unfortunately, vscode still does not show errors or warnings for the cargo-miri crate -- it used to be enough to add it to linkedProjects to achieve this, but it seems that things changed over the last few weeks/months. So instead I made cargo-miri/miri a symlink to miri, and changed the script to adjust its working dir correctly even when being called through a symlink -- now things seem to work again.

But it looks like overrideCommand and linkedProjects currently do not behave well when used together.

[RalfJung]

Current status: pretty much unchanged.

• When cargo-miri/miri does not exist, nothing at all happens in the UI. (So this is even worse than when I reported the issue.) In the RA server logs we get [ERROR flycheck] failed to restart flycheck command="./miri" "cargo" "clippy" "--message-format=json" error=No such file or directory (os error 2) which is not sufficient to diagnose the issue. After all ./miri ... works fine when run outside RA. The main missing bit of information is the working directory.
• To make things work, cargo-miri/miri needs to exist, and it needs to print the diagnostics for cargo-miri (it's not enough to have those diagnostics be printed by ./miri). However, Using --manifest-path, the filenames in error messages are relative to the manifest, not the current working directory cargo#11007 makes it hard to do anything else here.

Part of the problem here is that I expected the overrideCommand to be a single global setting, not something that RA uses separately for each project. #13065 should help a bit with that, at least.

For Miri, we have a pretty decent work-around with rust-lang/miri#2495. But for x.py we don't, and it'd be great to be able to get RA to work inside bootstrap there.

[RalfJung]

But for x.py we don't, and it'd be great to be able to get RA to work inside bootstrap there.

So here's a possible plan:

• Change RA so that when it wants to run flycheck on anything but the root Cargo.toml, rather than changing the working directory, it adds --manifest-path, but still interprets the paths in the errors as relative to where that manifest is stored (to work around Using --manifest-path, the filenames in error messages are relative to the manifest, not the current working directory cargo#11007). This only changes behavior for configurations that involve both multiple linked projects and an override command, and even then it will still work if the override command is cargo clippy or any other cargo subcommand that supports --manifest-path, so hopefully this has reasonably small fallout.
• Change x.py to simply ignore --manifest-path, so that using ./x.py check ... still works as an override command even when there are multiple linked projects.

I think together this should mean that the existing recommended override command for rustc will actually work as intended even when adding src/bootstrap/Cargo.toml to linkedProjects, and native diagnostics (as well as all the RA magic) should then work in bootstrap. It does mean that RA calls ./x.py multiple times (once for each linked project), but after the first run completes all the others should be instantaneous so that's fine, I think. (To improve this we'd need x.py to actually honor the --manifest-path argument.)

Cc @jyn514 for x.py/bootstrap involvement.

[Veykril]

So I've been brain storming a bit. First up, this also affects build-scripts (and proc-macro building as well therefore), not just checkOnSave. I believe there are 3 variants of what people might want from this:

• Run the command once, for the entire project
• Run the command once per workspace
  • run the command from the respective workspace's root
  • run the command from the project root and pass some arg to the command with the project path (maybe relative to the root or just absolute)

Does this seem right?

[RalfJung]

Yeah sounds good -- though if the per-workspace command is needed, then also having a global command does not seem that useful. I was thinking of global commands mostly because I was sure the command would be global (an implicit assumption I did not even realize I was making), and the documentation did not indicate anything else, so I was taken by surprise when I realized that it is per-workspace.

But doing this per-workspace makes sense, it'd just be good to find a way to do that that does not involve changing the working directory, while also working around rust-lang/cargo#11007.

[Veykril]

The working dir switching was done for cargo I imagine since that just works, but it might break for custom commands. I think offering both version, switching working dir or passing the project path offers the most flexibility for users in regards to override commands, so I don't think offering both hurts. The main problem is really implementing the first point, running it once for the entire project in regards to diagnostic paths as you've said with rust-lang/cargo#11007

[Veykril]

Also #8937 is probably related to this

[RalfJung]

Yeah, I don't think there even is a reliable way to take the file paths that come out of ./miri check or ./x.py check and figure out where they point -- different parts of the build use paths relative to different base directories.

But I think a per-workspace command that stays in the project root is 'good enough'. The command might ignore the --manifest-path and do some unnecessary work, but assuming reasonable caching that should usually stabilize quickly.

[jyn514]

• Change x.py to simply ignore --manifest-path, so that using ./x.py check ... still works as an override command even when there are multiple linked projects.

Why not do the same workaround you had for miri, have a symlink in src/bootstrap/x.py that links to x.py? That seems less hacky.

It does mean that RA calls ./x.py multiple times (once for each linked project), but after the first run completes all the others should be instantaneous so that's fine, I think. (To improve this we'd need x.py to actually honor the --manifest-path argument.)

I don't understand why that would help; the least work we can do after a full x.py check is "none", which is ~basically what we do today. Are you imagining x.py check --manifest-path src/bootstrap/Cargo.toml to behave the same as x.py check src/bootstrap? Why would that be faster?

[RalfJung]

Why not do the same workaround you had for miri, have a symlink in src/bootstrap/x.py that links to x.py? That seems less hacky.

Dunno, that symlink seems like a pretty big hack to me.

Also that won't work for x.py. x.py is pwd-dependent, it will put the build artifacts into a build folder relative to where it is invoked. I tried to change this many years ago but the PR got rejected -- this is by design.

Are you imagining x.py check --manifest-path src/bootstrap/Cargo.toml to behave the same as x.py check src/bootstrap? Why would that be faster?

Yes, and similar for the cranelift workspace -- "only check that workspace".

It would do less work, wouldn't it? No need to recheck all of rustc if we just changed bootstrap.

(Though what I'd really want is that when I work on library, it doesn't rebuild all of compiler on each change... but there's not really a good way for RA to know this I think.)

[jyn514]

(Though what I'd really want is that when I work on library, it doesn't rebuild all of compiler on each change... but there's not really a good way for RA to know this I think.)

You can tell RA that by changing the command to x check library bootstrap (instead of an unconditional check).

It would do less work, wouldn't it? No need to recheck all of rustc if we just changed bootstrap.

x.py won't rebuild rustc if you've only modified bootstrap. So an unconditionally x.py check isn't doing more work after the very first run.

Also that won't work for x.py. x.py is pwd-dependent, it will put the build artifacts into a build folder relative to where it is invoked. I tried to change this many years ago but the PR got rejected -- this is by design.

ah, well, fair enough. I guess ignoring --manifest-path is fine - I see the reason it's necessary for RA is for workspaces that aren't using a custom cargo wrapper, like you said above.

[RalfJung]

You can tell RA that by changing the command to x check library bootstrap (instead of an unconditional check).

I know. But I also often work on the compiler and then I of course want it to check the compiler.
Right now I just change the configuration each time I change from working on the compiler to working on the library, But ideally, the file I hit Ctrl-S in would determine which things it checks... just dreaming here. ;)

x.py won't rebuild rustc if you've only modified bootstrap. So an unconditionally x.py check isn't doing more work after the very first run.

Yeah, that first run can take minutes though.

But I agree it's probably not a big deal in practice. Changing things in the compiler will also rebuild cranelift, but that's probably a good thing -- RA seems to assume that changes to one workspace cannot cause errors in other workspaces (so it is enough to run the check-build for the workspace that changed), which is just a wrong assumption in general.

[Veykril]

I know. But I also often work on the compiler and then I of course want it to check the compiler.
Right now I just change the configuration each time I change from working on the compiler to working on the library, But ideally, the file I hit Ctrl-S in would determine which things it checks... just dreaming here. ;)

Maybe we can have the server interpolate into the override command, so that you could just pass --manifest-path {manifestPath} or {packageName} etc. Fixing this on the r-a side shouldn't be too much trouble (apart from only running one command for the entire project, but that isn't a hard requirement here right now), so this is more of a design issue right now. Though proper interpolation is probably way more than is really needed, unless the --manifest-path option isn't an option for rustc?

RA seems to assume that changes to one workspace cannot cause errors in other workspaces (so it is enough to run the check-build for the workspace that changed), which is just a wrong assumption in general.

This should not be the case, basically if you save a file, we fetch all crates that make use of this file, then collect all transwitive reverse dependency crates of those crates and finally we kick off the checkOnSave logic for all workspaces that contain at least on of those crates. Only if the file does not belong to some workspace we unconditionally trigger checkOnSave for all workspaces. At least that's how this is supposed to work.

[RalfJung]

Yeah, interpolation makes sense, though there also needs to be some way to control the working directory behavior (or else existing cargo clippy overrides will break on multi-workspace projects). Then we don't even need x.py to ignore an extra argument, we can just decide to not pass the argument.

That won't help with me having to edit the vscode config when doing library vs compiler work, as those are in the same workspace. But that's just a separate issue, sorry for bringing it up here.

This should not be the case, basically if you save a file, we fetch all crates that make use of this file, then collect all transwitive reverse dependency crates of those crates and finally we kick off the checkOnSave logic for all workspaces that contain at least on of those crates. Only if the file does not belong to some workspace we unconditionally trigger checkOnSave for all workspaces. At least that's how this is supposed to work.

Oh nice, that is a lot more clever than I expected. So we'll have to do some tests once the working directory part lands on the RA side.

[RalfJung]

The current status her as far as I know:

• for Miri we have a successful work-around by putting the miri script in each folder and having a ./miri cargo command which invokes cargo in the current working-dir. Then we can use the default invocation strategy. This means RA's check-builds of the crates that are not in the root (cargo-miri, miri-script) have a separate build cache from running ./miri check on the terminal, but that seems fine, these builds are fast and I don't usually run ./miri check on the terminal anyway since RA already did the check builds. (I will run ./miri test but that needs a separate build anyway.)
• for rustc, RA cannot show errors e.g. in the cranelift backend. rustc needs the "once", "root" invocation strategy. This means RA receives a JSON reply of diagnostics that have mixed working directories -- I don't think RA has a reliable way to disentangle that, it needs some solution for this cargo issue (closed in favor of Cargo should print appropriate relative paths when being run from a non-root folder cargo#9887). Maybe rustc should always use absolute paths in the JSON version of the errors to avoid any ambiguity? That was attempted in bootstrap: Add and use -Z absolute-file-paths rust#112812 with the conclusion "i think this should be in cargo and not rustc tbh". Cargo parses and re-prints the rustc JSON anyway as far as I know, so it might as well adjust the paths.

[RalfJung]

With recent developments (rust-lang/miri#3798, rust-lang/rust#128904), I think the options RA provides are entirely sufficient, and documentation has also been improved to explain the working directory situation. So this can be closed; what's left is possibly simplifying the implementation (#17848).