From 96a87e79845952086e7c58fb46e7e58b6b24a565 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 18 Aug 2023 21:16:23 +0000 Subject: [PATCH 1/7] Update proposed API for env collection --- .../terminalEnvVarCollectionService.ts | 89 +++++++++++-------- ...scode.proposed.envCollectionWorkspace.d.ts | 50 ++++++----- 2 files changed, 80 insertions(+), 59 deletions(-) diff --git a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts index 5dc716f82afa..258f40f20295 100644 --- a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts +++ b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts @@ -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, + GlobalEnvironmentVariableCollection, + EnvironmentVariableScope, +} from 'vscode'; import { pathExists } from 'fs-extra'; import { IExtensionActivationService } from '../../activation/types'; import { IApplicationShell, IApplicationEnvironment, IWorkspaceService } from '../../common/application/types'; @@ -20,7 +27,7 @@ import { } from '../../common/types'; import { Deferred, createDeferred } from '../../common/utils/async'; import { Interpreters } from '../../common/utils/localize'; -import { traceDecoratorVerbose, traceVerbose, traceWarn } from '../../logging'; +import { traceDecoratorVerbose, traceError, traceVerbose, traceWarn } from '../../logging'; import { IInterpreterService } from '../contracts'; import { defaultShells } from './service'; import { IEnvironmentActivationService, ITerminalEnvVarCollectionService } from './types'; @@ -61,52 +68,57 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ ) {} public async activate(resource: Resource): Promise { - if (!inTerminalEnvVarExperiment(this.experimentService)) { - this.context.environmentVariableCollection.clear(); - await this.handleMicroVenv(resource); + // TODO: Remove try...catch once proposed APIs being used are finalized. + try { + if (!inTerminalEnvVarExperiment(this.experimentService)) { + this.context.environmentVariableCollection.clear(); + await this.handleMicroVenv(resource); + if (!this.registeredOnce) { + this.interpreterService.onDidChangeInterpreter( + async (r) => { + await this.handleMicroVenv(r); + }, + this, + this.disposables, + ); + this.registeredOnce = true; + } + return; + } if (!this.registeredOnce) { this.interpreterService.onDidChangeInterpreter( async (r) => { - await this.handleMicroVenv(r); + this.showProgress(); + await this._applyCollection(r).ignoreErrors(); + this.hideProgress(); + }, + this, + this.disposables, + ); + this.applicationEnvironment.onDidChangeShell( + async (shell: string) => { + this.showProgress(); + this.processEnvVars = undefined; + // Pass in the shell where known instead of relying on the application environment, because of bug + // on VSCode: https://github.com/microsoft/vscode/issues/160694 + await this._applyCollection(undefined, shell).ignoreErrors(); + this.hideProgress(); }, this, this.disposables, ); this.registeredOnce = true; } - return; - } - if (!this.registeredOnce) { - this.interpreterService.onDidChangeInterpreter( - async (r) => { - this.showProgress(); - await this._applyCollection(r).ignoreErrors(); - this.hideProgress(); - }, - this, - this.disposables, - ); - this.applicationEnvironment.onDidChangeShell( - async (shell: string) => { - this.showProgress(); - this.processEnvVars = undefined; - // Pass in the shell where known instead of relying on the application environment, because of bug - // on VSCode: https://github.com/microsoft/vscode/issues/160694 - await this._applyCollection(undefined, shell).ignoreErrors(); - this.hideProgress(); - }, - this, - this.disposables, - ); - this.registeredOnce = true; + this._applyCollection(resource).ignoreErrors(); + } catch (ex) { + traceError(`Activating terminal env collection failed`, ex); } - this._applyCollection(resource).ignoreErrors(); } public async _applyCollection(resource: Resource, shell = this.applicationEnvironment.shell): Promise { const workspaceFolder = this.getWorkspaceFolder(resource); const settings = this.configurationService.getSettings(resource); - const envVarCollection = this.context.getEnvironmentVariableCollection({ workspaceFolder }); + const envVarCollection = this.getEnvironmentVariableCollection({ workspaceFolder }); // Clear any previously set env vars from collection envVarCollection.clear(); if (!settings.terminal.activateEnvironment) { @@ -232,22 +244,27 @@ 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.context.getEnvironmentVariableCollection({ workspaceFolder }); + const envVarCollection = this.getEnvironmentVariableCollection({ workspaceFolder }); const pathVarName = getSearchPathEnvVarNames()[0]; envVarCollection.replace( 'PATH', `${path.dirname(interpreter.path)}${path.delimiter}${process.env[pathVarName]}`, - { applyAtShellIntegration: true, applyAtProcessCreation: true }, + { applyAtShellIntegration: true }, ); return; } - this.context.getEnvironmentVariableCollection({ workspaceFolder }).clear(); } + this.getEnvironmentVariableCollection({ workspaceFolder }).clear(); } catch (ex) { traceWarn(`Microvenv failed as it is using proposed API which is constantly changing`, ex); } } + private getEnvironmentVariableCollection(scope?: EnvironmentVariableScope) { + const envVarCollection = this.context.environmentVariableCollection as GlobalEnvironmentVariableCollection; + return scope?.workspaceFolder ? envVarCollection.getScoped(scope) : envVarCollection; + } + private getWorkspaceFolder(resource: Resource): WorkspaceFolder | undefined { let workspaceFolder = this.workspaceService.getWorkspaceFolder(resource); if ( diff --git a/types/vscode.proposed.envCollectionWorkspace.d.ts b/types/vscode.proposed.envCollectionWorkspace.d.ts index 406e5b98cd47..494929ba15eb 100644 --- a/types/vscode.proposed.envCollectionWorkspace.d.ts +++ b/types/vscode.proposed.envCollectionWorkspace.d.ts @@ -4,30 +4,34 @@ *--------------------------------------------------------------------------------------------*/ declare module 'vscode' { + // https://github.com/microsoft/vscode/issues/171173 - // https://github.com/microsoft/vscode/issues/182069 + // export interface ExtensionContext { + // /** + // * Gets the extension's global environment variable collection for this workspace, enabling changes to be + // * applied to terminal environment variables. + // */ + // readonly environmentVariableCollection: GlobalEnvironmentVariableCollection; + // } - export interface ExtensionContext { - /** - * Gets the extension's environment variable collection for this workspace, enabling changes - * to be applied to terminal environment variables. - * - * @deprecated Use {@link getEnvironmentVariableCollection} instead. - */ - readonly environmentVariableCollection: EnvironmentVariableCollection; - /** - * 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. - */ - getEnvironmentVariableCollection(scope?: EnvironmentVariableScope): EnvironmentVariableCollection; - } + export interface GlobalEnvironmentVariableCollection extends EnvironmentVariableCollection { + /** + * Gets scope-specific environment variable collection for the extension. This enables alterations to + * terminal environment variables solely within the designated scope, and is applied in addition to (and + * after) the global collection. + * + * Each object obtained through this method is isolated and does not impact objects for other scopes, + * including the global collection. + * + * @param scope The scope to which the environment variable collection applies to. + */ + getScoped(scope: EnvironmentVariableScope): EnvironmentVariableCollection; + } - export type EnvironmentVariableScope = { - /** - * Any specific workspace folder to get collection for. If unspecified, collection applicable to all workspace folders is returned. - */ - workspaceFolder?: WorkspaceFolder; - }; + export type EnvironmentVariableScope = { + /** + * Any specific workspace folder to get collection for. If unspecified, collection applicable to all workspace folders is returned. + */ + workspaceFolder?: WorkspaceFolder; + }; } From e2e56a7818d4ce133c1de2e8a87f98fc49fd946e Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 18 Aug 2023 21:24:44 +0000 Subject: [PATCH 2/7] Fix --- .../activation/terminalEnvVarCollectionService.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts index 258f40f20295..4ac23576ce34 100644 --- a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts +++ b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts @@ -68,7 +68,6 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ ) {} public async activate(resource: Resource): Promise { - // TODO: Remove try...catch once proposed APIs being used are finalized. try { if (!inTerminalEnvVarExperiment(this.experimentService)) { this.context.environmentVariableCollection.clear(); @@ -249,12 +248,12 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ envVarCollection.replace( 'PATH', `${path.dirname(interpreter.path)}${path.delimiter}${process.env[pathVarName]}`, - { applyAtShellIntegration: true }, + { applyAtShellIntegration: true, applyAtProcessCreation: true }, ); return; } + this.getEnvironmentVariableCollection({ workspaceFolder }).clear(); } - this.getEnvironmentVariableCollection({ workspaceFolder }).clear(); } catch (ex) { traceWarn(`Microvenv failed as it is using proposed API which is constantly changing`, ex); } From 9981e61790dcd9b8c2993024ac82e59ccdda8896 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 23 Aug 2023 17:25:28 +0000 Subject: [PATCH 3/7] Update package-lock --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 84357451dc3c..e001c51ee220 100644 --- a/package-lock.json +++ b/package-lock.json @@ -128,7 +128,7 @@ "yargs": "^15.3.1" }, "engines": { - "vscode": "^1.82.0-20230809" + "vscode": "^1.82.0-20230823" } }, "node_modules/@aashutoshrathi/word-wrap": { diff --git a/package.json b/package.json index 9e5c3e596a43..bc7378aa521b 100644 --- a/package.json +++ b/package.json @@ -46,7 +46,7 @@ "theme": "dark" }, "engines": { - "vscode": "^1.82.0-20230809" + "vscode": "^1.82.0-20230823" }, "enableTelemetry": false, "keywords": [ From ebf69e8d1f5c511ba9bc694c3fbb20e7a2899cc4 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 23 Aug 2023 17:31:40 +0000 Subject: [PATCH 4/7] Fix comple --- .../activation/terminalEnvVarCollectionService.unit.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts index b63750e5bf73..9da267aaf849 100644 --- a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts +++ b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts @@ -74,7 +74,7 @@ suite('Terminal Environment Variable Collection Service', () => { getScopedEnvironmentVariableCollection(scope: EnvironmentVariableScope): EnvironmentVariableCollection; } >(); - when(context.getEnvironmentVariableCollection(anything())).thenReturn(instance(collection)); + when(context.environmentVariableCollection).thenReturn(instance(collection)); experimentService = mock(); when(experimentService.inExperimentSync(TerminalEnvVarActivation.experiment)).thenReturn(true); applicationEnvironment = mock(); From 40cb4c8196525abdd237cae828f3379837d1403e Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 23 Aug 2023 18:08:40 +0000 Subject: [PATCH 5/7] Fix unit tests --- .../terminalEnvVarCollectionService.ts | 11 ++++++++-- ...rminalEnvVarCollectionService.unit.test.ts | 22 ++++++++++--------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts index 4ac23576ce34..33d51e7e2ed0 100644 --- a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts +++ b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts @@ -232,6 +232,13 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ // PS1 should be set but no PS1 was set. return; } + const config = this.workspaceService + .getConfiguration('terminal') + .get('integrated.shellIntegration.enabled'); + if (!config) { + traceVerbose('PS1 is not set when shell integration is disabled.'); + return; + } } this.terminalPromptIsCorrect(resource); } @@ -259,9 +266,9 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ } } - private getEnvironmentVariableCollection(scope?: EnvironmentVariableScope) { + private getEnvironmentVariableCollection(scope: EnvironmentVariableScope = {}) { const envVarCollection = this.context.environmentVariableCollection as GlobalEnvironmentVariableCollection; - return scope?.workspaceFolder ? envVarCollection.getScoped(scope) : envVarCollection; + return envVarCollection.getScoped(scope); } private getWorkspaceFolder(resource: Resource): WorkspaceFolder | undefined { diff --git a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts index 9da267aaf849..e343ee8424c3 100644 --- a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts +++ b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts @@ -9,9 +9,10 @@ import { mock, instance, when, anything, verify, reset } from 'ts-mockito'; import { EnvironmentVariableCollection, EnvironmentVariableMutatorOptions, - EnvironmentVariableScope, + GlobalEnvironmentVariableCollection, ProgressLocation, Uri, + WorkspaceConfiguration, WorkspaceFolder, } from 'vscode'; import { @@ -44,12 +45,12 @@ suite('Terminal Environment Variable Collection Service', () => { let context: IExtensionContext; let shell: IApplicationShell; let experimentService: IExperimentService; - let collection: EnvironmentVariableCollection & { - getScopedEnvironmentVariableCollection(scope: EnvironmentVariableScope): EnvironmentVariableCollection; - }; + let collection: EnvironmentVariableCollection; + let globalCollection: GlobalEnvironmentVariableCollection; let applicationEnvironment: IApplicationEnvironment; let environmentActivationService: IEnvironmentActivationService; let workspaceService: IWorkspaceService; + let workspaceConfig: WorkspaceConfiguration; let terminalEnvVarCollectionService: TerminalEnvVarCollectionService; const progressOptions = { location: ProgressLocation.Window, @@ -62,19 +63,20 @@ suite('Terminal Environment Variable Collection Service', () => { setup(() => { workspaceService = mock(); + workspaceConfig = mock(); when(workspaceService.getWorkspaceFolder(anything())).thenReturn(undefined); when(workspaceService.workspaceFolders).thenReturn(undefined); + when(workspaceService.getConfiguration('terminal')).thenReturn(instance(workspaceConfig)); + when(workspaceConfig.get('integrated.shellIntegration.enabled')).thenReturn(true); platform = mock(); when(platform.osType).thenReturn(getOSType()); interpreterService = mock(); context = mock(); shell = mock(); - collection = mock< - EnvironmentVariableCollection & { - getScopedEnvironmentVariableCollection(scope: EnvironmentVariableScope): EnvironmentVariableCollection; - } - >(); - when(context.environmentVariableCollection).thenReturn(instance(collection)); + globalCollection = mock(); + collection = mock(); + when(context.environmentVariableCollection).thenReturn(instance(globalCollection)); + when(globalCollection.getScoped(anything())).thenReturn(instance(collection)); experimentService = mock(); when(experimentService.inExperimentSync(TerminalEnvVarActivation.experiment)).thenReturn(true); applicationEnvironment = mock(); From a2b08f25d41a930f51b3a83a544673fcd53f99bd Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 23 Aug 2023 18:09:54 +0000 Subject: [PATCH 6/7] Nit --- .../activation/terminalEnvVarCollectionService.unit.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts index e343ee8424c3..4fca222d1c56 100644 --- a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts +++ b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts @@ -67,7 +67,7 @@ suite('Terminal Environment Variable Collection Service', () => { when(workspaceService.getWorkspaceFolder(anything())).thenReturn(undefined); when(workspaceService.workspaceFolders).thenReturn(undefined); when(workspaceService.getConfiguration('terminal')).thenReturn(instance(workspaceConfig)); - when(workspaceConfig.get('integrated.shellIntegration.enabled')).thenReturn(true); + when(workspaceConfig.get('integrated.shellIntegration.enabled')).thenReturn(true); platform = mock(); when(platform.osType).thenReturn(getOSType()); interpreterService = mock(); From e24cdd26cb73d0ce68e1c69fd33c3af9d3355cf0 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 23 Aug 2023 18:12:48 +0000 Subject: [PATCH 7/7] Add more test related to shell integration --- ...rminalEnvVarCollectionService.unit.test.ts | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts index 4fca222d1c56..7393f4ad07ad 100644 --- a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts +++ b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts @@ -293,6 +293,34 @@ suite('Terminal Environment Variable Collection Service', () => { expect(result).to.equal(true); }); + test('Correct track that prompt was set for PS1 if shell integration is disabled', async () => { + reset(workspaceConfig); + when(workspaceConfig.get('integrated.shellIntegration.enabled')).thenReturn(false); + when(platform.osType).thenReturn(OSType.Linux); + const envVars: NodeJS.ProcessEnv = { VIRTUAL_ENV: 'prefix/to/venv', PS1: '(.venv)', ...process.env }; + const ps1Shell = 'bash'; + const resource = Uri.file('a'); + const workspaceFolder: WorkspaceFolder = { + uri: Uri.file('workspacePath'), + name: 'workspace1', + index: 0, + }; + when(interpreterService.getActiveInterpreter(resource)).thenResolve(({ + type: PythonEnvType.Virtual, + } as unknown) as PythonEnvironment); + when(workspaceService.getWorkspaceFolder(resource)).thenReturn(workspaceFolder); + when( + environmentActivationService.getActivatedEnvironmentVariables(resource, undefined, undefined, ps1Shell), + ).thenResolve(envVars); + when(collection.replace(anything(), anything(), anything())).thenReturn(); + + await terminalEnvVarCollectionService._applyCollection(resource, ps1Shell); + + const result = terminalEnvVarCollectionService.isTerminalPromptSetCorrectly(resource); + + expect(result).to.equal(false); + }); + test('Correct track that prompt was not set for non-Windows zsh where PS1 is set', async () => { when(platform.osType).thenReturn(OSType.Linux); const envVars: NodeJS.ProcessEnv = { VIRTUAL_ENV: 'prefix/to/venv', PS1: '(.venv)', ...process.env };