Extend `TargetSpec` functionality to rust-project.json #16135

davidbarsky · 2023-12-15T23:39:31Z

Resolves #15892.

This PR, as structured, adds runnable support to rust-project.json-based workspaces, and that works pretty nicely. It's not as polished as I'd like it to be (namely: interpolation of test_id feels a bit janky and undocumented, the), but I'm opening this PR as a draft for general direction/feedback.

I think that this approach can also make #15476 unnecessary by adding a flycheck_command to rust-project.json while also extending similar functionality to Cargo only build the impacted crate.

bors · 2023-12-20T14:13:34Z

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

Veykril

Overall implementation wise it looks good to me, al though I am still a bit on the fence wrt to having flycheck be in here. I am personally still of the opinion that flycheck should not be a direct part of r-a (but a separate extension altogether). Adding it to this file makes that less clear of a separation.

Veykril · 2024-01-10T09:14:13Z

crates/project-model/src/project_json.rs

+pub enum TargetKindData {
+    Bin,
+    /// Any kind of Cargo lib crate-type (dylib, rlib, proc-macro, ...).
+    Lib,
+    Example,
+    Test,
+    Bench,
+    BuildScript,
+    Other,
+}


This ties the cargo workspace architecture into project-json, I'm not sure that this was something @matklad had in mind when giving this alternative format for project descriptions.

Is there a need for things like BuildScript and Example? Bin, Lib and Test (and Other ofc) seem fine as they are rather general.

I think #16135 (comment) answers this, but, yeah good catch of the specific smell here!

They're not really needed, no—they were meant to try and make project-json more Cargo-like. I only really need Bin, Lib and Test if this PR doesn't take the approach that matklad suggested in his comment.

Veykril · 2024-01-10T09:22:34Z

crates/rust-analyzer/src/target_spec.rs

+    /// [`project_manifest`] corresponds to the file defining a crate. With Cargo,
+    /// this will be a Cargo.toml, but with a non-Cargo build system, this might be
+    /// a `TARGETS` or `BUCK` file. Multiple, distinct crates can share a
+    /// single `project_manifest`.


This comment confuses me given where its placed, this is the field of a CargoTargetSpec, so this should always be a Cargo.toml.

Ah—the intention behind this comment is to communicate that a non-Cargo project can also have a TargetSpec (née CargoTargetSpec), with the intention of trying to make non-Cargo build systems be more Cargo-like in order to reduce the maintenance burden on rust-analyzer, but as mentioned elsewhere, that might be a design bug.

I get that, it's just that the location for this command seems ill placed (as it talks about this struct, but also the copy, yet we are in the docs on the field), though i don't have a better spot for it either aside from somewhere in the module documentation.

Veykril · 2024-01-10T09:24:30Z

docs/dev/lsp-extensions.md

@@ -372,7 +372,7 @@ interface Runnable {
 }
 ```

-rust-analyzer supports only one `kind`, `"cargo"`. The `args` for `"cargo"` look like this:
+rust-analyzer supports two `kind`s of runnables, `"cargo"` and `"rustproject"`. The `args` for `"cargo"` look like this:


projectjson, not rustproject right?

Yes, sorry! I get the two switched up in my head.

Veykril · 2024-01-10T09:24:43Z

docs/dev/lsp-extensions.md

@@ -385,6 +385,16 @@ rust-analyzer supports only one `kind`, `"cargo"`. The `args` for `"cargo"` look
 }
 ```

+The args for `"jsonproject"` look like:


projectjson

Veykril · 2024-01-10T09:25:55Z

editors/code/src/run.ts

+            args: args.slice(1),
+            cwd: projectJsonRunnableArgs.workspaceRoot,
+            env: prepareEnv(runnable, config.runnablesExtraEnv),
+            overrideCargo: "buck2",


Does this mean only buck2 is supported for projectjson runnables?

Yeah, ideally we'd fetch buck2 string from projectJsonRunnableArgs

Oh, yeah, sorry—I meant to pull this string from configuration. Didn’t realize I left it here.

matklad · 2024-01-10T11:01:19Z

crates/project-model/src/project_json.rs

@@ -202,6 +243,8 @@ struct CrateData {
    is_proc_macro: bool,
    #[serde(default)]
    repository: Option<String>,
+    #[serde(default)]
+    target_spec: Option<TargetSpecData>,


Some notes on JSON side of things here:

I'd maybe call the top-level thing build_info, as thre might be multiple targets (in buck2 sense) concerning a single crate. And, well, target is already overloaded, so it's better to avoid pilling more meanings onto that poor word.

I think the core feature here should be an inclusion of opaque blob of data, which is transmitted over LSP so that the custom client may interpret this. So, I'd say the basis should look like

build_info: Option<BuildInfo> struct BuildInfo { data: serde_json::Value }

Eg, one can imagine a situation where all builds are remote, and the way you "run" software is by issueing an HTTP request to a remote execution server, and opening a page in the web-browser with results. In this situation, the data would be an URL to call, and the client-side extension would take care of HTTPing.

But we also should support "standard" workflows where you just run some command line.

So, something like (pardon my TypeScript)

struct BuildInfo { data: serde_json::Value runnables: Vec<Runnable> } struct Runnable { kind: "build" | "check" | "test" | "run" | string, label: string, argv: string[], cwd: string }

This shouldn't use a json map, but rather an array, to be more flexible. Eg, in the future we might want to add --release and non---release versions of tests, and with a list it should be simpler. More generally, I've recently read some JSON API guidlines somewhere (don't remember whether though), and one guideline that stuck with me is that you should represent maps in JSON as arrays (assoc lists). I think that's a good guideline and something we should be using in gerneral.

Then, the kind should be a semi-open enum. rust-analyzer should treat build, run, check and test specially, but it should also be able to show other tasks in GUI. Eg, you can imagine a "flush" task for an embedded build system, or some such.

We might want to split check into two --- one check that is run for the user and displays errors in the terminal, and another check that runs with message-format=json and is used for flycheck. I guess we could call those check and flycheck?

For tests, we need to think how to enable filtering. The core feature is that you can run a single test using cargo test -- $TEST_NAME, and all tests using cargo test. Note that this might be very different in non-cargo build systems. Eg, it could be builder test and builder test --test-filter=$TEST_NAME, where builder test --test-filter='' would run zero tests, rather than all tests. So I think for test, we should also split it into two: "testall" and "testone". And, for "testone", I think we want to make argv a template. That is, argv could look like

["builder", "test", "--test-filter=$$TEST_NAME$$", "./buildfile.b"]

We shouldn't assume that filter is always last, and that its always a separate argument.

Thanks for the feedback here!

I'd maybe call the top-level thing build_info, as thre might be multiple targets (in buck2 sense) concerning a single crate. And, well, target is already overloaded, so it's better to avoid pilling more meanings onto that poor word.

That's a fair point. I didn't intend for the naming to be Buck/Bazel-esque "targets", I mean to invoke the target concept in Cargo, of all things. In any case, target is too overloaded and I'll move over to build_info.

I think the core feature here should be an inclusion of opaque blob of data, which is transmitted over LSP so that the custom client may interpret this.

Is the idea that a companion extension to rust-lang.rust-analyzer (e.g., something for Buck or Bazel) would be responsible for assembling that opaque blob of data into a some sort of meaningful runnable command or is more that since I'm making this change now, I should make it more futureproof ("[B]ut we also should support "standard" workflows where you just run some command line.")? I ask since I'm working on a variable interpreter/template thingy, which—I think—should make expressing arbitrary build/test commands a lot easier in rust-analyzer itself, which reduces the pressure/need for end-users to install other extensions for runnables.

This shouldn't use a json map, but rather an array, to be more flexible. Eg, in the future we might want to add --release and non---release versions of tests, and with a list it should be simpler. More generally, I've recently read some JSON API guidlines somewhere (don't remember whether though), and one guideline that stuck with me is that you should represent maps in JSON as arrays (assoc lists). I think that's a good guideline and something we should be using in gerneral.

That seems reasonable to me! I was considering how to tackle this for Buck¹ and this seems like a good approach.

Then, the kind should be a semi-open enum. rust-analyzer should treat build, run, check and test specially, but it should also be able to show other tasks in GUI. Eg, you can imagine a "flush" task for an embedded build system, or some such.

Yeah, that seems smart.

We might want to split check into two --- one check that is run for the user and displays errors in the terminal, and another check that runs with message-format=json and is used for flycheck. I guess we could call those check and flycheck?

I think now's a good time to make this change, especially if the new runnable API is a breaking change.

For tests, we need to think how to enable filtering. The core feature is that you can run a single test using cargo test -- $TEST_NAME, and all tests using cargo test. Note that this might be very different in non-cargo build systems. Eg, it could be builder test and builder test --test-filter=$TEST_NAME, where builder test --test-filter='' would run zero tests, rather than all tests. So I think for test, we should also split it into two: "testall" and "testone". And, for "testone", I think we want to make argv a template. That is, argv could look like: [redacted for quoting ease]

I don't think I've fully gotten this into my head yet (sorry, lots of travel!) but I think I see the utility of a change like this.

I think I'd like like a debug variant as well, but I haven't really through this specific aspect through.

Footnotes

For context, buck2 is moving to a modifier-style approach. A user can write buck2 build repo//foo:bar?linux,release, which translate to "build repo//foo:bar for Linux using release". there's a more verbose --modifier-style API, but I suspect that both options can work when called from rust-analyzer or I suspect the Buck team will be flexible in implementing this. ↩

Veykril · 2024-01-12T09:26:54Z

Overall implementation wise it looks good to me, al though I am still a bit on the fence wrt to having flycheck be in here. I am personally still of the opinion that flycheck should not be a direct part of r-a (but a separate extension altogether). Adding it to this file makes that less clear of a separation.

I guess whether the format has that or not makes little difference, the command itself is heavily tied into the rest of the specification here whether it is supported by a separate extension or r-a.

davidbarsky · 2024-01-12T20:33:05Z

Overall implementation wise it looks good to me, al though I am still a bit on the fence wrt to having flycheck be in here. I am personally still of the opinion that flycheck should not be a direct part of r-a (but a separate extension altogether). Adding it to this file makes that less clear of a separation.

I guess whether the format has that or not makes little difference, the command itself is heavily tied into the rest of the specification here whether it is supported by a separate extension or r-a.

While I agree with you that flycheck is a hack, I've found myself of the opinion that rust-analyzer (and specifically, its users who are not interested/aware of the need to install a build system-specific extension) can likely get a ridiculous amount of mileage through a templates commands and a promise from build systems to place rustc diagnostics in a deterministic location (like, say, stdout).

And yeah, I think the command's specification is largely immaterial to where it runs, or at the very least, it's not a binding commitment in one direction or another.

bors · 2024-01-19T09:56:12Z

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

bors · 2024-02-20T10:01:15Z

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

bors · 2024-03-15T13:28:09Z

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

Refactor extension to support arbitrary shell command runnables Currently, the extension assumes that all runnables invoke cargo. Arguments are sometimes full CLI arguments, and sometimes arguments passed to a cargo subcommand. Refactor the extension so that tasks are just a `program` and a list of strings `args`, and rename `CargoTask` to `RustTask` to make it generic. (This was factored out of #16135 and tidied.)

bors · 2024-03-29T02:19:48Z

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

bors · 2024-04-01T10:30:00Z

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

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 15, 2023

davidbarsky force-pushed the davidbarsky/extend-cargotargetspec-to-rust-project branch 4 times, most recently from 20733f6 to 8903c89 Compare December 19, 2023 16:57

davidbarsky force-pushed the davidbarsky/extend-cargotargetspec-to-rust-project branch from 8903c89 to d91586f Compare December 24, 2023 00:17

Veykril reviewed Jan 10, 2024

View reviewed changes

Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2024

matklad reviewed Jan 10, 2024

View reviewed changes

Veykril mentioned this pull request Jan 10, 2024

Add $manifest_path substitution for cargo override commands #13528

Closed

davidbarsky mentioned this pull request Jan 31, 2024

feature: Substitute $saved_file in custom check commands #15476

Merged

P1n3appl3 mentioned this pull request Jan 31, 2024

Add an Optional CargoTargetSpec-like Data to rust-project.json #15892

Open

Wilfred force-pushed the davidbarsky/extend-cargotargetspec-to-rust-project branch from d91586f to ec581f9 Compare February 16, 2024 19:50

Wilfred force-pushed the davidbarsky/extend-cargotargetspec-to-rust-project branch from ec581f9 to 11c7eac Compare March 12, 2024 19:52

This was referenced Mar 14, 2024

Refactor extension to support arbitrary shell command runnables #16839

Merged

Allow rust-project.json to include arbitrary shell commands for runnables #16840

Open

Wilfred force-pushed the davidbarsky/extend-cargotargetspec-to-rust-project branch 2 times, most recently from 089a35d to 843626e Compare March 28, 2024 00:26

Extend TargetSpec functionality to rust-project.json

4c8759c

Wilfred force-pushed the davidbarsky/extend-cargotargetspec-to-rust-project branch from 843626e to 4c8759c Compare March 29, 2024 21:41

davidbarsky closed this Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend `TargetSpec` functionality to rust-project.json #16135

Extend `TargetSpec` functionality to rust-project.json #16135

davidbarsky commented Dec 15, 2023

bors commented Dec 20, 2023

Veykril left a comment

Veykril Jan 10, 2024

matklad Jan 10, 2024

davidbarsky Jan 10, 2024

Veykril Jan 10, 2024

davidbarsky Jan 10, 2024

Veykril Jan 12, 2024 •

edited

Veykril Jan 10, 2024

davidbarsky Jan 10, 2024

Veykril Jan 10, 2024

Veykril Jan 10, 2024

matklad Jan 10, 2024

davidbarsky Jan 10, 2024

matklad Jan 10, 2024

davidbarsky Jan 12, 2024

Veykril commented Jan 12, 2024

davidbarsky commented Jan 12, 2024

bors commented Jan 19, 2024

bors commented Feb 20, 2024

bors commented Mar 15, 2024

bors commented Mar 29, 2024

bors commented Apr 1, 2024

Extend TargetSpec functionality to rust-project.json #16135

Extend TargetSpec functionality to rust-project.json #16135

Conversation

davidbarsky commented Dec 15, 2023

bors commented Dec 20, 2023

Veykril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Veykril Jan 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Veykril commented Jan 12, 2024

davidbarsky commented Jan 12, 2024

bors commented Jan 19, 2024

bors commented Feb 20, 2024

bors commented Mar 15, 2024

bors commented Mar 29, 2024

bors commented Apr 1, 2024

Extend `TargetSpec` functionality to rust-project.json #16135

Extend `TargetSpec` functionality to rust-project.json #16135

Veykril Jan 12, 2024 •

edited