Skip to content

Apply environment variables after shell initialization scripts are run in pythonTerminalEnvVarActivation experiment #21290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
May 26, 2023
Merged

Apply environment variables after shell initialization scripts are run in pythonTerminalEnvVarActivation experiment #21290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
62a1668
Update proposed APIs to be used in Terminal activation experiment
May 23, 2023
6f6a121
Fix tests
May 23, 2023
48b20f4
Update VSCode engine
May 24, 2023
d0c031a
Update even lower
May 24, 2023
eb9ec4c
how abt
May 24, 2023
c02afe2
Update vscode engine to irignal
May 24, 2023
70ef676
Use latest insiders
May 24, 2023
2d9683b
Try
May 24, 2023
b7a6d1e
Update package-lock
May 24, 2023
f704700
Merge remote-tracking branch 'upstream/main' into kartik/updatePropos…
May 24, 2023
3254d7c
Update vscode engine to insiders which is supposed to be released today
May 24, 2023
05df610
Update engine to latest
May 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package-lock.json
View file
Open in desktop

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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;
}
}