helix icon indicating copy to clipboard operation
helix copied to clipboard

allow specifying environment for language servers in language.toml

Open TotalKrill opened this issue 1 year ago • 26 comments

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

TotalKrill avatar Sep 28 '22 13:09 TotalKrill

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

the-mikedavis avatar Sep 28 '22 14:09 the-mikedavis

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

TotalKrill avatar Sep 28 '22 15:09 TotalKrill

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.

nrdxp avatar Sep 29 '22 00:09 nrdxp

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.

jlloh avatar Sep 30 '22 07:09 jlloh

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.

sudormrfbin avatar Oct 13 '22 17:10 sudormrfbin

@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.

StephenWakely avatar Nov 11 '22 11:11 StephenWakely

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!

TotalKrill avatar Nov 16 '22 12:11 TotalKrill

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

archseer avatar Nov 17 '22 01:11 archseer

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.

image

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

StephenWakely avatar Nov 17 '22 11:11 StephenWakely

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

TotalKrill avatar Nov 21 '22 16:11 TotalKrill

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.

nrdxp avatar Nov 21 '22 16:11 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.

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?

StephenWakely avatar Nov 21 '22 17:11 StephenWakely

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.

nrdxp avatar Nov 22 '22 05:11 nrdxp

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.

jlloh avatar Nov 22 '22 05:11 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.

archseer avatar Nov 22 '22 05:11 archseer

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

TotalKrill avatar Nov 22 '22 10:11 TotalKrill

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.

jlloh avatar Nov 22 '22 13:11 jlloh

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.

StephenWakely avatar Nov 22 '22 15:11 StephenWakely

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.

korken89 avatar Dec 01 '22 11:12 korken89

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.

archseer avatar Dec 01 '22 16:12 archseer

@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 :)

korken89 avatar Dec 01 '22 17:12 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 :)

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 avatar Dec 01 '22 22:12 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.

dead10ck avatar Dec 01 '22 22:12 dead10ck

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.

StephenWakely avatar Dec 02 '22 11:12 StephenWakely

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

nrdxp avatar Dec 03 '22 01:12 nrdxp

Merged your comments, using githubs suggestion for doing so.

TotalKrill avatar Dec 05 '22 09:12 TotalKrill