search

posit-dev / positron

Positron, a next-generation data science IDE
https://positron.posit.co
Other
2.82k stars 90 forks source link

remove python extension override of PATH variable #5354

Closed isabelizimm closed 1 week ago

isabelizimm commented 1 week ago

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: https://github.com/microsoft/vscode-python/issues/20492#issuecomment-1379454451 and https://github.com/microsoft/vscode/issues/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

juliasilge commented 1 week ago

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

seeM commented 1 week ago

[...] 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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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.

https://github.com/microsoft/vscode/issues/188235 https://github.com/microsoft/vscode/issues/94081 https://github.com/golang/vscode-go/issues/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 commented 1 week ago

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