Skip to content

Commit

Permalink
Apply environment variables after shell initialization scripts are ru…
Browse files Browse the repository at this point in the history 
…n in `pythonTerminalEnvVarActivation` experiment (#21290)

For #11039 #20822
Closes #21297

Update proposed APIs to be used in Terminal activation experiment.
  • Loading branch information
Kartik Raj authored May 26, 2023
1 parent 72f7ef8 commit c213491
Show file tree
Hide file tree
Showing 6 changed files with 141 additions and 70 deletions.
2 changes: 1 addition & 1 deletion package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
"testObserver",
"quickPickItemTooltip",
"envCollectionWorkspace",
"saveEditor"
"saveEditor",
"envCollectionOptions"
],
"author": {
"name": "Microsoft Corporation"
Expand All @@ -45,7 +46,7 @@
"theme": "dark"
},
"engines": {
"vscode": "^1.78.0-20230421"
"vscode": "^1.79.0-20230526"
},
"keywords": [
"python",
Expand Down
38 changes: 26 additions & 12 deletions src/client/interpreter/activation/terminalEnvVarCollectionService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,14 @@

import * as path from 'path';
import { inject, injectable } from 'inversify';
import { ProgressOptions, ProgressLocation, MarkdownString, WorkspaceFolder } from 'vscode';
import {
ProgressOptions,
ProgressLocation,
MarkdownString,
WorkspaceFolder,
EnvironmentVariableCollection,
EnvironmentVariableScope,
} from 'vscode';
import { pathExists } from 'fs-extra';
import { IExtensionActivationService } from '../../activation/types';
import { IApplicationShell, IApplicationEnvironment, IWorkspaceService } from '../../common/application/types';
Expand Down Expand Up @@ -108,6 +115,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
undefined,
shell,
);
const envVarCollection = this.getEnvironmentVariableCollection(workspaceFolder);
if (!env) {
const shellType = identifyShellFromShellPath(shell);
const defaultShell = defaultShells[this.platform.osType];
Expand All @@ -117,7 +125,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
await this._applyCollection(resource, defaultShell?.shell);
return;
}
this.context.environmentVariableCollection.clear({ workspaceFolder });
envVarCollection.clear();
this.previousEnvVars = _normCaseKeys(process.env);
return;
}
Expand All @@ -129,25 +137,32 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
if (prevValue !== value) {
if (value !== undefined) {
traceVerbose(`Setting environment variable ${key} in collection to ${value}`);
this.context.environmentVariableCollection.replace(key, value, { workspaceFolder });
envVarCollection.replace(key, value, { applyAtShellIntegration: true });
} else {
traceVerbose(`Clearing environment variable ${key} from collection`);
this.context.environmentVariableCollection.delete(key, { workspaceFolder });
envVarCollection.delete(key);
}
}
});
Object.keys(previousEnv).forEach((key) => {
// If the previous env var is not in the current env, clear it from collection.
if (!(key in env)) {
traceVerbose(`Clearing environment variable ${key} from collection`);
this.context.environmentVariableCollection.delete(key, { workspaceFolder });
envVarCollection.delete(key);
}
});
const displayPath = this.pathUtils.getDisplayName(settings.pythonPath, workspaceFolder?.uri.fsPath);
const description = new MarkdownString(`${Interpreters.activateTerminalDescription} \`${displayPath}\``);
this.context.environmentVariableCollection.setDescription(description, {
workspaceFolder,
});
envVarCollection.description = description;
}

private getEnvironmentVariableCollection(workspaceFolder?: WorkspaceFolder) {
const envVarCollection = this.context.environmentVariableCollection as EnvironmentVariableCollection & {
getScopedEnvironmentVariableCollection(scope: EnvironmentVariableScope): EnvironmentVariableCollection;
};
return workspaceFolder
? envVarCollection.getScopedEnvironmentVariableCollection({ workspaceFolder })
: envVarCollection;
}

private async handleMicroVenv(resource: Resource) {
Expand All @@ -156,12 +171,11 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
if (interpreter?.envType === EnvironmentType.Venv) {
const activatePath = path.join(path.dirname(interpreter.path), 'activate');
if (!(await pathExists(activatePath))) {
this.context.environmentVariableCollection.replace(
const envVarCollection = this.getEnvironmentVariableCollection(workspaceFolder);
envVarCollection.replace(
'PATH',
`${path.dirname(interpreter.path)}${path.delimiter}${process.env.Path}`,
{
workspaceFolder,
},
{ applyAtShellIntegration: true },
);
return;
}
Expand Down
66 changes: 35 additions & 31 deletions src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,13 @@ import * as sinon from 'sinon';
import { assert, expect } from 'chai';
import { cloneDeep } from 'lodash';
import { mock, instance, when, anything, verify, reset } from 'ts-mockito';
import { EnvironmentVariableCollection, ProgressLocation, Uri, WorkspaceFolder } from 'vscode';
import {
EnvironmentVariableCollection,
EnvironmentVariableScope,
ProgressLocation,
Uri,
WorkspaceFolder,
} from 'vscode';
import {
IApplicationShell,
IApplicationEnvironment,
Expand Down Expand Up @@ -39,7 +45,10 @@ suite('Terminal Environment Variable Collection Service', () => {
let context: IExtensionContext;
let shell: IApplicationShell;
let experimentService: IExperimentService;
let collection: EnvironmentVariableCollection;
let collection: EnvironmentVariableCollection & {
getScopedEnvironmentVariableCollection(scope: EnvironmentVariableScope): EnvironmentVariableCollection;
};
let scopedCollection: EnvironmentVariableCollection;
let applicationEnvironment: IApplicationEnvironment;
let environmentActivationService: IEnvironmentActivationService;
let workspaceService: IWorkspaceService;
Expand All @@ -62,7 +71,13 @@ suite('Terminal Environment Variable Collection Service', () => {
interpreterService = mock<IInterpreterService>();
context = mock<IExtensionContext>();
shell = mock<IApplicationShell>();
collection = mock<EnvironmentVariableCollection>();
collection = mock<
EnvironmentVariableCollection & {
getScopedEnvironmentVariableCollection(scope: EnvironmentVariableScope): EnvironmentVariableCollection;
}
>();
scopedCollection = mock<EnvironmentVariableCollection>();
when(collection.getScopedEnvironmentVariableCollection(anything())).thenReturn(instance(scopedCollection));
when(context.environmentVariableCollection).thenReturn(instance(collection));
experimentService = mock<IExperimentService>();
when(experimentService.inExperimentSync(TerminalEnvVarActivation.experiment)).thenReturn(true);
Expand Down Expand Up @@ -166,12 +181,12 @@ suite('Terminal Environment Variable Collection Service', () => {
).thenResolve(envVars);

when(collection.replace(anything(), anything(), anything())).thenResolve();
when(collection.delete(anything(), anything())).thenResolve();
when(collection.delete(anything())).thenResolve();

await terminalEnvVarCollectionService._applyCollection(undefined, customShell);

verify(collection.replace('CONDA_PREFIX', 'prefix/to/conda', anything())).once();
verify(collection.delete('PATH', anything())).once();
verify(collection.delete('PATH')).once();
});

test('Verify envs are not applied if env activation is disabled', async () => {
Expand All @@ -187,7 +202,7 @@ suite('Terminal Environment Variable Collection Service', () => {
).thenResolve(envVars);

when(collection.replace(anything(), anything(), anything())).thenResolve();
when(collection.delete(anything(), anything())).thenResolve();
when(collection.delete(anything())).thenResolve();
reset(configService);
when(configService.getSettings(anything())).thenReturn(({
terminal: { activateEnvironment: false },
Expand All @@ -197,10 +212,10 @@ suite('Terminal Environment Variable Collection Service', () => {
await terminalEnvVarCollectionService._applyCollection(undefined, customShell);

verify(collection.replace('CONDA_PREFIX', 'prefix/to/conda', anything())).never();
verify(collection.delete('PATH', anything())).never();
verify(collection.delete('PATH')).never();
});

test('Verify correct scope is used when applying envs and setting description', async () => {
test('Verify correct options are used when applying envs and setting description', async () => {
const envVars: NodeJS.ProcessEnv = { CONDA_PREFIX: 'prefix/to/conda', ..._normCaseKeys(process.env) };
delete envVars.PATH;
const resource = Uri.file('a');
Expand All @@ -214,25 +229,16 @@ suite('Terminal Environment Variable Collection Service', () => {
environmentActivationService.getActivatedEnvironmentVariables(resource, undefined, undefined, customShell),
).thenResolve(envVars);

when(collection.replace(anything(), anything(), anything())).thenCall((_e, _v, scope) => {
assert.deepEqual(scope, { workspaceFolder });
return Promise.resolve();
});
when(collection.delete(anything(), anything())).thenCall((_e, scope) => {
assert.deepEqual(scope, { workspaceFolder });
when(scopedCollection.replace(anything(), anything(), anything())).thenCall((_e, _v, options) => {
assert.deepEqual(options, { applyAtShellIntegration: true });
return Promise.resolve();
});
let description = '';
when(collection.setDescription(anything(), anything())).thenCall((d, scope) => {
assert.deepEqual(scope, { workspaceFolder });
description = d.value;
});
when(scopedCollection.delete(anything())).thenResolve();

await terminalEnvVarCollectionService._applyCollection(resource, customShell);

verify(collection.replace('CONDA_PREFIX', 'prefix/to/conda', anything())).once();
verify(collection.delete('PATH', anything())).once();
expect(description).to.equal(`${Interpreters.activateTerminalDescription} \`${displayPath}\``);
verify(scopedCollection.replace('CONDA_PREFIX', 'prefix/to/conda', anything())).once();
verify(scopedCollection.delete('PATH')).once();
});

test('Only relative changes to previously applied variables are applied to the collection', async () => {
Expand All @@ -251,7 +257,7 @@ suite('Terminal Environment Variable Collection Service', () => {
).thenResolve(envVars);

when(collection.replace(anything(), anything(), anything())).thenResolve();
when(collection.delete(anything(), anything())).thenResolve();
when(collection.delete(anything())).thenResolve();

await terminalEnvVarCollectionService._applyCollection(undefined, customShell);

Expand All @@ -270,8 +276,8 @@ suite('Terminal Environment Variable Collection Service', () => {

await terminalEnvVarCollectionService._applyCollection(undefined, customShell);

verify(collection.delete('CONDA_PREFIX', anything())).once();
verify(collection.delete('RANDOM_VAR', anything())).once();
verify(collection.delete('CONDA_PREFIX')).once();
verify(collection.delete('RANDOM_VAR')).once();
});

test('If no activated variables are returned for custom shell, fallback to using default shell', async () => {
Expand All @@ -294,12 +300,12 @@ suite('Terminal Environment Variable Collection Service', () => {
).thenResolve(envVars);

when(collection.replace(anything(), anything(), anything())).thenResolve();
when(collection.delete(anything(), anything())).thenResolve();
when(collection.delete(anything())).thenResolve();

await terminalEnvVarCollectionService._applyCollection(undefined, customShell);

verify(collection.replace('CONDA_PREFIX', 'prefix/to/conda', anything())).once();
verify(collection.delete(anything(), anything())).never();
verify(collection.delete(anything())).never();
});

test('If no activated variables are returned for default shell, clear collection', async () => {
Expand All @@ -313,12 +319,10 @@ suite('Terminal Environment Variable Collection Service', () => {
).thenResolve(undefined);

when(collection.replace(anything(), anything(), anything())).thenResolve();
when(collection.delete(anything(), anything())).thenResolve();
when(collection.setDescription(anything(), anything())).thenReturn();
when(collection.delete(anything())).thenResolve();

await terminalEnvVarCollectionService._applyCollection(undefined, defaultShell?.shell);

verify(collection.clear(anything())).once();
verify(collection.setDescription(anything(), anything())).never();
verify(collection.clear()).once();
});
});
56 changes: 56 additions & 0 deletions types/src/vscode-dts/vscode.proposed.envCollectionOptions.d.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

declare module 'vscode' {

// https://github.com/microsoft/vscode/issues/179476

/**
* Options applied to the mutator.
*/
export interface EnvironmentVariableMutatorOptions {
/**
* Apply to the environment just before the process is created.
*
* Defaults to true.
*/
applyAtProcessCreation?: boolean;

/**
* Apply to the environment in the shell integration script. Note that this _will not_ apply
* the mutator if shell integration is disabled or not working for some reason.
*
* Defaults to false.
*/
applyAtShellIntegration?: boolean;
}

/**
* A type of mutation and its value to be applied to an environment variable.
*/
export interface EnvironmentVariableMutator {
/**
* Options applied to the mutator.
*/
readonly options: EnvironmentVariableMutatorOptions;
}

export interface EnvironmentVariableCollection extends Iterable<[variable: string, mutator: EnvironmentVariableMutator]> {
/**
* @param options Options applied to the mutator.
*/
replace(variable: string, value: string, options?: EnvironmentVariableMutatorOptions): void;

/**
* @param options Options applied to the mutator.
*/
append(variable: string, value: string, options?: EnvironmentVariableMutatorOptions): void;

/**
* @param options Options applied to the mutator.
*/
prepend(variable: string, value: string, options?: EnvironmentVariableMutatorOptions): void;
}
}
44 changes: 20 additions & 24 deletions types/vscode.proposed.envCollectionWorkspace.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,34 +5,30 @@

declare module 'vscode' {

// https://github.com/microsoft/vscode/issues/171173
// https://github.com/microsoft/vscode/issues/182069

export interface EnvironmentVariableMutator {
readonly type: EnvironmentVariableMutatorType;
readonly value: string;
readonly scope: EnvironmentVariableScope | undefined;
}

export interface EnvironmentVariableCollection extends Iterable<[variable: string, mutator: EnvironmentVariableMutator]> {
/**
* Sets a description for the environment variable collection, this will be used to describe the changes in the UI.
* @param description A description for the environment variable collection.
* @param scope Specific scope to which this description applies to.
*/
setDescription(description: string | MarkdownString | undefined, scope?: EnvironmentVariableScope): void;
replace(variable: string, value: string, scope?: EnvironmentVariableScope): void;
append(variable: string, value: string, scope?: EnvironmentVariableScope): void;
prepend(variable: string, value: string, scope?: EnvironmentVariableScope): void;
get(variable: string, scope?: EnvironmentVariableScope): EnvironmentVariableMutator | undefined;
delete(variable: string, scope?: EnvironmentVariableScope): void;
clear(scope?: EnvironmentVariableScope): void;

}
// export interface ExtensionContext {
// /**
// * Gets the extension's environment variable collection for this workspace, enabling changes
// * to be applied to terminal environment variables.
// *
// * @param scope The scope to which the environment variable collection applies to.
// */
// readonly environmentVariableCollection: EnvironmentVariableCollection & { getScopedEnvironmentVariableCollection(scope: EnvironmentVariableScope): EnvironmentVariableCollection };
// }

export type EnvironmentVariableScope = {
/**
* The workspace folder to which this collection applies to. If unspecified, collection applies to all workspace folders.
*/
* Any specific workspace folder to get collection for. If unspecified, collection applicable to all workspace folders is returned.
*/
workspaceFolder?: WorkspaceFolder;
};

export interface EnvironmentVariableCollection extends Iterable<[variable: string, mutator: EnvironmentVariableMutator]> {
/**
* A description for the environment variable collection, this will be used to describe the
* changes in the UI.
*/
description: string | MarkdownString | undefined;
}
}

0 comments on commit c213491

Please sign in to comment.