-
-
Notifications
You must be signed in to change notification settings - Fork 895
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
Implement automatic downloading facilities #1272
Conversation
Thanks, the changes for |
I will take a look after this is merged. |
lsp-pwsh.el
Outdated
@@ -32,7 +32,7 @@ | |||
|
|||
(defgroup lsp-pwsh nil | |||
"LSP support for PowerShell, using the PowerShellEditorServices." | |||
:group 'lsp-mode | |||
:group 'lsp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why changed to lsp
group?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be reverted.
It is not something urgent - it will make lsp-mode consistent since there will be a unified way to install the server. It will help also implementing the dashboard with available servers - ATM you cannot check whether the server is present because it will trigger installation. I will do a PR. |
Addressed @kiennq and @seagle0128 comments. |
@yyoncho I think we should store lsp server files under I am not 100% sure about convention on this but:
|
Otherwise the changes look good to me, I tried both installing and updating the server. I will try to implement async download mechanism via |
Make sense, I will update it.
I wish we can have the download method in lsp-mode.el so we can reuse it across servers. E. g. lsp- The same about the extract method - based on the extension of the file checks for pwsh, unzip, untar, etc. If not present - shows an error. |
lsp-pwsh.el
Outdated
callback | ||
lsp-pwsh-exe "-noprofile" "-noninteractive" "-nologo" | ||
"-ex" "bypass" "-command" "Expand-Archive" | ||
"-Path" temp-file "-DestinationPath" lsp-pwsh-dir)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be (file-name-directory lsp-pwsh-dir)
, the parent of lsp-pwsh-dir
. The reason is because the archive is containing a folder that has same name as lsp-pwsh-dir
so we need to expand it into parent folder for it to replace lsp-pwsh-dir
.
1f670a3
to
0dbd698
Compare
@yyoncho Thank you for this PR. I think it is great, and I am looking forward to seeing a new version of lsp-mode getting this facility. I don't know why it needs an integration with Nix when it can download and install server executables in itself. Yet I like to install servers using Nix, so let me leave a few comments on this ticket.
Is it mandatory for the nix wrapper to follow this convention? Nix installs executables in ~/.nix-profile/bin by default. Executables installed as part of npm packages won't have |
lsp-clients.el
Outdated
:npm | ||
"javascript-typescript-langserver" | ||
(lambda (success? msg) | ||
(funcall callback success? msg)))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the callback function takes a boolean status flag as an argument. In JavaScript, promises take success and error callbacks separately. I think it is a matter of preference, but I'd personally prefer lsp-package-ensure
having two distinct callback functions for success and failure. What do you think? Depending on your decision, I will extend the API of my nix-env-install
to support callbacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will experiment which interface results in better code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to take a look at emacs-async for design of async APIs. It would be possible to support concurrent downloads, even if you probably don't need it in this package.
No, let's keep it in that folder only if it makes sense. Can you suggest better interface for lsp-package-path? I am not npm/nix expert. Not sure whether striping/adding .bin to the path is an option |
I am thinking of an API like this: (defcustom lsp-package-backends
'(nix npm builtin
;; system simply finds an executable from exec-path
system))
"Package managers that can be used to install server executables."
:type '(repeat symbol))
(defun lsp-find-executable (command packages)
(-some (lambda (backend)
(cond
((eq backend 'system)
(executable-find command))
((lsp-backend-available-p backend)
(lsp-backend-find-executable backend command
(alist-get backend packages)))))
lsp-package-backends))
(cl-defgeneric lsp-backend-available-p (backend))
(cl-defgeneric lsp-backend-find-executable (backend command &optional package))
;; An example client definition depending on an external package registry
(lsp-register-client
(make-lsp-client :new-connection (lsp-stdio-connection (lambda ()
(cons "javascript-typescript-stdio"
lsp-clients-typescript-javascript-server-args)))
:dependencies '(("javascript-typescript-stdio"
((npm "javascript-typescript-langserver"))))
:activation-fn 'lsp-typescript-javascript-tsx-jsx-activate-p
:priority -3
:completion-in-comments? t
:server-id 'jsts-ls))
;; An example client with builtin download function
(lsp-register-client
(make-lsp-client ...
:dependencies '(("fsautocomplete" ((builtin lsp-fsharp--fsac-install)))) The executable path needs to be resolved by
I personally find the API of eglot to be more concise yet satisfactory than this package. Unfortunately, it didn't work with bash-language-server, and lsp-mode seems to have more resources and velocity, so I will probably use this package. |
This can be enforced by no-littering package. It would be better to add this package to no-littering once this PR is merged. (If anyone doesn't, I will.) Nonetheless, the author of the package suggests that you not add an extra prefix, i.e. Related discussion: syl20bnr/spacemacs#5947 |
@akirak Thank you. Your suggestion looks fine except for the fact that we are not always looking for executables - the eglot implementation is oversimplified which leads to reports like this - eclipse-jdtls/eclipse.jdt.ls#748 and awkward workarounds. And yes, having all of the so-called providers under the same interface does not make sense and won't work. Following your example, I would modify it to include the parameters needed for each backend: ; dependency declaration follows
((:system "typescript-language-server")
(:nix :package "typescript-language-server" :binary-name "the-binary-name")
(:npm :package "typescript-language-server" :path ".bin/typescript-language-server")) ;; here we don't always look for binary and we need the binary name.
((:custom :function ,lsp-pwsh--download)) I would also give up the generics for simple defcustom to make disabling of particular provider easier - see https://stackoverflow.com/questions/59201891/how-to-undefine-defmethods |
For me, it works either way. |
@yyoncho Thank you for your comment. I don't know much about how I'd prefer dependency definitions like this to avoid duplicates: (defun lsp-check-dependencies (dependencies)
(dolist (dep dependencies)
(pcase dep
(`(executable ,name ,specs)
(unless (lsp-find-executable name)
;; Install a package from one of the specs
\.\.\.)))))
(defun lsp-find-executable (command packages)
(-some (lambda (backend)
(cond
((eq backend 'system)
(executable-find command))
;; Nix can handle multiple package repositories
((eq backend 'nix)
(lsp-backend-find-executable backend command
packages))
((lsp-backend-available-p backend)
(lsp-backend-find-executable backend command
(alist-get backend packages)))))
lsp-package-backends))
;; Define dependencies
'((executable "typescript-language-server"
((npm :package "typescript-language-server")
;; Add an explicit nix package once it is available in the official repo
(nix :repo nixpkgs :package "typescript-language-server"))))
'((executable fsautocomplete-executable
((builtin :function lsp-fsharp--fsac-install)))) Nix can support various types of packages (e.g. npm, pip, etc.) through third-party wrappers as well as from its official package repository nixpkgs. I have already added support for node2nix to support npm packages, and I may support other external package systems in the future. Thus the Nix provider will receive all package specs during installation. Yes, all backends/provides are not equal, and nix is treated as special in this example.
You can disable particular providers by customizing (defcustom lsp-package-backends
'(nix npm builtin
;; system simply finds an executable from exec-path
system)
"Package managers that can be used to install server executables."
:type '(repeat symbol))
(use-package lsp-mode
:custom
;; Use only specific backends/providers
(lsp-package-backends '(npm builtin system)))
(use-package lsp-mode
:config
;; Disable nix backend
(cl-delete 'nix lsp-package-backends)) I personally don't want to let users write generics to customize a package. It might not be easy for some people to understand generics in (CL) Emacs Lisp. Also, it will become difficult to upgrade the API of generic methods if you allow users to redefine implementations. Customizing variables is the standard way in Emacs and hides the internals. If you like this idea, I will send a PR to help with the implementation. I want to do that, because it will help me understand this package. Thanks. |
Yes, I agree - we should have customization options for the common scenarios and empower the expert users to do advanced customizations via elisp.
I like the idea but I am already working on the change. So let me finish the initial version then we will let it evolve to make it easier to use (e. g. have common methods for extracting/downloading deps) and complete (e. g. support nix, vscode market place, etc). Also, feel free to pick any of the open items or propose/discuss over the existing feature - we encourage contributions. |
@yyoncho I see. Please merge this PR as soon as it is finished. It looks already sufficient, and I don't think it needs to support Nix right now.
I wish to continue my experiments with Nix by manually installing servers, so I will probably disable the automatic downloading feature for some languages, but it should never hinder your development. Thank you for sharing your contexts. |
9ef7960
to
6bc3603
Compare
- Fixes emacs-lsp#506 - Implemented base facilities for downloading and installing language servers - added new field :download-server-fn in lsp--client structure which will be called with (client callback update?). - Implemented automatic installation via npm for jsts-ls and ts-ls. - All servers(when feasible) should be installed under `lsp-server-install-dir`. @akirak - check the cl-defgenerics in lsp-mode - let me know if they are sufficient to implement the nix variands. @razzmatazz - I have ported CSharp implementation but I havent converted it into async. @seagle0128 - lsp-python-ms is not ported. In general, it will work as it is but it will ask to install the server even if you are in different file. @TOTBWF - F# same as C#. @kiennq powershell installation is converted to async one.
Fixes Feature: automatic server installation #506
Implemented base facilities for downloading and installing language servers
added new field :download-server-fn in lsp--client structure which will be
called with (client callback update?).
Implemented automatic installation via npm for jsts-ls and ts-ls.
All servers(when feasible) should be installed under
lsp-server-install-dir
.@akirak - check the cl-defgenerics in lsp-mode - let me know if they are
sufficient to implement the nix variands.
@razzmatazz - I have ported CSharp implementation but I havent converted it into
async.
@seagle0128 - lsp-python-ms is not ported. In general, it will work as it is but
it will ask to install the server even if you are about to start language server not being in python file.
@TOTBWF - F# same as C#.
@kiennq powershell installation is converted to async one.