nim-lang / vscode-nim

A VS Code plugin for the Nim language
Other
63 stars 5 forks source link

+ if nimlangserver is not found, ask the user before attempting to 'nimble install' it. #46

Closed nickysn closed 4 months ago

nickysn commented 4 months ago

I don't like plugins that auto install stuff without asking me. I might want to install nimlangserver through other means, such as the distro package manager. Or maybe I'm working on the nimlangserver itself, and I have a working development version, but I forgot to add it to the PATH. Or maybe I want to install it manually from the console, so I can inspect any build messages and debug issues. In such cases, I don't want stuff auto installed in my home directory. And people who do want that, can simply click "Yes" in the dialog shown.

Screenshot of the new dialog: image

jmgomez commented 4 months ago

Im not sure about this as it goes against the principle "everything works out of the box without user intervention" that we are aiming to. If you want to install through other means, and you do it before, it shouldnt install anything extra. We could also have an option that the user can toggle on/off to skip the installation.

nickysn commented 4 months ago

Im not sure about this as it goes against the principle "everything works out of the box without user intervention" that we are aiming to.

I think it's an acceptable compromise to expect the user to click once on "Yes" in a dialog. We can also make the dialog modal, so the user has to choose something, and the dialog doesn't disappear after a timeout, but I think that's too intrusive. But you can try to set "modal" to true and see how that looks like.

I'm all for shipping a working binary with the extension (or downloading one), but a "nimble install" should require user confirmation IMO.

If you want to install through other means, and you do it before, it shouldnt install anything extra.

And if you don't do it before? Too late, sorry, we've installed several packages through nimble in your home directory. I don't like that. There are cases, where that's not desirable, and once it's done, it's difficult to undo.

We could also have an option that the user can toggle on/off to skip the installation.

That would be useless, if it's "on" by default.

jmgomez commented 4 months ago

We can also make the dialog modal, so the user has to choose something, and the dialog doesn't disappear after a timeout.

This would be better but still (i.e. with a timeout consider the case where the user doesnt have the window open when it completes the installation).

That would be useless, if it's "on" by default.

Not quite, only the first time. Here we assume the user know what he is doing and dont want to be bother i.e. have a dev version of the lang server.

I'm all for shipping a working binary with the extension (or downloading one), but a "nimble install" should require user confirmation IMO.

What's the difference? (consider we can make the installation local to the extension)

nickysn commented 4 months ago

We can also make the dialog modal, so the user has to choose something, and the dialog doesn't disappear after a timeout.

This would be better but still (i.e. with a timeout consider the case where the user doesnt have the window open when it completes the installation).

The dialog appears every time you open a new instance of VS Code and you open a .nim file for the first time in this VS Code instance, and there's no nimlangserver in the PATH. So, unless the user installs nimlangserver via other means (for example, via the package manager of the distro), the dialog will keep appearing. So, if the user misses it once, and it disappears, the user will probably see it again and will have to chance to 'nimble install' the nimlangserver.

That would be useless, if it's "on" by default.

Not quite, only the first time. Here we assume the user know what he is doing and dont want to be bother i.e. have a dev version of the lang server.

That's not only for people, who do langserver development, but also for people, who want to install nimlangserver through their distro's package manager. For example, NixOS already packages nimlangserver. Debian packages Nim, so it's likely they will package nimlangserver as well, etc.

I'm all for shipping a working binary with the extension (or downloading one), but a "nimble install" should require user confirmation IMO.

What's the difference? (consider we can make the installation local to the extension)

Nimble is a package manager, with a shared state in the user's home directory, so it affects all projects the user has, that use nimble. A binary, local to the extension will be contained in a separate directory. What happens if nimble is already running and compiling something else? What happens if I want to uninstall nimlangserver from nimble, so I can install it through my distro package manager, for example? Can I still clean it from nimble, and delete its dependencies, while keeping my other nimble packages, that I use in other projects?

jmgomez commented 4 months ago

The dialog appears every time you open a new instance of VS Code and you open a .nim file for the first time in this VS Code instance

So this makes it perfect for an opt-in and we compromise in the configuration side rather than the convention.

Nimble is a package manager..

It has support at least to some extend to local packages. If it doesnt cover this particular use case (avoid polluting the global packages) maybe we should implement this feature

PS Feel free to merge it if you feel like it, I dont have strong opinions about it. But keep in mind we are diverging a bit from "everything just work" (which oc, we are not there anyways)

arnetheduck commented 4 months ago

Nimble is a package manager, with a shared state in the user's home directory, so it affects all projects the user has, that use nimble.

this is a bug in nimble, mostly (that installs things globally by default) - that said, the extension should indeed keep it local and isolated (afair nimble -l will do that).

distro package manager,

this is rather high-maintenance, ie then you get bug reports when there's an old version in the distro (which tends to be out-of-date or special in other ways) while advantages tend to be limited - in particular because the extension manages updates of nimlangserver just like vscode manages updates of the extension - the distro pm in particular does not manage vscode exts.

nickysn commented 4 months ago

Nimble is a package manager, with a shared state in the user's home directory, so it affects all projects the user has, that use nimble.

this is a bug in nimble, mostly (that installs things globally by default) - that said, the extension should indeed keep it local and isolated (afair nimble -l will do that).

I agree that the extension should keep the things, that installs local and isolated. Do you agree that, until this nimble bug is fixed and until the extension can detect that the nimble version has this bug fixed (which it currently doesn't), it's better to ask the user, before running any nimble commands?

distro package manager,

this is rather high-maintenance, ie then you get bug reports when there's an old version in the distro (which tends to be out-of-date or special in other ways) while advantages tend to be limited - in particular because the extension manages updates of nimlangserver just like vscode manages updates of the extension - the distro pm in particular does not manage vscode exts.

It is the choice of our users whether they want to use their distro package manager or not. Btw, some distros also manage VS code extensions, NixOS for example does that, see: https://github.com/NixOS/nixpkgs/tree/master/pkgs/applications/editors/vscode/extensions Besides, we still offer notification if the user's nimlangserver version is out of date, regardless of how it's installed, and if we don't detect any version, we still offer them to 'nimble install' it. The only thing that changes is now that we ask them. Is that ok?

arnetheduck commented 4 months ago

until this nimble bug is fixed

nimble has a flag for controlling the installation folder - it can simply choose a folder somewhere inside the extension - the bug is in "defaults" for some of the commands but that's a separate UX question. For now, this can be fixed in the extension.

It is the choice of our users

Sure, though every choice and option increases the support requirements exponentially because all combinations of choices can happen. nimlangserver is a good example: if it was embedded in the extension / isolated and (mostly) didn't cause a fuss, nobody would have noticed it could be an option even ;) In an ideal world, we'd compile nls to wasm and call it directly from the extension, as part of the extension itself - had it been done this way, nobody would have asked for an option for this to begin with, but options have a tendency to ossify and perpetuate themselves.

Anyway, like @jmgomez, I'm not vehemently opposed to this, but it's worth asking the question whether the option exists because there is work to do on the plugin (like the lack of isolation) or whether it exists because it's a good idea to begin with. If it's the former, it's perhaps better to start with addressing the root cause of why an option is being asked for and then see if it's still needed.

nickysn commented 4 months ago

until this nimble bug is fixed

nimble has a flag for controlling the installation folder - it can simply choose a folder somewhere inside the extension - the bug is in "defaults" for some of the commands but that's a separate UX question. For now, this can be fixed in the extension.

I didn't know that. It's a useful improvement, if we decide to continue relying on nimble for installation. We can discuss this in: https://github.com/nim-lang/vscode-nim/issues/9

It is the choice of our users

Sure, though every choice and option increases the support requirements exponentially because all combinations of choices can happen.

But it is a fallacy to assume that we control people's choice in this case. Distro package managers are a reality, as well as people using them. Distros will eventually package nimlangserver and that is simply a fact of life. Some (like NixOS) already do, and it's only a matter of time until Debian, Ubuntu, Fedora, etc. do the same. It is a choice that their users make to install software from their distro or from other sources, and we don't control that choice. And if our extension causes trouble with some distro, we'll get bug reports about it, no matter what we do.

nimlangserver is a good example: if it was embedded in the extension / isolated and (mostly) didn't cause a fuss, nobody would have noticed it could be an option even ;) In an ideal world, we'd compile nls to wasm and call it directly from the extension, as part of the extension itself - had it been done this way, nobody would have asked for an option for this to begin with, but options have a tendency to ossify and perpetuate themselves.

Why wasm? It's a VM, that runs slower and uses more memory. We already have native binaries for 99%+ of the platforms our users use. They are automatically build in GitHub CI. Why not bundle them, instead?

Anyway, like @jmgomez, I'm not vehemently opposed to this, but it's worth asking the question whether the option exists because there is work to do on the plugin (like the lack of isolation) or whether it exists because it's a good idea to begin with. If it's the former, it's perhaps better to start with addressing the root cause of why an option is being asked for and then see if it's still needed.

Both. There's still work to do on the plugin and there are users, who want to use their distro's package manager. We also can't and don't want to support every distro with a single binary, especially unorthodox non-LSB compliant ones, such as NixOS. In fact, here's some trivial proof: https://github.com/nim-lang/langserver/pull/188 What should we do about it? Should I merge it or not? I can merge it, but it doesn't solve the problem. The message will only be seen by people, who run 'nimble install nimlangserver' from the console, but not by people, who rely on the extension to install the server. A nimlangserver, packaged by the distro will have all their dependencies met, and this will be maintained by the distro maintainers, not by us.

Perhaps this should be merged and we can continue discussing how to improve server installation here: https://github.com/nim-lang/vscode-nim/issues/9

If there aren't any objections, I will merge this. Later, we can remove this dialog, especially if we decide to not rely on nimble for installation of the server.

arnetheduck commented 4 months ago

But it is a fallacy to assume that we control people's choice in this case.

we do - by offering the option to run a different langserver or not - ie consider a world in which nimlangserver doesn't exist as a binary, but is rather linked into the vscode extension (as wasm or native binary doesn't matter) - this is already the case with the extension itself, there is no expectation that the distro PM will package vscode extensions because it's part of the core vscode feature set to manage its own extensions - by the same logic, the extension can manage its own nimlangserver.

You're coming from a place where the option exists - I'm describing a world in with the option wasn't there to begin with: if everything was isolated and the nimlangserver binary was shipped inside the vscode extension so that it would entirely be managed by vscode (with nimlangserver being just an implementation detail of the extension itself and not a "separate entity"), would it be a valuable feature to run a different, potentially incompatible nimlangserver? What would the advantage of that be?

arnetheduck commented 4 months ago

We also can't and don't want to support every distro with a single binary,

We can and it's simple: by not depending on libpcre3 at all and linking libc statically, like modern distribution environments usually do (musl + native nim re engine) or by using wasm which is shipped with vscode and doesn't have these problems to begin with. Notably, this is also why vscode doesn't rely on distro package managers to ship extensions - it's messy.

ie understand that the distro package management model is difficult to maintain, which is why modern development systems tend to rely on other methodologies (go has its own static runtime, flatpak and docker are used to avoid distro packaging and gain distro-independence etc etc) - you're describing a solution to a self-inflicted problem.

arnetheduck commented 4 months ago

If there aren't any objections, I will merge this.

No objections from me, but notably no enthusiasm either, because it represents churn that doesn't seem to bring us any closer to a desired end goal and as noted in other comments, takes us a step away from it.

nickysn commented 4 months ago

We also can't and don't want to support every distro with a single binary,

We can and it's simple: by not depending on libpcre3 at all and linking libc statically, like modern distribution environments usually do (musl + native nim re engine) or by using wasm which is shipped with vscode and doesn't have these problems to begin with. Notably, this is also why vscode doesn't rely on distro package managers to ship extensions - it's messy.

ie understand that the distro package management model is difficult to maintain, which is why modern development systems tend to rely on other methodologies (go has its own static runtime, flatpak and docker are used to avoid distro packaging and gain distro-independence etc etc) - you're describing a solution to a self-inflicted problem.

Please contribute your suggestions here: https://github.com/nim-lang/vscode-nim/issues/9

arnetheduck commented 4 months ago

Please contribute your suggestions here:

fwiw, this point is (mostly) already noted in the relevant original issue:

https://github.com/nim-lang/langserver/issues/187#issuecomment-1960867142

nickysn commented 4 months ago

But it is a fallacy to assume that we control people's choice in this case.

we do - by offering the option to run a different langserver or not - ie consider a world in which nimlangserver doesn't exist as a binary, but is rather linked into the vscode extension (as wasm or native binary doesn't matter) - this is already the case with the extension itself, there is no expectation that the distro PM will package vscode extensions because it's part of the core vscode feature set to manage its own extensions - by the same logic, the extension can manage its own nimlangserver.

But we're living in this world, where nimlangserver is a standalone binary, implementing the LSP protocol - a protocol which is designed to make it easy to implement language support in different editors in IDEs. Of course, you can expect that distros will ship nimlangserver, together with Nim plugins for Vim, Emacs, Neovim, Helix and many others, all through their distro package managers. So, a nimlangserver will eventually be available.

You're coming from a place where the option exists - I'm describing a world in with the option wasn't there to begin with: if everything was isolated and the nimlangserver binary was shipped inside the vscode extension so that it would entirely be managed by vscode (with nimlangserver being just an implementation detail of the extension itself and not a "separate entity"), would it be a valuable feature to run a different, potentially incompatible nimlangserver? What would the advantage of that be?

Under Linux, the advantage would be that a language server, shipped with the distro will be linked against the distro's libs and receive security updates through the distro's package manager. Under Windows and macOS, I see no such advantage, as these operating systems have an excellent ABI stability, which Linux does not have (at least, not at the shared library level). So, I'm all for bundling a binary for Windows and macOS, and also for Linux, but for Linux we need this option, because we can't realistically provide binary support for all distros. So, IMO, long term, this needs to stay primarily for Linux. And for Windows and macOS it's only temporary, because we use nimble there as well, which is also not ideal, but can be fixed, so later the option to use a different language server will remain only for language server development purposes.

nickysn commented 4 months ago

Please contribute your suggestions here:

fwiw, this point is (mostly) already noted in the relevant original issue:

nim-lang/langserver#187 (comment)

No, the "original" issue (as in "Preceding all others in time; first.") is actually this one: https://github.com/nim-lang/vscode-nim/issues/9

Anyhow, the point is, I'm going to merge this, and this issue is going to get closed, so let's make sure we keep the valuable suggestions in an open issue, so they're not forgotten.