Support MIX_TARGET?

[jjcarstens]

This isn't so much an issue as a feature request.

For those of use who do a lot of Nerves development, using elixir-ls and the vscode extension results in many error and warning lines because some dependencies are dependent on MIX_TARGET != :host and basically just requires having to turn off dialyzer and other fun features of this language server.

What is the general thought about supporting MIX_TARGET as a setting to be used when fetching deps and building plt?

I'd be happy to do the work, but might need a few hints (I have some local stuff where I can set the setting, but can't quite get it to work when fetching deps...)

[axelson]

Hi @jjcarstens, supporting MIX_TARGET is definitely in scope for the language server. Can you explain some more about how you see MIX_TARGET working? Would it just be statically set to one value for the entire repository or does it need to be changed dynamically?

[jjcarstens]

Okay, so a few thoughts and hopefully they make sense to you:

• I really only see this being used as a workspace setting and not globally (I’d always just have my global target set to host)
• I think it can lean more to the static side rather than dynamic. A Nerves project could, in theory, support much more than one target, but prob 90% of the time the project is focused on just one target in development. I don’t see myself changing the target for the background language server stuff that often. And if I do, I’m open to making that a manual process and forcing a reload

[axelson]

Okay that makes sense. So it sounds like once we have an in-repo configuration file (which is almost done and is tracked in #226) then that can be used to set the workspace setting (additionally someone could add a gui setting to the vscode extension). Then if the user wants to change the target they just need to change the setting in the file and restart ElixirLS. I can push up my branch with the configuration file if you want to base it off of there but it should be relatively easy to integrate after.

For the implementation I'd imagine that we'd primarily be adding a new key to ElixirLS.LanguageServer.Server's state and then use the value of that key when building in ElixirLS.LanguageServer.Build. Although it doesn't look like the elixir compiler/mix provides any hooks to control MIX_TARGET so it will probably be necessary to use System.put_env/2, which is global to the BEAM, but given the current architecture (one ElixirLS beam instance per project) that is okay.

[jjcarstens]

So Mix >= 1.8 has Mix.target/1 for setting it in the state and dealing with dependencies that are target specific. i.e
{:my_dep, "~> 1.0, targets: [:rpi3]}

But, elixir-ls minimum version is 1.7. I'm guessing the extension is built and published from that version, right? Does that mean all these language server dep fetching and building happens under an older elixir version as well? If so, this might be moot and we would miss out on actually getting the mix integration with MIX_TARGET?

This is my attempt so far but it doesn't seem to be working. MIX_TARGET gets set from Vscode config, but unused when fetching deps - Although, I could just be totally messing things up with trying to test it with vscode.
master...jjcarstens:mix-target

[jjcarstens]

And here's how I'm setting that state (just in vscode)
elixir-lsp/vscode-elixir-ls@master...jjcarstens:mix-target

[axelson]

@jjcarstens do you have a repo that could be used to easily test if the correct MIX_TARGET is being used?

But, elixir-ls minimum version is 1.7. I'm guessing the extension is built and published from that version, right? Does that mean all these language server dep fetching and building happens under an older elixir version as well? If so, this might be moot and we would miss out on actually getting the mix integration with MIX_TARGET?

ElixirLS is built with 1.7 but runs under the Elixir version that the user is running (which is a bit dicey), but that should allow supporting MIX_TARGET.

[jjcarstens]

Here's a simple contrived example to test:
https://github.com/jjcarstens/target_test

I'd expect this warning to go away in the main module.

You can also test with a brand new Nerves project with mix nerves.new something

Or, this is a pretty simple Nerves based fw project that you can clone and immediately get all the red:
https://github.com/elixir-circuits/circuits_quickstart

[axelson]

Okay, so I played with https://github.com/jjcarstens/target_test a bit and everything appears to work. My guess is that you weren't actually running ElixirLS with your changes (a good way to verify that is to change the startup message in apps/language_server/lib/language_server/cli.ex).

But anyway I was looking at the Elixir docs in more detail and I noticed that it says:

This feature is considered experimental and may change in future releases.

Which I didn't previously realize (do you know if that is still accurate?). That coupled with the fact that I have a workaround to suggest means that I'm leaning towards not including support directly in ElixirLS.

The workaround is to edit your ~/.config/elixir_ls/setup.sh file and add a check like:

if [[ "$PWD" == "/home/jason/dev/forks/tmp/target_test" ]]; then
	export MIX_TARGET=code
fi

Documentation about the setup.sh file can be found at https://github.com/elixir-lsp/elixir-ls/tree/f7b0524f721fddfda39f48d26bea23b978e8d5e7#local-setup

[jjcarstens]

Totally possible I wasn't running things right. I did add some print statements for MIX_TARGET in there and it seemed to be be right - But it wasn't checking the env and was just checking what was in Mix.State so there could be a difference there.

Is it experimental? -> Not sure. It has been in there for ~1.5 years and hasn't changed. Nerves is totally integrated with it now. So I feel like it was just forgotten about and no one removed that note? I put out a few feelers and this just happened: elixir-lang/elixir#10056 🤣

For the workaround, does it have to be ~/.config/elixir_ls/setup.sh or will it read that at a repo level as well? (my_repo/.elixir_ls/setup.sh) - Just getting full picture

So if the answer is "its no longer experimental" and that PR is merged, do you want to include MIX_TARGET in ElixirLS? I'm happy either way now that I know the work around exists - It might just determine if I make a note somewhere about it for posterity or if this issue existence is enough

[axelson]

Okay, so now that MIX_TARGET is officially non-experimental let's move forward on this. I think it is too early to remove support for Elixir 1.7 but I would also like to avoid using a private API if possible. So my proposal is to use Mix.target/1 alongside a runtime Elixir version check (e.g. Version.match?(System.version(), ">= 1.8.0")). On 1.7.x if a MIX_TARGET is specified than an error message should be logged using JsonRpc.show_message(:warning, message).

For the workaround, does it have to be ~/.config/elixir_ls/setup.sh or will it read that at a repo level as well? (my_repo/.elixir_ls/setup.sh) - Just getting full picture

Only ~/.config/elixir_ls/setup.sh is read (which is why I suggested checking the PWD in the script).

[jjcarstens]

🙌 yas!

Love it. Just to clarify, are you planning to implement this proposal or want me to? (I’m good either way, just not sure from that statement and don’t want to double up work 😉)

[axelson]

I'd love it if you took this on (as discussed over slack)

[axelson]

Oops, wrong button