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

[WIP] Improve elixir-ls compilation #32

Closed
wants to merge 2 commits into from
Closed

[WIP] Improve elixir-ls compilation #32

wants to merge 2 commits into from

Conversation

darwintantuco
Copy link
Contributor

@darwintantuco darwintantuco commented Aug 13, 2020

An attempt to improve elixir-ls compilation.
I notice elixir-ls uses asdf together with asdf-erlang and asdf-elixir. (checkout https://github.com/elixir-lsp/elixir-ls/blob/master/.tool-versions).
Maybe we can also use it by default for compilation.
I think this will reduce the need to compile elixir-ls manually (https://github.com/elixir-lsp/coc-elixir#server-fails-to-start)

Although this will require users to have asdf, asdf-erlang and asdf-elixir installed in their machine.
Not sure if this is ideal but seemed to work well on my testing.

@darwintantuco
Copy link
Contributor Author

darwintantuco commented Aug 13, 2020

Tested it by pointing coc-elixir to my fork e.g. Plug 'darwintantuco/coc-elixir', {'do': 'yarn install && yarn prepack'}
Unable to test :CocInstall coc-elixir though

@darwintantuco
Copy link
Contributor Author

not ideal I guess, maybe we will only call asdf install for asdf-elixir and asdf-erlang users?

@amiralies
Copy link
Collaborator

You're right,
two things should happen

  • When publishing to npm , We've to compile els with minimum supported erl/elixir version
  • For users who install using this repo with plugin managers (ie plug), we should create a script which uses asdf if available otherwise fall back to regular elixir installation

@darwintantuco
Copy link
Contributor Author

ahh right 👍
I realize I should have created an issue first, oops, sorry
anyways, happy to check this out :D

@darwintantuco darwintantuco changed the title Improve elixir-ls compilation [WIP] Improve elixir-ls compilation Aug 17, 2020
@darwintantuco
Copy link
Contributor Author

darwintantuco commented Aug 17, 2020

if the user have asdf-elixir and asdf-erlang installed, can we call asdf install?
I think the idea is to always use the correct/compatible version of elixir and erlang in compilation.
although this would take additional time in installation, as it will download elixir and erlang defined in .tool-versions (if not yet downloaded)

@amiralies
Copy link
Collaborator

I'm not sure about this, I think i've to ask @axelson to help

@darwintantuco
Copy link
Contributor Author

looks like .tool-versions is removed on master (elixir-lsp/elixir-ls#351)
closing this PR

@darwintantuco
Copy link
Contributor Author

darwintantuco commented Aug 20, 2020

still, it would be nice if we can figure out a way to improve current compilation to avoid the need to build/compile elixir-ls manually, as specified in https://github.com/elixir-lsp/coc-elixir#server-fails-to-start

will let you know if I come up with something :D

@axelson
Copy link
Member

axelson commented Aug 21, 2020

Would it be possible to add some code that checks if the server needs to be recompiled? And it could do that by checking if the current version of elixir and erlang is different the version used to compile ElixirLS? And if it is then coc-elixir could remove the existing release and recompile (or perhaps it could compile each elixir-erlang) pair into a new directory.

As a side-note it is generally better for elixir-ls to run on the same version of elixir (and to a lesser degree erlang) that it was compiled with (here's one specific annoying issue elixir-lsp/elixir-ls#193). So I'm likely to move ElixirLS in the direction of compiling before launching than the reverse, although ideally that would be something that is built into ElixirLS rather than having to run that for each extension.

@darwintantuco
Copy link
Contributor Author

As I understand, the plugin will compile ElixirLS once during installation using elixir/erlang versions installed on the user's machine.
Checkout package.json.
Hmm I think that's possible, maybe we could create a script that checks for versions and compare it to recommended versions? or maybe we can improve the error message when installation failed due to ElixirLS compilation.
just thinking out loud :D

@axelson
Copy link
Member

axelson commented Aug 24, 2020

Does coc-elixir share one global installation of elixir-ls or is it one per project? Because if it's shared if you are using different versions of Elixir and Erlang you will run into problems.

@darwintantuco
Copy link
Contributor Author

I think coc-elixir uses one global installation of elixir-ls.
I see, I wonder how vscode plugin compile elixir-ls.

@axelson
Copy link
Member

axelson commented Aug 25, 2020

VSCode is released with a precompiled version of ElixirLS that was built with minimal elixir and erlang versions: https://github.com/elixir-lsp/elixir-ls/blob/f61e81f1ca7f564741a386dc353510d92469a838/.release-tool-versions

@darwintantuco
Copy link
Contributor Author

Interesting!
I wonder if we can also release coc-elixir with a precompiled version of ElixirLS.
So that users won’t need to compile it on installation.
Will investigate when I have the time.

@darwintantuco
Copy link
Contributor Author

sorry just had the time

It seems coc-elixir is already released with a precompiled version of ElixirLS.
and it works well when you install it using :CocInstall coc-elixir.

Apologies, I think some of my statements before is wrong and only applicable if you install elixir-ls via
Plug 'elixir-lsp/coc-elixir', {'do': 'yarn install && yarn prepack'} - which will eventually compile elixir-ls on users end.
At the moment, the only thing I can think of is to commit compiled elixir-ls, which is not ideal.

@amiralies
Copy link
Collaborator

Maybe we can have a release branch with compiled elixir-ls artifacts, but for now I think it's fine to use :CocInstall or manually compile elixir-ls and use config option elixir.pathToElixirLS.

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