non-Jedi / eglot-jl

Wrapper for using Julia LanguageServer.jl with emacs eglot
Creative Commons Zero v1.0 Universal
61 stars 11 forks source link

Let eglot-jl.jl determine the default project path #9

Closed ffevotte closed 4 years ago

ffevotte commented 4 years ago

Thanks for eglot-jl! I just switched from lsp-mode, and found the eglot experience much smoother thanks to your eglot-jl adapter.

This PR lets eglot-jl.jl determine the default project path. I see a few advantages to it:

This also simplifies running the language server manually, since no argument is mandatory anymore: when no arguments are provided, the default depot path is taken from the environment (or "" if not found).

ffevotte commented 4 years ago

Here are a few ideas to follow-up on this:

  1. make it possible (and even easy!) to compile a custom system image for the Language Server, so that users willing to spend some time upfront could benefit from much reduced server initialization times afterwards. Early tests on my system: the Language Server normally starts responding to requests after 18s. With a system image, this is reduced to around 2s.

  2. simplify eglot-jl--project-try, so that it uses julia to find the project root (that would be activated if julia --project was run from the source file directory). This would be a change from the current behaviour, for files that are not in a project. Currently, a new server is launched for each directory in which a source file is edited. With this proposal, the same server would be used for all source files that don't belong to any project (they would actually be considered to belong to the default julia project/environment). I'm actually not sure what type of behavior we'd actually want...

Please let me know what you think about these ideas. I already have semi-working implementations for both of these, that I could easily turn into PRs if you're interested...

non-Jedi commented 4 years ago

For your followups, I think 1 is probably out of scope for this package, but I wouldn't mind linking in the README to simple instructions for doing so. I just foresee all sorts of messed up environments if we're juggling system images. https://github.com/julia-vscode/LanguageServer.jl/pull/695 may be of interest here. If we could keep the implementation clean and simple (unlikely to cause bugs when upgrading the language server) I guess I wouldn't be opposed to this as long as there was an easy way to turn it off.

My long-term preference is that the logic in eglot-jl--project-try lives in the julia-mode repo: https://github.com/JuliaEditorSupport/julia-emacs/pull/108. But that probably isn't happening until most major distros drop support for emacs 24. If it's going to live in julia-mode.el, we definitely don't want it to depend on having a julia binary available, so I'm somewhat leery of doing so here. I'm also not sure we want to introduce 0.4 seconds of load time into locating a project:

❯ /usr/bin/time julia --startup-file=no --project -E 'Base.load_path()[1]'
"/home/adam/repos/Crev.jl/Project.toml"
0.40user 0.28system 0:00.37elapsed 183%CPU (0avgtext+0avgdata 166844maxresident)k
72inputs+0outputs (1major+26665minor)pagefaults 0swaps

As far a I know when a language server is running, that would directly add those 0.4 seconds of latency on the time it takes to open any new julia file.

ffevotte commented 4 years ago

I think 1 is probably out of scope for this package, but I wouldn't mind linking in the README to simple instructions for doing so. I just foresee all sorts of messed up environments if we're juggling system images. julia-vscode/LanguageServer.jl#695 may be of interest here. If we could keep the implementation clean and simple (unlikely to cause bugs when upgrading the language server) I guess I wouldn't be opposed to this as long as there was an easy way to turn it off.

You're right. If we go this way, I think:

I hadn't thought about the possibility that LanguageServer itself gets upgraded. I guess this won't happen for MELPA users (who will get every new eglot-jl release in a freshly downloaded repository, without any obsolete system image in it). However, this might very well happen for users who simply download/clone the repo somewhere and use it from there...

As far a I know when a language server is running, that would directly add those 0.4 seconds of latency on the time it takes to open any new julia file.

Yes, I had overlooked that. 0.4 seconds seemed negligible in comparison to the latency involved with running a new server. However, you're of course right about the cost when opening new files in projects for which a server is already running.

Thanks for the insight!

matsievskiysv commented 4 years ago

julia-vscode/LanguageServer.jl#695 may be of interest here. If we could keep the implementation clean and simple (unlikely to cause bugs when upgrading the language server) I guess I wouldn't be opposed to this as long as there was an easy way to turn it off.

The idea behind that PR is to compile LanguageServer into binary package, not the environment. This would allow to merge this repo into lsp-mode, providing a link to the LanguageServer binary releases in Readme. No Julia required.

ffevotte commented 4 years ago

The last commit implements the following logic:

As an aside, sensible defaults are used for the source directory and depot path if they are not provided in the command-line.

Please let me know if there are other changes you'd like me to make. Your comments so far have led to this new version, which I think is much more robust than my initial implementation. Thanks!

non-Jedi commented 4 years ago

This is great. Thanks so much!

non-Jedi commented 4 years ago

@gdkrmr this approach may be of interest to you for lsp-julia so users won't run into problems like https://discourse.julialang.org/t/lsp-julia-in-emacs-not-finding-environment-folder/39582