remove python extension override of PATH variable

[isabelizimm]

Related to #4511

the Python extension is using a newer version of VSCode, with a GlobalEnvironmentVariableCollection which overwrites other extension's contributions to environment variables, such as PATH. This moves to using EnvironmentVariableCollection, which will only touch its extension contributions.

From what I understand, the original change was made to be able to set multiple folder level scopes in a workspace, eg, set different interpreters for different folders inside a workspace: microsoft/vscode-python#20492 (comment) and microsoft/vscode#171173
Positron operates (or at least the interpreter dropdown UI shows) 1 interpreter per workspace, so I don’t think the scoping stuff applies to us.

QA Notes

• make sure Quarto is in your local positron directory. AFAICT this does not automatically populate on yarn runs, so you might have to put it there manually. (At build time, it is at ~/Applications/Positron.app/Contents/Resources/app/quarto/bin, which is being properly identified by the Quarto extension 👍)
• delete other local Quarto installations (likely by running rm -fr ~/Applications/quarto)
• open up Positron
• run quarto --version in terminal, should not give any errors

[juliasilge]

Just kicked off this action to check it out with a release build.

[juliasilge]

This worked for me! I removed my local installation of Quarto and installed the release build that I kicked off earlier. I then see:

• quarto --version failing with "command not found" in my terminal
• quarto --version succeeding with the bundled version of Quarto in the Positron integrated terminal

I also can preview an .Rmd while having a pyenv-managed Python env up and running (both in the Positron console and the integrated terminal), which I believe was the original broken example.

As far as this:

From what I understand, the original change was made to be able to set multiple folder level scopes in a workspace, eg, set different interpreters for different folders inside a workspace

It's pretty complicated to understand why GlobalEnvironmentVariableCollection is here from my perspective, but your explanation seems right @isabelizimm. We are going to be looking at "more than one console session at a time" soon, but certainly we have no plans whatsoever for "different interpreters for subfolders in your workspace".

[seeM]

the Python extension is using a newer version of VSCode, with a GlobalEnvironmentVariableCollection which overwrites other extension's contributions to environment variables, such as PATH. This moves to using EnvironmentVariableCollection, which will only touch its extension contributions.

I'm not sure about this. My understanding is that GlobalEnvironmentVariableCollection means "not scoped to a workspace", but both of these types are always supposed to be scoped to an extension – that's why they're accessed via ExtensionContext.environmentVariableCollection.

I'm surprised that removing workspace scoping would affect the Python extension overriding
environment variables from other extensions. I wonder if we're just getting lucky by removing the scoping, for example, maybe the Python extension is now working globally and Quarto is scoped to a workspace, and that way they're somehow not colliding?

The Python extension should already be trying to retain any existing PATH entries (see the existing logic here). Is there perhaps an issue in that logic? I'm curious why the existing logic isn't working well with Quarto without these scoping changes.

[seeM]

[...] but certainly we have no plans whatsoever for "different interpreters for subfolders in your workspace".

My understanding is this functionality is for multi-root workspaces (which I personally use when developing Positron), which we don't currently support in Positron but are well-supported by VSCode and the Python extension.

[juliasilge]

FWIW I think multi-root workspaces with different Python interpreters per folder (as opposed to multi-root workspaces that use all the same Python interpreter) is not something we are wanting to prioritize anytime soon, if that helps making a call on these different options for environment variable collection.

I will admit that I am still fuzzy what is going on with GlobalEnvironmentVariableCollection, even after trying to read through issues/PRs on both the upstream repos.

[seeM]

FWIW I think multi-root workspaces with different Python interpreters per folder (as opposed to multi-root workspaces that use all the same Python interpreter) is not something we are wanting to prioritize anytime soon

Agreed. I'm happy for us to merge this, if it fixes the underlying issue and doesn't break anything else. I'm just slightly concerned about whether that's really the case since I don't understand why removing workspace folder scoping would affect interactions between extensions.

Either I'm missing something, or something else is going on and a slightly different fix could be more robust, or it could even be a bug in VSCode?

[isabelizimm]

To give some extra context:

The exact location where there is a problem is this line where the value is being prepended to PATH. If that is commented out, the quarto PATH is added as expected. The value added seems to be correct (eg, '/Users/isabelzimmerman/.pyenv/versions/3.10.7/envs/positron/bin:'), and the logic around there is for finding the right value, not necessarily altering the path. It seems that the actual prepending is overwriting. I'm pretty confident that the scopes was the original reason for the change to a GlobalEnvironmentVariableCollection from EnvironmentVariableCollection, but I unfortunately do agree it feels a little mysterious why changing the scope fixes this particular problem 😩

I'll go ahead and merge this for now, but I've added a list of potentially related bugs in vscode/other extensions in case this comes up again.

microsoft/vscode#188235 microsoft/vscode#94081 golang/vscode-go#2864

(Other musing: I briefly thought that the problem is based off the fact that extensions can only alter an env var 1x and maybe since both quarto and python are built in we can't alter 1+ times. However, since each extension is running its own activation, I don't think its the case.)

[seeM]

@isabelizimm thanks the extra context is helpful! Those issues do seem related but hopefully this change will fix it.