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

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Wilfred
Copy link
Contributor

@Wilfred Wilfred commented Mar 15, 2024

This is a follow-up on #16135, resolving the feedback raised :)

Allow rust-project.json to include shell runnables, of the form:

{
  "build_info": {
    "label": "//project/foo:my-crate",
    "target_kind": "bin",
    "shell_runnables": [
      {
        "kind": "run",
        "program": "buck2",
        "args": ["run", "//project/foo:my-crate"]
      },
      {
        "kind": "test_one",
        "program": "test_runner",
        "args": ["--name=$$TEST_NAME$$"]
      }
    ]
  }
}

If these runnable configs are present for the current crate in rust-project.json, offer them as runnables in VS Code.

This PR required some boring changes to APIs that previously only handled cargo situations. I've split out these changes as commits labelled 'refactor', so it's easy to see the interesting changes.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 15, 2024
@bors
Copy link
Collaborator

bors commented Mar 15, 2024

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

@Wilfred Wilfred force-pushed the shell_runnable branch 5 times, most recently from 65e9be3 to c7bdd91 Compare March 15, 2024 23:14
@Wilfred Wilfred marked this pull request as ready for review March 15, 2024 23:14
@bors
Copy link
Collaborator

bors commented Mar 18, 2024

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

@bors
Copy link
Collaborator

bors commented Mar 21, 2024

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

@@ -87,6 +87,21 @@ pub struct Crate {
pub(crate) exclude: Vec<AbsPathBuf>,
pub(crate) is_proc_macro: bool,
pub(crate) repository: Option<String>,
pub build_info: Option<BuildInfo>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i meant to change this in the original PR, but build_info's runnables should be under ProjectJson, not the Crate. label/target_kind should stay under Crate, however.

(that assumes a Bazel/Buck-style labeling system, but even I think for non-Cargo build systems, that's a good enough bar to set.)


```typescript
{
kind: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should the LSP client use the kind field? Does it describe what the runnable does? Then how does it differ then from label field of the parent Runnable?

Also should the "shell" kind have a expectTest field just like the "cargo" kind?

BTW, how should the LSP client interpret the workspaceRoot, expectTest, and overrideCargo fields?

Additionally, I think it would make sense to copy the example from the Pull Request into the lsp-extension.md file, or just reference a user-level documentation describing this feature.

Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, shouldn't this PR explain the new build_info field of rust-project.json in manual.adoc?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When expectTest is set the client should set the UPDATE_EXPECT env var to 1 when invoking the runnable. ideally there should be a field for arbitrary env var overrides in the payload, then this field could be replaced by the server instructing the client to just set the env var with that general mechanism as we'll need this for other things as well.

overrideCargo I think should just replace the cargo command that is being used by the client.

@bors
Copy link
Collaborator

bors commented Mar 29, 2024

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

@Veykril
Copy link
Member

Veykril commented Apr 18, 2024

This is a follow-up on #16135, resolving the feedback raised :)

So this means #16135 can be closed in favor of this one?

@davidbarsky
Copy link
Contributor

This is a follow-up on #16135, resolving the feedback raised :)

So this means #16135 can be closed in favor of this one?

Yup! Just closed. Wilfred or I will push to this branch as needed.

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me (aside from what has been raised already by others)

@Veykril Veykril removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 18, 2024
@Veykril Veykril added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants