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

Feature: Provide "Update" callback for servers #200

Closed
tjdevries opened this issue Apr 22, 2020 · 13 comments
Closed

Feature: Provide "Update" callback for servers #200

tjdevries opened this issue Apr 22, 2020 · 13 comments
Labels
enhancement New feature or request

Comments

@tjdevries
Copy link

Servers are constantly evolving and getting better, we should make it easy to update (even if it's just delete the folder and install again) for servers.

@teto Thoughts?

@tjdevries tjdevries added the enhancement New feature or request label Apr 22, 2020
@teto
Copy link
Member

teto commented Apr 22, 2020

I am thoroughy against it xD It's the role of a package manager to keep the software up to date. I imagine this will be an endless source of bug reports/pull requests and we just don't have the resources to deal with all the edgecases, bump our software to deal with all the langague server updates

@tjdevries
Copy link
Author

tjdevries commented Apr 22, 2020

Maybe I didn't make myself clear. I don't think WE should write complicated update scripts. However, we currently provide "Install" scripts and then after you install them, it's difficult to figure out what should be done to update the server. For example, update for any of the JS servers would probably just be "cd $install_location; npm update" or something like that.

@DanCardin
Copy link

fwiw, i just went looking for how to do this and found this issue.

I would be happy with :LspUninstall foo (basically just deletes the folder given to me by :LspInstallInfo), or :LspReinstall (uninstall + install).

@theJian
Copy link
Contributor

theJian commented Apr 23, 2020

uninstall + install would be an easy way to implement this. Providing an update command means nvim-lsp have to know how every package manager updates their packages, even it is simple like "cd $install_location; npm update".

@thchha
Copy link
Contributor

thchha commented Apr 23, 2020

I just started contributing, but when adding support for the first server, I also stumbled upon this functionality. I do not like the fact that we even bother with installing language servers. There are already plugin-managers a user can opt-in to. I think the installation is not transparent (but convenient) to the user, updating is not taken care of and the additional complexity when fully supported is not neccessary.
This repo calls itself a place of common configuration (which in the end are utilities for binding to nvim, right?) and the functionality for installing and updating across different build systems is in conflict with common idioms like YAGNI, KISS and the unix philosophy of doing only one thing, but this to the most effective extend.

So my proposal is the following:

  • Remove all functionalities for installing servers.
  • Optionally provide an option for external plugin-managers to hook (read inject) into this plugin instead.

For example a plugin should be able to require the 'nvim-lsp'-module and inject the server. Lua-plugins are not fully integrated into neoim yet, right?

@teto
Copy link
Member

teto commented Apr 24, 2020

@norcalli @h-michael I am of the opinion of @thchha : lets remove LspIntall/Uninstall. But you are the main maintainers here so that's your call, I just know I would not like to deal with such code.

@h-michael
Copy link
Contributor

I personally like to remove the functionality of the installer.
I think the language server manager's functionality should be separate from config (and I think it should be generic rather than Nvim's plugin).

@ckipp01
Copy link
Contributor

ckipp01 commented Apr 25, 2020

Just throwing another maybe wild idea out there, but I think it's also worth asking what the actual value of this repo becomes if it only contains a way to configure a language server. I'm not saying there is not value in that, but I wondering if there is a way to make it even more valuable than that?

What if instead of this being a collection of various configurations it just became a repo that language server maintainers could fork and add in the various install, update, and configuration settings for their related servers. Most of the time they have the most up-to-date info and a unified way to install their server plus the most insight on what settings and configuration need to actually be set. As a server maintainer I know I'd personally rather have a nicely documented skeleton for me to easily plug in in what my users need and host this in our own org for users to install and use. Then we could triage the majority of the issues that probably have to do with our own servers than the actual Nvim LSP integration.

By doing this it would allow the maintainers of this repo to do what they do best, maintain the integrity of how that integration is done and to ensure that it's done in a unified format for all server maintainers to use. It also allows the server maintainers to do what they do best, handle the installation and configuration of the server.

I also realized I'm not hitting on the negative points of this approach. I know some server maintainers don't want to care about the client side of things and how they are integrated. However, I think it's at least a valid idea to be discussed.

Either way, I'm glad this repo exists, and I'm extremely thankful for all the work that went into having native LSP support in Nvim.

@teto
Copy link
Member

teto commented Apr 25, 2020

As far as I am concerned, it can't be successful at managing installations ever since packaging is too hard.

It's true that this repo does little. I wish we eventually can use nvim's lsp easily out of the box without installing this plugin (during lsp development, the split allowed to iterate faster maybe). It will still have value in sharing examples on how to configure nvim, the supported lsp servers etc.
So you may not have to install the plugin but it would still make sense to keep the configurations in a separate repo.

@thchha
Copy link
Contributor

thchha commented Apr 25, 2020

I think I get your point @ckipp01: You want your users to seamlessly integrate your server into neovim (read any client), that shall be much appreciated! I like how both our answers are very opinionated 😄

What if instead of this being a collection of various configurations it just became a repo that language server maintainers could fork and add in the various install, update, and configuration settings for their related servers. Most of the time they have the most up-to-date info and a unified way to install their server plus the most insight on what settings and configuration need to actually be set. As a server maintainer I know I'd personally rather have a nicely documented skeleton for me to easily plug in in what my users need and host this in our own org for users to install and use. Then we could triage the majority of the issues that probably have to do with our own servers than the actual Nvim LSP integration.

This repo already provides a configuration skeleton which a server maintainer can fork and suggest to its users. At least I guess. So you could offer an server-specific plugin, which injects into the users lsp-configurations.
By automatically updating your server you shift away the authority of the user so that he looses his ability to decide upon his own software. Even though the protocol is standarized and in this context it is probably less likely but the user could heavily depend on a version of both programs. With your comment you also mixed up two distinct things in my opinion: The build process of the software you offer and the users software which he decided to integrate your application in.
In my opinion it is a crucial safety and security concern to start an update process for an external program not regarding to the current executing process (that is, neovim).

To aid your concerns I think what this repository could offer is an option to inform the user via neovim about stuff from your server. Therefore you could check against your current version and prompt the user appropriate (without the additional overhead of maintaining a window-neovim-plugin) via an callback-method specific to our coupling? So this repository could integrate these messages into the neovim ui in a consistent manner.. This would decouple neovim from your update process, respects the users authority and helps you maintain a satisfied userbase.

Nonetheless, if a user wants to postpone an update these configurations get even more complicated. By no means, I do not want shift the current vision. But obiously I already showed my passion about this topic 🤣

@ckipp01
Copy link
Contributor

ckipp01 commented Apr 25, 2020

I think I get your point @ckipp01: You want your users to seamlessly integrate your server into neovim (read any client), that shall be much appreciated! I like how both our answers are very opinionated 😄

You nailed it. 😆

For real, yes, that's a goal I'm sure everyone can agree on.

By automatically updating your server you shift away the authority of the user so that he looses his ability to decide upon his own software.

I'll clarify that I totally agree. This should never be automatically upgraded. What I more meant to say was that in our other clients we make it crystal clear that when a new version of the server is out, the user gets notified, and with one click, they can update to the newest. That's ideal in my mind. However, I also agree with @teto that it's impossible to do in this repo. It's not even a good goal as it'd just be a nightmare to maintain.

Again, I just wanted to throw the random idea out there. Either way, there is a ton of value in having examples and base configurations centralized. I just want to do it in a way that both helps server maintainers ensure their users are getting the best experience and also ensure that the maintainers of this repo and the Nvim LSP aren't get bombarded with irrelevant questions on configuration that may be able to be answered by the server maintainers.

@justinmk
Copy link
Member

justinmk commented Apr 27, 2020

I think the installation is not transparent (but convenient) to the user, updating is not taken care of and the additional complexity when fully supported is not necessary.

good point. Personally I've not used :LspInstall, I think users get almost as much value simply by some suggestions in the docs like (npm install foo).

:LspInstall was always a best-effort thing, we tried to make this clear, but it sounds like there are not many voices in favor of it.

To be clear: if we remove :LspInstall, the "knowledge" for each install should be transferred to the docs.

@mjlbach
Copy link
Contributor

mjlbach commented Jan 3, 2021

missed one, this was closed by #498

Some people have expressed interest in a companion repository (not maintained by us) for installing language servers (I don't know of anyone actually doing this). Others, like metals have implemented installer/update feature building off neovim core language server support. We've also been moving functionality out of this repo into core to facilitate that, so it is become closer to a "minimal" set of default configurations, rather than any infrastructure other language specific plugins would depend on.

@mjlbach mjlbach closed this as completed Jan 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

9 participants