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

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

Merged
merged 2 commits into from
May 14, 2020

Conversation

ffevotte
Copy link
Collaborator

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:

  • eglot-jl.el does not have to find the project itself (this could be extended to simplify eglot-jl--project-try as well)
  • eglot-jl-default-environment becomes unnecessary
  • the default environment is automatically adapted to the julia version in use

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).

- eglot-jl.el does not have to find the project itself (this could be extended
  to simplify `eglot-jl--project-try` as well)
- `eglot-jl-default-environment` becomes unnecessary
- the default environment is automatically adapted to the julia version in use

This also simplifies running the language server manually, since no argument is
mandatory anymore.
@ffevotte
Copy link
Collaborator Author

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...

Copy link
Owner

@non-Jedi non-Jedi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I've been meaning to fix this story up for some time, and I've already had at least one new user run into problems because of this.

I'm still waffling on whether its best to simply not allow the user to specify a default environment (as you've done here) or better to set it to the empty string but leave it customizable. There might not be any situations where it's useful to allow customization, so I think I'm leaning towards your approach.

After fixing the project confusion in the review comments, this should be good to go.

eglot-jl.jl Outdated Show resolved Hide resolved
@non-Jedi
Copy link
Owner

non-Jedi commented May 12, 2020

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. 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.

My long-term preference is that the logic in eglot-jl--project-try lives in the julia-mode repo: JuliaEditorSupport/julia-emacs#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
Copy link
Collaborator Author

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:

  • things will be strictly opt-in: no compilation by default, but either detailed instructions in the README or a simple Emacs command to create a sysimage for users who want it
  • we have to have an easy way to turn it off if things fail
  • we have to account for the possibility that Julia itself gets upgraded, which would make a system image obsolete

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
Copy link

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.

Copy link
Owner

@non-Jedi non-Jedi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've come around to the hackery. Once niggles are addressed this should be good to merge. Or I might merge and add another commit myself to fix niggles depending on the time I have in the next couple days.

eglot-jl.el Outdated Show resolved Hide resolved
eglot-jl.jl Outdated Show resolved Hide resolved
eglot-jl.el Show resolved Hide resolved
`eglot-jl.jl` takes a source directory as first argument. It reuses internal
functions from `Base` to find the correct project for this directory.

The major version number is bumped following the removal of the
`eglot-jl-default-environment` customization option.
@ffevotte
Copy link
Collaborator Author

ffevotte commented May 13, 2020

The last commit implements the following logic:

  • the lisp part sends the current source file directory to the julia script. This relies on eglot running the function in eglot-server-programs from the source file buffer (not sure whether this is part of the API, but this is what the current implementation also assumes)

  • the julia part gets this source directory, and uses internal functions from Base in order to reimplement the logic of finding the correct project environment: if the directory is within a project, use that; otherwise, use the default @v#.# environment.

  • the entire julia script runs within the eglot-jl environment, and the --project command-line switch is not abused anymore

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 non-Jedi merged commit 9d48245 into non-Jedi:master May 14, 2020
@non-Jedi
Copy link
Owner

This is great. Thanks so much!

@non-Jedi
Copy link
Owner

non-Jedi commented May 16, 2020

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants