Stream: t-compiler/wg-rls-2.0

Topic: Runnables


vsrs (Jun 02 2020 at 12:40, on Zulip):

matklad said:

I'd say "running" runnables should be left to the client

to run a runnable we need a compilation artifact, i.e. we need to know how to build it. And this is a main problem. I think a client should not know how to build the artifact, just be able to run it.

vsrs (Jun 02 2020 at 12:40, on Zulip):

Jeremy Kolb said:

@vsrs I think there's some truth to that. It would be nice if we could provide some sort of standard "debugger engine hooks" or conform to some standard "test adapter api" for those types of things.

There is https://build-server-protocol.github.io/, but I do not like how it works.

matklad (Jun 02 2020 at 12:40, on Zulip):

Not sure, I think building artifact should also be a responsibility of the client. I think we shouldn't really assume any specific build system

matklad (Jun 02 2020 at 12:41, on Zulip):

@vsrs @Jeremy Kolb splited the topic here

matklad (Jun 02 2020 at 12:42, on Zulip):

Not sure, I think building artifact should also be a responsibility of the client

The reasoning is that the user should have direct access to the processes being run (ie, they should see stdout/stderrr and a button to kill a runaway process). It seems this is best handled by the client

vsrs (Jun 02 2020 at 12:42, on Zulip):

Than much more logic will be on the client side. And each client should implement it from scratch.

matklad (Jun 02 2020 at 12:44, on Zulip):

That's true, but I also think that this is how it should work, sadly. Different clients have different UXes for runnning external processes, and I think an ideal language plugin should leverage those uxes,

matklad (Jun 02 2020 at 12:44, on Zulip):

Well, I can also imagine some generic "process handler" on the protocl level, but that goes beyond just running...

vsrs (Jun 02 2020 at 12:46, on Zulip):

Make sense. Maybe then it worth to have a set of commands implemented on the client side (build, run, debug) and a way the client can let server know about this commands, so the server can use them?

matklad (Jun 02 2020 at 12:46, on Zulip):

The two specific problems I am thinking about are:

matklad (Jun 02 2020 at 12:48, on Zulip):

Ideally, there should be second-order client plugins. For example, one can imagine a plugin to run a test under miri or under some code coverage tracking tool. These plugins should be implementable without changing code inside rust-analyze

Jeremy Kolb (Jun 02 2020 at 12:52, on Zulip):

vsrs said:

Jeremy Kolb said:

@vsrs I think there's some truth to that. It would be nice if we could provide some sort of standard "debugger engine hooks" or conform to some standard "test adapter api" for those types of things.

There is https://build-server-protocol.github.io/, but I do not like how it works.

FYI I kind of asked about this: https://github.com/build-server-protocol/build-server-protocol/issues/120

matklad (Jun 02 2020 at 12:54, on Zulip):

@vsrs do you know if it would be possible to provide extension API on the VS Code side?

vsrs (Jun 02 2020 at 12:54, on Zulip):

May be then we should replace Runnable bin field with something like type? So the client can interpret it and do the appropriate actions. Not just merely pass it to a shell.

matklad (Jun 02 2020 at 12:54, on Zulip):

Like, to maybe move debugger integrations outside ?

matklad (Jun 02 2020 at 12:55, on Zulip):

@vsrs that's exactly what I want to do yeah

vsrs (Jun 02 2020 at 12:55, on Zulip):

matklad said:

vsrs do you know if it would be possible to provide extension API on the VS Code side?

I'm not sure what are you talking about.

matklad (Jun 02 2020 at 12:56, on Zulip):

More specifically, I want to provide two views of runnable on the LSP layer:

runnable {
   "cargo": { "package": "foo", "target": "bar" ... }
    "process": {"bin": "cargo", "args": []}
}
matklad (Jun 02 2020 at 12:56, on Zulip):

So that smart clients can intercept cargo and plugin debugging inside

matklad (Jun 02 2020 at 12:56, on Zulip):

And dumb clients can just run a preconfigured process

matklad (Jun 02 2020 at 12:57, on Zulip):

I'm not sure what are you talking about.

Yeah, bad wording on my side. I guess, I am asking this: can someone write a VS Code extension, which would depend on rust-analyzer extensions, and would be able to use rust-analzyer's runnables?

matklad (Jun 02 2020 at 12:57, on Zulip):

Basically, can one extension call functions from another extensions?

vsrs (Jun 02 2020 at 12:59, on Zulip):

And dumb clients can just run a preconfigured process

Anyway a client have to support LSP extention, so why not to support cargo... too. Not sure that we need process.

vsrs (Jun 02 2020 at 13:01, on Zulip):

matklad said:

I guess, I am asking this: can someone write a VS Code extension, which would depend on rust-analyzer extensions, and would be able to use rust-analzyer's runnables?

Yes, I think it is possible. Just not sure it is the best way to go.

vsrs (Jun 02 2020 at 13:05, on Zulip):

A side question. Is it possible to run several tests with cargo?
something like cargo pkg::test1_explicit_name pkg::test2_explicit_name, etc..

matklad (Jun 02 2020 at 13:07, on Zulip):

@vsrs not at the moment I think

matklad (Jun 02 2020 at 13:08, on Zulip):

You can filter by test name prefix, but not by arbitrary set of tests

matklad (Jun 02 2020 at 13:08, on Zulip):

There's a long-standing issue on Cargo to allow that (from IntelliJ days)

vsrs (Jun 02 2020 at 13:08, on Zulip):

:(

matklad (Jun 02 2020 at 13:09, on Zulip):

Hm, yeah, this is really tough :(

matklad (Jun 02 2020 at 13:09, on Zulip):

Who should do m.insert("RUST_BACKTRACE".to_string(), "short".to_string()); ? Client? Or the Server?

vsrs (Jun 02 2020 at 13:10, on Zulip):

Thanks.

I need this to implement Run all tests test_lens.gif

vsrs (Jun 02 2020 at 13:12, on Zulip):

matklad said:

Who should do m.insert("RUST_BACKTRACE".to_string(), "short".to_string()); ? Client? Or the Server?

I thinks someone who knows the build context. So looks like it should be client (if he is responsible for building)

matklad (Jun 02 2020 at 13:14, on Zulip):

Yeah, I think this probably makes sense

vsrs (Jun 02 2020 at 13:17, on Zulip):

And what if supply RA as an extension bundle: rust-analyzer itself and a cargo-based additional extension responsible for lenses, build targets, etc. I've just checked there no problem using ra commands from withing another extension.

matklad (Jun 02 2020 at 13:18, on Zulip):

@vsrs how does the following look to you?

#[derive(Deserialize, Serialize, Debug)]
#[serde(rename_all = "camelCase")]
pub struct Runnable {
    pub range: Range,
    pub label: String,
    pub kind: RunnableKind,
    pub args: CargoRunnable,
}

#[derive(Serialize, Deserialize, Debug)]
#[serde(rename_all = "lowercase")]
pub enum RunnableKind {
    Cargo,
}

struct CargoRunnable {
    cargo_toml: PathBuf,
    command: String,
    // --package and --lib stuff
    cargo_args: Vec<String>,
    // stuff after --
    executable_args: Vec<String>,
}
vsrs (Jun 02 2020 at 13:23, on Zulip):

I would add pub name_range: TextRange to Runnable. To reference a test in UI, for example. or maybe 'navigation_range'

matklad (Jun 02 2020 at 13:25, on Zulip):

I think that should be target_range (with possible addion of target_selection_range later)

matklad (Jun 02 2020 at 13:25, on Zulip):

Maybe we should just use LocationLink here even...

vsrs (Jun 02 2020 at 13:30, on Zulip):

Sorry, I messed up the types. Of course in LSP it is Range or (better) Location.

matklad (Jun 02 2020 at 13:35, on Zulip):

Ah, so you think its importrant that we have both the full range and name range?

matklad (Jun 02 2020 at 13:35, on Zulip):

That seams reasonable, should just plug LocationLink in there then

vsrs (Jun 02 2020 at 13:37, on Zulip):

Yes, definitely. You can look at the gif above. When I hit 5 tests I need name range to show references to tests

vsrs (Jun 02 2020 at 13:41, on Zulip):

LocationLink may not be necessary. There is no origin in this case. Simple Location looks better for me.

matklad (Jun 02 2020 at 13:42, on Zulip):

But Location has only one range

matklad (Jun 02 2020 at 13:42, on Zulip):

I think we need range of the whole function, as well as the range of identifier

vsrs (Jun 02 2020 at 13:45, on Zulip):

Ups, I thought you want to leave existing range field and add LocationLink to a name.

Yes, I'm +1 for the single LocationLink :)

matklad (Jun 02 2020 at 15:09, on Zulip):

Hm, one wrinkle here is that code-lens are implemented on the server...

matklad (Jun 02 2020 at 15:10, on Zulip):

I wonder if we should move them to the client-side instead... They need custom commands anyway...

vsrs (Jun 02 2020 at 15:33, on Zulip):

Why not pass the list of supported commands to the server during initialization? As sort of client capabilities.

matklad (Jun 02 2020 at 15:35, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/pull/4710/files

vsrs (Jun 02 2020 at 15:47, on Zulip):

vsrs said:

Why not pass the list of supported commands to the server during initialization? As sort of client capabilities.

Though.. it's not a good idea. It's better to leave all the logic on one side. The client - ok let it be.

Last update: Sep 27 2020 at 13:45UTC