Feature: "Download latest language server" command for VS Code

[Veetaha]

The task is to implement "Download latest language server" command that the users will be able to use thru the command palette (by pressing Ctrl+Shift+P).

This command does the following:

• Fetches latest binary release metadata.
• If the local version of ra_lsp_server prebuilt binary is greater or equal to the released one just exits early
• If the extension is already running a prebuilt binary of ra_lsp_server, then it is stopped
• If $__RA_LSP_SERVER_DEBUG env variable or "rust-analyzer.raLspServerPath" are pointing to the prebuilt binary, i.e. ~/.config/Code/User/globalStorage/matklad.rust-analyzer/ra_lsp_server-linux the lsp server is shutdown.
• Otherwise the current ra_lsp_server process is not affected.
• Downloads the proper ra_lsp_server-(linux|mac|windows.exe) binary and stores (overwrites if it already exists) it in globalStoragePath
• Restarts the ra_lsp_process with the new downloaded binary if it was stopped.

Notes:
I have already tried to implement this but failed to do this because I estimated that it would bring a lot of changes to code in existing PR. This is due to the design of our class Config in config.ts. I initially wanted to update the config with WorkspaceConfig["update"] method so that when issuing "Download latest language server" command rust-analyzer.raLspServer setting is overwritten in the user's settings.json. But this is a bit complex with the current config design and it is compilated to decide which VS Code config to update since it has the notion of default, user, workspace and workspace folder configs that are merged in lower-scope-first priority. Not sure whether we should try to overwrite settings.json or not when issuing this command.

What are you thoughts @matklad, @lnicola, @kiljacken, @bjorn3?

[Veetaha]

Also, it would be helpful if we switched to something like semver for the naming of our releases to easier compare versions and track breaking and backward-compatible changes. Can you please elaborate on the chosen versioning scheme, @matklad?

[kjeremy]

Is there any way around setting rust-analyzer.raLspServer? It might make more sense to have some logic based on published versions.

[Veetaha]

raLspServerParh is too simple and straightforward to build some logic (don't even imagine what specific logic) around it, IMO

[kjeremy]

What I mean is: shouldn't only published versions of the client attempt to download the server?

[matklad]

What I mean is: shouldn't only published versions of the client attempt to download the server?

I am thinking that we might actually want want to have an explicit setting for "download the server", "raLspServer": "bundled", and, if no setting is specified, show do the following

• if ther's ra_lsp_server in PATH, ask wherether to download or to use the one from path, and persist choice in the settings
• if there's no server in PATH, ask to download, but keep default settings.

[Veetaha]

I think another configuration property will bring unnecessary complexity and more invariants to preserve. Besides that, bundled binary should be the unspoken default and when we introduce "raLspServer": "bundled" it doesn't change a thing. We can just tweak the current implementation such that if the user didn't specify raLspServerPath and does have ra_lsp_server in $PATH in the message we add yet another button:

• Download now
• Use custom ra_lsp_server
• Cancel

[matklad]

The problem with that is that they'll see the message every time, until they remove ra-lsp-server from path.

But maybe that's OK?

But, in general, there's a value in distinguishing between "no config is specified, so use a default" and "a default is explicitly specified", as the latter allows smother evolution (you can change the default without breaking existing users).

[Veetaha]

My main concern is not to burden the users with tons of configuration and reading up the docs on them. The common user should download the extension and it should just work. If she uses some platform where we don't have suitable prebuilt binaries, then she should just build them herself and opt in to specify raLspServerPath to them and that's all. There is no need to ask the user for Download now thereafter. Also, once the user presses Use custom ra_lsp_server we just manually update her settings.json with "raLspServerPath": "ra_lsp_server" and don't ask her thereafter too.
Also, if you want to be explicit about the default of using prebuilt binaries you can specify:

{
    "rust-analyzer.raLspServerPath": null
}

Though this is too unintuitive, as for me.
And don't forget that these settings should be forward-compatible with auto-updates detection in future.

[Veetaha]

I understand your concern too, maybe we need to investigate how other LSP projects tackle this.

[Veetaha]

• cpptools:
  They provide platform-specific packages (binaries are packed in .vsix archives) for offline installation, but the extension that is published to the marketplace downloads the binaries (they have some logic to identify which way the extension was installed).
  It's very hard to understand their code (it is quite a big and messy codebase).
  They have quite a list of binaries to download in package.json, I've found only something that looks like an option to override a path for clang_format

• kotlin
  Downloads the binaries from github almost as we do, provides a single setting for a path to the language server. They check if it is present and only if not, they download the binary (as we do), link to code

• omnisharp
  Do uses the same "runtimeDependencies" property in package.json as cpptools (haven't found anything on that property name in Google, it is used by the extension itself, not sacral meaning).
  "omnisharp.path" setting allows users to specify an explicit absolute path to OminSharp executable, a specific version number, or "latest". If a version number or "latest" is specified, the appropriate binary is downloaded

[Veetaha]

The approach of OmniSharp seems like a compromise to me.
We can make it better like:

{
    "rust-analyzer.language-server": {
        "path": "/some/path" // "ra_lsp_server" is not treated specially (it just works through $PATH as now)
    },
    // "path" and "prebuilt" are mutually exclusive
    "rust-analyzer.language-server": {
        "download": "latest" // or "2020-02-10" or semver version if we switch to that...
        // latest means autoupdates are "on" (when we implement them)
    }
} 

Or if you don't like nesting, we may change these to "rust-analyzer.language-server.path" and "rust-analyzer.language-server.download" respectively.
@matklad , @kjeremy , would you still stand for "raLspServer": "bundled"? I wouldn't argue much if you do still think it is better.

[Veetaha]

I'd accept any config shape. I just want to get this work done by the upcoming release so that the users will be able to upgrade to the latest ra_lsp_server prebuilt binary without having to figure out where their globalStoragePath points to and manually remove the binary...

[matklad]

We can make it better like:

I actually think that omnisharp approach is best.

Internally, we definitely should have a single config

enum ServerBinary {
    ReleaseChannel(alpha | nightly), // we should start publishing nightlies at some point
    Path(PathBuf),
    Version(String), // don't think we'll need it any time soon
}

Extenally, I think it would be useful if this config maps to a single property in the config. If there are two properties, we'll have trouble validating them both, and that wold be unclear for the user which one take precedence. The problem is that we don't have enums in JSON, but strings seems to be pretty self-explanatory:

"langauge-server": "latest",
"langauge-server": "latest-nightly",
"langauge-server": "ra_lsp_server",
"langauge-server": "~/projects/rust-analyzer/target/release/ra_lsp_server",
"langauge-server": "2020-02-01",

Of course, by making certain string magical, we prevent cases like "what If I have a binary whcih is literarly called latest?", but I think this corner cases are non-existent in practice.

[Veetaha]

Yes, internally it will map to a single enum config property too. This already looks almost exactly like the enum you have written, just the type tag is explicit (though in Rust it is well-hidden from us).
https://github.com/rust-analyzer/rust-analyzer/blob/5bf669860984a2c058b3bdc3e43b4993a0f25b31/editors/code/src/installation/interfaces.ts#L14-L55

Well, I am glad that we accepted the proposal to simplify the config to a single property. Though I am not sure that we should change the config in scope if implementing the Download latest language server command... Or what is your feeling about that, @matklad?

Also, I'd like to add some bits to the initial algorithm for the command in the description.

• Our download script also saves the version of the downloaded binary in ctx.globalStorage property.
• The command overrides setting.json file of the user to "langauge-server": "latest" or "language-server": "<specific_version>" once the user issues the command (not sure which one exactly to choose yet). But this is complicated, since the user can have multiple configs (multiple settings.json files), namely default, global, workspace, workspace folder.

For the first impl, I suggest to only override the global config, since we do use one globalStoragePath for our binary and overriding language-server settings in particular workspace would break other workspaces anyway...

[Veetaha]

@matklad, I have reevaluated this issue and I've found some drastic flaw in our current downloading process.

I know you said that this might not be an issue, but please-please evaluate the following rationale carefully and rationally (very please):

• We download ra_lsp_server into globalStoragePath which (I checked) is not cleared when you uninstall the extension, and probably (didn't check) also not cleared when you update the extension through the marketplace.
• When the user updates to a new version of vscode TypeScript extension we only check that the binary at globalStoragePath is available, but we don't ensure that its version is compatible with the updated TypeScript extension. (I know you said not bothering about versions and just suppose the user always has latest lsp server and frontend, but in the current implementation we are actually creating the version inconsistency ourselves!)
• We cannot guarantee that in case of version drift between the frontend and lsp server everything works fine.

Based on these statements I have concluded the following:

• We must keep the version of lsp server and frontend equal to each other by default. If the user wants to update her lsp server, she is ought to update the frontend too (otherwise version drift).
• The user may opt in using a custom implementation of ra_lsp_server (e.g. built from sources) by specifying a path to it, so that she manually manages keeps the versions of her lsp server and frontend in sync.

Based on the conclusions I'd like to propose the following:

• We do not provide a "Download latest language server" command (close this issue).
• The TypeScript extension never downloads the latest version of ra_lsp_server, but instead downloads the version that is equal to the version of TypeScript frontend itself.
• We check the availability of a new rust-analyzer release each time the extension is activated and issue a pop-up info message that proposes the user to update her extension (if she has auto-updates from marketplace turned off).
• Upon extension activation we check the version of ra_lsp_server and compare it with the version of TypeScript frontend itself. If they mismatch we do download the same released version of ra_lsp_server from github and override the existing binary.

If you have arguments to this, please refer to a specific statement and propose a better solution! I'd like you to think of not the closest, but more distant future. We've already implemented a solution for near-future, now we should improve it and make our release process stable and not care about it thereafter.

[matklad]

- We cannot guarantee that in case of version drift between the frontend and lsp server everything works fine. I don’t thinks this is a significant problem, I feel “update to the latest version” will work fine in the majority of cases, and will be simpler than keeping explicit versions database. It also allows us to release servers faster than clients, which is something we want for deploying hot fixes. But my primary consideration is doing the simplest thing possible first.

…

On Saturday, 15 February 2020, Veetaha ***@***.***> wrote: @matklad <https://github.com/matklad>, I have reevaluated this issue and I've found some drastic flaw in our current downloading process. I know you said that this might not be an issue, but please-please evaluate the following rationale carefully and rationally (very please): - We download ra_lsp_server into globalStoragePath which (I checked) is not cleared when you uninstall the extension, and probably (not checked) also not cleared when you updated the extension through the marketplace. - When the user updates to a new version of vscode TypeScript extension we only check that the binary at globalStoragePath is available, but we don't ensure that it's version is compatible with the updated TypeScript extension. (I know you said not bothering about versions and just suppose the user always has latest lsp server and frontend, but in the current implementation we are actually creating the version inconsistency ourselves!) - We cannot guarantee that in case of version drift between the frontend and lsp server everything works fine. Based on these statements I have concluded the following: - We *must* keep the version of lsp server and frontend equal to each other by default. If the user wants to update his lsp server, he *is ought to* update the frontend too (otherwise version drift). - The user *may* opt in using a custom implementation of ra_lsp_server (e.g. built from sources) by specifying a path to it, so that he manually manages keeps the versions of his lsp server and frontend in sync. Based on the conclusions I'd like to propose the following: - We do not provide a "Download latest language server" command (close *this* issue). - The TypeScript extension never downloads the latest version of ra_lsp_server, but instead downloads the version that is equal to the version of TypeScript frontend itself. - We check the availability of a new rust-analyzer release each time the extension is activated and issue a pop-up info message that proposes the user to update his extension (if she has auto-updates from marketplace turned off). - Upon extension activation we check the version of ra_lsp_server and compare it with the version of TypeScript frontend itself. If they mismatch we do download *the same* released version of ra_lsp_server from github and override the existing binary. If you have arguments to this, please refer to a specific statement and propose a better solution! I'd like you to think of not the closest, but more distant future. We've already implemented a solution for near-future, now we should improve it and make our release process stable and not care about it thereafter. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3073>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M5KGLETRM2EBMM5BHDRC7VWFANCNFSM4KSEU7MA> .

[Veetaha]

@matklad, I agree on doing the simplest possible thing first! It may seem that keeping versions in sync is the harder thing, though trust me, Download latest language server is the harder (I've already tried to implement it and there are a lot of caveats, which I wrote about you in PM).

I didn't propose to keep explicit versions database in no sentence! And what do you mean by versions database? All the versions are kept at GitHub releases automatically and we just need to specify the release name to fetch the appropriate one for our TypeScript extension frontend.

So upon each release we hardcode its name in our binaries so that the frontend knows which ra_lsp_server to download from GitHub releases and that's all (the same approach omnisharp practices in their extension). We may consider auto-checking for updates separately, this is just a ux-improvement.

Besides that single statement, do you have arguments for the rest of my thinking?

[matklad]

I am actually fine with either solution, but (without examining details to close), Download latest language server stlll seems easier (and more flexible re hotfixes, but this is a minor point) to me.

Like, we already have code to download the server. We only need to

a) shutdown the existing server
b) run the existing download code without modifications
c) restart the client.

But, again, I haven't looked closely at the relevant APIs so I might be mistaking.