Prepending PATH env var with environmentVariableCollection doesn't work on macOS

[DanTup]

This was covered a little bit in #94153 but that issue is locked and can't be discussed there. This feature feels pretty broken on macOS and I hope might be revisited.

The issue is on macOS prepending to PATH:

context.environmentVariableCollection.prepend("PATH", "/foo/my/path:");

This ends up being added to the end of the path, rather than the start. @Tyriar wrote up why this is in microsoft/vscode-docs@38653d4 and offered two workarounds, however neither of them are usable in my testing (as well as not really being feasible to tell all users to change):

• "terminal.integrated.inheritEnv": false makes no difference at all, the PATH till is added to the end (this is a problem because the purpose of using prepend above is that we need to override any existing SDK on PATH with the one the user just selected)
• "terminal.integrated.shellArgs": [] results in a completely empty PATH besides the values I've prepended, which means that pretty much everything else is missing and won't work. This might be solvable by pushing process.env.PATH into environmentVariableCollection too, however the prompt to the user would look ridiculous as it would include their entire PATH

I appreciate this probably isn't VS Code's fault, but given macOS is fairly common and this behaviour seems to happen on a clean macOS install (it happens on both of mine, and I've customised almost nothing) it feels like it's worth trying to fix (maybe even if it means sending export PATH=foo:$PATH once the shell is initialised? 😄).

[Tyriar]

@DanTup to see whether it's a vscode problem or otherwise you can do a trace log to see what environment the terminal is starting up with https://github.com/microsoft/vscode/wiki/Terminal-Issues#enabling-trace-logging

If the prepend is there, I don't think there's anything we can do?

[DanTup]

@Tyriar I believe the issue is as you describe, which is the default shell startup is building its wn path (with path_helper?) and then pushing that on to the front.

I don't know the solution, but it feels broken enough that it seems worthwhile trying to solve. For example could VS Code pass its own script to the shell to execute that does the prepending? Or could it reliable send an export PATH= command after the terminal initialises? (I don't like that, but if there's no other option and it works reliably, it seems better than this feature being broken on macOS?).

[DanTup]

I had tried with your samples and calling bash -l does break this in a terminal too.

I'm not saying it's VS Code's fault, I just think it's worth VS Code trying to work around in order to have this feature work well. I waited a really long time for this feature, and it sucks that it doesn't seem like I'll be able to use it 😞

[Tyriar]

Yeah this is annoying... I really don't like sending text but it does seem like a workaround is in order. How about the terminal checks on macOS for whether -l or --login is set and then sends export commands as you suggest? Making sure they work on sh, bash, zsh and fish is the bar it would need to meet (mainly just need to verify on fish).

[DanTup]

How about the terminal checks on macOS for whether -l or --login is set

I guess you mean whether it's set in shellArgs (in case the user overrode that from default)? Sounds reasonable to me, though I'm not very familiar with this stuff. If you have a possible fix, I'm definitely happy to test it out on my MacBooks here (with both have this issue) :-)

[Tyriar]

Yes I mean checking the args the terminal is launched with, which will take shellArgs and any explicit args into account if an extension launched it, to be safe it might also be a good idea to make sure the executable is set to something expected too.

[hyangah]

We (vscode-go) were trying to use this API to change the PATH in terminal when a user switches the go version, and encountered the same issue.

Following the suggestion, I first tried to sendText command to each shell terminal on darwin in addition to the environmentVariableCollection.prepend call. I also registered an onDidOpenTerminal handler to send newly opened terminals the shell command. This seemed to work ok, except immediately after environmentVariableCollection.prepend was applied - The environment change indicator showed up and when the user clicked 'Relaunch terminal', the login shell was applied again. This can be confusing.

So, I switched to completely avoid the call of envirnmentVariableCollection.prepend on darwin.
Now it's less confusing, but this approach is not ideal obviously. We had to give up another advantage of environmentVariableCollection, persisting the mutation across sessions, and had to use our own way of storing the state. Lost consistency across platforms is also sad.

[hyangah]

@Tyriar I probably misinterpreted your following suggesting in my implementation. Here you meant that the workaround is to send the export commands when -l or --login is set. Right?

How about the terminal checks on macOS for whether -l or --login is set and then sends export commands as you suggest? Making sure they work on sh, bash, zsh and fish is the bar it would need to meet (mainly just need to verify on fish).

[jahan01]

@Tyriar I would also like to have this work on macOS. Any variable you set is probably overridden by the rc files.
Most importantly since this approach is being suggested as workaround for python virtual env activation in terminal (microsoft/vscode-python#11039) and few others, would like to make this work through some hacks.

How about the workaround used by Jetbrains IDEs? They seem to have define the startup order and after sourcing the .zshrc they inject whatever they are trying to insert.

Its not ideal and looks very ugly, but better than sending texts after activation. I remember you were also not a big fan sending texts to terminal - #98490 (comment)

I was trying to write an extension to set paths and variables, and they are comfortably overwritten by the startup scripts.

collection.prepend("PATH", `${pythonPath}:`);
collection.replace("JAVA_HOME", javaDir);

So replace doesn't really work and prepend actually appends to last or it's not there, depending on how rcfiles export the PATH.

[DanTup]

@Tyriar

We may be able to leverage the upcoming shell integration feature to set the environment variables if needed.

I'm not very familiar with how this works, but now a lot of it has shipped do you have a better idea of whether it can solve this? Judging by the issues being linked here above, it isn't only Dart that would really like this. If you think it'd work but don't plan to work on it, could you provide any pointers for if someone wanted to try and contribute it?

Thanks!

[Tyriar]

@DanTup actually had a discussion with the Python team around this so I'm guessing this will get prioritized soon. The way I'm envisioning this working is for some environment variable being set (VSCODE_ENV_CONTRIBUTION or something?) and the shell integration script checking that and re-applying it after the init scripts run

[DanTup]

Awesome, thanks! I'm definitely interested in testing this if/when there's something more concrete :-)

[Tyriar]

@DanTup the fix for this just got merged if you want to take it for a spin in tomorrow's build. What it does is on mac, when you use a login shell and shell integration is enabled, we'll add the prepended items inside the shell integration script after the profile has run. This should work for bash, zsh and fish (pwsh didn't seem to need the fix).

[DanTup]

@Tyriar thanks! This seems to work. However, the UI when changing it doesn't seem very clear:

There's a yellow warning triangle here, but hovering over it or clicking on it gives no indication of the problem. There's another white warning triangle on the side, but the hover for it doesn't appear for quite a long time after you hover over it. I don't think it's very discoverable if a user has an existing session and this changes (and this is made worse by the terminal seeming to persist across reloads).

Is there anything that can be done to improve this? (I can file a new issue about it if necessary).

[Tyriar]

@DanTup we're going to remove the white one in #151160 in favor of the "tab status". Created #171498 to fix the info not showing up in the "single tab"

[DanTup]

Great, thanks! :-)