From 30e26c2dc47981f03dbcfdfb66440ebe9b211de8 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 23 Aug 2023 11:25:17 -0700 Subject: [PATCH] Update proposed API for env collection (#21819) For https://github.com/microsoft/vscode-python/issues/20822 Blocked on https://github.com/microsoft/vscode/issues/171173#issuecomment-1679208632 --- package-lock.json | 2 +- package.json | 2 +- .../terminalEnvVarCollectionService.ts | 93 ++++++++++++------- ...rminalEnvVarCollectionService.unit.test.ts | 50 ++++++++-- ...scode.proposed.envCollectionWorkspace.d.ts | 50 +++++----- 5 files changed, 127 insertions(+), 70 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": [ diff --git a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts index 5dc716f82afa..33d51e7e2ed0 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,56 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ ) {} public async activate(resource: Resource): Promise { - if (!inTerminalEnvVarExperiment(this.experimentService)) { - this.context.environmentVariableCollection.clear(); - await this.handleMicroVenv(resource); + 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) { @@ -221,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); } @@ -232,7 +250,7 @@ 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', @@ -241,13 +259,18 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ ); 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 envVarCollection.getScoped(scope); + } + private getWorkspaceFolder(resource: Resource): WorkspaceFolder | undefined { let workspaceFolder = this.workspaceService.getWorkspaceFolder(resource); if ( diff --git a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts index b63750e5bf73..7393f4ad07ad 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.getEnvironmentVariableCollection(anything())).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(); @@ -291,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 }; 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; + }; }