Skip to content

Commit

Permalink
remove python extension override of PATH variable (#5354)
Browse files Browse the repository at this point in the history
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

---------

Signed-off-by: Isabel Zimmerman <[email protected]>
  • Loading branch information
isabelizimm authored Nov 14, 2024
1 parent e3a1ef1 commit 64e113a
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@ import { inject, injectable } from 'inversify';
import {
MarkdownString,
WorkspaceFolder,
GlobalEnvironmentVariableCollection,
EnvironmentVariableScope,
// --- Start Positron ---
// GlobalEnvironmentVariableCollection,
// EnvironmentVariableScope,
EnvironmentVariableCollection,
// --- End Positron ---
EnvironmentVariableMutatorOptions,
ProgressLocation,
} from 'vscode';
Expand Down Expand Up @@ -169,7 +172,10 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
private async _applyCollectionImpl(resource: Resource, shell = this.applicationEnvironment.shell): Promise<void> {
const workspaceFolder = this.getWorkspaceFolder(resource);
const settings = this.configurationService.getSettings(resource);
const envVarCollection = this.getEnvironmentVariableCollection({ workspaceFolder });
// --- Start Positron ---
// remove workspace folder scope to avoid overwriting other extensions' env vars
const envVarCollection = this.getEnvironmentVariableCollection();
// --- End Positron ---
if (!settings.terminal.activateEnvironment) {
envVarCollection.clear();
traceVerbose('Activating environments in terminal is disabled for', resource?.fsPath);
Expand Down Expand Up @@ -368,9 +374,12 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
private async handleMicroVenv(resource: Resource) {
try {
const settings = this.configurationService.getSettings(resource);
const workspaceFolder = this.getWorkspaceFolder(resource);
// --- Start Positron ---
// remove workspace folder scope to avoid overwriting other extensions' env vars
// const workspaceFolder = this.getWorkspaceFolder(resource);
if (!settings.terminal.activateEnvironment) {
this.getEnvironmentVariableCollection({ workspaceFolder }).clear();
this.getEnvironmentVariableCollection().clear();
// --- End Positron ---
traceVerbose(
'Do not activate microvenv as activating environments in terminal is disabled for',
resource?.fsPath,
Expand All @@ -381,7 +390,9 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
if (interpreter?.envType === EnvironmentType.Venv) {
const activatePath = path.join(path.dirname(interpreter.path), 'activate');
if (!(await pathExists(activatePath))) {
const envVarCollection = this.getEnvironmentVariableCollection({ workspaceFolder });
// --- Start Positron ---
// remove workspace folder scope to avoid overwriting other extensions' env vars
const envVarCollection = this.getEnvironmentVariableCollection();
const pathVarName = getSearchPathEnvVarNames()[0];
envVarCollection.replace(
'PATH',
Expand All @@ -390,7 +401,8 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
);
return;
}
this.getEnvironmentVariableCollection({ workspaceFolder }).clear();
this.getEnvironmentVariableCollection().clear();
// --- End Positron ---
}
} catch (ex) {
traceWarn(`Microvenv failed as it is using proposed API which is constantly changing`, ex);
Expand All @@ -412,9 +424,18 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
};
}

private getEnvironmentVariableCollection(scope: EnvironmentVariableScope = {}) {
const envVarCollection = this.context.environmentVariableCollection as GlobalEnvironmentVariableCollection;
return envVarCollection.getScoped(scope);
// --- Start Positron ---
// Global Environment Variable Collections will overwrite other additions to the same environment variable
// from other extensions, eg, Quarto

private getEnvironmentVariableCollection() {
// private getEnvironmentVariableCollection(scope: EnvironmentVariableScope = {}) {
// const envVarCollection = this.context.environmentVariableCollection as GlobalEnvironmentVariableCollection;
// return envVarCollection.getScoped(scope);

const envVarCollection = this.context.environmentVariableCollection as EnvironmentVariableCollection;
return envVarCollection;
// --- End Positron ---
}

private getWorkspaceFolder(resource: Resource): WorkspaceFolder | undefined {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import { mock, instance, when, anything, verify, reset } from 'ts-mockito';
import {
EnvironmentVariableCollection,
EnvironmentVariableMutatorOptions,
GlobalEnvironmentVariableCollection,
// --- Start Positron ---
// GlobalEnvironmentVariableCollection,
// --- End Positron ---
ProgressLocation,
Uri,
WorkspaceFolder,
Expand Down Expand Up @@ -47,7 +49,9 @@ suite('Terminal Environment Variable Collection Service', () => {
let shell: IApplicationShell;
let experimentService: IExperimentService;
let collection: EnvironmentVariableCollection;
let globalCollection: GlobalEnvironmentVariableCollection;
// --- Start Positron ---
// let globalCollection: GlobalEnvironmentVariableCollection;
// --- End Positron ---
let applicationEnvironment: IApplicationEnvironment;
let environmentActivationService: IEnvironmentActivationService;
let workspaceService: IWorkspaceService;
Expand Down Expand Up @@ -78,10 +82,12 @@ suite('Terminal Environment Variable Collection Service', () => {
const envVarProvider = mock<IEnvironmentVariablesProvider>();
shellIntegrationService = mock<IShellIntegrationService>();
when(shellIntegrationService.isWorking()).thenResolve(true);
globalCollection = mock<GlobalEnvironmentVariableCollection>();
// --- Start Positron ---
// globalCollection = mock<GlobalEnvironmentVariableCollection>();
collection = mock<EnvironmentVariableCollection>();
when(context.environmentVariableCollection).thenReturn(instance(globalCollection));
when(globalCollection.getScoped(anything())).thenReturn(instance(collection));
when(context.environmentVariableCollection as EnvironmentVariableCollection).thenReturn(instance(collection));
// when(globalCollection.getScoped(anything())).thenReturn(instance(collection));
// --- End Positron ---
experimentService = mock<IExperimentService>();
when(experimentService.inExperimentSync(TerminalEnvVarActivation.experiment)).thenReturn(true);
applicationEnvironment = mock<IApplicationEnvironment>();
Expand Down

0 comments on commit 64e113a

Please sign in to comment.