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

Discussion: Change packaging approach #115

Closed
axelson opened this issue Feb 7, 2020 · 24 comments
Closed

Discussion: Change packaging approach #115

axelson opened this issue Feb 7, 2020 · 24 comments

Comments

@axelson
Copy link
Member

axelson commented Feb 7, 2020

Currently ElixirLS is packaged with https://github.com/JakeBecker/mix_task_archive_deps as .ez archives. This approach means that the language server is run by a different version of elixir than it was compiled with which can cause issues such as #107

ElixirLS used to run as an escript but that was changed in v0.2.4: 25 Oct 2017:

Package ElixirLS as .ez archives instead of escripts. This should make asdf installs work.

Goals/Requirements:

  1. End-user's code should be compiled with the version of elixir and erlang that they have in their .tool-versions or installed globally
    a. This means that reported warnings/errors match the version of Elixir that they're using, not earlier or later versions
  2. The majority of the ElixirLS/ElixirSense code should run on the same version of elixir that it was compiled on
  3. For users of the vscode-elixir-ls package it should continue to be a one-click install

Possible approaches:

  1. Continue to use mix_task_archive_deps
  2. Use escripts
  3. Use a release
  4. Other?
  5. Separate BEAM instance with source code (being fleshed out in [WIP] Beam splitter #136)

Potential issues to be aware of:

  1. With mix_task_archive_deps there can only be one version of a dependency running which is mainly an issue for Jason and Dialyxir (but if we added more dependencies it could become a larger issue)
  2. Compiled Elixir code relies on Elixir internals that could change (see Exception from Logger when running on a 1.10 project #107 for an example)
    a. This is the issue that is currently causing the most trouble
  3. Elixir releases are generally recommended to be built on the same version of OS that they will run on
  4. Escripts require Erlang to be installed on the target system

I will come back and flesh out the issues and possible approaches later, along with more clear guidelines on what the requirements are.
Edit: added above

@cdegroot
Copy link
Contributor

Why not just distribute the sources? You need the compiler and whatnot available anyway and this way you could just on-the-fly compile sources for the specific OTP+Elixir version (and cache them, of course, making it a mostly-one-time thing)

@cdegroot
Copy link
Contributor

(w.r.t. package conflicts - I've known Java code that just rewrites packages on-the-fly so you'd end up with ElixirLS.deps.Jason, say, as a package name. I don't think it'd be super hard with Elixir)

@axelson
Copy link
Member Author

axelson commented Feb 13, 2020

Why not just distribute the sources?

This is an interesting idea that I hadn't considered before! Especially because there are no nifs (which would make compilation more difficult)

(w.r.t. package conflicts - I've known Java code that just rewrites packages on-the-fly so you'd end up with ElixirLS.deps.Jason, say, as a package name. I don't think it'd be super hard with Elixir)

Interesting, I didn't realize that was sometimes done in Java world. I think dynamically renaming the modules wouldn't be too difficult, but I'm more concerned about successfully renaming the application name (e.g. :jason)

@victorolinasc
Copy link
Contributor

I've been thinking about this problem for a while and I think that all current options are not 100% satisfactory IMHO.

I've been studying the source code and the core mechanisms that the server uses in order to provide its capabilities and it all boils down to having the project source and its dependencies compiled and loaded in a running BEAM instance. ElixirSense and ElixirLS depend on this core principle so that they can use :code and other introspection tools. It seems like there will be no escape from this.

(I've taken some general notes of the initialization/build process of the server as I think this could be included in the project to help people trying to understand/solve things. If you think so I'll gladly open a PR. I plan on writing a more in depth post about it)

I think the definite solution would be something akin to what the Erlang language server has done: we would have on BEAM instance running with ONLY a minor dep that would just broadcast compiler events/state/diagnostics about the project to another BEAM instance that could have any dependencies it would want. On this scenario it would be something like:

1 - Elixir Language Server;
N - BEAM nodes only purging/compiling/loading modules (one per project);

The server itself could be an escript that would run on many versions of Erlang and the broadcaster could be an archive as it would automatically be in all projects' scopes. But before diving into possible implementation details, I think would solve the issue of having a "clean" BEAM instance with the project's code loaded and checked on the same Erlang version as the project runs. I might try this approach some time in the future but I think this could add to the discussion.

@lukaszsamson
Copy link
Collaborator

Excellent writeup @victorolinasc. I added a small comment on code loading. Can you create a PR with this wrapped as .md documentation. I guess it's going to be useful to other contributors.

Regarding splitting elixir-ls into several os processes - I think it's the cleanest. I don't know much about Java class loaders etc. but a few years ago I did something related in .net land using ApplicationDomains (you could load an .dll assembly into a sandboxed domain, do some introspection with Reflection and unload it when done).

@victorolinasc
Copy link
Contributor

@lukaszsamson thanks for your comment! I'll add it to flow and open a PR later today!

Do you think it would be a lot of work to have it as separated processes?

@cdegroot
Copy link
Contributor

I'm gonna see today how far I get (we have hackday at work) https://github.com/cdegroot/elixir-ls/tree/beam-splitter; I'm gonna start inverting the "who starts what" relationship somewhat (versus @victorolinasc's proposal) because I think that it's easier to get it correct. See the fork's new subapplication's README for details (that's all I have done so far, I like README-driven development :-) )

@lukaszsamson the JVM makes a lot of this simpler, because a class is uniquely identified by not only its name, but also its classloader. But let's not go there, this should work and smells more "OTP-y".

@cdegroot cdegroot mentioned this issue Feb 21, 2020
5 tasks
@axelson
Copy link
Member Author

axelson commented Feb 23, 2020

I've updated the issue description with goals and requirements, are there any that seem incorrect or that I missed?

@axelson
Copy link
Member Author

axelson commented Feb 28, 2020

@lukaszsamson I wonder if you have time to take a look at #136 and if you have any thoughts. I'm starting to think that we may need to take an incremental approach to get there because so much code in ElixirSense (and ElixirLS) as well is dependent on running in the same beam instance as the user's source code. Maybe we need to initially keep ElixirSense running in the same beam instance but run ElixirLS in a stand-alone instance, but then move more and more of the code to run in the stand-alone instance.

@lukaszsamson
Copy link
Collaborator

I’ll take a look but I won’t have time to make any tests. I wonder if an incremental approach wouldn’t complicate things too much.

@cdegroot
Copy link
Contributor

cdegroot commented Mar 2, 2020

(no promises about time commitments - my espresso machine's thermostat seems to be wonky which may mean I'll be sidetracked PIDing it using Nerves - but I'll plod on if only to get a feel for the code. Structurally, I think the PR is were it needs to be modulo one nagging bug I'm ignoring, so it's a good time for me to get acquainted with the actual LSP bits)

@axelson
Copy link
Member Author

axelson commented Apr 16, 2020

erlang_ls is moving to at least 2 nodes for similar reasons as us: erlang-ls/erlang_ls#670 once that PR is merged maybe we can learn something from it.

@lukaszsamson
Copy link
Collaborator

This comment suggests a workaround for the problem with dependencies JakeBecker/elixir-ls#97 (comment)

@tomekowal
Copy link
Contributor

I wonder if it would be possible to go one step further and make elixir_ls dev dependency of a project, like credo or dialyxir. It would guarantee, elixir_lsp uses the same version of Erlang/Elixir and JSON libraries as the project. It seems simpler to me than a global server with multiple instances running different Elixir versions communicating with it.

It could produce different problems: e.g. someone needs to update their JSON lib version to use elixir_ls or maybe editors could have problems finding binary relative to the project path instead of a global one. Not sure if it is worth the tradeoff.

Excuse me, if I am talking nonsense, I am only starting to learn about LSP.

@ymtszw
Copy link

ymtszw commented Nov 26, 2020

Just a minor input from bystander, adding to @tomekowal's suggestion.
OCaml's current mainstream VSCode extension (vscode-ocaml-platform) actually takes exactly that approach.

Its installation instruction states:

  1. Install this extension from the VSCode Marketplace (or by entering ext install ocamllabs.ocaml-platform at the command palette Ctrl+P (Cmd+Shift+P on MacOS)
  2. Open a OCaml/ReasonML project (File > Add Folder to Workspace...)
  3. Install OCaml-LSP with opam or esy. E.g. opam install ocaml-lsp-server

Opam is the counterpart for mix in Elixir.

@axelson
Copy link
Member Author

axelson commented Nov 26, 2020

I'm not sure that I like the compromises required for a dependency-based approach. Namely:

  • As a developer working on a library that doesn't already use ElixirLS I would have to add ElixirLS to the deps and then ensure that I don't accidentally submit the changes to mix.exs and mix.lock when I make a PR
  • Potential version incompatibilities between the VSCode extension and ElixirLS

I'm not saying that I couldn't be convinced, but currently I don't favor that approach. But it is also true that the current packaging approach is not working very well.

@lukaszsamson
Copy link
Collaborator

It could work with if there's a sane fallback to default build. I fear that as @axelson said we'd have to deal with a slew of bugs from older elixir-ls versions.
Personally, I'm most convinced by https://github.com/elixir-lsp/elixir-ls/pull/121/files approach. OFC it would need to be implemented in sh/bat to be cross platform

@mpanarin
Copy link

Maybe the new https://github.com/burrito-elixir/burrito can help us here?

@lukaszsamson
Copy link
Collaborator

@Benjamin-Philip

I have been procrastinating in making the switch from alchemist.el for a few months now.
I realised that the main reason that I have not made the switch yet is because I don't want to manually download and manage elixir-ls binaries as is the current norm.

Thus I have 2 questions:
Is this possible with elixir-ls? Is there any reason why this hasn't already been done?

ElixirLS used to run as an escript but that was changed in v0.2.4: 25 Oct 2017: Package ElixirLS as .ez archives instead of escripts. This should make asdf installs work.
I don't know if this explanation is/was valid

Would a PR for such functionality be accepted?

Maybe. There is also a proposal of distributing it as source code #121

Another project that I have contributed to, Livebook supports multiple methods of installation, which includes, cloning from source, installing an escript from hex, or as a docker image.

Replicating the solution used in Livebook is something I have in plan

@Benjamin-Philip
Copy link

Package ElixirLS as .ez archives instead of escripts. This should make asdf installs work.
I don't know if this explanation is/was valid.

I don't think that should be a problem anymore. I believe you just need to reshim elixir on installing an escript.

@lukaszsamson
Copy link
Collaborator

@Benjamin-Philip for reference I looked up the PR and issue that removed escripts JakeBecker/elixir-ls#12 JakeBecker/elixir-ls#13
While it would be trivial to build and publish escripts to mix it stil does not solve the main problem with this project. We cannot simply embed elixir in the escript. elixirLS hast to run on the same version that the project is using (be it system/asdf or other install). Different elixir version may not be able to build a user's project and we should not force upgrades/downgrades. If we don't embed elixir (as in Jake's original version) then we have the same problem as we do with .ez archives. escripts are precompiled and if you run on a different elixir/OTP version than the one it was built on things start to break down.

@Benjamin-Philip
Copy link

@Benjamin-Philip for reference I looked up the PR and issue that removed escripts JakeBecker/elixir-ls#12 JakeBecker/elixir-ls#13
While it would be trivial to build and publish escripts to mix it stil does not solve the main problem with this project. We cannot simply embed elixir in the escript. elixirLS hast to run on the same version that the project is using (be it system/asdf or other install). Different elixir version may not be able to build a user's project and we should not force upgrades/downgrades. If we don't embed elixir (as in Jake's original version) then we have the same problem as we do with .ez archives. escripts are precompiled and if you run on a different elixir/OTP version than the one it was built on things start to break down.

I see why we don't use escripts.

One possible thing we could do is compile ElixirLS with all supported permutations of Elixir and OTP, and then depend on the package manager to install the most appropriate option.

This starts to breakdown when a user has an unsupported permutation. The package manager also may not install the correct option. This would also be non trivial to implement on the CI.

I am not sure if escripts installed using a git source are precompiled or compiled on a user's machine.

@lukaszsamson
Copy link
Collaborator

I have another idea. Since we are going to require 1.12 we can think of distributing elixir-ls as a Mix.install script. This way the vscode extension will use the project version of elixir and OTP, download a release from hexpm (or GH if thats more convenient) and build appropriate versions of beams. This will achieve the same results as @cdegroot proposal of source distribution with a benefit of free cross platform build provided by mix.

@skbolton
Copy link

skbolton commented May 2, 2023

erlang_ls is moving to at least 2 nodes for similar reasons as us: erlang-ls/erlang_ls#670 once that PR is merged maybe we can learn something from it.

I am going to take a stab at implementing this. They are 2 years into this architecture and still leaning into it. I think it could solve quite a few headaches we experience in this project. I will report back.

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 a pull request may close this issue.

9 participants