allow specifying environment for language servers in language.toml

[TotalKrill]

This allows setting some environment variables for the language servers, as an addition to args and command

[the-mikedavis]

Why is this necessary? To my knowledge, all language servers take configuration through the LSP initialization request

[TotalKrill]

Why is this necessary? To my knowledge, all language servers take configuration through the LSP initialization request

There are usecases where you wish to specify an env variable for rust-analyzer, maybe not specifically to configure rust-analyzer to use specific env variables while evaluating proc-macros and build.rs files.

At least you can configure env variables in vs-code for this usecase, so I dunno

[nrdxp]

FWIW I use a Nix derivation to capture language server and environmental dependencies. The default is an empty list of both so I can easily share it with others.

Then I set my opinionated preferences in a different package that uses it as a base.

[jlloh]

Just started trying out Helix, and I was trying to use it for a legacy Golang project that did not support gomodules. To disable gomodules you need to set an environment variable export GO111MODULE=off (see here).

This is one use-case I can think of the top of my head.

[the-mikedavis]

Why not a HashMap<String, String> which would be a toml table?

language-server = { command = "mylang-lsp", args = [], env = { ENV1 = "value1", ENV2 = "value2" } }

This would also make it possible to write the env more clearly when there are many:

[[language]]
name = "mylang"

[language.language-server] 
command = "mylang-lsp"
args = []

[language.language-server.env]
ENV1 = "value1"
ENV2 = "value2"
# ...

[the-mikedavis]

Also, this is a bit of a nitpick but I'd prefer environment which is more readable

[TotalKrill]

That's a good point, and yes I think it would be better!

[sudormrfbin]

A usecase for this from #4246:

Is there an easy way to set the CARGO_TARGET_DIR env var when running rust-analyzer?

I'd like to set it to something like /tmp/rust-analyzer so it doesn't lock the project directory for when I run cargo build.

[StephenWakely]

@TotalKrill do we have any progress on this PR? Is there anything I can do to help move it forward?

I am building from this PR as my daily driver, it would be nice to get this merged into master.

[TotalKrill]

Yeah sorry, I think the only thing to do to move this forward is to actually perform the changes suggested by @the-mikedavis, the code-changes required are minimal.

However I ran into some confusion with getting things working due to other bugs with my lsp, and well, I have no good clue on how to verify that changes actually do what they are supposed to. I have no time to work on this in the forseeable future, due to bad time-planning and initiating projects that are very costly for me personally to miss.

So send a pull-request to my branch, and I will update that and it will move it forward, alternatively create a new branch and I can defer to that in this pull request. Either works for me!

[archseer]

Why not just set these ENV variables before starting hx? For example using dotenv in the same directory.

[StephenWakely]

So send a pull-request to my branch, and I will update that and it will move it forward, alternatively create a new branch and I can defer to that in this pull request. Either works for me!

I have a branch here: https://github.com/StephenWakely/helix/tree/lsp_env_variables

For some reason it isn't giving me the option to PR to your fork.

Are you able to update this PR to my branch? I'm not 100% sure that is possible.

[TotalKrill]

I manually cherry picked this instead, thanks for making the final changes @StephenWakely, I would personally see this as mergable now

[nrdxp]

The question is, do we really need this feature? What's wrong with a wrapper and/or shell environment like direnv or dotenv as already mentioned.

[StephenWakely]

The question is, do we really need this feature? What's wrong with a wrapper and/or shell environment like direnv or dotenv as already mentioned.

The thing that really grabbed me about Helix is its simplicity. To get up and running and working with Rust I have < 10 lines of config. I can share these settings very easily with coworkers to get them up and running straight away.

Setting up ENV vars would involve setting up something external to Helix that is an extra layer of complexity involving setting up aliases or other. I'm not totally familiar with dotenv or direnv, but I believe they change the environment variables based on the current folder - wouldn't that then cause conflicts when running cargo test from the command line? That's all extra friction which would prevent people from starting out.

Given it's going to be a relatively commonly used feature for a number of different language servers, I'm not seeing sufficient downsides that would make merging this problematic?

[nrdxp]

I'm not seeing sufficient downsides that would make merging this problematic?

I'm not arguing from an up/down side, but really a question of scope. I would argue this is outside the scope of Helix simply because there are well established, common tools such as shell wrappers to do such trivial manipulation of the users environment, and are indeed more powerful and versatile (overriding default values, etc), so I see no reason we should even bother reimplementing any of that here, but its a small change set, I won't be particularly offended if it is.

[jlloh]

My two cents: there's nothing existentially wrong with adding this functionality.

This new feature is entirely optional right? It's the same as manipulating languages.toml for anything else, out of the box it should work, but if you want to use a different LSP or formatter, you can. So I see it as an optional augmentation that you can decide to use or not.

If you decide not to set it in the toml file, but instead want to set it in your bash environment, it's up to the user. But I personally don't quite see (yet) why it shouldn't be a configurable part of helix.

[archseer]

There's nothing wrong with it, but it adds extra code that we have to maintain forever or until deprecation.

If you're using export GO111MODULE=off you likely want to apply it to everything including go build and not just gopls. If you're sure you want to scope it to helix, env GO111MODULE=off hx is what you want.

[TotalKrill]

I would argue that the freedom and utility for changing the environment variables for the LSP is equally important as allowing for changing the command itself.

Besides the amount of code to be maintained seems quite trivial, and from what I gather, removing this in the future, would not break onfigs either, since extra keys are ignored.

Basically, its both harmless to add, and useful in special circumstances

[jlloh]

There's nothing wrong with it, but it adds extra code that we have to maintain forever or until deprecation.

If you're using export GO111MODULE=off you likely want to apply it to everything including go build and not just gopls. If you're sure you want to scope it to helix, env GO111MODULE=off hx is what you want.

Fair points, especially regarding Go. Can't quite think of why one would set this only for Helix, and not for go build. And as you rightly pointed out, it's entirely possible to do it in your shell environment with an alias if needed.

I retract my argument and defer to your judgement.

[StephenWakely]

With Rust you have a variable CARGO_TARGET_DIR which is where Cargo puts all the build artifacts. Often you want to be running cargo build or cargo test on the command line whilst editing files in Helix. Whilst building this folder is locked so we don't get two build processes overwriting each other.

If both the command line and Helix have the same setting for this location you find that editing a file will cause the build directory to be locked which means you have to wait (sometimes for many minutes) before you can cargo test.

Setting CARGO_TARGET_DIR to a different folder for Helix is a pretty essential setting for Rust.

[korken89]

Hi, just to chime in.

For us in embedded Rust this feature is great (and sorely missed when moving from VSCode).
What is de-facto standard in the embedded Rust ecosystem is defmt which is a logging framework, and it is setup the same way as env-logger.
So to set things up you need to add an DEFMT_LOG=trace for example to make it do things.

Why defmt needs this feature and not env-logger comes from that env-logger work on types that implement Debug or Display however due to the binary compression used in embedded systems you need to implement the trait defmt::Format for the types you want to print.
So in short, defmt cannot fallback to Debug/Display.

The issue then comes if you sprinkle the code with info!/debug!/trace!/error! and the environment variable is not set, then the compiler cannot check that the types implement defmt::Format.
So when you write in helix all seems fine, no LSP errors. And then when you build and get the correct environment variables - everything throws compile errors.

With this we could easily get rid of this papercut, and the embedded community would indeed love it.

[archseer]

Is this something VSCode supports?

To me it still seems like you'd want DEFMT_LOG set automatically via dotenv when you enter the directory.

[korken89]

@archseer indeed it does, it allows for setting an environment variable.

On the other comment, generally no. You want helix to set the log level to trace so all calls are evaluated and checked.
While in the actual project you generally build/run with only error or warn unless really looking for some error and debugging.
Why one does not want it set by default is that it's common, at least in our developments, to forget and set correct levels for logging before deploying if it's automatic.
But that's specific to our way of working, while the more general use case is as explained before.

I hope I make sense :)

[dead10ck]

@archseer indeed it does, it allows for setting an environment variable.

On the other comment, generally no. You want helix to set the log level to trace so all calls are evaluated and checked. While in the actual project you generally build/run with only error or warn unless really looking for some error and debugging. Why one does not want it set by default is that it's common, at least in our developments, to forget and set correct levels for logging before deploying if it's automatic. But that's specific to our way of working, while the more general use case is as explained before.

I hope I make sense :)

Could you help me understand your workflow a little better? This env var could easily be set to trace in the shell instance in which you are running helix, while set to info/warn in the shell instance where you are building at the command line. Same goes for CARGO_TARGET_DIR

[dead10ck]

My take on this is that it would really only make sense to control env vars from helix if you need the var set one way in helix and a different way for your LSP. And I can't think of any case where it would be necessary to do this.

[StephenWakely]

easily

I wouldn't exactly say easily. Obviously it's not utterly impossible, but it is an extra layer of friction that breaks the flow of concentration.

My setup is not strictly one where I edit all my files in one terminal and run Cargo in another. I find I constantly jump between tmux panes, editing files in any of them as I go. For example, I'll run cargo test in a terminal, this pops up a build error, I'll quickly edit that file in the same terminal and run cargo test again. Sometimes that turns into a rabbit hole and I'll spend all day in that terminal, occasionally firing up another pane to run cargo watch or cargo build. My flow is thus fairly erratic.

So I couldn't really rely on setting and unsetting this var every time.

I would need to create a wrapper script or alias that sets this var and runs Helix. It's not too hard in fairness, but its another step that would need to be done whenever I install a new machine or when I convince a colleague to try out Helix, which I feel that goes against the ethos of simplicity and minimal setup that Helix is going for.

[nrdxp]

I would need to create a wrapper script or alias that sets this var and runs Helix. It's not too hard in fairness

How is that any different that opening a toml and editing it? They are nearly identical

[archseer]

To me this still seems like the wrong way of setting ENV vars, but it's reasonably small, there's demand for it & there's some precedence with VSCode supporting this.

Some minor comments then it looks good to merge

[TotalKrill]

Merged your comments, using githubs suggestion for doing so.