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: Improving the deps handling #201

Closed
axelson opened this issue Apr 12, 2020 · 4 comments
Closed

Discussion: Improving the deps handling #201

axelson opened this issue Apr 12, 2020 · 4 comments

Comments

@axelson
Copy link
Member

axelson commented Apr 12, 2020

Current state of affairs:

  • ElixirLS has a separate _build directory (within the .elixir_ls folder)
  • ElixirLS shares the deps folder with the main repository
  • ElixirLS automatically downloads deps when you change your mix.exs
  • ElixirLS sometimes gets stuck with a compilation error, possibly related to the automatic deps downloading

In #197 it has been proposed that ElixirLS should have it's own deps folder (in addition to _build). We have also discussed in Slack the issues with the automatic dep downloading (although I'm not sure we have an actual issue filed for it since it can be resolved by removing the .elixir_ls directory, although that is very annoying). I hypothesize that sharing the deps directory is related to the ElixirLS build getting stuck in a bad state.

My proposal is to:

  1. Give ElixirLS it's own deps folder within the .elixir_ls directory
  2. Remove the automatic dep downloading (since it is not reliable enough and relies on a significant amount of mix private api's)
  3. Instead, when mix.exs is updated with new dependencies show the user a message like "mix.exs has been updated and ElixirLS requires a restart (or restart your entire editor)"
  4. Whenever the server starts ensure that mix deps.get is run (if it is needed)

Any other approaches/feedback?

@cblage
Copy link

cblage commented Apr 12, 2020

My 2 cents: if the deps directory gets moved into .elixir_ls the auto-download feature should work without issues so I don't know if it's necessary to get rid of it (maybe make it optional?).

Agree 100% with having a separate deps from the main project tho 😃

@victorolinasc
Copy link
Contributor

Beware of the noob idea ahead!!!

What if we didn't depend on mix/compiling/downloading at all? I mean, we need code "loaded" into the BEAM instance for completion, references and so on. As far as I can tell, if we could simply load what the main project does with deps/code/etc, than it wouldn't be needed to perform all those complicated tasks.

I know we depend on "compiling" for some diagnostics info like compiler warnings and so on. But, can't we mix the loading of compiled code with a parser (like this one) and provide all the features we currently provide?

Sorry if there is a huge hole on my theory... I am NOT an expert at all on compilers and language servers...

@axelson
Copy link
Member Author

axelson commented Apr 29, 2020

@victorolinasc I'm pretty sure that not compiling the code would require a substantial re-write of most of the tooling in ElixirLS/ElixirSense.

We might be able to use MIX_DEPS_PATH which was added in 1.10:

  • MIX_DEPS_PATH - sets the project Mix.Project.deps_path/0 config

Although the lack of support on earlier elixir versions could prove to be an issue (but maybe it is fine if the deps continues to be shared in that case).

My worry after this is implemented is that if there is a failure in deps fetching, it may be more difficult to recover from. But if we make this opt-in via #226 we can have a longer testing period which would significantly help.

@lukaszsamson
Copy link
Collaborator

Closing this for now. Auto dep download is disabled by default and I don't have plans for changing that.

What if we didn't depend on mix/compiling/downloading at all?

With tracer support we are moving closer to being able to do that ;)

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

No branches or pull requests

4 participants