internal: Implement invocation strategy config #13128

Veykril · 2022-08-27T16:29:13Z

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

jyn514 · 2022-09-15T00:58:44Z

@Veykril the "OnceInRoot" strategy looks like exactly what we want :) I'd love to see this merged, is there any way I can help out? Maybe test this locally on rust-lang/rust for you? Not sure how to do that but I'm happy to try.

Veykril · 2022-09-15T10:07:36Z

There is nothing specific blocking this PR aside from me finishing iirc in regards to build scripts.

For checkOnSave(that is rustc diagnostic reporting) OnceInRoot is kind of blocked on cargo I believe. I'll have to look into that a bit, but for a tool to work properly for use with checkOnSave with OnceInRoot it has to prefix all the file paths in diagnostics such that they are relative to the project root (cargo currently always just emits src/file/path/... which r-a has to manually prefix the workspace to, but that obviously doesn't work if we don't run this per workspace)

Because of the problem with cargo actually, none of the modes other than what r-a currently does works with cargo emitted diagnostics by default, rust-lang/cargo#11007

Veykril · 2022-09-15T10:27:13Z

Testing it would be great as well though (I don't really know the workspace setup in rustc to reliable know how to check this right now). In theory to test that, all you need to do is set the cargo.buildScripts.invocationStrategy to "once_in_root" and set the other buildScript stuff as usual for the rustc repo. I assume the x.py script should emit the json metadata for all workspaces already in which case it should just work.

Veykril · 2022-09-15T11:34:54Z

Okay, the PR is basically done now, but checkOnSave while implemented, won't work without changes to x.py or cargo. The build script stuff should just work assuming x.py emits all the metadata for the workspaces when building with proper paths (this is an assumption, we'd need to test that). checkOnSave will not work as is though, cargo emits paths in diagnostics relative to the workspace root, that means if x.py forwards the diagnostics as is to r-a, r-a won't know what files to associate these with, because it now gets diagnostics with workspace relative paths for all workspaces without the corresponding workspaces, so either x.py has to fix these paths up (as it should know from what workspaces the diagnostics come) or we need to fix rust-lang/cargo#11007 assuming x.py uses that under the hood. rust-lang/cargo#11007 needs to be fixed nevertheless for per_workspace_with_manifest_path to work as well.

RalfJung · 2022-09-15T15:48:35Z

run the specified command per workspace in the project root with --manifest-path path/to/Cargo.toml passed to the arguments

So you decided against adding interpolation for this? That seemed like the more general thing.

checkOnSave will not work as is though, cargo emits paths in diagnostics relative to the workspace root

In "per workspace in the project root" mode, we could declare that RA interprets diagnostics paths relative to the folder containing the workspace toml file -- that should work, right?

"once in the project root" indeed cannot currently work due to rust-lang/cargo#11007 (unless we teach bootstrap to translate the paths it receives from cargo).

Veykril · 2022-09-15T16:28:42Z

So you decided against adding interpolation for this? That seemed like the more general thing.

Not strictly, this was just simpler to implement for now. I wanted to get the general thing working first, it probably makes sense to switch this to intepolation before finalizing the PR.

In "per workspace in the project root" mode, we could declare that RA interprets diagnostics paths relative to the folder containing the workspace toml file -- that should work, right?

Right, that mode actually works fine as is, not sure why I thought differently, there is no problem here since we still know the workspaces.

bors · 2022-09-18T17:53:54Z

☔ The latest upstream changes (presumably #13058) made this pull request unmergeable. Please resolve the merge conflicts.

jyn514 · 2022-09-18T23:19:52Z

Testing it would be great as well though (I don't really know the workspace setup in rustc to reliable know how to check this right now). In theory to test that, all you need to do is set the cargo.buildScripts.invocationStrategy to "once_in_root" and set the other buildScript stuff as usual for the rustc repo. I assume the x.py script should emit the json metadata for all workspaces already in which case it should just work.

Great :) how can I use this build of rust-analyzer with rust-lang/rust? I see https://github.com/rust-lang/rust-analyzer/tree/master/docs/dev#launching-rust-analyzer, but I tried cargo xtask install --server and updating rust-analyzer.server.path but it didn't seem to do anything.

Veykril · 2022-09-19T07:22:32Z

Hmm, odd, that should be enough. You could also try building via cargo build -p rust-analyzer --release and point rust-analyzer.server.path to the build in your target then. If you are using VSCode you can check if you are using the self-build server by running the show RA version command (accesible by default via ctrl + shift + P), if that says version 0.0.0 it is using your custom build.

bors · 2022-09-19T15:04:38Z

☔ The latest upstream changes (presumably #13259) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2022-09-20T12:45:08Z

☔ The latest upstream changes (presumably #13268) made this pull request unmergeable. Please resolve the merge conflicts.

Veykril · 2022-09-26T17:42:43Z

Alright this is now ready (the interpolation implemented here is just a replacement of $manifest_path, doing a proper system can follow as that's all that's needed for now).

There is basically only the question of naming left, what should the configs be called, current names are once_in_root and per_workspace, I think these are fine but I am open to more ideas.

More important is the name for the interpolation var $manifest_path, should we go with snake case? SCREAMING_SNAKE_CASE? are bare name fines, or should we have some kind of namespaced system in place. Getting this right from the start would be nice, as we don't really want to have to support this special case otherwise. Alternatively, the interpolation thing can be left out of the PR, but iirc the interpolation stuff is already enough for miri to work properly right @RalfJung?

I also tested that once_in_root works with linked_projects in the rust repo. checkOnSave obviously doesn't as x.py with once_in_root doesn't fix up the paths in diagnostics for r-a to understand.

RalfJung · 2022-09-27T14:04:08Z

Alternatively, the interpolation thing can be left out of the PR, but iirc the interpolation stuff is already enough for miri to work properly right @RalfJung?

Miri is fine even without this PR, since we introduced a ./miri clippy wrapper.

But rustc needs something like this PR -- specifically per_workspace with the working dir being the project root. It doesn't actually need the manifest_path interpolation variable though, so tying that to the working dir is a bit unfortunate for rustc. The approach I suggested for rustc would involve doing ./x.py check compiler/rustc for each project, each time in the project root. Since the build is cached, re-running the command for each project is not a big deal. Basically what will happen is that when invoking this for the bootstrap project, RA will see the diagnostics for all projects but will discard most of them since they are for the wrong files. Then RA calls this again for the main compiler, gets the same diagnostics (hopefully, we need to ensure x.py re-emits diagnostics) and finds the diagnostics for the main workspace. (And then maybe another round for the cranelift workspace.)

Not sure how much sense that makes but it could work. I am just not sure if x.py really re-emits diagnostics properly.

Veykril · 2022-09-27T14:23:08Z

Ye, tying the working directory to the interpolation var seems rather odd, I realized this as well now. Will probably split the per_workspace one into two with just the working dir differing between them. And then remove the interpolation stuff for now

But rustc needs something like this PR -- specifically per_workspace with the working dir being the project root.

Ye, the diagnostics/checkOnSave stuff the bootstrap people will probably have to figure out, but this PR should give them all the tools needed for that then with the last changes I just mentioned if I see this right. once_in_root works properly for the build scripts at least so there is nothing that needs fixing there on the x.py side at least.

Thanks for dropping the valuable information here :)

RalfJung · 2022-09-27T14:32:21Z

per_workspace_from_root without interpolation is a bit odd though since it means the script has no way to know which project even needs to be built. Granted, x.py can't (currently) make use of that information, but in general it seems like that would be good to have...

Veykril · 2022-09-27T14:39:18Z

Well, it would be certainly great if x.py could make use of the manifest-path arg, because a check right now is more expensive than it needs to be I think. At least from a quick test running x.py check takes a long time even if I'm only interested in one workspace (though I have almost no experience with the rustc codebase).

I'm a bit split now though, you are right that per_workspace_in_root is rather pointless, so the tying of the interpolation to the working dir does make somewhat sense :/ Guess I'll think some more about the general interpolation syntax then

jyn514 · 2022-09-27T15:31:29Z

Not sure how much sense that makes but it could work. I am just not sure if x.py really re-emits diagnostics properly.

It should. We run cargo under the hood, which will replay its diagnostic cache.

bors · 2022-10-10T13:57:57Z

☔ The latest upstream changes (presumably #13385) made this pull request unmergeable. Please resolve the merge conflicts.

googleson78 · 2022-10-19T14:10:56Z

crates/flycheck/src/lib.rs

+                    with_manifest_path = true;
+                    cmd.arg(arg.replace(
+                        "$manifest_path",
+                        &self.root.join("Cargo.toml").display().to_string(),


There should be a check for rust-project.json here, I think (and of course placing rust-project.json when it's being used instead)

Note that due to how cargo works, none of the modes currently work for r-a

Veykril · 2022-10-19T21:43:48Z

I removed the simplistic interpolation from this, as I'll rather do that properly with a bit more thought behind. The PR itself is good to go though and the rustc repo at least benefits from the once setting for build scripts (and technically checkOnSave if the x.py script fixes up the paths)

Veykril · 2022-10-19T21:49:03Z

@bors r+

bors · 2022-10-19T21:49:04Z

📌 Commit 448d34b has been approved by Veykril

It is now in the queue for this repository.

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 · 2022-10-19T21:49:12Z

⌛ Testing commit 448d34b with merge a8e4319...

Veykril · 2022-10-19T21:53:14Z

@bors r+

bors · 2022-10-19T21:53:15Z

📌 Commit 4673236 has been approved by Veykril

It is now in the queue for this repository.

bors · 2022-10-19T21:53:22Z

⌛ Testing commit 4673236 with merge a77ac93...

bors · 2022-10-19T22:10:52Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing a77ac93 to master...

Implement invocation location config This allows setting the working directory for build-scripts on flycheck Complements #13128 This will be followed up by one more PR that adds a few simple interpolation vars for `overrideCommand`, with that we should cover the needs for most build systems I believe.

ShayBox · 2023-05-22T16:31:17Z

Could someone clarify what these settings do exactly? the descriptions are pretty vague imo and I don't understand them.

jyn514 mentioned this pull request Sep 15, 2022

rust-analyzer doesn't work on bootstrap anymore rust-lang/rust#101070

Closed

Veykril force-pushed the invocation-strategy branch 2 times, most recently from f32ea38 to c104f0e Compare September 15, 2022 11:31

Veykril mentioned this pull request Sep 19, 2022

Feature proposal: pass arguments to overriden commands in a json to make them more flexible and use other rust-analyzer settings #13258

Closed

Veykril force-pushed the invocation-strategy branch from 81e9571 to 5bd7d26 Compare September 19, 2022 14:15

Veykril force-pushed the invocation-strategy branch from 5bd7d26 to 18cbf41 Compare September 19, 2022 15:45

Veykril force-pushed the invocation-strategy branch 2 times, most recently from 1f983d2 to aba8ad7 Compare September 26, 2022 16:40

Veykril mentioned this pull request Sep 26, 2022

Proper serversided config interpolation #13297

Closed

Veykril force-pushed the invocation-strategy branch from aba8ad7 to a3549dd Compare September 26, 2022 17:37

Veykril marked this pull request as ready for review September 26, 2022 17:38

Veykril mentioned this pull request Oct 13, 2022

feature: for all config values that override commands, canonicalize the provided path #13405

Closed

googleson78 reviewed Oct 19, 2022

View reviewed changes

Veykril mentioned this pull request Oct 19, 2022

Seemingly incompatible design between rust-project.json and checkOnSave.overrideCommand #13437

Open

Veykril added 4 commits October 19, 2022 23:21

Implement invocation strategy config for build scripts

7e2c41d

Implement invocation strategy config for checkOnSave

4a287d2

Note that due to how cargo works, none of the modes currently work for r-a

Use correct invocation strategy config for checkOnSave

5174b65

{manifest-path} interpolation

7db5029

Veykril force-pushed the invocation-strategy branch 2 times, most recently from bb57ffe to 448d34b Compare October 19, 2022 21:42

Remove simplistic interpolation for manifest-path

4673236

Veykril force-pushed the invocation-strategy branch from 448d34b to 4673236 Compare October 19, 2022 21:53

Veykril mentioned this pull request Oct 19, 2022

Update rust-analyzer suggestions rust-lang/rustc-dev-guide#1487

Merged

bors merged commit a77ac93 into rust-lang:master Oct 19, 2022

This was referenced Oct 19, 2022

Support multiple targets for checkOnSave (in conjunction with cargo 1.64.0+) #13290

Merged

feat: add a new config that limits workspace checks to single crates #13336

Closed

davidbarsky mentioned this pull request Oct 20, 2022

Deeper rust-analyzer Integration with Non-Cargo Build Systems #13446

Open

Veykril deleted the invocation-strategy branch October 22, 2022 18:19

Veykril mentioned this pull request Oct 22, 2022

internal: Implement invocation location config #13466

Merged

Veykril changed the title ~~Implement invocation strategy config~~ internal: Implement invocation strategy config Oct 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal: Implement invocation strategy config #13128

internal: Implement invocation strategy config #13128

Veykril commented Aug 27, 2022 •

edited

Loading

jyn514 commented Sep 15, 2022

Veykril commented Sep 15, 2022 •

edited

Loading

Veykril commented Sep 15, 2022

Veykril commented Sep 15, 2022

RalfJung commented Sep 15, 2022 •

edited

Loading

Veykril commented Sep 15, 2022

bors commented Sep 18, 2022

jyn514 commented Sep 18, 2022

Veykril commented Sep 19, 2022

bors commented Sep 19, 2022

bors commented Sep 20, 2022

Veykril commented Sep 26, 2022 •

edited

Loading

RalfJung commented Sep 27, 2022

Veykril commented Sep 27, 2022 •

edited

Loading

RalfJung commented Sep 27, 2022

Veykril commented Sep 27, 2022

jyn514 commented Sep 27, 2022

bors commented Oct 10, 2022

googleson78 Oct 19, 2022

Veykril commented Oct 19, 2022 •

edited

Loading

Veykril commented Oct 19, 2022

bors commented Oct 19, 2022

bors commented Oct 19, 2022

Veykril commented Oct 19, 2022

bors commented Oct 19, 2022

bors commented Oct 19, 2022

bors commented Oct 19, 2022

ShayBox commented May 22, 2023

internal: Implement invocation strategy config #13128

internal: Implement invocation strategy config #13128

Conversation

Veykril commented Aug 27, 2022 • edited Loading

jyn514 commented Sep 15, 2022

Veykril commented Sep 15, 2022 • edited Loading

Veykril commented Sep 15, 2022

Veykril commented Sep 15, 2022

RalfJung commented Sep 15, 2022 • edited Loading

Veykril commented Sep 15, 2022

bors commented Sep 18, 2022

jyn514 commented Sep 18, 2022

Veykril commented Sep 19, 2022

bors commented Sep 19, 2022

bors commented Sep 20, 2022

Veykril commented Sep 26, 2022 • edited Loading

RalfJung commented Sep 27, 2022

Veykril commented Sep 27, 2022 • edited Loading

RalfJung commented Sep 27, 2022

Veykril commented Sep 27, 2022

jyn514 commented Sep 27, 2022

bors commented Oct 10, 2022

googleson78 Oct 19, 2022

Choose a reason for hiding this comment

Veykril commented Oct 19, 2022 • edited Loading

Veykril commented Oct 19, 2022

bors commented Oct 19, 2022

bors commented Oct 19, 2022

Veykril commented Oct 19, 2022

bors commented Oct 19, 2022

bors commented Oct 19, 2022

bors commented Oct 19, 2022

ShayBox commented May 22, 2023

Veykril commented Aug 27, 2022 •

edited

Loading

Veykril commented Sep 15, 2022 •

edited

Loading

RalfJung commented Sep 15, 2022 •

edited

Loading

Veykril commented Sep 26, 2022 •

edited

Loading

Veykril commented Sep 27, 2022 •

edited

Loading

Veykril commented Oct 19, 2022 •

edited

Loading