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

[julia]: Better configurability for LanguageServer.jl install location. #1153

Merged
merged 1 commit into from Aug 17, 2021

Conversation

fredrikekre
Copy link
Contributor

I think after this patch, no user configuration is necessary unless you want to have LanguageServer.jl installed into a custom package environment.

lua/lspconfig/julials.lua Outdated Show resolved Hide resolved
@fredrikekre fredrikekre force-pushed the fe/julia-v2 branch 2 times, most recently from 4995d0b to 3eb4b93 Compare August 17, 2021 12:05
@mjlbach
Copy link
Contributor

mjlbach commented Aug 17, 2021

Should we just set the current hardcoded cmd to what is recommended in the installation instructions in this PR? Is there a reason a user would ever prefer a global installation of LanguageServer.jl? That would simplify things. We might also include instructions for compiling a system image wtih PackageCompiler.jl

@fredrikekre
Copy link
Contributor Author

fredrikekre commented Aug 17, 2021

If you are okay with that then I believe that is "best practice" since the language server process is an isolated thing and it is good to have an isolated package environment for it.

@mjlbach
Copy link
Contributor

mjlbach commented Aug 17, 2021

I think it's way easier than suggesting these overrides, and we're already providing installation instructions so why not follow best practice?

```sh
julia -e 'using Pkg; Pkg.add("LanguageServer"); Pkg.add("SymbolServer")'
julia --project=~/.julia/environments/nvim-lspconfig -e 'using Pkg; Pkg.add("LanguageServer")'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Julia 1.7 and above this can be spelled

julia --project=@nvim-lspconfig -e 'using Pkg; Pkg.add("LanguageServer")'

but lets wait (at least) until that is released before recommending that.

Comment on lines +10 to +18
# Load LanguageServer.jl: attempt to load from ~/.julia/environments/nvim-lspconfig
# with the regular load path as a fallback
ls_install_path = joinpath(
get(DEPOT_PATH, 1, joinpath(homedir(), ".julia")),
"environments", "nvim-lspconfig"
)
pushfirst!(LOAD_PATH, ls_install_path)
using LanguageServer
popfirst!(LOAD_PATH)
Copy link
Contributor

@mjlbach mjlbach Aug 17, 2021

Choose a reason for hiding this comment

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

Why do we still need all of this if we are passing --project to the julia process? Is this preferable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not passing --project actually. It is a bit tricky, because that would interfere with the project discovery on the lines after. I think the only options are the PR as is, or to juggle with Base.ACTIVE_PROJECT[] instead in an equally "hacky" way.

Copy link
Contributor

@mjlbach mjlbach Aug 17, 2021

Choose a reason for hiding this comment

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

Sorry, I meant to imply "why shouldn't we just pass --project?"

I think I understand now, Base.active_project() would then return the project.toml containing directory for LanguageServer.jl vs the one containing the project meant to be analyzed/passed to the LanguageServer.jl instance.

For now, this is good. I still think there's a little bit of duplication in functionality between our root detection in lspconfig and the project.toml detection for figuring out the appropriate julia environment.

@mjlbach
Copy link
Contributor

mjlbach commented Aug 17, 2021

@fredrikekre is this ready now?

@fredrikekre
Copy link
Contributor Author

Yes. It shouldn't (:crossed_fingers:) break for anyone since the config still fall backs to the global env. Only difference should be that we look in .julia/environments/nvim-lspconfig first.

@mjlbach mjlbach merged commit 79a73d1 into neovim:master Aug 17, 2021
@fredrikekre fredrikekre deleted the fe/julia-v2 branch August 17, 2021 15:31
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.

None yet

3 participants