From a0a5a57efaaa7e57d433680c3c69592a2057b0fe Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 12 Jan 2023 22:12:51 +0530 Subject: [PATCH 01/27] Initial commit --- src/client/interpreter/activation/service.ts | 35 ++++++++++++++++++-- src/client/interpreter/contracts.ts | 2 +- src/client/interpreter/interpreterService.ts | 6 ++-- src/client/jupyter/jupyterIntegration.ts | 2 +- src/test/linters/lint.provider.test.ts | 5 +-- 5 files changed, 41 insertions(+), 9 deletions(-) diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts index e4074b070853..635b68e31f87 100644 --- a/src/client/interpreter/activation/service.ts +++ b/src/client/interpreter/activation/service.ts @@ -11,7 +11,7 @@ import { IPlatformService } from '../../common/platform/types'; import * as internalScripts from '../../common/process/internal/scripts'; import { ExecutionResult, IProcessServiceFactory } from '../../common/process/types'; import { ITerminalHelper, TerminalShellType } from '../../common/terminal/types'; -import { ICurrentProcess, IDisposable, Resource } from '../../common/types'; +import { ICurrentProcess, IDisposable, IExtensionContext, Resource } from '../../common/types'; import { sleep } from '../../common/utils/async'; import { InMemoryCache } from '../../common/utils/cacheUtils'; import { OSType } from '../../common/utils/platform'; @@ -102,6 +102,7 @@ export class EnvironmentActivationServiceCache { @injectable() export class EnvironmentActivationService implements IEnvironmentActivationService, IDisposable { private readonly disposables: IDisposable[] = []; + private previousEnvVars = process.env; private readonly activatedEnvVariablesCache = new EnvironmentActivationServiceCache(); constructor( @inject(ITerminalHelper) private readonly helper: ITerminalHelper, @@ -111,6 +112,7 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi @inject(IWorkspaceService) private workspace: IWorkspaceService, @inject(IInterpreterService) private interpreterService: IInterpreterService, @inject(IEnvironmentVariablesProvider) private readonly envVarsService: IEnvironmentVariablesProvider, + @inject(IExtensionContext) private context: IExtensionContext, ) { this.envVarsService.onDidEnvironmentVariablesChange( () => this.activatedEnvVariablesCache.clear(), @@ -119,10 +121,15 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi ); this.interpreterService.onDidChangeInterpreter( - () => this.activatedEnvVariablesCache.clear(), + async (resource) => { + this.activatedEnvVariablesCache.clear(); + await this.initializeEnvironmentCollection(resource); + }, this, this.disposables, ); + + this.initializeEnvironmentCollection(undefined).ignoreErrors(); } public dispose(): void { @@ -322,4 +329,28 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi const js = output.substring(output.indexOf('{')).trim(); return parse(js); } + + private async initializeEnvironmentCollection(resource: Resource) { + const env = await this.getActivatedEnvironmentVariables(resource); + if (!env) { + this.context.environmentVariableCollection.clear(); + this.previousEnvVars = process.env; + return; + } + const previousEnv = this.previousEnvVars; + Object.keys(previousEnv).forEach((key) => { + // If the previous env var is not in the current env, then explicitly add it so it can cleared later. + if (!(key in env)) { + env[key] = ''; + } + }); + this.previousEnvVars = env; + for (const key in env) { + const value = env[key]; + const prevValue = previousEnv[key]; + if (value !== undefined && prevValue !== value) { + this.context.environmentVariableCollection.replace(key, value); + } + } + } } diff --git a/src/client/interpreter/contracts.ts b/src/client/interpreter/contracts.ts index ec504802bcfc..bfaebd235f19 100644 --- a/src/client/interpreter/contracts.ts +++ b/src/client/interpreter/contracts.ts @@ -76,7 +76,7 @@ export interface IInterpreterService { readonly refreshPromise: Promise | undefined; readonly onDidChangeInterpreters: Event; onDidChangeInterpreterConfiguration: Event; - onDidChangeInterpreter: Event; + onDidChangeInterpreter: Event; onDidChangeInterpreterInformation: Event; /** * Note this API does not trigger the refresh but only works with the current refresh if any. Information diff --git a/src/client/interpreter/interpreterService.ts b/src/client/interpreter/interpreterService.ts index 151b86508b8c..2fee9aaec22e 100644 --- a/src/client/interpreter/interpreterService.ts +++ b/src/client/interpreter/interpreterService.ts @@ -58,7 +58,7 @@ export class InterpreterService implements Disposable, IInterpreterService { return this.pyenvs.getRefreshPromise(); } - public get onDidChangeInterpreter(): Event { + public get onDidChangeInterpreter(): Event { return this.didChangeInterpreterEmitter.event; } @@ -80,7 +80,7 @@ export class InterpreterService implements Disposable, IInterpreterService { private readonly interpreterPathService: IInterpreterPathService; - private readonly didChangeInterpreterEmitter = new EventEmitter(); + private readonly didChangeInterpreterEmitter = new EventEmitter(); private readonly didChangeInterpreterInformation = new EventEmitter(); @@ -226,7 +226,7 @@ export class InterpreterService implements Disposable, IInterpreterService { this.didChangeInterpreterConfigurationEmitter.fire(resource); if (this._pythonPathSetting === '' || this._pythonPathSetting !== pySettings.pythonPath) { this._pythonPathSetting = pySettings.pythonPath; - this.didChangeInterpreterEmitter.fire(); + this.didChangeInterpreterEmitter.fire(resource); reportActiveInterpreterChanged({ path: pySettings.pythonPath, resource: this.serviceContainer.get(IWorkspaceService).getWorkspaceFolder(resource), diff --git a/src/client/jupyter/jupyterIntegration.ts b/src/client/jupyter/jupyterIntegration.ts index 556ff93f240a..aedc24b1e8bf 100644 --- a/src/client/jupyter/jupyterIntegration.ts +++ b/src/client/jupyter/jupyterIntegration.ts @@ -63,7 +63,7 @@ type PythonApiForJupyterExtension = { /** * IInterpreterService */ - onDidChangeInterpreter: Event; + onDidChangeInterpreter: Event; /** * IInterpreterService */ diff --git a/src/test/linters/lint.provider.test.ts b/src/test/linters/lint.provider.test.ts index 680dfecc0277..760c2282ba05 100644 --- a/src/test/linters/lint.provider.test.ts +++ b/src/test/linters/lint.provider.test.ts @@ -24,6 +24,7 @@ import { IPersistentStateFactory, IPythonSettings, Product, + Resource, WORKSPACE_MEMENTO, } from '../../client/common/types'; import { createDeferred } from '../../client/common/utils/async'; @@ -171,12 +172,12 @@ suite('Linting - Provider', () => { }); test('Lint on change interpreters', async () => { - const e = new vscode.EventEmitter(); + const e = new vscode.EventEmitter(); interpreterService.setup((x) => x.onDidChangeInterpreter).returns(() => e.event); const linterProvider = new LinterProvider(serviceContainer); await linterProvider.activate(); - e.fire(); + e.fire(undefined); engine.verify((x) => x.lintOpenPythonFiles(), TypeMoq.Times.once()); }); From 2302f8bca38ead938ccbe682fd52732d35e0232c Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 9 Feb 2023 19:43:57 +0530 Subject: [PATCH 02/27] Add progress notification for activating terminals --- src/client/common/utils/localize.ts | 1 + src/client/interpreter/activation/service.ts | 36 ++++++++++++++++++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 60cbfd295f88..3cce1d3610ac 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -193,6 +193,7 @@ export namespace Interpreters { export const condaInheritEnvMessage = l10n.t( 'We noticed you\'re using a conda environment. If you are experiencing issues with this environment in the integrated terminal, we recommend that you let the Python extension change "terminal.integrated.inheritEnv" to false in your user settings. [Learn more](https://aka.ms/AA66i8f).', ); + export const activatingTerminals = l10n.t('Reactivating terminals...'); export const activatedCondaEnvLaunch = l10n.t( 'We noticed VS Code was launched from an activated conda environment, would you like to select it?', ); diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts index 635b68e31f87..d378fb5b0587 100644 --- a/src/client/interpreter/activation/service.ts +++ b/src/client/interpreter/activation/service.ts @@ -5,14 +5,14 @@ import '../../common/extensions'; import { inject, injectable } from 'inversify'; -import { IWorkspaceService } from '../../common/application/types'; +import { IApplicationShell, IWorkspaceService } from '../../common/application/types'; import { PYTHON_WARNINGS } from '../../common/constants'; import { IPlatformService } from '../../common/platform/types'; import * as internalScripts from '../../common/process/internal/scripts'; import { ExecutionResult, IProcessServiceFactory } from '../../common/process/types'; import { ITerminalHelper, TerminalShellType } from '../../common/terminal/types'; import { ICurrentProcess, IDisposable, IExtensionContext, Resource } from '../../common/types'; -import { sleep } from '../../common/utils/async'; +import { createDeferred, Deferred, sleep } from '../../common/utils/async'; import { InMemoryCache } from '../../common/utils/cacheUtils'; import { OSType } from '../../common/utils/platform'; import { IEnvironmentVariablesProvider } from '../../common/variables/types'; @@ -32,6 +32,8 @@ import { } from '../../logging'; import { Conda } from '../../pythonEnvironments/common/environmentManagers/conda'; import { StopWatch } from '../../common/utils/stopWatch'; +import { Interpreters } from '../../common/utils/localize'; +import { ProgressLocation, ProgressOptions } from 'vscode'; const ENVIRONMENT_PREFIX = 'e8b39361-0157-4923-80e1-22d70d46dee6'; const CACHE_DURATION = 10 * 60 * 1000; @@ -102,6 +104,7 @@ export class EnvironmentActivationServiceCache { @injectable() export class EnvironmentActivationService implements IEnvironmentActivationService, IDisposable { private readonly disposables: IDisposable[] = []; + private deferred: Deferred | undefined; private previousEnvVars = process.env; private readonly activatedEnvVariablesCache = new EnvironmentActivationServiceCache(); constructor( @@ -113,6 +116,7 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi @inject(IInterpreterService) private interpreterService: IInterpreterService, @inject(IEnvironmentVariablesProvider) private readonly envVarsService: IEnvironmentVariablesProvider, @inject(IExtensionContext) private context: IExtensionContext, + @inject(IApplicationShell) private shell: IApplicationShell, ) { this.envVarsService.onDidEnvironmentVariablesChange( () => this.activatedEnvVariablesCache.clear(), @@ -122,8 +126,10 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi this.interpreterService.onDidChangeInterpreter( async (resource) => { + this.showProgress(); this.activatedEnvVariablesCache.clear(); await this.initializeEnvironmentCollection(resource); + this.hideProgress(); }, this, this.disposables, @@ -353,4 +359,30 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi } } } + + @traceDecoratorVerbose('Display activating terminals') + private showProgress(): void { + if (!this.deferred) { + this.createProgress(); + } + } + + @traceDecoratorVerbose('Hide activating terminals') + private hideProgress(): void { + if (this.deferred) { + this.deferred.resolve(); + this.deferred = undefined; + } + } + + private createProgress() { + const progressOptions: ProgressOptions = { + location: ProgressLocation.Window, + title: Interpreters.activatingTerminals, + }; + this.shell.withProgress(progressOptions, () => { + this.deferred = createDeferred(); + return this.deferred.promise; + }); + } } From 24ff4419a95feaaf2bf9a1977b2fe66e78e37360 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 14 Feb 2023 19:35:41 +0530 Subject: [PATCH 03/27] Only do it when experiment is enabled --- src/client/common/experiments/groups.ts | 4 +++ src/client/interpreter/activation/service.ts | 27 ++++++++++++++++++-- src/client/interpreter/serviceRegistry.ts | 1 + 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/client/common/experiments/groups.ts b/src/client/common/experiments/groups.ts index 7d9c27bf33e9..cf27d2282a29 100644 --- a/src/client/common/experiments/groups.ts +++ b/src/client/common/experiments/groups.ts @@ -6,3 +6,7 @@ export enum ShowExtensionSurveyPrompt { export enum ShowToolsExtensionPrompt { experiment = 'pythonPromptNewToolsExt', } + +export enum TerminalAutoActivation { + experiment = 'pythonTerminalAutoActivation', +} diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts index d378fb5b0587..cba1400c4ea0 100644 --- a/src/client/interpreter/activation/service.ts +++ b/src/client/interpreter/activation/service.ts @@ -11,7 +11,7 @@ import { IPlatformService } from '../../common/platform/types'; import * as internalScripts from '../../common/process/internal/scripts'; import { ExecutionResult, IProcessServiceFactory } from '../../common/process/types'; import { ITerminalHelper, TerminalShellType } from '../../common/terminal/types'; -import { ICurrentProcess, IDisposable, IExtensionContext, Resource } from '../../common/types'; +import { ICurrentProcess, IDisposable, IExperimentService, IExtensionContext, Resource } from '../../common/types'; import { createDeferred, Deferred, sleep } from '../../common/utils/async'; import { InMemoryCache } from '../../common/utils/cacheUtils'; import { OSType } from '../../common/utils/platform'; @@ -34,6 +34,8 @@ import { Conda } from '../../pythonEnvironments/common/environmentManagers/conda import { StopWatch } from '../../common/utils/stopWatch'; import { Interpreters } from '../../common/utils/localize'; import { ProgressLocation, ProgressOptions } from 'vscode'; +import { TerminalAutoActivation } from '../../common/experiments/groups'; +import { IExtensionSingleActivationService } from '../../activation/types'; const ENVIRONMENT_PREFIX = 'e8b39361-0157-4923-80e1-22d70d46dee6'; const CACHE_DURATION = 10 * 60 * 1000; @@ -102,7 +104,12 @@ export class EnvironmentActivationServiceCache { } @injectable() -export class EnvironmentActivationService implements IEnvironmentActivationService, IDisposable { +export class EnvironmentActivationService + implements IEnvironmentActivationService, IExtensionSingleActivationService, IDisposable { + public readonly supportedWorkspaceTypes: { untrustedWorkspace: boolean; virtualWorkspace: boolean } = { + untrustedWorkspace: false, + virtualWorkspace: false, + }; private readonly disposables: IDisposable[] = []; private deferred: Deferred | undefined; private previousEnvVars = process.env; @@ -117,13 +124,19 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi @inject(IEnvironmentVariablesProvider) private readonly envVarsService: IEnvironmentVariablesProvider, @inject(IExtensionContext) private context: IExtensionContext, @inject(IApplicationShell) private shell: IApplicationShell, + @inject(IExperimentService) private experimentService: IExperimentService, ) { this.envVarsService.onDidEnvironmentVariablesChange( () => this.activatedEnvVariablesCache.clear(), this, this.disposables, ); + } + public async activate(): Promise { + if (!this.isEnvCollectionEnabled()) { + return; + } this.interpreterService.onDidChangeInterpreter( async (resource) => { this.showProgress(); @@ -138,6 +151,16 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi this.initializeEnvironmentCollection(undefined).ignoreErrors(); } + private isEnvCollectionEnabled() { + if (this.workspace.workspaceFile) { + return false; + } + if (!this.experimentService.inExperimentSync(TerminalAutoActivation.experiment)) { + // TODO: return false; + } + return true; + } + public dispose(): void { this.disposables.forEach((d) => d.dispose()); } diff --git a/src/client/interpreter/serviceRegistry.ts b/src/client/interpreter/serviceRegistry.ts index 422776bd5e43..782d49e18302 100644 --- a/src/client/interpreter/serviceRegistry.ts +++ b/src/client/interpreter/serviceRegistry.ts @@ -108,4 +108,5 @@ export function registerTypes(serviceManager: IServiceManager): void { IEnvironmentActivationService, EnvironmentActivationService, ); + serviceManager.addBinding(IEnvironmentActivationService, IExtensionSingleActivationService); } From bc7c4bf04b2b89a94d6e6b427a015bae0aca8ab7 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 14 Feb 2023 19:51:25 +0530 Subject: [PATCH 04/27] Restore old code --- .eslintignore | 1 - src/client/interpreter/activation/service.ts | 92 ++++++++++++++------ 2 files changed, 65 insertions(+), 28 deletions(-) diff --git a/.eslintignore b/.eslintignore index 083b9d650d0c..bce20d67c2c9 100644 --- a/.eslintignore +++ b/.eslintignore @@ -146,7 +146,6 @@ src/client/interpreter/configuration/services/workspaceUpdaterService.ts src/client/interpreter/configuration/services/workspaceFolderUpdaterService.ts src/client/interpreter/helpers.ts src/client/interpreter/virtualEnvs/condaInheritEnvPrompt.ts -src/client/interpreter/activation/service.ts src/client/interpreter/display/index.ts src/client/api.ts diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts index cba1400c4ea0..d9f903079923 100644 --- a/src/client/interpreter/activation/service.ts +++ b/src/client/interpreter/activation/service.ts @@ -1,10 +1,14 @@ +/* eslint-disable max-classes-per-file */ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. + 'use strict'; + import '../../common/extensions'; import { inject, injectable } from 'inversify'; +import { ProgressLocation, ProgressOptions } from 'vscode'; import { IApplicationShell, IWorkspaceService } from '../../common/application/types'; import { PYTHON_WARNINGS } from '../../common/constants'; import { IPlatformService } from '../../common/platform/types'; @@ -33,7 +37,6 @@ import { import { Conda } from '../../pythonEnvironments/common/environmentManagers/conda'; import { StopWatch } from '../../common/utils/stopWatch'; import { Interpreters } from '../../common/utils/localize'; -import { ProgressLocation, ProgressOptions } from 'vscode'; import { TerminalAutoActivation } from '../../common/experiments/groups'; import { IExtensionSingleActivationService } from '../../activation/types'; @@ -55,6 +58,8 @@ const condaRetryMessages = [ 'The directory is not empty', ]; +type EnvironmentVariables = { [key: string]: string | undefined }; + /** * This class exists so that the environment variable fetching can be cached in between tests. Normally * this cache resides in memory for the duration of the EnvironmentActivationService's lifetime, but in the case @@ -63,15 +68,19 @@ const condaRetryMessages = [ */ export class EnvironmentActivationServiceCache { private static useStatic = false; + private static staticMap = new Map>(); + private normalMap = new Map>(); - public static forceUseStatic() { + public static forceUseStatic(): void { EnvironmentActivationServiceCache.useStatic = true; } - public static forceUseNormal() { + + public static forceUseNormal(): void { EnvironmentActivationServiceCache.useStatic = false; } + public get(key: string): InMemoryCache | undefined { if (EnvironmentActivationServiceCache.useStatic) { return EnvironmentActivationServiceCache.staticMap.get(key); @@ -79,7 +88,7 @@ export class EnvironmentActivationServiceCache { return this.normalMap.get(key); } - public set(key: string, value: InMemoryCache) { + public set(key: string, value: InMemoryCache): void { if (EnvironmentActivationServiceCache.useStatic) { EnvironmentActivationServiceCache.staticMap.set(key, value); } else { @@ -87,7 +96,7 @@ export class EnvironmentActivationServiceCache { } } - public delete(key: string) { + public delete(key: string): void { if (EnvironmentActivationServiceCache.useStatic) { EnvironmentActivationServiceCache.staticMap.delete(key); } else { @@ -95,7 +104,7 @@ export class EnvironmentActivationServiceCache { } } - public clear() { + public clear(): void { // Don't clear during a test as the environment isn't going to change if (!EnvironmentActivationServiceCache.useStatic) { this.normalMap.clear(); @@ -110,10 +119,15 @@ export class EnvironmentActivationService untrustedWorkspace: false, virtualWorkspace: false, }; + private readonly disposables: IDisposable[] = []; + private deferred: Deferred | undefined; - private previousEnvVars = process.env; + + private previousEnvVars = normCaseKeys(process.env); + private readonly activatedEnvVariablesCache = new EnvironmentActivationServiceCache(); + constructor( @inject(ITerminalHelper) private readonly helper: ITerminalHelper, @inject(IPlatformService) private readonly platform: IPlatformService, @@ -164,6 +178,7 @@ export class EnvironmentActivationService public dispose(): void { this.disposables.forEach((d) => d.dispose()); } + @traceDecoratorVerbose('getActivatedEnvironmentVariables', TraceOptions.Arguments) public async getActivatedEnvironmentVariables( resource: Resource, @@ -203,6 +218,7 @@ export class EnvironmentActivationService throw ex; }); } + public async getEnvironmentActivationShellCommands( resource: Resource, interpreter?: PythonEnvironment, @@ -213,6 +229,7 @@ export class EnvironmentActivationService } return this.helper.getEnvironmentActivationShellCommands(resource, shellInfo.shellType, interpreter); } + public async getActivatedEnvironmentVariablesImpl( resource: Resource, interpreter?: PythonEnvironment, @@ -220,11 +237,11 @@ export class EnvironmentActivationService ): Promise { const shellInfo = defaultShells[this.platform.osType]; if (!shellInfo) { - return; + return undefined; } try { let command: string | undefined; - let [args, parse] = internalScripts.printEnvVariables(); + const [args, parse] = internalScripts.printEnvVariables(); args.forEach((arg, i) => { args[i] = arg.toCommandArgumentForPythonExt(); }); @@ -247,10 +264,10 @@ export class EnvironmentActivationService ); traceVerbose(`Activation Commands received ${activationCommands} for shell ${shellInfo.shell}`); if (!activationCommands || !Array.isArray(activationCommands) || activationCommands.length === 0) { - return; + return undefined; } // Run the activate command collect the environment from it. - const activationCommand = this.fixActivationCommands(activationCommands).join(' && '); + const activationCommand = fixActivationCommands(activationCommands).join(' && '); // In order to make sure we know where the environment output is, // put in a dummy echo we can look for command = `${activationCommand} && echo '${ENVIRONMENT_PREFIX}' && python ${args.join(' ')}`; @@ -342,15 +359,13 @@ export class EnvironmentActivationService throw e; } } + return undefined; } - protected fixActivationCommands(commands: string[]): string[] { - // Replace 'source ' with '. ' as that works in shell exec - return commands.map((cmd) => cmd.replace(/^source\s+/, '. ')); - } @traceDecoratorError('Failed to parse Environment variables') @traceDecoratorVerbose('parseEnvironmentOutput', TraceOptions.None) - protected parseEnvironmentOutput(output: string, parse: (out: string) => NodeJS.ProcessEnv | undefined) { + // eslint-disable-next-line class-methods-use-this + private parseEnvironmentOutput(output: string, parse: (out: string) => NodeJS.ProcessEnv | undefined) { if (output.indexOf(ENVIRONMENT_PREFIX) === -1) { return parse(output); } @@ -363,24 +378,31 @@ export class EnvironmentActivationService const env = await this.getActivatedEnvironmentVariables(resource); if (!env) { this.context.environmentVariableCollection.clear(); - this.previousEnvVars = process.env; + this.previousEnvVars = normCaseKeys(process.env); return; } const previousEnv = this.previousEnvVars; - Object.keys(previousEnv).forEach((key) => { - // If the previous env var is not in the current env, then explicitly add it so it can cleared later. - if (!(key in env)) { - env[key] = ''; - } - }); this.previousEnvVars = env; - for (const key in env) { + Object.keys(env).forEach((key) => { const value = env[key]; const prevValue = previousEnv[key]; - if (value !== undefined && prevValue !== value) { - this.context.environmentVariableCollection.replace(key, value); + if (prevValue !== value) { + if (value !== undefined) { + traceVerbose(`Setting environment variable ${key} in collection to ${value}`); + this.context.environmentVariableCollection.replace(key, value); + } else { + traceVerbose(`Clearing environment variable ${key} from collection`); + this.context.environmentVariableCollection.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); + } + }); } @traceDecoratorVerbose('Display activating terminals') @@ -409,3 +431,19 @@ export class EnvironmentActivationService }); } } + +function normCaseKeys(env: EnvironmentVariables): EnvironmentVariables { + const result: EnvironmentVariables = {}; + Object.keys(env).forEach((key) => { + // `os.environ` script used to get env vars normalizes keys to upper case: + // https://github.com/python/cpython/issues/101754 + // So convert `process.env` keys to upper case to match. + result[key.toUpperCase()] = env[key]; + }); + return result; +} + +function fixActivationCommands(commands: string[]): string[] { + // Replace 'source ' with '. ' as that works in shell exec + return commands.map((cmd) => cmd.replace(/^source\s+/, '. ')); +} From 7748b92990751662810a139e1d3152371971dae9 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 23 Feb 2023 05:58:14 -0800 Subject: [PATCH 05/27] Do not send activation commands when experiment is enabled --- src/client/common/experiments/groups.ts | 4 ++-- src/client/common/experiments/helpers.ts | 18 ++++++++++++++++++ src/client/common/terminal/activator/index.ts | 7 +++++-- src/client/interpreter/activation/service.ts | 10 ++-------- src/client/providers/terminalProvider.ts | 10 ++++++++-- 5 files changed, 35 insertions(+), 14 deletions(-) create mode 100644 src/client/common/experiments/helpers.ts diff --git a/src/client/common/experiments/groups.ts b/src/client/common/experiments/groups.ts index cf27d2282a29..b575a116096c 100644 --- a/src/client/common/experiments/groups.ts +++ b/src/client/common/experiments/groups.ts @@ -7,6 +7,6 @@ export enum ShowToolsExtensionPrompt { experiment = 'pythonPromptNewToolsExt', } -export enum TerminalAutoActivation { - experiment = 'pythonTerminalAutoActivation', +export enum TerminalEnvVarActivation { + experiment = 'pythonTerminalEnvVarActivation', } diff --git a/src/client/common/experiments/helpers.ts b/src/client/common/experiments/helpers.ts new file mode 100644 index 000000000000..6223a07cd149 --- /dev/null +++ b/src/client/common/experiments/helpers.ts @@ -0,0 +1,18 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +import { workspace } from 'vscode'; +import { IExperimentService } from '../types'; +import { TerminalEnvVarActivation } from './groups'; + +export function inTerminalEnvVarExperiment(experimentService: IExperimentService): boolean { + if (workspace.workspaceFile) { + return false; + } + if (!experimentService.inExperimentSync(TerminalEnvVarActivation.experiment)) { + // TODO: return false; + } + return true; +} diff --git a/src/client/common/terminal/activator/index.ts b/src/client/common/terminal/activator/index.ts index 5bc76c0cb0f8..752d512d4c30 100644 --- a/src/client/common/terminal/activator/index.ts +++ b/src/client/common/terminal/activator/index.ts @@ -5,7 +5,8 @@ import { inject, injectable, multiInject } from 'inversify'; import { Terminal } from 'vscode'; -import { IConfigurationService } from '../../types'; +import { inTerminalEnvVarExperiment } from '../../experiments/helpers'; +import { IConfigurationService, IExperimentService } from '../../types'; import { ITerminalActivationHandler, ITerminalActivator, ITerminalHelper, TerminalActivationOptions } from '../types'; import { BaseTerminalActivator } from './base'; @@ -17,6 +18,7 @@ export class TerminalActivator implements ITerminalActivator { @inject(ITerminalHelper) readonly helper: ITerminalHelper, @multiInject(ITerminalActivationHandler) private readonly handlers: ITerminalActivationHandler[], @inject(IConfigurationService) private readonly configurationService: IConfigurationService, + @inject(IExperimentService) private readonly experimentService: IExperimentService, ) { this.initialize(); } @@ -37,7 +39,8 @@ export class TerminalActivator implements ITerminalActivator { options?: TerminalActivationOptions, ): Promise { const settings = this.configurationService.getSettings(options?.resource); - const activateEnvironment = settings.terminal.activateEnvironment; + const activateEnvironment = + settings.terminal.activateEnvironment && !inTerminalEnvVarExperiment(this.experimentService); if (!activateEnvironment || options?.hideFromUser) { return false; } diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts index d9f903079923..1fcea1e9eb93 100644 --- a/src/client/interpreter/activation/service.ts +++ b/src/client/interpreter/activation/service.ts @@ -37,8 +37,8 @@ import { import { Conda } from '../../pythonEnvironments/common/environmentManagers/conda'; import { StopWatch } from '../../common/utils/stopWatch'; import { Interpreters } from '../../common/utils/localize'; -import { TerminalAutoActivation } from '../../common/experiments/groups'; import { IExtensionSingleActivationService } from '../../activation/types'; +import { inTerminalEnvVarExperiment } from '../../common/experiments/helpers'; const ENVIRONMENT_PREFIX = 'e8b39361-0157-4923-80e1-22d70d46dee6'; const CACHE_DURATION = 10 * 60 * 1000; @@ -166,13 +166,7 @@ export class EnvironmentActivationService } private isEnvCollectionEnabled() { - if (this.workspace.workspaceFile) { - return false; - } - if (!this.experimentService.inExperimentSync(TerminalAutoActivation.experiment)) { - // TODO: return false; - } - return true; + return inTerminalEnvVarExperiment(this.experimentService); } public dispose(): void { diff --git a/src/client/providers/terminalProvider.ts b/src/client/providers/terminalProvider.ts index ee1de62acd8c..d047ea4b6d82 100644 --- a/src/client/providers/terminalProvider.ts +++ b/src/client/providers/terminalProvider.ts @@ -4,8 +4,9 @@ import { Disposable, Terminal } from 'vscode'; import { IActiveResourceService, ICommandManager } from '../common/application/types'; import { Commands } from '../common/constants'; +import { inTerminalEnvVarExperiment } from '../common/experiments/helpers'; import { ITerminalActivator, ITerminalServiceFactory } from '../common/terminal/types'; -import { IConfigurationService } from '../common/types'; +import { IConfigurationService, IExperimentService } from '../common/types'; import { swallowExceptions } from '../common/utils/decorators'; import { IServiceContainer } from '../ioc/types'; import { captureTelemetry, sendTelemetryEvent } from '../telemetry'; @@ -24,9 +25,14 @@ export class TerminalProvider implements Disposable { @swallowExceptions('Failed to initialize terminal provider') public async initialize(currentTerminal: Terminal | undefined): Promise { const configuration = this.serviceContainer.get(IConfigurationService); + const experimentService = this.serviceContainer.get(IExperimentService); const pythonSettings = configuration.getSettings(this.activeResourceService.getActiveResource()); - if (currentTerminal && pythonSettings.terminal.activateEnvInCurrentTerminal) { + if ( + currentTerminal && + pythonSettings.terminal.activateEnvInCurrentTerminal && + !inTerminalEnvVarExperiment(experimentService) + ) { const hideFromUser = 'hideFromUser' in currentTerminal.creationOptions && currentTerminal.creationOptions.hideFromUser; if (!hideFromUser) { From 9bd40f7f1112ca45a5c6d057e58ccdb5bbb2f412 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 23 Feb 2023 06:51:43 -0800 Subject: [PATCH 06/27] Use `conda run` for debugging when in experiment --- .../debugger/extension/adapter/factory.ts | 24 +++++++++++++++++-- .../common/environmentManagers/conda.ts | 6 ++++- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/src/client/debugger/extension/adapter/factory.ts b/src/client/debugger/extension/adapter/factory.ts index fc9232729eae..bf25fd48bbf2 100644 --- a/src/client/debugger/extension/adapter/factory.ts +++ b/src/client/debugger/extension/adapter/factory.ts @@ -16,16 +16,18 @@ import { import { EXTENSION_ROOT_DIR } from '../../../constants'; import { IInterpreterService } from '../../../interpreter/contracts'; import { traceLog, traceVerbose } from '../../../logging'; -import { PythonEnvironment } from '../../../pythonEnvironments/info'; +import { EnvironmentType, PythonEnvironment } from '../../../pythonEnvironments/info'; import { sendTelemetryEvent } from '../../../telemetry'; import { EventName } from '../../../telemetry/constants'; import { AttachRequestArguments, LaunchRequestArguments } from '../../types'; import { IDebugAdapterDescriptorFactory } from '../types'; import { showErrorMessage } from '../../../common/vscodeApis/windowApis'; import { Common, Interpreters } from '../../../common/utils/localize'; -import { IPersistentStateFactory } from '../../../common/types'; +import { IExperimentService, IPersistentStateFactory } from '../../../common/types'; import { Commands } from '../../../common/constants'; import { ICommandManager } from '../../../common/application/types'; +import { inTerminalEnvVarExperiment } from '../../../common/experiments/helpers'; +import { Conda } from '../../../pythonEnvironments/common/environmentManagers/conda'; // persistent state names, exported to make use of in testing export enum debugStateKeys { @@ -38,6 +40,7 @@ export class DebugAdapterDescriptorFactory implements IDebugAdapterDescriptorFac @inject(ICommandManager) private readonly commandManager: ICommandManager, @inject(IInterpreterService) private readonly interpreterService: IInterpreterService, @inject(IPersistentStateFactory) private persistentState: IPersistentStateFactory, + @inject(IExperimentService) private experimentService: IExperimentService, ) {} public async createDebugAdapterDescriptor( @@ -186,6 +189,23 @@ export class DebugAdapterDescriptorFactory implements IDebugAdapterDescriptorFac if ((interpreter.version?.major ?? 0) < 3 || (interpreter.version?.minor ?? 0) <= 6) { this.showDeprecatedPythonMessage(); } + if (interpreter.envType === EnvironmentType.Conda && inTerminalEnvVarExperiment(this.experimentService)) { + const conda = await Conda.getConda(); + if (conda) { + const args = await conda.getRunPythonArgs( + { + prefix: interpreter.envPath ?? '', + name: interpreter.envName, + }, + false, + false, + false, + ); + if (args) { + return args; + } + } + } return interpreter.path.length > 0 ? [interpreter.path] : []; } return []; diff --git a/src/client/pythonEnvironments/common/environmentManagers/conda.ts b/src/client/pythonEnvironments/common/environmentManagers/conda.ts index bb5720a15312..a4a216d478f5 100644 --- a/src/client/pythonEnvironments/common/environmentManagers/conda.ts +++ b/src/client/pythonEnvironments/common/environmentManagers/conda.ts @@ -508,6 +508,7 @@ export class Conda { env: CondaEnvInfo, forShellExecution?: boolean, isolatedFlag = false, + useOutputMarker = true, ): Promise { const condaVersion = await this.getCondaVersion(); if (condaVersion && lt(condaVersion, CONDA_RUN_VERSION)) { @@ -530,7 +531,10 @@ export class Conda { if (isolatedFlag) { python.push('-I'); } - return [...python, OUTPUT_MARKER_SCRIPT]; + if (useOutputMarker) { + python.push(OUTPUT_MARKER_SCRIPT); + } + return python; } /** From 25a0f74fad77306633c53ccebcc5d73c3d1bd1bb Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 23 Feb 2023 07:34:41 -0800 Subject: [PATCH 07/27] Revert "Use `conda run` for debugging when in experiment" This reverts commit afc44e26b189d04d4b6221632a0edce44e7d2210. --- .../debugger/extension/adapter/factory.ts | 24 ++----------------- .../common/environmentManagers/conda.ts | 6 +---- 2 files changed, 3 insertions(+), 27 deletions(-) diff --git a/src/client/debugger/extension/adapter/factory.ts b/src/client/debugger/extension/adapter/factory.ts index bf25fd48bbf2..fc9232729eae 100644 --- a/src/client/debugger/extension/adapter/factory.ts +++ b/src/client/debugger/extension/adapter/factory.ts @@ -16,18 +16,16 @@ import { import { EXTENSION_ROOT_DIR } from '../../../constants'; import { IInterpreterService } from '../../../interpreter/contracts'; import { traceLog, traceVerbose } from '../../../logging'; -import { EnvironmentType, PythonEnvironment } from '../../../pythonEnvironments/info'; +import { PythonEnvironment } from '../../../pythonEnvironments/info'; import { sendTelemetryEvent } from '../../../telemetry'; import { EventName } from '../../../telemetry/constants'; import { AttachRequestArguments, LaunchRequestArguments } from '../../types'; import { IDebugAdapterDescriptorFactory } from '../types'; import { showErrorMessage } from '../../../common/vscodeApis/windowApis'; import { Common, Interpreters } from '../../../common/utils/localize'; -import { IExperimentService, IPersistentStateFactory } from '../../../common/types'; +import { IPersistentStateFactory } from '../../../common/types'; import { Commands } from '../../../common/constants'; import { ICommandManager } from '../../../common/application/types'; -import { inTerminalEnvVarExperiment } from '../../../common/experiments/helpers'; -import { Conda } from '../../../pythonEnvironments/common/environmentManagers/conda'; // persistent state names, exported to make use of in testing export enum debugStateKeys { @@ -40,7 +38,6 @@ export class DebugAdapterDescriptorFactory implements IDebugAdapterDescriptorFac @inject(ICommandManager) private readonly commandManager: ICommandManager, @inject(IInterpreterService) private readonly interpreterService: IInterpreterService, @inject(IPersistentStateFactory) private persistentState: IPersistentStateFactory, - @inject(IExperimentService) private experimentService: IExperimentService, ) {} public async createDebugAdapterDescriptor( @@ -189,23 +186,6 @@ export class DebugAdapterDescriptorFactory implements IDebugAdapterDescriptorFac if ((interpreter.version?.major ?? 0) < 3 || (interpreter.version?.minor ?? 0) <= 6) { this.showDeprecatedPythonMessage(); } - if (interpreter.envType === EnvironmentType.Conda && inTerminalEnvVarExperiment(this.experimentService)) { - const conda = await Conda.getConda(); - if (conda) { - const args = await conda.getRunPythonArgs( - { - prefix: interpreter.envPath ?? '', - name: interpreter.envName, - }, - false, - false, - false, - ); - if (args) { - return args; - } - } - } return interpreter.path.length > 0 ? [interpreter.path] : []; } return []; diff --git a/src/client/pythonEnvironments/common/environmentManagers/conda.ts b/src/client/pythonEnvironments/common/environmentManagers/conda.ts index a4a216d478f5..bb5720a15312 100644 --- a/src/client/pythonEnvironments/common/environmentManagers/conda.ts +++ b/src/client/pythonEnvironments/common/environmentManagers/conda.ts @@ -508,7 +508,6 @@ export class Conda { env: CondaEnvInfo, forShellExecution?: boolean, isolatedFlag = false, - useOutputMarker = true, ): Promise { const condaVersion = await this.getCondaVersion(); if (condaVersion && lt(condaVersion, CONDA_RUN_VERSION)) { @@ -531,10 +530,7 @@ export class Conda { if (isolatedFlag) { python.push('-I'); } - if (useOutputMarker) { - python.push(OUTPUT_MARKER_SCRIPT); - } - return python; + return [...python, OUTPUT_MARKER_SCRIPT]; } /** From 83d648999cb58c737abb50d455504325144b1da0 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 23 Feb 2023 08:11:59 -0800 Subject: [PATCH 08/27] Pass in env variables when `python` is explicitly mentioned --- .../extension/configuration/resolvers/helper.ts | 13 +++++++++++-- .../extension/configuration/resolvers/launch.ts | 13 ++++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/client/debugger/extension/configuration/resolvers/helper.ts b/src/client/debugger/extension/configuration/resolvers/helper.ts index 781b25a26510..0ccaa9964054 100644 --- a/src/client/debugger/extension/configuration/resolvers/helper.ts +++ b/src/client/debugger/extension/configuration/resolvers/helper.ts @@ -13,7 +13,10 @@ import { getSearchPathEnvVarNames } from '../../../../common/utils/exec'; export const IDebugEnvironmentVariablesService = Symbol('IDebugEnvironmentVariablesService'); export interface IDebugEnvironmentVariablesService { - getEnvironmentVariables(args: LaunchRequestArguments): Promise; + getEnvironmentVariables( + args: LaunchRequestArguments, + baseVars?: EnvironmentVariables, + ): Promise; } @injectable() @@ -23,7 +26,10 @@ export class DebugEnvironmentVariablesHelper implements IDebugEnvironmentVariabl @inject(ICurrentProcess) private process: ICurrentProcess, ) {} - public async getEnvironmentVariables(args: LaunchRequestArguments): Promise { + public async getEnvironmentVariables( + args: LaunchRequestArguments, + baseVars?: EnvironmentVariables, + ): Promise { const pathVariableName = getSearchPathEnvVarNames()[0]; // Merge variables from both .env file and env json variables. @@ -37,6 +43,9 @@ export class DebugEnvironmentVariablesHelper implements IDebugEnvironmentVariabl // "overwrite: true" to ensure that debug-configuration env variable values // take precedence over env file. this.envParser.mergeVariables(debugLaunchEnvVars, env, { overwrite: true }); + if (baseVars) { + this.envParser.mergeVariables(baseVars, env); + } // Append the PYTHONPATH and PATH variables. this.envParser.appendPath(env, debugLaunchEnvVars[pathVariableName]); diff --git a/src/client/debugger/extension/configuration/resolvers/launch.ts b/src/client/debugger/extension/configuration/resolvers/launch.ts index d5cb419e031d..2cdb34e56f59 100644 --- a/src/client/debugger/extension/configuration/resolvers/launch.ts +++ b/src/client/debugger/extension/configuration/resolvers/launch.ts @@ -9,6 +9,8 @@ import { InvalidPythonPathInDebuggerServiceId } from '../../../../application/di import { IDiagnosticsService, IInvalidPythonPathInDebuggerService } from '../../../../application/diagnostics/types'; import { IConfigurationService } from '../../../../common/types'; import { getOSType, OSType } from '../../../../common/utils/platform'; +import { EnvironmentVariables } from '../../../../common/variables/types'; +import { IEnvironmentActivationService } from '../../../../interpreter/activation/types'; import { IInterpreterService } from '../../../../interpreter/contracts'; import { DebuggerTypeName } from '../../../constants'; import { DebugOptions, DebugPurpose, LaunchRequestArguments } from '../../../types'; @@ -24,6 +26,7 @@ export class LaunchConfigurationResolver extends BaseConfigurationResolver { + const isPythonSet = debugConfiguration.python !== undefined; if (debugConfiguration.python === undefined) { debugConfiguration.python = debugConfiguration.pythonPath; } @@ -96,10 +100,17 @@ export class LaunchConfigurationResolver extends BaseConfigurationResolver Date: Thu, 23 Feb 2023 15:26:43 -0800 Subject: [PATCH 09/27] Allow to run for different shells --- .../shellDetectors/baseShellDetector.ts | 34 ++++++++------- src/client/interpreter/activation/service.ts | 43 +++++++++++++++---- .../common/environmentManagers/conda.ts | 27 +++++++----- 3 files changed, 70 insertions(+), 34 deletions(-) diff --git a/src/client/common/terminal/shellDetectors/baseShellDetector.ts b/src/client/common/terminal/shellDetectors/baseShellDetector.ts index 4716af1a34e4..657c51e82af6 100644 --- a/src/client/common/terminal/shellDetectors/baseShellDetector.ts +++ b/src/client/common/terminal/shellDetectors/baseShellDetector.ts @@ -54,22 +54,26 @@ export abstract class BaseShellDetector implements IShellDetector { terminal?: Terminal, ): TerminalShellType | undefined; public identifyShellFromShellPath(shellPath: string): TerminalShellType { - // Remove .exe extension so shells can be more consistently detected - // on Windows (including Cygwin). - const basePath = shellPath.replace(/\.exe$/, ''); + return identifyShellFromShellPath(shellPath); + } +} - const shell = Array.from(detectableShells.keys()).reduce((matchedShell, shellToDetect) => { - if (matchedShell === TerminalShellType.other) { - const pat = detectableShells.get(shellToDetect); - if (pat && pat.test(basePath)) { - return shellToDetect; - } +export function identifyShellFromShellPath(shellPath: string): TerminalShellType { + // Remove .exe extension so shells can be more consistently detected + // on Windows (including Cygwin). + const basePath = shellPath.replace(/\.exe$/, ''); + + const shell = Array.from(detectableShells.keys()).reduce((matchedShell, shellToDetect) => { + if (matchedShell === TerminalShellType.other) { + const pat = detectableShells.get(shellToDetect); + if (pat && pat.test(basePath)) { + return shellToDetect; } - return matchedShell; - }, TerminalShellType.other); + } + return matchedShell; + }, TerminalShellType.other); - traceVerbose(`Shell path '${shellPath}', base path '${basePath}'`); - traceVerbose(`Shell path identified as shell '${shell}'`); - return shell; - } + traceVerbose(`Shell path '${shellPath}', base path '${basePath}'`); + traceVerbose(`Shell path identified as shell '${shell}'`); + return shell; } diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts index 1fcea1e9eb93..f8c7e7727cac 100644 --- a/src/client/interpreter/activation/service.ts +++ b/src/client/interpreter/activation/service.ts @@ -9,7 +9,7 @@ import '../../common/extensions'; import { inject, injectable } from 'inversify'; import { ProgressLocation, ProgressOptions } from 'vscode'; -import { IApplicationShell, IWorkspaceService } from '../../common/application/types'; +import { IApplicationEnvironment, IApplicationShell, IWorkspaceService } from '../../common/application/types'; import { PYTHON_WARNINGS } from '../../common/constants'; import { IPlatformService } from '../../common/platform/types'; import * as internalScripts from '../../common/process/internal/scripts'; @@ -39,6 +39,7 @@ import { StopWatch } from '../../common/utils/stopWatch'; import { Interpreters } from '../../common/utils/localize'; import { IExtensionSingleActivationService } from '../../activation/types'; import { inTerminalEnvVarExperiment } from '../../common/experiments/helpers'; +import { identifyShellFromShellPath } from '../../common/terminal/shellDetectors/baseShellDetector'; const ENVIRONMENT_PREFIX = 'e8b39361-0157-4923-80e1-22d70d46dee6'; const CACHE_DURATION = 10 * 60 * 1000; @@ -139,6 +140,7 @@ export class EnvironmentActivationService @inject(IExtensionContext) private context: IExtensionContext, @inject(IApplicationShell) private shell: IApplicationShell, @inject(IExperimentService) private experimentService: IExperimentService, + @inject(IApplicationEnvironment) private applicationEnvironment: IApplicationEnvironment, ) { this.envVarsService.onDidEnvironmentVariablesChange( () => this.activatedEnvVariablesCache.clear(), @@ -155,14 +157,14 @@ export class EnvironmentActivationService async (resource) => { this.showProgress(); this.activatedEnvVariablesCache.clear(); - await this.initializeEnvironmentCollection(resource); + await this.applyCollection(resource); this.hideProgress(); }, this, this.disposables, ); - this.initializeEnvironmentCollection(undefined).ignoreErrors(); + this.initializeCollection(undefined).ignoreErrors(); } private isEnvCollectionEnabled() { @@ -178,13 +180,14 @@ export class EnvironmentActivationService resource: Resource, interpreter?: PythonEnvironment, allowExceptions?: boolean, + shell?: string, ): Promise { const stopWatch = new StopWatch(); // Cache key = resource + interpreter. const workspaceKey = this.workspace.getWorkspaceFolderIdentifier(resource); interpreter = interpreter ?? (await this.interpreterService.getActiveInterpreter(resource)); const interpreterPath = this.platform.isWindows ? interpreter?.path.toLowerCase() : interpreter?.path; - const cacheKey = `${workspaceKey}_${interpreterPath}`; + const cacheKey = `${workspaceKey}_${interpreterPath}_${shell}`; if (this.activatedEnvVariablesCache.get(cacheKey)?.hasData) { return this.activatedEnvVariablesCache.get(cacheKey)!.data; @@ -192,7 +195,7 @@ export class EnvironmentActivationService // Cache only if successful, else keep trying & failing if necessary. const cache = new InMemoryCache(CACHE_DURATION); - return this.getActivatedEnvironmentVariablesImpl(resource, interpreter, allowExceptions) + return this.getActivatedEnvironmentVariablesImpl(resource, interpreter, allowExceptions, shell) .then((vars) => { cache.data = vars; this.activatedEnvVariablesCache.set(cacheKey, cache); @@ -228,11 +231,16 @@ export class EnvironmentActivationService resource: Resource, interpreter?: PythonEnvironment, allowExceptions?: boolean, + shell?: string, ): Promise { - const shellInfo = defaultShells[this.platform.osType]; + let shellInfo = defaultShells[this.platform.osType]; if (!shellInfo) { return undefined; } + if (shell) { + const customShellType = identifyShellFromShellPath(shell); + shellInfo = { shellType: customShellType, shell }; + } try { let command: string | undefined; const [args, parse] = internalScripts.printEnvVariables(); @@ -368,9 +376,15 @@ export class EnvironmentActivationService return parse(js); } - private async initializeEnvironmentCollection(resource: Resource) { - const env = await this.getActivatedEnvironmentVariables(resource); + private async applyCollection(resource: Resource, shell?: string) { + const env = await this.getActivatedEnvironmentVariables(resource, undefined, undefined, shell); if (!env) { + if (shell) { + // Default shells are known to work, hence we can safely ignore errors. However commands to fetch + // env vars may fail in custom shells due to unknown reasons, so do not clear collection even on + // failure. + return; + } this.context.environmentVariableCollection.clear(); this.previousEnvVars = normCaseKeys(process.env); return; @@ -399,6 +413,19 @@ export class EnvironmentActivationService }); } + private async initializeCollection(resource: Resource) { + await this.applyCollection(resource); + await this.applyCollectionForSelectedShell(resource); + } + + private async applyCollectionForSelectedShell(resource: Resource) { + const customShellType = identifyShellFromShellPath(this.applicationEnvironment.shell); + if (customShellType !== defaultShells[this.platform.osType]?.shellType) { + // If the user has a custom shell which different from default shell, we need to re-apply the environment collection. + await this.applyCollection(resource, this.applicationEnvironment.shell); + } + } + @traceDecoratorVerbose('Display activating terminals') private showProgress(): void { if (!this.deferred) { diff --git a/src/client/pythonEnvironments/common/environmentManagers/conda.ts b/src/client/pythonEnvironments/common/environmentManagers/conda.ts index bb5720a15312..33f5964aa1fc 100644 --- a/src/client/pythonEnvironments/common/environmentManagers/conda.ts +++ b/src/client/pythonEnvironments/common/environmentManagers/conda.ts @@ -22,6 +22,7 @@ import { isTestExecution } from '../../../common/constants'; import { traceError, traceVerbose } from '../../../logging'; import { OUTPUT_MARKER_SCRIPT } from '../../../common/process/internal/scripts'; import { splitLines } from '../../../common/stringUtils'; +import { SpawnOptions } from '../../../common/process/types'; export const AnacondaCompanyName = 'Anaconda, Inc.'; export const CONDAPATH_SETTING_KEY = 'condaPath'; @@ -248,7 +249,7 @@ export class Conda { * need a Conda instance should use getConda() to obtain it, and should never access * this property directly. */ - private static condaPromise: Promise | undefined; + private static condaPromise = new Map>(); private condaInfoCached: Promise | undefined; @@ -263,18 +264,18 @@ export class Conda { * @param command - Command used to spawn conda. This has the same meaning as the * first argument of spawn() - i.e. it can be a full path, or just a binary name. */ - constructor(readonly command: string, shellCommand?: string) { + constructor(readonly command: string, shellCommand?: string, private readonly shellPath?: string) { this.shellCommand = shellCommand ?? command; onDidChangePythonSetting(CONDAPATH_SETTING_KEY, () => { - Conda.condaPromise = undefined; + Conda.condaPromise = new Map>(); }); } - public static async getConda(): Promise { - if (Conda.condaPromise === undefined || isTestExecution()) { - Conda.condaPromise = Conda.locate(); + public static async getConda(shellPath?: string): Promise { + if (Conda.condaPromise.get(shellPath) === undefined || isTestExecution()) { + Conda.condaPromise.set(shellPath, Conda.locate(shellPath)); } - return Conda.condaPromise; + return Conda.condaPromise.get(shellPath); } /** @@ -283,7 +284,7 @@ export class Conda { * * @return A Conda instance corresponding to the binary, if successful; otherwise, undefined. */ - private static async locate(): Promise { + private static async locate(shellPath?: string): Promise { traceVerbose(`Searching for conda.`); const home = getUserHomeDir(); const customCondaPath = getPythonSetting(CONDAPATH_SETTING_KEY); @@ -392,9 +393,9 @@ export class Conda { const condaBatFile = await getCondaBatFile(condaPath); try { if (condaBatFile) { - const condaBat = new Conda(condaBatFile); + const condaBat = new Conda(condaBatFile, undefined, shellPath); await condaBat.getInfo(); - conda = new Conda(condaPath, condaBatFile); + conda = new Conda(condaPath, condaBatFile, shellPath); } } catch (ex) { traceVerbose('Failed to spawn conda bat file', condaBatFile, ex); @@ -432,7 +433,11 @@ export class Conda { @cache(30_000, true, 10_000) // eslint-disable-next-line class-methods-use-this private async getInfoImpl(command: string): Promise { - const result = await exec(command, ['info', '--json'], { timeout: CONDA_GENERAL_TIMEOUT }); + const options: SpawnOptions = { timeout: CONDA_GENERAL_TIMEOUT }; + if (this.shellPath) { + options.shell = this.shellPath; + } + const result = await exec(command, ['info', '--json'], options); traceVerbose(`${command} info --json: ${result.stdout}`); return JSON.parse(result.stdout); } From d3aa8db2d4576f37d3cac34330ae5788e6f7af06 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 23 Feb 2023 15:48:37 -0800 Subject: [PATCH 10/27] Update proposed APIs --- package.json | 1 + .../application/applicationEnvironment.ts | 14 +++++--------- src/client/common/application/types.ts | 4 ++++ src/client/interpreter/activation/service.ts | 7 ++++++- types/vscode.proposed.envShellEvent.d.ts | 16 ++++++++++++++++ types/vscode.proposed.testObserver.d.ts | 18 ------------------ 6 files changed, 32 insertions(+), 28 deletions(-) create mode 100644 types/vscode.proposed.envShellEvent.d.ts diff --git a/package.json b/package.json index 5c1c9334d887..a18b9228ddd1 100644 --- a/package.json +++ b/package.json @@ -19,6 +19,7 @@ "publisher": "ms-python", "enabledApiProposals": [ "quickPickSortByLabel", + "envShellEvent", "testObserver" ], "author": { diff --git a/src/client/common/application/applicationEnvironment.ts b/src/client/common/application/applicationEnvironment.ts index e3d78477996d..cf5e13d5ecaf 100644 --- a/src/client/common/application/applicationEnvironment.ts +++ b/src/client/common/application/applicationEnvironment.ts @@ -70,19 +70,15 @@ export class ApplicationEnvironment implements IApplicationEnvironment { public get extensionName(): string { return this.packageJson.displayName; } - /** - * At the time of writing this API, the vscode.env.shell isn't officially released in stable version of VS Code. - * Using this in stable version seems to throw errors in VSC with messages being displayed to the user about use of - * unstable API. - * Solution - log and suppress the errors. - * @readonly - * @type {(string)} - * @memberof ApplicationEnvironment - */ + public get shell(): string { return vscode.env.shell; } + public get onDidChangeShell(): vscode.Event { + return vscode.env.onDidChangeShell; + } + public get packageJson(): any { return require('../../../../package.json'); } diff --git a/src/client/common/application/types.ts b/src/client/common/application/types.ts index 69caf30a261b..1b054eda687c 100644 --- a/src/client/common/application/types.ts +++ b/src/client/common/application/types.ts @@ -1048,6 +1048,10 @@ export interface IApplicationEnvironment { * @memberof IApplicationShell */ readonly shell: string; + /** + * An {@link Event} which fires when the default shell changes. + */ + readonly onDidChangeShell: Event; /** * Gets the vscode channel (whether 'insiders' or 'stable'). */ diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts index f8c7e7727cac..d660c7ef3def 100644 --- a/src/client/interpreter/activation/service.ts +++ b/src/client/interpreter/activation/service.ts @@ -163,6 +163,11 @@ export class EnvironmentActivationService this, this.disposables, ); + this.applicationEnvironment.onDidChangeShell( + () => this.applyCollectionForSelectedShell(), + this, + this.disposables, + ); this.initializeCollection(undefined).ignoreErrors(); } @@ -248,7 +253,7 @@ export class EnvironmentActivationService args[i] = arg.toCommandArgumentForPythonExt(); }); if (interpreter?.envType === EnvironmentType.Conda) { - const conda = await Conda.getConda(); + const conda = await Conda.getConda(shell); const pythonArgv = await conda?.getRunPythonArgs({ name: interpreter.envName, prefix: interpreter.envPath ?? '', diff --git a/types/vscode.proposed.envShellEvent.d.ts b/types/vscode.proposed.envShellEvent.d.ts new file mode 100644 index 000000000000..8fed971ef711 --- /dev/null +++ b/types/vscode.proposed.envShellEvent.d.ts @@ -0,0 +1,16 @@ +/*--------------------------------------------------------------------------------------------- + * 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' { + + // See https://github.com/microsoft/vscode/issues/160694 + export namespace env { + + /** + * An {@link Event} which fires when the default shell changes. + */ + export const onDidChangeShell: Event; + } +} diff --git a/types/vscode.proposed.testObserver.d.ts b/types/vscode.proposed.testObserver.d.ts index 2bdb21d74732..d4465affbf2f 100644 --- a/types/vscode.proposed.testObserver.d.ts +++ b/types/vscode.proposed.testObserver.d.ts @@ -68,24 +68,6 @@ declare module 'vscode' { readonly removed: ReadonlyArray; } - /** - * A test item is an item shown in the "test explorer" view. It encompasses - * both a suite and a test, since they have almost or identical capabilities. - */ - export interface TestItem { - /** - * Marks the test as outdated. This can happen as a result of file changes, - * for example. In "auto run" mode, tests that are outdated will be - * automatically rerun after a short delay. Invoking this on a - * test with children will mark the entire subtree as outdated. - * - * Extensions should generally not override this method. - */ - // todo@api still unsure about this - invalidateResults(): void; - } - - /** * TestResults can be provided to the editor in {@link tests.publishTestResult}, * or read from it in {@link tests.testResults}. From 92f08c61f9e607786b1d032804748ac32243123c Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 24 Feb 2023 10:03:02 -0800 Subject: [PATCH 11/27] Update vscode types --- package-lock.json | 16 ++++++++-------- package.json | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/package-lock.json b/package-lock.json index bb4c7c20a69e..5ea88800712a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -69,7 +69,7 @@ "@types/stack-trace": "0.0.29", "@types/tmp": "^0.0.33", "@types/uuid": "^8.3.4", - "@types/vscode": "^1.74.0", + "@types/vscode": "^1.75.0", "@types/which": "^2.0.1", "@types/winreg": "^1.2.30", "@types/xml2js": "^0.4.2", @@ -129,7 +129,7 @@ "yargs": "^15.3.1" }, "engines": { - "vscode": "^1.75.0-20230123" + "vscode": "^1.75.0" } }, "node_modules/@azure/abort-controller": { @@ -1140,9 +1140,9 @@ "dev": true }, "node_modules/@types/vscode": { - "version": "1.74.0", - "resolved": "https://registry.npmjs.org/@types/vscode/-/vscode-1.74.0.tgz", - "integrity": "sha512-LyeCIU3jb9d38w0MXFwta9r0Jx23ugujkAxdwLTNCyspdZTKUc43t7ppPbCiPoQ/Ivd/pnDFZrb4hWd45wrsgA==", + "version": "1.75.1", + "resolved": "https://registry.npmjs.org/@types/vscode/-/vscode-1.75.1.tgz", + "integrity": "sha512-emg7wdsTFzdi+elvoyoA+Q8keEautdQHyY5LNmHVM4PTpY8JgOTVADrGVyXGepJ6dVW2OS5/xnLUWh+nZxvdiA==", "dev": true }, "node_modules/@types/which": { @@ -16329,9 +16329,9 @@ "dev": true }, "@types/vscode": { - "version": "1.74.0", - "resolved": "https://registry.npmjs.org/@types/vscode/-/vscode-1.74.0.tgz", - "integrity": "sha512-LyeCIU3jb9d38w0MXFwta9r0Jx23ugujkAxdwLTNCyspdZTKUc43t7ppPbCiPoQ/Ivd/pnDFZrb4hWd45wrsgA==", + "version": "1.75.1", + "resolved": "https://registry.npmjs.org/@types/vscode/-/vscode-1.75.1.tgz", + "integrity": "sha512-emg7wdsTFzdi+elvoyoA+Q8keEautdQHyY5LNmHVM4PTpY8JgOTVADrGVyXGepJ6dVW2OS5/xnLUWh+nZxvdiA==", "dev": true }, "@types/which": { diff --git a/package.json b/package.json index a18b9228ddd1..1fc3485bb495 100644 --- a/package.json +++ b/package.json @@ -41,7 +41,7 @@ "theme": "dark" }, "engines": { - "vscode": "^1.75.0-20230123" + "vscode": "^1.75.0" }, "keywords": [ "python", @@ -1869,7 +1869,7 @@ "@types/stack-trace": "0.0.29", "@types/tmp": "^0.0.33", "@types/uuid": "^8.3.4", - "@types/vscode": "^1.74.0", + "@types/vscode": "^1.75.0", "@types/which": "^2.0.1", "@types/winreg": "^1.2.30", "@types/xml2js": "^0.4.2", From 42042afb1e682a41c40eac03c86c15a540c6e807 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 24 Feb 2023 14:07:24 -0800 Subject: [PATCH 12/27] Add desc in `package.json` --- package.json | 12 ++++++++---- package.nls.json | 1 + 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/package.json b/package.json index 1fc3485bb495..5c5006cabb90 100644 --- a/package.json +++ b/package.json @@ -427,12 +427,14 @@ "enum": [ "All", "pythonSurveyNotification", - "pythonPromptNewToolsExt" + "pythonPromptNewToolsExt", + "pythonTerminalEnvVarActivation" ], "enumDescriptions": [ "%python.experiments.All.description%", "%python.experiments.pythonSurveyNotification.description%", - "%python.experiments.pythonPromptNewToolsExt.description%" + "%python.experiments.pythonPromptNewToolsExt.description%", + "%python.experiments.pythonTerminalEnvVarActivation.description%" ] }, "scope": "machine", @@ -446,12 +448,14 @@ "enum": [ "All", "pythonSurveyNotification", - "pythonPromptNewToolsExt" + "pythonPromptNewToolsExt", + "pythonTerminalEnvVarActivation" ], "enumDescriptions": [ "%python.experiments.All.description%", "%python.experiments.pythonSurveyNotification.description%", - "%python.experiments.pythonPromptNewToolsExt.description%" + "%python.experiments.pythonPromptNewToolsExt.description%", + "%python.experiments.pythonTerminalEnvVarActivation.description%" ] }, "scope": "machine", diff --git a/package.nls.json b/package.nls.json index 010fb2eec0ea..61b92dcfa320 100644 --- a/package.nls.json +++ b/package.nls.json @@ -37,6 +37,7 @@ "python.experiments.All.description": "Combined list of all experiments.", "python.experiments.pythonSurveyNotification.description": "Denotes the Python Survey Notification experiment.", "python.experiments.pythonPromptNewToolsExt.description": "Denotes the Python Prompt New Tools Extension experiment.", + "python.experiments.pythonTerminalEnvVarActivation.description": "Enables use of environment variables to activate terminals instead of sending activation commands.", "python.formatting.autopep8Args.description": "Arguments passed in. Each argument is a separate item in the array.", "python.formatting.autopep8Path.description": "Path to autopep8, you can use a custom version of autopep8 by modifying this setting to include the full path.", "python.formatting.blackArgs.description": "Arguments passed in. Each argument is a separate item in the array.", From 357c955865cbb96a91e7af049e203a7f52f6592f Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 24 Feb 2023 14:14:34 -0800 Subject: [PATCH 13/27] Try...catch --- .../common/application/applicationEnvironment.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/client/common/application/applicationEnvironment.ts b/src/client/common/application/applicationEnvironment.ts index cf5e13d5ecaf..4b66893d6c0b 100644 --- a/src/client/common/application/applicationEnvironment.ts +++ b/src/client/common/application/applicationEnvironment.ts @@ -7,6 +7,7 @@ import { inject, injectable } from 'inversify'; import * as path from 'path'; import { parse } from 'semver'; import * as vscode from 'vscode'; +import { traceError } from '../../logging'; import { Channel } from '../constants'; import { IPlatformService } from '../platform/types'; import { ICurrentProcess, IPathUtils } from '../types'; @@ -76,7 +77,14 @@ export class ApplicationEnvironment implements IApplicationEnvironment { } public get onDidChangeShell(): vscode.Event { - return vscode.env.onDidChangeShell; + try { + return vscode.env.onDidChangeShell; + } catch (ex) { + traceError('Failed to get onDidChangeShell API', ex); + // `onDidChangeShell` is a proposed API at the time of writing this, so wrap this in a try...catch + // block in case the API is removed or changed. + return new vscode.EventEmitter().event; + } } public get packageJson(): any { From 81888f292c5d337a10be7c7d1687162b860b3cc5 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 24 Feb 2023 14:19:32 -0800 Subject: [PATCH 14/27] Workaround VSCode bug --- src/client/interpreter/activation/service.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts index d660c7ef3def..ceda15ab90ee 100644 --- a/src/client/interpreter/activation/service.ts +++ b/src/client/interpreter/activation/service.ts @@ -164,7 +164,11 @@ export class EnvironmentActivationService this.disposables, ); this.applicationEnvironment.onDidChangeShell( - () => this.applyCollectionForSelectedShell(), + async (shell: string) => { + // 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.applyCollectionForSelectedShell(undefined, shell); + }, this, this.disposables, ); @@ -423,8 +427,8 @@ export class EnvironmentActivationService await this.applyCollectionForSelectedShell(resource); } - private async applyCollectionForSelectedShell(resource: Resource) { - const customShellType = identifyShellFromShellPath(this.applicationEnvironment.shell); + private async applyCollectionForSelectedShell(resource: Resource, shell = this.applicationEnvironment.shell) { + const customShellType = identifyShellFromShellPath(shell); if (customShellType !== defaultShells[this.platform.osType]?.shellType) { // If the user has a custom shell which different from default shell, we need to re-apply the environment collection. await this.applyCollection(resource, this.applicationEnvironment.shell); From c2a37a4f42b1553a8607aec28f65fcd00785ec71 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 24 Feb 2023 14:30:29 -0800 Subject: [PATCH 15/27] Add comment --- src/client/common/experiments/helpers.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/client/common/experiments/helpers.ts b/src/client/common/experiments/helpers.ts index 6223a07cd149..ee3504e15a2f 100644 --- a/src/client/common/experiments/helpers.ts +++ b/src/client/common/experiments/helpers.ts @@ -9,10 +9,12 @@ import { TerminalEnvVarActivation } from './groups'; export function inTerminalEnvVarExperiment(experimentService: IExperimentService): boolean { if (workspace.workspaceFile) { + // Don't run experiment in multi-root workspaces for now, requires work on VSCode: + // https://github.com/microsoft/vscode/issues/171173 return false; } if (!experimentService.inExperimentSync(TerminalEnvVarActivation.experiment)) { - // TODO: return false; + return false; } return true; } From 1368d9385778767dbba91ece38a5ac45821ad1f6 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 24 Feb 2023 14:33:54 -0800 Subject: [PATCH 16/27] Clear collection if experiment is not enabled --- package.json | 2 +- src/client/interpreter/activation/service.ts | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index 5c5006cabb90..0cde0318b0df 100644 --- a/package.json +++ b/package.json @@ -41,7 +41,7 @@ "theme": "dark" }, "engines": { - "vscode": "^1.75.0" + "vscode": "^1.75.0-20230123" }, "keywords": [ "python", diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts index ceda15ab90ee..2603e8a191e5 100644 --- a/src/client/interpreter/activation/service.ts +++ b/src/client/interpreter/activation/service.ts @@ -151,6 +151,7 @@ export class EnvironmentActivationService public async activate(): Promise { if (!this.isEnvCollectionEnabled()) { + this.context.environmentVariableCollection.clear(); return; } this.interpreterService.onDidChangeInterpreter( From 3a69050d988dc2e89dfbe81035d5692abf428f1f Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 24 Feb 2023 15:22:11 -0800 Subject: [PATCH 17/27] Log shell in which command is run --- src/client/common/process/logger.ts | 8 ++- src/client/common/utils/localize.ts | 4 -- .../common/environmentManagers/conda.ts | 18 +++--- src/test/common/process/logger.unit.test.ts | 57 ++++++------------- 4 files changed, 32 insertions(+), 55 deletions(-) diff --git a/src/client/common/process/logger.ts b/src/client/common/process/logger.ts index ebb1ad019a48..b4fe7ed96384 100644 --- a/src/client/common/process/logger.ts +++ b/src/client/common/process/logger.ts @@ -12,6 +12,7 @@ import { getOSType, getUserHomeDir, OSType } from '../utils/platform'; import { IProcessLogger, SpawnOptions } from './types'; import { escapeRegExp } from 'lodash'; import { replaceAll } from '../stringUtils'; +import { identifyShellFromShellPath } from '../terminal/shellDetectors/baseShellDetector'; @injectable() export class ProcessLogger implements IProcessLogger { @@ -27,8 +28,11 @@ export class ProcessLogger implements IProcessLogger { ? [fileOrCommand, ...args].map((e) => e.trimQuotes().toCommandArgumentForPythonExt()).join(' ') : fileOrCommand; const info = [`> ${this.getDisplayCommands(command)}`]; - if (options && options.cwd) { - info.push(`${Logging.currentWorkingDirectory} ${this.getDisplayCommands(options.cwd)}`); + if (options?.cwd) { + info.push(`cwd: ${this.getDisplayCommands(options.cwd)}`); + } + if (typeof options?.shell === 'string') { + info.push(`shell: ${identifyShellFromShellPath(options?.shell)}`); } info.forEach((line) => { diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 3cce1d3610ac..4a6affe51587 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -240,10 +240,6 @@ export namespace OutputChannelNames { export const pythonTest = l10n.t('Python Test Log'); } -export namespace Logging { - export const currentWorkingDirectory = l10n.t('cwd:'); -} - export namespace Linters { export const selectLinter = l10n.t('Select Linter'); } diff --git a/src/client/pythonEnvironments/common/environmentManagers/conda.ts b/src/client/pythonEnvironments/common/environmentManagers/conda.ts index 33f5964aa1fc..88178d02d58a 100644 --- a/src/client/pythonEnvironments/common/environmentManagers/conda.ts +++ b/src/client/pythonEnvironments/common/environmentManagers/conda.ts @@ -251,7 +251,7 @@ export class Conda { */ private static condaPromise = new Map>(); - private condaInfoCached: Promise | undefined; + private condaInfoCached = new Map | undefined>(); /** * Carries path to conda binary to be used for shell execution. @@ -384,7 +384,7 @@ export class Conda { // Probe the candidates, and pick the first one that exists and does what we need. for await (const condaPath of getCandidates()) { traceVerbose(`Probing conda binary: ${condaPath}`); - let conda = new Conda(condaPath); + let conda = new Conda(condaPath, undefined, shellPath); try { await conda.getInfo(); if (getOSType() === OSType.Windows && (isTestExecution() || condaPath !== customCondaPath)) { @@ -421,10 +421,12 @@ export class Conda { * Corresponds to "conda info --json". */ public async getInfo(useCache?: boolean): Promise { - if (!useCache || !this.condaInfoCached) { - this.condaInfoCached = this.getInfoImpl(this.command); + let condaInfoCached = this.condaInfoCached.get(this.shellPath); + if (!useCache || !condaInfoCached) { + condaInfoCached = this.getInfoImpl(this.command, this.shellPath); + this.condaInfoCached.set(this.shellPath, condaInfoCached); } - return this.condaInfoCached; + return condaInfoCached; } /** @@ -432,10 +434,10 @@ export class Conda { */ @cache(30_000, true, 10_000) // eslint-disable-next-line class-methods-use-this - private async getInfoImpl(command: string): Promise { + private async getInfoImpl(command: string, shellPath: string | undefined): Promise { const options: SpawnOptions = { timeout: CONDA_GENERAL_TIMEOUT }; - if (this.shellPath) { - options.shell = this.shellPath; + if (shellPath) { + options.shell = shellPath; } const result = await exec(command, ['info', '--json'], options); traceVerbose(`${command} info --json: ${result.stdout}`); diff --git a/src/test/common/process/logger.unit.test.ts b/src/test/common/process/logger.unit.test.ts index 3b0fc239e183..ebce120b7e6c 100644 --- a/src/test/common/process/logger.unit.test.ts +++ b/src/test/common/process/logger.unit.test.ts @@ -11,7 +11,6 @@ import untildify = require('untildify'); import { WorkspaceFolder } from 'vscode'; import { IWorkspaceService } from '../../../client/common/application/types'; import { ProcessLogger } from '../../../client/common/process/logger'; -import { Logging } from '../../../client/common/utils/localize'; import { getOSType, OSType } from '../../../client/common/utils/platform'; import * as logging from '../../../client/logging'; @@ -41,7 +40,7 @@ suite('ProcessLogger suite', () => { logger.logProcess('test', ['--foo', '--bar'], options); sinon.assert.calledWithExactly(traceLogStub, `> test --foo --bar`); - sinon.assert.calledWithExactly(traceLogStub, `${Logging.currentWorkingDirectory} ${options.cwd}`); + sinon.assert.calledWithExactly(traceLogStub, `cwd: ${options.cwd}`); }); test('Logger adds quotes around arguments if they contain spaces', async () => { @@ -49,10 +48,7 @@ suite('ProcessLogger suite', () => { logger.logProcess('test', ['--foo', '--bar', 'import test'], options); sinon.assert.calledWithExactly(traceLogStub, `> test --foo --bar "import test"`); - sinon.assert.calledWithExactly( - traceLogStub, - `${Logging.currentWorkingDirectory} ${path.join('debug', 'path')}`, - ); + sinon.assert.calledWithExactly(traceLogStub, `cwd: ${path.join('debug', 'path')}`); }); test('Logger preserves quotes around arguments if they contain spaces', async () => { @@ -60,10 +56,7 @@ suite('ProcessLogger suite', () => { logger.logProcess('test', ['--foo', '--bar', '"import test"'], options); sinon.assert.calledWithExactly(traceLogStub, `> test --foo --bar "import test"`); - sinon.assert.calledWithExactly( - traceLogStub, - `${Logging.currentWorkingDirectory} ${path.join('debug', 'path')}`, - ); + sinon.assert.calledWithExactly(traceLogStub, `cwd: ${path.join('debug', 'path')}`); }); test('Logger converts single quotes around arguments to double quotes if they contain spaces', async () => { @@ -71,10 +64,7 @@ suite('ProcessLogger suite', () => { logger.logProcess('test', ['--foo', '--bar', "'import test'"], options); sinon.assert.calledWithExactly(traceLogStub, `> test --foo --bar "import test"`); - sinon.assert.calledWithExactly( - traceLogStub, - `${Logging.currentWorkingDirectory} ${path.join('debug', 'path')}`, - ); + sinon.assert.calledWithExactly(traceLogStub, `cwd: ${path.join('debug', 'path')}`); }); test('Logger removes single quotes around arguments if they do not contain spaces', async () => { @@ -82,10 +72,7 @@ suite('ProcessLogger suite', () => { logger.logProcess('test', ['--foo', '--bar', "'importtest'"], options); sinon.assert.calledWithExactly(traceLogStub, `> test --foo --bar importtest`); - sinon.assert.calledWithExactly( - traceLogStub, - `${Logging.currentWorkingDirectory} ${path.join('debug', 'path')}`, - ); + sinon.assert.calledWithExactly(traceLogStub, `cwd: ${path.join('debug', 'path')}`); }); test('Logger replaces the path/to/home with ~ in the current working directory', async () => { @@ -93,10 +80,7 @@ suite('ProcessLogger suite', () => { logger.logProcess('test', ['--foo', '--bar'], options); sinon.assert.calledWithExactly(traceLogStub, `> test --foo --bar`); - sinon.assert.calledWithExactly( - traceLogStub, - `${Logging.currentWorkingDirectory} ${path.join('~', 'debug', 'path')}`, - ); + sinon.assert.calledWithExactly(traceLogStub, `cwd: ${path.join('~', 'debug', 'path')}`); }); test('Logger replaces the path/to/home with ~ in the command path where the home path IS at the beginning of the path', async () => { @@ -104,7 +88,7 @@ suite('ProcessLogger suite', () => { logger.logProcess(path.join(untildify('~'), 'test'), ['--foo', '--bar'], options); sinon.assert.calledWithExactly(traceLogStub, `> ${path.join('~', 'test')} --foo --bar`); - sinon.assert.calledWithExactly(traceLogStub, `${Logging.currentWorkingDirectory} ${options.cwd}`); + sinon.assert.calledWithExactly(traceLogStub, `cwd: ${options.cwd}`); }); test('Logger replaces the path/to/home with ~ in the command path where the home path IS at the beginning of the path but another arg contains other ref to home folder', async () => { @@ -112,7 +96,7 @@ suite('ProcessLogger suite', () => { logger.logProcess(path.join(untildify('~'), 'test'), ['--foo', path.join(untildify('~'), 'boo')], options); sinon.assert.calledWithExactly(traceLogStub, `> ${path.join('~', 'test')} --foo ${path.join('~', 'boo')}`); - sinon.assert.calledWithExactly(traceLogStub, `${Logging.currentWorkingDirectory} ${options.cwd}`); + sinon.assert.calledWithExactly(traceLogStub, `cwd: ${options.cwd}`); }); test('Logger replaces the path/to/home with ~ in the command path where the home path IS at the beginning of the path between doble quotes', async () => { @@ -120,7 +104,7 @@ suite('ProcessLogger suite', () => { logger.logProcess(`"${path.join(untildify('~'), 'test')}" "--foo" "--bar"`, undefined, options); sinon.assert.calledWithExactly(traceLogStub, `> "${path.join('~', 'test')}" "--foo" "--bar"`); - sinon.assert.calledWithExactly(traceLogStub, `${Logging.currentWorkingDirectory} ${options.cwd}`); + sinon.assert.calledWithExactly(traceLogStub, `cwd: ${options.cwd}`); }); test('Logger replaces the path/to/home with ~ in the command path where the home path IS NOT at the beginning of the path', async () => { @@ -128,7 +112,7 @@ suite('ProcessLogger suite', () => { logger.logProcess(path.join('net', untildify('~'), 'test'), ['--foo', '--bar'], options); sinon.assert.calledWithExactly(traceLogStub, `> ${path.join('net', '~', 'test')} --foo --bar`); - sinon.assert.calledWithExactly(traceLogStub, `${Logging.currentWorkingDirectory} ${options.cwd}`); + sinon.assert.calledWithExactly(traceLogStub, `cwd: ${options.cwd}`); }); test('Logger replaces the path/to/home with ~ in the command path where the home path IS NOT at the beginning of the path but another arg contains other ref to home folder', async () => { @@ -143,7 +127,7 @@ suite('ProcessLogger suite', () => { traceLogStub, `> ${path.join('net', '~', 'test')} --foo ${path.join('~', 'boo')}`, ); - sinon.assert.calledWithExactly(traceLogStub, `${Logging.currentWorkingDirectory} ${options.cwd}`); + sinon.assert.calledWithExactly(traceLogStub, `cwd: ${options.cwd}`); }); test('Logger replaces the path/to/home with ~ in the command path where the home path IS NOT at the beginning of the path between doble quotes', async () => { @@ -151,7 +135,7 @@ suite('ProcessLogger suite', () => { logger.logProcess(`"${path.join('net', untildify('~'), 'test')}" "--foo" "--bar"`, undefined, options); sinon.assert.calledWithExactly(traceLogStub, `> "${path.join('net', '~', 'test')}" "--foo" "--bar"`); - sinon.assert.calledWithExactly(traceLogStub, `${Logging.currentWorkingDirectory} ${options.cwd}`); + sinon.assert.calledWithExactly(traceLogStub, `cwd: ${options.cwd}`); }); test('Logger replaces the path/to/home with ~ if shell command is provided', async () => { @@ -159,7 +143,7 @@ suite('ProcessLogger suite', () => { logger.logProcess(`"${path.join(untildify('~'), 'test')}" "--foo" "--bar"`, undefined, options); sinon.assert.calledWithExactly(traceLogStub, `> "${path.join('~', 'test')}" "--foo" "--bar"`); - sinon.assert.calledWithExactly(traceLogStub, `${Logging.currentWorkingDirectory} ${options.cwd}`); + sinon.assert.calledWithExactly(traceLogStub, `cwd: ${options.cwd}`); }); test('Logger replaces the path to workspace with . if exactly one workspace folder is opened', async () => { @@ -167,10 +151,7 @@ suite('ProcessLogger suite', () => { logger.logProcess(`"${path.join('path', 'to', 'workspace', 'test')}" "--foo" "--bar"`, undefined, options); sinon.assert.calledWithExactly(traceLogStub, `> ".${path.sep}test" "--foo" "--bar"`); - sinon.assert.calledWithExactly( - traceLogStub, - `${Logging.currentWorkingDirectory} .${path.sep + path.join('debug', 'path')}`, - ); + sinon.assert.calledWithExactly(traceLogStub, `cwd: .${path.sep + path.join('debug', 'path')}`); }); test('On Windows, logger replaces both backwards and forward slash version of path to workspace with . if exactly one workspace folder is opened', async function () { @@ -182,20 +163,14 @@ suite('ProcessLogger suite', () => { logger.logProcess(`"${path.join('path', 'to', 'workspace', 'test')}" "--foo" "--bar"`, undefined, options); sinon.assert.calledWithExactly(traceLogStub, `> ".${path.sep}test" "--foo" "--bar"`); - sinon.assert.calledWithExactly( - traceLogStub, - `${Logging.currentWorkingDirectory} .${path.sep + path.join('debug', 'path')}`, - ); + sinon.assert.calledWithExactly(traceLogStub, `cwd: .${path.sep + path.join('debug', 'path')}`); traceLogStub.resetHistory(); options = { cwd: path.join('path\\to\\workspace', 'debug', 'path') }; logger.logProcess(`"${path.join('path', 'to', 'workspace', 'test')}" "--foo" "--bar"`, undefined, options); sinon.assert.calledWithExactly(traceLogStub, `> ".${path.sep}test" "--foo" "--bar"`); - sinon.assert.calledWithExactly( - traceLogStub, - `${Logging.currentWorkingDirectory} .${path.sep + path.join('debug', 'path')}`, - ); + sinon.assert.calledWithExactly(traceLogStub, `cwd: .${path.sep + path.join('debug', 'path')}`); }); test("Logger doesn't display the working directory line if there is no options parameter", async () => { From fe9fb80bd2b0e506a9f399c9a4b99939330be109 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 24 Feb 2023 15:22:26 -0800 Subject: [PATCH 18/27] Revert changes to proposed api --- src/client/common/process/logger.ts | 1 - types/vscode.proposed.testObserver.d.ts | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/client/common/process/logger.ts b/src/client/common/process/logger.ts index b4fe7ed96384..5c8f04cbec30 100644 --- a/src/client/common/process/logger.ts +++ b/src/client/common/process/logger.ts @@ -7,7 +7,6 @@ import { inject, injectable } from 'inversify'; import { traceLog } from '../../logging'; import { IWorkspaceService } from '../application/types'; import { isCI, isTestExecution } from '../constants'; -import { Logging } from '../utils/localize'; import { getOSType, getUserHomeDir, OSType } from '../utils/platform'; import { IProcessLogger, SpawnOptions } from './types'; import { escapeRegExp } from 'lodash'; diff --git a/types/vscode.proposed.testObserver.d.ts b/types/vscode.proposed.testObserver.d.ts index d4465affbf2f..9481cb1a32ac 100644 --- a/types/vscode.proposed.testObserver.d.ts +++ b/types/vscode.proposed.testObserver.d.ts @@ -68,6 +68,24 @@ declare module 'vscode' { readonly removed: ReadonlyArray; } + /** + * A test item is an item shown in the "test explorer" view. It encompasses + * both a suite and a test, since they have almost or identical capabilities. + */ + export interface TestItem { + /** + * Marks the test as outdated. This can happen as a result of file changes, + * for example. In "auto run" mode, tests that are outdated will be + * automatically rerun after a short delay. Invoking this on a + * test with children will mark the entire subtree as outdated. + * + * Extensions should generally not override this method. + */ + // todo@api still unsure about this + invalidateResults(): void; + } + + /** * TestResults can be provided to the editor in {@link tests.publishTestResult}, * or read from it in {@link tests.testResults}. From 660befb9617eab27e46946bf94aaa843a6e2825e Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 24 Feb 2023 15:29:58 -0800 Subject: [PATCH 19/27] Only fallback to using default shell if selected shell fails --- src/client/interpreter/activation/service.ts | 21 ++++++-------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts index 2603e8a191e5..5b6d24f33900 100644 --- a/src/client/interpreter/activation/service.ts +++ b/src/client/interpreter/activation/service.ts @@ -158,7 +158,7 @@ export class EnvironmentActivationService async (resource) => { this.showProgress(); this.activatedEnvVariablesCache.clear(); - await this.applyCollection(resource); + await this.applyCollectionForSelectedShell(resource); this.hideProgress(); }, this, @@ -174,7 +174,7 @@ export class EnvironmentActivationService this.disposables, ); - this.initializeCollection(undefined).ignoreErrors(); + this.applyCollectionForSelectedShell(undefined).ignoreErrors(); } private isEnvCollectionEnabled() { @@ -390,9 +390,9 @@ export class EnvironmentActivationService const env = await this.getActivatedEnvironmentVariables(resource, undefined, undefined, shell); if (!env) { if (shell) { - // Default shells are known to work, hence we can safely ignore errors. However commands to fetch - // env vars may fail in custom shells due to unknown reasons, so do not clear collection even on - // failure. + // Commands to fetch env vars may fail in custom shells due to unknown reasons, in that case + // fallback to default shells as they are known to work better. + await this.applyCollection(resource); return; } this.context.environmentVariableCollection.clear(); @@ -423,17 +423,8 @@ export class EnvironmentActivationService }); } - private async initializeCollection(resource: Resource) { - await this.applyCollection(resource); - await this.applyCollectionForSelectedShell(resource); - } - private async applyCollectionForSelectedShell(resource: Resource, shell = this.applicationEnvironment.shell) { - const customShellType = identifyShellFromShellPath(shell); - if (customShellType !== defaultShells[this.platform.osType]?.shellType) { - // If the user has a custom shell which different from default shell, we need to re-apply the environment collection. - await this.applyCollection(resource, this.applicationEnvironment.shell); - } + await this.applyCollection(resource, shell); } @traceDecoratorVerbose('Display activating terminals') From 02fba0811e5002ae32a3f775a1170d49d3eb0927 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 24 Feb 2023 15:44:01 -0800 Subject: [PATCH 20/27] Simplify --- src/client/interpreter/activation/service.ts | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts index 5b6d24f33900..1c9aa9854079 100644 --- a/src/client/interpreter/activation/service.ts +++ b/src/client/interpreter/activation/service.ts @@ -158,7 +158,7 @@ export class EnvironmentActivationService async (resource) => { this.showProgress(); this.activatedEnvVariablesCache.clear(); - await this.applyCollectionForSelectedShell(resource); + await this.applyCollection(resource); this.hideProgress(); }, this, @@ -168,13 +168,13 @@ export class EnvironmentActivationService async (shell: string) => { // 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.applyCollectionForSelectedShell(undefined, shell); + await this.applyCollection(undefined, shell); }, this, this.disposables, ); - this.applyCollectionForSelectedShell(undefined).ignoreErrors(); + this.applyCollection(undefined).ignoreErrors(); } private isEnvCollectionEnabled() { @@ -386,10 +386,11 @@ export class EnvironmentActivationService return parse(js); } - private async applyCollection(resource: Resource, shell?: string) { + private async applyCollection(resource: Resource, shell = this.applicationEnvironment.shell) { const env = await this.getActivatedEnvironmentVariables(resource, undefined, undefined, shell); if (!env) { - if (shell) { + const shellType = identifyShellFromShellPath(shell); + if (defaultShells[this.platform.osType]?.shellType !== shellType) { // Commands to fetch env vars may fail in custom shells due to unknown reasons, in that case // fallback to default shells as they are known to work better. await this.applyCollection(resource); @@ -423,10 +424,6 @@ export class EnvironmentActivationService }); } - private async applyCollectionForSelectedShell(resource: Resource, shell = this.applicationEnvironment.shell) { - await this.applyCollection(resource, shell); - } - @traceDecoratorVerbose('Display activating terminals') private showProgress(): void { if (!this.deferred) { From 8e3908cf093cc2a3715e3bd880ab8a736846f629 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 24 Feb 2023 17:48:51 -0800 Subject: [PATCH 21/27] Refactor --- src/client/interpreter/activation/service.ts | 130 +-------------- .../terminalEnvVarCollectionService.ts | 150 ++++++++++++++++++ src/client/interpreter/activation/types.ts | 1 + src/client/interpreter/serviceRegistry.ts | 6 +- .../activation/service.unit.test.ts | 6 +- 5 files changed, 163 insertions(+), 130 deletions(-) create mode 100644 src/client/interpreter/activation/terminalEnvVarCollectionService.ts diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts index 1c9aa9854079..bd5a37d75389 100644 --- a/src/client/interpreter/activation/service.ts +++ b/src/client/interpreter/activation/service.ts @@ -8,15 +8,14 @@ import '../../common/extensions'; import { inject, injectable } from 'inversify'; -import { ProgressLocation, ProgressOptions } from 'vscode'; -import { IApplicationEnvironment, IApplicationShell, IWorkspaceService } from '../../common/application/types'; +import { IWorkspaceService } from '../../common/application/types'; import { PYTHON_WARNINGS } from '../../common/constants'; import { IPlatformService } from '../../common/platform/types'; import * as internalScripts from '../../common/process/internal/scripts'; import { ExecutionResult, IProcessServiceFactory } from '../../common/process/types'; import { ITerminalHelper, TerminalShellType } from '../../common/terminal/types'; -import { ICurrentProcess, IDisposable, IExperimentService, IExtensionContext, Resource } from '../../common/types'; -import { createDeferred, Deferred, sleep } from '../../common/utils/async'; +import { ICurrentProcess, IDisposable, Resource } from '../../common/types'; +import { sleep } from '../../common/utils/async'; import { InMemoryCache } from '../../common/utils/cacheUtils'; import { OSType } from '../../common/utils/platform'; import { IEnvironmentVariablesProvider } from '../../common/variables/types'; @@ -36,9 +35,6 @@ import { } from '../../logging'; import { Conda } from '../../pythonEnvironments/common/environmentManagers/conda'; import { StopWatch } from '../../common/utils/stopWatch'; -import { Interpreters } from '../../common/utils/localize'; -import { IExtensionSingleActivationService } from '../../activation/types'; -import { inTerminalEnvVarExperiment } from '../../common/experiments/helpers'; import { identifyShellFromShellPath } from '../../common/terminal/shellDetectors/baseShellDetector'; const ENVIRONMENT_PREFIX = 'e8b39361-0157-4923-80e1-22d70d46dee6'; @@ -59,8 +55,6 @@ const condaRetryMessages = [ 'The directory is not empty', ]; -type EnvironmentVariables = { [key: string]: string | undefined }; - /** * This class exists so that the environment variable fetching can be cached in between tests. Normally * this cache resides in memory for the duration of the EnvironmentActivationService's lifetime, but in the case @@ -114,8 +108,7 @@ export class EnvironmentActivationServiceCache { } @injectable() -export class EnvironmentActivationService - implements IEnvironmentActivationService, IExtensionSingleActivationService, IDisposable { +export class EnvironmentActivationService implements IEnvironmentActivationService, IDisposable { public readonly supportedWorkspaceTypes: { untrustedWorkspace: boolean; virtualWorkspace: boolean } = { untrustedWorkspace: false, virtualWorkspace: false, @@ -123,10 +116,6 @@ export class EnvironmentActivationService private readonly disposables: IDisposable[] = []; - private deferred: Deferred | undefined; - - private previousEnvVars = normCaseKeys(process.env); - private readonly activatedEnvVariablesCache = new EnvironmentActivationServiceCache(); constructor( @@ -137,10 +126,6 @@ export class EnvironmentActivationService @inject(IWorkspaceService) private workspace: IWorkspaceService, @inject(IInterpreterService) private interpreterService: IInterpreterService, @inject(IEnvironmentVariablesProvider) private readonly envVarsService: IEnvironmentVariablesProvider, - @inject(IExtensionContext) private context: IExtensionContext, - @inject(IApplicationShell) private shell: IApplicationShell, - @inject(IExperimentService) private experimentService: IExperimentService, - @inject(IApplicationEnvironment) private applicationEnvironment: IApplicationEnvironment, ) { this.envVarsService.onDidEnvironmentVariablesChange( () => this.activatedEnvVariablesCache.clear(), @@ -149,38 +134,6 @@ export class EnvironmentActivationService ); } - public async activate(): Promise { - if (!this.isEnvCollectionEnabled()) { - this.context.environmentVariableCollection.clear(); - return; - } - this.interpreterService.onDidChangeInterpreter( - async (resource) => { - this.showProgress(); - this.activatedEnvVariablesCache.clear(); - await this.applyCollection(resource); - this.hideProgress(); - }, - this, - this.disposables, - ); - this.applicationEnvironment.onDidChangeShell( - async (shell: string) => { - // 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); - }, - this, - this.disposables, - ); - - this.applyCollection(undefined).ignoreErrors(); - } - - private isEnvCollectionEnabled() { - return inTerminalEnvVarExperiment(this.experimentService); - } - public dispose(): void { this.disposables.forEach((d) => d.dispose()); } @@ -385,81 +338,6 @@ export class EnvironmentActivationService const js = output.substring(output.indexOf('{')).trim(); return parse(js); } - - private async applyCollection(resource: Resource, shell = this.applicationEnvironment.shell) { - const env = await this.getActivatedEnvironmentVariables(resource, undefined, undefined, shell); - if (!env) { - const shellType = identifyShellFromShellPath(shell); - if (defaultShells[this.platform.osType]?.shellType !== shellType) { - // Commands to fetch env vars may fail in custom shells due to unknown reasons, in that case - // fallback to default shells as they are known to work better. - await this.applyCollection(resource); - return; - } - this.context.environmentVariableCollection.clear(); - this.previousEnvVars = normCaseKeys(process.env); - return; - } - const previousEnv = this.previousEnvVars; - this.previousEnvVars = env; - Object.keys(env).forEach((key) => { - const value = env[key]; - const prevValue = previousEnv[key]; - if (prevValue !== value) { - if (value !== undefined) { - traceVerbose(`Setting environment variable ${key} in collection to ${value}`); - this.context.environmentVariableCollection.replace(key, value); - } else { - traceVerbose(`Clearing environment variable ${key} from collection`); - this.context.environmentVariableCollection.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); - } - }); - } - - @traceDecoratorVerbose('Display activating terminals') - private showProgress(): void { - if (!this.deferred) { - this.createProgress(); - } - } - - @traceDecoratorVerbose('Hide activating terminals') - private hideProgress(): void { - if (this.deferred) { - this.deferred.resolve(); - this.deferred = undefined; - } - } - - private createProgress() { - const progressOptions: ProgressOptions = { - location: ProgressLocation.Window, - title: Interpreters.activatingTerminals, - }; - this.shell.withProgress(progressOptions, () => { - this.deferred = createDeferred(); - return this.deferred.promise; - }); - } -} - -function normCaseKeys(env: EnvironmentVariables): EnvironmentVariables { - const result: EnvironmentVariables = {}; - Object.keys(env).forEach((key) => { - // `os.environ` script used to get env vars normalizes keys to upper case: - // https://github.com/python/cpython/issues/101754 - // So convert `process.env` keys to upper case to match. - result[key.toUpperCase()] = env[key]; - }); - return result; } function fixActivationCommands(commands: string[]): string[] { diff --git a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts new file mode 100644 index 000000000000..e54e332f25ef --- /dev/null +++ b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts @@ -0,0 +1,150 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { inject, injectable } from 'inversify'; +import { ProgressOptions, ProgressLocation } from 'vscode'; +import { IExtensionSingleActivationService } from '../../activation/types'; +import { IApplicationShell, IApplicationEnvironment } from '../../common/application/types'; +import { inTerminalEnvVarExperiment } from '../../common/experiments/helpers'; +import { IPlatformService } from '../../common/platform/types'; +import { identifyShellFromShellPath } from '../../common/terminal/shellDetectors/baseShellDetector'; +import { IExtensionContext, IExperimentService, Resource, IDisposableRegistry } from '../../common/types'; +import { Deferred, createDeferred } from '../../common/utils/async'; +import { Interpreters } from '../../common/utils/localize'; +import { EnvironmentVariables } from '../../common/variables/types'; +import { traceDecoratorVerbose, traceVerbose } from '../../logging'; +import { IInterpreterService } from '../contracts'; +import { defaultShells } from './service'; +import { IEnvironmentActivationService } from './types'; + +@injectable() +export class TerminalEnvVarCollectionService implements IExtensionSingleActivationService { + public readonly supportedWorkspaceTypes: { untrustedWorkspace: boolean; virtualWorkspace: boolean } = { + untrustedWorkspace: false, + virtualWorkspace: false, + }; + + private deferred: Deferred | undefined; + + private previousEnvVars = normCaseKeys(process.env); + + constructor( + @inject(IPlatformService) private readonly platform: IPlatformService, + @inject(IInterpreterService) private interpreterService: IInterpreterService, + @inject(IExtensionContext) private context: IExtensionContext, + @inject(IApplicationShell) private shell: IApplicationShell, + @inject(IExperimentService) private experimentService: IExperimentService, + @inject(IApplicationEnvironment) private applicationEnvironment: IApplicationEnvironment, + @inject(IDisposableRegistry) private disposables: IDisposableRegistry, + @inject(IEnvironmentActivationService) private environmentActivationService: IEnvironmentActivationService, + ) {} + + public async activate(): Promise { + if (!inTerminalEnvVarExperiment(this.experimentService)) { + this.context.environmentVariableCollection.clear(); + return; + } + this.interpreterService.onDidChangeInterpreter( + async (resource) => { + this.showProgress(); + await this.applyCollection(resource); + this.hideProgress(); + }, + this, + this.disposables, + ); + this.applicationEnvironment.onDidChangeShell( + async (shell: string) => { + this.showProgress(); + // 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); + this.hideProgress(); + }, + this, + this.disposables, + ); + + this.applyCollection(undefined).ignoreErrors(); + } + + private async applyCollection(resource: Resource, shell = this.applicationEnvironment.shell) { + const env = await this.environmentActivationService.getActivatedEnvironmentVariables( + resource, + undefined, + undefined, + shell, + ); + if (!env) { + const shellType = identifyShellFromShellPath(shell); + if (defaultShells[this.platform.osType]?.shellType !== shellType) { + // Commands to fetch env vars may fail in custom shells due to unknown reasons, in that case + // fallback to default shells as they are known to work better. + await this.applyCollection(resource); + return; + } + this.context.environmentVariableCollection.clear(); + this.previousEnvVars = normCaseKeys(process.env); + return; + } + const previousEnv = this.previousEnvVars; + this.previousEnvVars = env; + Object.keys(env).forEach((key) => { + const value = env[key]; + const prevValue = previousEnv[key]; + if (prevValue !== value) { + if (value !== undefined) { + traceVerbose(`Setting environment variable ${key} in collection to ${value}`); + this.context.environmentVariableCollection.replace(key, value); + } else { + traceVerbose(`Clearing environment variable ${key} from collection`); + this.context.environmentVariableCollection.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); + } + }); + } + + @traceDecoratorVerbose('Display activating terminals') + private showProgress(): void { + if (!this.deferred) { + this.createProgress(); + } + } + + @traceDecoratorVerbose('Hide activating terminals') + private hideProgress(): void { + if (this.deferred) { + this.deferred.resolve(); + this.deferred = undefined; + } + } + + private createProgress() { + const progressOptions: ProgressOptions = { + location: ProgressLocation.Window, + title: Interpreters.activatingTerminals, + }; + this.shell.withProgress(progressOptions, () => { + this.deferred = createDeferred(); + return this.deferred.promise; + }); + } +} + +function normCaseKeys(env: EnvironmentVariables): EnvironmentVariables { + const result: EnvironmentVariables = {}; + Object.keys(env).forEach((key) => { + // `os.environ` script used to get env vars normalizes keys to upper case: + // https://github.com/python/cpython/issues/101754 + // So convert `process.env` keys to upper case to match. + result[key.toUpperCase()] = env[key]; + }); + return result; +} diff --git a/src/client/interpreter/activation/types.ts b/src/client/interpreter/activation/types.ts index 9508147a3552..d8e4ae16dbca 100644 --- a/src/client/interpreter/activation/types.ts +++ b/src/client/interpreter/activation/types.ts @@ -12,6 +12,7 @@ export interface IEnvironmentActivationService { resource: Resource, interpreter?: PythonEnvironment, allowExceptions?: boolean, + shell?: string, ): Promise; getEnvironmentActivationShellCommands( resource: Resource, diff --git a/src/client/interpreter/serviceRegistry.ts b/src/client/interpreter/serviceRegistry.ts index 782d49e18302..15dd6de7b722 100644 --- a/src/client/interpreter/serviceRegistry.ts +++ b/src/client/interpreter/serviceRegistry.ts @@ -6,6 +6,7 @@ import { IExtensionActivationService, IExtensionSingleActivationService } from '../activation/types'; import { IServiceManager } from '../ioc/types'; import { EnvironmentActivationService } from './activation/service'; +import { TerminalEnvVarCollectionService } from './activation/terminalEnvVarCollectionService'; import { IEnvironmentActivationService } from './activation/types'; import { InterpreterAutoSelectionService } from './autoSelection/index'; import { InterpreterAutoSelectionProxyService } from './autoSelection/proxy'; @@ -108,5 +109,8 @@ export function registerTypes(serviceManager: IServiceManager): void { IEnvironmentActivationService, EnvironmentActivationService, ); - serviceManager.addBinding(IEnvironmentActivationService, IExtensionSingleActivationService); + serviceManager.addSingleton( + IExtensionSingleActivationService, + TerminalEnvVarCollectionService, + ); } diff --git a/src/test/interpreters/activation/service.unit.test.ts b/src/test/interpreters/activation/service.unit.test.ts index d50b2b5d5995..28a289be23e0 100644 --- a/src/test/interpreters/activation/service.unit.test.ts +++ b/src/test/interpreters/activation/service.unit.test.ts @@ -18,7 +18,7 @@ import { ProcessServiceFactory } from '../../../client/common/process/processFac import { IProcessService, IProcessServiceFactory } from '../../../client/common/process/types'; import { TerminalHelper } from '../../../client/common/terminal/helper'; import { ITerminalHelper } from '../../../client/common/terminal/types'; -import { ICurrentProcess } from '../../../client/common/types'; +import { ICurrentProcess, Resource } from '../../../client/common/types'; import { getNamesAndValues } from '../../../client/common/utils/enum'; import { Architecture, OSType } from '../../../client/common/utils/platform'; import { EnvironmentVariablesProvider } from '../../../client/common/variables/environmentVariablesProvider'; @@ -48,7 +48,7 @@ suite('Interpreters Activation - Python Environment Variables', () => { let workspace: IWorkspaceService; let interpreterService: IInterpreterService; let onDidChangeEnvVariables: EventEmitter; - let onDidChangeInterpreter: EventEmitter; + let onDidChangeInterpreter: EventEmitter; const pythonInterpreter: PythonEnvironment = { path: '/foo/bar/python.exe', version: new SemVer('3.6.6-final'), @@ -68,7 +68,7 @@ suite('Interpreters Activation - Python Environment Variables', () => { interpreterService = mock(InterpreterService); workspace = mock(WorkspaceService); onDidChangeEnvVariables = new EventEmitter(); - onDidChangeInterpreter = new EventEmitter(); + onDidChangeInterpreter = new EventEmitter(); when(envVarsService.onDidEnvironmentVariablesChange).thenReturn(onDidChangeEnvVariables.event); when(interpreterService.onDidChangeInterpreter).thenReturn(onDidChangeInterpreter.event); when(interpreterService.getActiveInterpreter(anything())).thenResolve(interpreter); From 75b8e4b308c39fc533322d633fee6df1a246ceb3 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 24 Feb 2023 23:03:53 -0800 Subject: [PATCH 22/27] Fix compile errors --- src/client/interpreter/activation/service.ts | 5 --- .../terminals/activator/index.unit.test.ts | 17 ++++++++-- .../resolvers/launch.unit.test.ts | 11 +++++-- .../activation/service.unit.test.ts | 3 -- .../testing/common/debugLauncher.unit.test.ts | 10 +++++- types/vscode.proposed.testObserver.d.ts | 32 +++++++++---------- 6 files changed, 49 insertions(+), 29 deletions(-) diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts index bd5a37d75389..897dad9cb75d 100644 --- a/src/client/interpreter/activation/service.ts +++ b/src/client/interpreter/activation/service.ts @@ -109,11 +109,6 @@ export class EnvironmentActivationServiceCache { @injectable() export class EnvironmentActivationService implements IEnvironmentActivationService, IDisposable { - public readonly supportedWorkspaceTypes: { untrustedWorkspace: boolean; virtualWorkspace: boolean } = { - untrustedWorkspace: false, - virtualWorkspace: false, - }; - private readonly disposables: IDisposable[] = []; private readonly activatedEnvVariablesCache = new EnvironmentActivationServiceCache(); diff --git a/src/test/common/terminals/activator/index.unit.test.ts b/src/test/common/terminals/activator/index.unit.test.ts index 9dff5a800cad..6ed47fdbe2d3 100644 --- a/src/test/common/terminals/activator/index.unit.test.ts +++ b/src/test/common/terminals/activator/index.unit.test.ts @@ -12,7 +12,12 @@ import { ITerminalActivator, ITerminalHelper, } from '../../../../client/common/terminal/types'; -import { IConfigurationService, IPythonSettings, ITerminalSettings } from '../../../../client/common/types'; +import { + IConfigurationService, + IExperimentService, + IPythonSettings, + ITerminalSettings, +} from '../../../../client/common/types'; suite('Terminal Activator', () => { let activator: TerminalActivator; @@ -20,12 +25,14 @@ suite('Terminal Activator', () => { let handler1: TypeMoq.IMock; let handler2: TypeMoq.IMock; let terminalSettings: TypeMoq.IMock; + let experimentService: TypeMoq.IMock; setup(() => { baseActivator = TypeMoq.Mock.ofType(); terminalSettings = TypeMoq.Mock.ofType(); handler1 = TypeMoq.Mock.ofType(); handler2 = TypeMoq.Mock.ofType(); const configService = TypeMoq.Mock.ofType(); + experimentService = TypeMoq.Mock.ofType(); configService .setup((c) => c.getSettings(TypeMoq.It.isAny())) .returns(() => { @@ -33,11 +40,17 @@ suite('Terminal Activator', () => { terminal: terminalSettings.object, } as unknown) as IPythonSettings; }); + experimentService.setup((e) => e.inExperimentSync(TypeMoq.It.isAny())).returns(() => false); activator = new (class extends TerminalActivator { protected initialize() { this.baseActivator = baseActivator.object; } - })(TypeMoq.Mock.ofType().object, [handler1.object, handler2.object], configService.object); + })( + TypeMoq.Mock.ofType().object, + [handler1.object, handler2.object], + configService.object, + experimentService.object, + ); }); async function testActivationAndHandlers( activationSuccessful: boolean, diff --git a/src/test/debugger/extension/configuration/resolvers/launch.unit.test.ts b/src/test/debugger/extension/configuration/resolvers/launch.unit.test.ts index e7e256468f84..4c39b615741d 100644 --- a/src/test/debugger/extension/configuration/resolvers/launch.unit.test.ts +++ b/src/test/debugger/extension/configuration/resolvers/launch.unit.test.ts @@ -21,6 +21,7 @@ import { getInfoPerOS } from './common'; import * as platform from '../../../../../client/common/utils/platform'; import * as windowApis from '../../../../../client/common/vscodeApis/windowApis'; import * as workspaceApis from '../../../../../client/common/vscodeApis/workspaceApis'; +import { IEnvironmentActivationService } from '../../../../../client/interpreter/activation/types'; getInfoPerOS().forEach(([osName, osType, path]) => { if (osType === platform.OSType.Unknown) { @@ -31,12 +32,13 @@ getInfoPerOS().forEach(([osName, osType, path]) => { let debugProvider: DebugConfigurationProvider; let pythonExecutionService: TypeMoq.IMock; let helper: TypeMoq.IMock; + const envVars = { FOO: 'BAR' }; let diagnosticsService: TypeMoq.IMock; let configService: TypeMoq.IMock; let debugEnvHelper: TypeMoq.IMock; let interpreterService: TypeMoq.IMock; - + let environmentActivationService: TypeMoq.IMock; let getActiveTextEditorStub: sinon.SinonStub; let getOSTypeStub: sinon.SinonStub; let getWorkspaceFolderStub: sinon.SinonStub; @@ -59,6 +61,10 @@ getInfoPerOS().forEach(([osName, osType, path]) => { } function setupIoc(pythonPath: string, workspaceFolder?: WorkspaceFolder) { + environmentActivationService = TypeMoq.Mock.ofType(); + environmentActivationService + .setup((e) => e.getActivatedEnvironmentVariables(TypeMoq.It.isAny())) + .returns(() => Promise.resolve(envVars)); configService = TypeMoq.Mock.ofType(); diagnosticsService = TypeMoq.Mock.ofType(); debugEnvHelper = TypeMoq.Mock.ofType(); @@ -84,7 +90,7 @@ getInfoPerOS().forEach(([osName, osType, path]) => { } configService.setup((c) => c.getSettings(TypeMoq.It.isAny())).returns(() => settings.object); debugEnvHelper - .setup((x) => x.getEnvironmentVariables(TypeMoq.It.isAny())) + .setup((x) => x.getEnvironmentVariables(TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns(() => Promise.resolve({})); debugProvider = new LaunchConfigurationResolver( @@ -92,6 +98,7 @@ getInfoPerOS().forEach(([osName, osType, path]) => { configService.object, debugEnvHelper.object, interpreterService.object, + environmentActivationService.object, ); } diff --git a/src/test/interpreters/activation/service.unit.test.ts b/src/test/interpreters/activation/service.unit.test.ts index 28a289be23e0..002189d412db 100644 --- a/src/test/interpreters/activation/service.unit.test.ts +++ b/src/test/interpreters/activation/service.unit.test.ts @@ -322,9 +322,6 @@ suite('Interpreters Activation - Python Environment Variables', () => { verify(envVarsService.getEnvironmentVariables(resource)).twice(); verify(processService.shellExec(anything(), anything())).twice(); } - test('Cache Variables get cleared when changing interpreter', async () => { - await testClearingCache(onDidChangeInterpreter.fire.bind(onDidChangeInterpreter)); - }); test('Cache Variables get cleared when changing env variables file', async () => { await testClearingCache(onDidChangeEnvVariables.fire.bind(onDidChangeEnvVariables)); }); diff --git a/src/test/testing/common/debugLauncher.unit.test.ts b/src/test/testing/common/debugLauncher.unit.test.ts index dfe9e8ce5e99..b8d6e85ba331 100644 --- a/src/test/testing/common/debugLauncher.unit.test.ts +++ b/src/test/testing/common/debugLauncher.unit.test.ts @@ -28,6 +28,7 @@ import { LaunchOptions } from '../../../client/testing/common/types'; import { ITestingSettings } from '../../../client/testing/configuration/types'; import { TestProvider } from '../../../client/testing/types'; import { isOs, OSType } from '../../common'; +import { IEnvironmentActivationService } from '../../../client/interpreter/activation/types'; use(chaiAsPromised); @@ -39,12 +40,18 @@ suite('Unit Tests - Debug Launcher', () => { let settings: TypeMoq.IMock; let debugEnvHelper: TypeMoq.IMock; let interpreterService: TypeMoq.IMock; + let environmentActivationService: TypeMoq.IMock; let getWorkspaceFolderStub: sinon.SinonStub; let getWorkspaceFoldersStub: sinon.SinonStub; let pathExistsStub: sinon.SinonStub; let readFileStub: sinon.SinonStub; + const envVars = { FOO: 'BAR' }; setup(async () => { + environmentActivationService = TypeMoq.Mock.ofType(); + environmentActivationService + .setup((e) => e.getActivatedEnvironmentVariables(TypeMoq.It.isAny())) + .returns(() => Promise.resolve(envVars)); interpreterService = TypeMoq.Mock.ofType(); serviceContainer = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); const configService = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); @@ -94,6 +101,7 @@ suite('Unit Tests - Debug Launcher', () => { configService, debugEnvHelper.object, interpreterService.object, + environmentActivationService.object, ); } function setupDebugManager( @@ -110,7 +118,7 @@ suite('Unit Tests - Debug Launcher', () => { expected.args = debugArgs; debugEnvHelper - .setup((d) => d.getEnvironmentVariables(TypeMoq.It.isAny())) + .setup((x) => x.getEnvironmentVariables(TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns(() => Promise.resolve(expected.env)); debugService diff --git a/types/vscode.proposed.testObserver.d.ts b/types/vscode.proposed.testObserver.d.ts index 9481cb1a32ac..2bdb21d74732 100644 --- a/types/vscode.proposed.testObserver.d.ts +++ b/types/vscode.proposed.testObserver.d.ts @@ -68,22 +68,22 @@ declare module 'vscode' { readonly removed: ReadonlyArray; } - /** - * A test item is an item shown in the "test explorer" view. It encompasses - * both a suite and a test, since they have almost or identical capabilities. - */ - export interface TestItem { - /** - * Marks the test as outdated. This can happen as a result of file changes, - * for example. In "auto run" mode, tests that are outdated will be - * automatically rerun after a short delay. Invoking this on a - * test with children will mark the entire subtree as outdated. - * - * Extensions should generally not override this method. - */ - // todo@api still unsure about this - invalidateResults(): void; - } + /** + * A test item is an item shown in the "test explorer" view. It encompasses + * both a suite and a test, since they have almost or identical capabilities. + */ + export interface TestItem { + /** + * Marks the test as outdated. This can happen as a result of file changes, + * for example. In "auto run" mode, tests that are outdated will be + * automatically rerun after a short delay. Invoking this on a + * test with children will mark the entire subtree as outdated. + * + * Extensions should generally not override this method. + */ + // todo@api still unsure about this + invalidateResults(): void; + } /** From 2b6688c3f6ca4b11bd2ab50f53ce62deb0a5e3f2 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 24 Feb 2023 23:27:39 -0800 Subject: [PATCH 23/27] Add test for debugger scenario --- src/test/debugger/envVars.test.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/test/debugger/envVars.test.ts b/src/test/debugger/envVars.test.ts index 71c5b8e62650..6aa0dea4d8c6 100644 --- a/src/test/debugger/envVars.test.ts +++ b/src/test/debugger/envVars.test.ts @@ -76,6 +76,27 @@ suite('Resolving Environment Variables when Debugging', () => { test('Confirm basic environment variables exist when launched in intergrated terminal', () => testBasicProperties('integratedTerminal', 2)); + test('Confirm base environment variables are merged without overwriting when provided', async () => { + const env: Record = { DO_NOT_OVERWRITE: '1' }; + const args = ({ + program: '', + pythonPath: '', + args: [], + envFile: '', + console, + env, + } as any) as LaunchRequestArguments; + + const baseEnvVars = { CONDA_PREFIX: 'path/to/conda/env', DO_NOT_OVERWRITE: '0' }; + const envVars = await debugEnvParser.getEnvironmentVariables(args, baseEnvVars); + expect(envVars).not.be.undefined; + expect(Object.keys(envVars)).lengthOf(4, 'Incorrect number of variables'); + expect(envVars).to.have.property('PYTHONUNBUFFERED', '1', 'Property not found'); + expect(envVars).to.have.property('PYTHONIOENCODING', 'UTF-8', 'Property not found'); + expect(envVars).to.have.property('CONDA_PREFIX', 'path/to/conda/env', 'Property not found'); + expect(envVars).to.have.property('DO_NOT_OVERWRITE', '1', 'Property not found'); + }); + test('Confirm basic environment variables exist when launched in debug console', async () => { let expectedNumberOfVariables = Object.keys(mockProcess.env).length; if (mockProcess.env['PYTHONUNBUFFERED'] === undefined) { From 1b0f94d4af80788f720613f4b21b8a42ff98c23b Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 24 Feb 2023 23:31:36 -0800 Subject: [PATCH 24/27] Add tst --- src/test/providers/terminal.unit.test.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/test/providers/terminal.unit.test.ts b/src/test/providers/terminal.unit.test.ts index 9a62b560dc99..603c0710f8c5 100644 --- a/src/test/providers/terminal.unit.test.ts +++ b/src/test/providers/terminal.unit.test.ts @@ -7,9 +7,15 @@ import * as TypeMoq from 'typemoq'; import { Disposable, Terminal, Uri } from 'vscode'; import { IActiveResourceService, ICommandManager, IWorkspaceService } from '../../client/common/application/types'; import { Commands } from '../../client/common/constants'; +import { TerminalEnvVarActivation } from '../../client/common/experiments/groups'; import { TerminalService } from '../../client/common/terminal/service'; import { ITerminalActivator, ITerminalServiceFactory } from '../../client/common/terminal/types'; -import { IConfigurationService, IPythonSettings, ITerminalSettings } from '../../client/common/types'; +import { + IConfigurationService, + IExperimentService, + IPythonSettings, + ITerminalSettings, +} from '../../client/common/types'; import { IServiceContainer } from '../../client/ioc/types'; import { TerminalProvider } from '../../client/providers/terminalProvider'; @@ -18,13 +24,17 @@ suite('Terminal Provider', () => { let commandManager: TypeMoq.IMock; let workspace: TypeMoq.IMock; let activeResourceService: TypeMoq.IMock; + let experimentService: TypeMoq.IMock; let terminalProvider: TerminalProvider; const resource = Uri.parse('a'); setup(() => { serviceContainer = TypeMoq.Mock.ofType(); commandManager = TypeMoq.Mock.ofType(); + experimentService = TypeMoq.Mock.ofType(); + experimentService.setup((e) => e.inExperimentSync(TerminalEnvVarActivation.experiment)).returns(() => false); activeResourceService = TypeMoq.Mock.ofType(); workspace = TypeMoq.Mock.ofType(); + serviceContainer.setup((c) => c.get(IExperimentService)).returns(() => experimentService.object); serviceContainer.setup((c) => c.get(ICommandManager)).returns(() => commandManager.object); serviceContainer.setup((c) => c.get(IWorkspaceService)).returns(() => workspace.object); serviceContainer.setup((c) => c.get(IActiveResourceService)).returns(() => activeResourceService.object); From 6b64c7cc23d31b44ee91098766d87244cfbde7fa Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 6 Mar 2023 16:36:19 -0800 Subject: [PATCH 25/27] Add tests for collecton --- .../terminalEnvVarCollectionService.ts | 24 +-- ...rminalEnvVarCollectionService.unit.test.ts | 176 ++++++++++++++++++ 2 files changed, 188 insertions(+), 12 deletions(-) create mode 100644 src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts diff --git a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts index e54e332f25ef..f5af71b3f2ca 100644 --- a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts +++ b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts @@ -11,7 +11,6 @@ import { identifyShellFromShellPath } from '../../common/terminal/shellDetectors import { IExtensionContext, IExperimentService, Resource, IDisposableRegistry } from '../../common/types'; import { Deferred, createDeferred } from '../../common/utils/async'; import { Interpreters } from '../../common/utils/localize'; -import { EnvironmentVariables } from '../../common/variables/types'; import { traceDecoratorVerbose, traceVerbose } from '../../logging'; import { IInterpreterService } from '../contracts'; import { defaultShells } from './service'; @@ -19,14 +18,14 @@ import { IEnvironmentActivationService } from './types'; @injectable() export class TerminalEnvVarCollectionService implements IExtensionSingleActivationService { - public readonly supportedWorkspaceTypes: { untrustedWorkspace: boolean; virtualWorkspace: boolean } = { + public readonly supportedWorkspaceTypes = { untrustedWorkspace: false, virtualWorkspace: false, }; private deferred: Deferred | undefined; - private previousEnvVars = normCaseKeys(process.env); + private previousEnvVars = _normCaseKeys(process.env); constructor( @inject(IPlatformService) private readonly platform: IPlatformService, @@ -47,7 +46,7 @@ export class TerminalEnvVarCollectionService implements IExtensionSingleActivati this.interpreterService.onDidChangeInterpreter( async (resource) => { this.showProgress(); - await this.applyCollection(resource); + await this._applyCollection(resource); this.hideProgress(); }, this, @@ -58,17 +57,17 @@ export class TerminalEnvVarCollectionService implements IExtensionSingleActivati this.showProgress(); // 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); + await this._applyCollection(undefined, shell); this.hideProgress(); }, this, this.disposables, ); - this.applyCollection(undefined).ignoreErrors(); + this._applyCollection(undefined).ignoreErrors(); } - private async applyCollection(resource: Resource, shell = this.applicationEnvironment.shell) { + public async _applyCollection(resource: Resource, shell = this.applicationEnvironment.shell): Promise { const env = await this.environmentActivationService.getActivatedEnvironmentVariables( resource, undefined, @@ -77,14 +76,15 @@ export class TerminalEnvVarCollectionService implements IExtensionSingleActivati ); if (!env) { const shellType = identifyShellFromShellPath(shell); - if (defaultShells[this.platform.osType]?.shellType !== shellType) { + const defaultShell = defaultShells[this.platform.osType]; + if (defaultShell?.shellType !== shellType) { // Commands to fetch env vars may fail in custom shells due to unknown reasons, in that case // fallback to default shells as they are known to work better. - await this.applyCollection(resource); + await this._applyCollection(resource, defaultShell?.shell); return; } this.context.environmentVariableCollection.clear(); - this.previousEnvVars = normCaseKeys(process.env); + this.previousEnvVars = _normCaseKeys(process.env); return; } const previousEnv = this.previousEnvVars; @@ -138,8 +138,8 @@ export class TerminalEnvVarCollectionService implements IExtensionSingleActivati } } -function normCaseKeys(env: EnvironmentVariables): EnvironmentVariables { - const result: EnvironmentVariables = {}; +export function _normCaseKeys(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { + const result: NodeJS.ProcessEnv = {}; Object.keys(env).forEach((key) => { // `os.environ` script used to get env vars normalizes keys to upper case: // https://github.com/python/cpython/issues/101754 diff --git a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts new file mode 100644 index 000000000000..0db93887c483 --- /dev/null +++ b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts @@ -0,0 +1,176 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +import { expect } from 'chai'; +import { cloneDeep } from 'lodash'; +import { mock, instance, when, anything, verify, reset } from 'ts-mockito'; +import { EnvironmentVariableCollection, ProgressLocation } from 'vscode'; +import { IApplicationShell, IApplicationEnvironment } from '../../../client/common/application/types'; +import { TerminalEnvVarActivation } from '../../../client/common/experiments/groups'; +import { IPlatformService } from '../../../client/common/platform/types'; +import { IExtensionContext, IExperimentService } from '../../../client/common/types'; +import { Interpreters } from '../../../client/common/utils/localize'; +import { getOSType } from '../../../client/common/utils/platform'; +import { defaultShells } from '../../../client/interpreter/activation/service'; +import { + TerminalEnvVarCollectionService, + _normCaseKeys, +} from '../../../client/interpreter/activation/terminalEnvVarCollectionService'; +import { IEnvironmentActivationService } from '../../../client/interpreter/activation/types'; +import { IInterpreterService } from '../../../client/interpreter/contracts'; + +suite('Terminal Environment Variable Collection Service', () => { + let platform: IPlatformService; + let interpreterService: IInterpreterService; + let context: IExtensionContext; + let shell: IApplicationShell; + let experimentService: IExperimentService; + let collection: EnvironmentVariableCollection; + let applicationEnvironment: IApplicationEnvironment; + let environmentActivationService: IEnvironmentActivationService; + let terminalEnvVarCollectionService: TerminalEnvVarCollectionService; + const progressOptions = { + location: ProgressLocation.Window, + title: Interpreters.activatingTerminals, + }; + const customShell = 'powershell'; + const defaultShell = defaultShells[getOSType()]; + + setup(() => { + platform = mock(); + when(platform.osType).thenReturn(getOSType()); + interpreterService = mock(); + context = mock(); + shell = mock(); + collection = mock(); + when(context.environmentVariableCollection).thenReturn(instance(collection)); + experimentService = mock(); + when(experimentService.inExperimentSync(TerminalEnvVarActivation.experiment)).thenReturn(true); + applicationEnvironment = mock(); + when(applicationEnvironment.shell).thenReturn(customShell); + when(shell.withProgress(anything(), anything())) + .thenCall((options, _) => { + expect(options).to.deep.equal(progressOptions); + }) + .thenResolve(); + environmentActivationService = mock(); + terminalEnvVarCollectionService = new TerminalEnvVarCollectionService( + instance(platform), + instance(interpreterService), + instance(context), + instance(shell), + instance(experimentService), + instance(applicationEnvironment), + [], + instance(environmentActivationService), + ); + }); + + test('If activated variables are returned for custom shell, apply it correctly to the collection', async () => { + const envVars: NodeJS.ProcessEnv = { CONDA_PREFIX: 'prefix/to/conda', ..._normCaseKeys(process.env) }; + delete envVars.PATH; + when( + environmentActivationService.getActivatedEnvironmentVariables( + anything(), + undefined, + undefined, + customShell, + ), + ).thenResolve(envVars); + + when(collection.replace(anything(), anything())).thenResolve(); + when(collection.delete(anything())).thenResolve(); + + await terminalEnvVarCollectionService._applyCollection(undefined, customShell); + + verify(collection.replace('CONDA_PREFIX', 'prefix/to/conda')).once(); + verify(collection.delete('PATH')).once(); + }); + + test('Only relative changes to previously applied variables are applied to the collection', async () => { + const envVars: NodeJS.ProcessEnv = { + RANDOM_VAR: 'random', + CONDA_PREFIX: 'prefix/to/conda', + ..._normCaseKeys(process.env), + }; + when( + environmentActivationService.getActivatedEnvironmentVariables( + anything(), + undefined, + undefined, + customShell, + ), + ).thenResolve(envVars); + + when(collection.replace(anything(), anything())).thenResolve(); + when(collection.delete(anything())).thenResolve(); + + await terminalEnvVarCollectionService._applyCollection(undefined, customShell); + + const newEnvVars = cloneDeep(envVars); + delete newEnvVars.CONDA_PREFIX; + newEnvVars.RANDOM_VAR = undefined; // Deleting the variable from the collection is the same as setting it to undefined. + reset(environmentActivationService); + when( + environmentActivationService.getActivatedEnvironmentVariables( + anything(), + undefined, + undefined, + customShell, + ), + ).thenResolve(newEnvVars); + + await terminalEnvVarCollectionService._applyCollection(undefined, customShell); + + 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 () => { + when( + environmentActivationService.getActivatedEnvironmentVariables( + anything(), + undefined, + undefined, + customShell, + ), + ).thenResolve(undefined); + const envVars = { CONDA_PREFIX: 'prefix/to/conda', ..._normCaseKeys(process.env) }; + when( + environmentActivationService.getActivatedEnvironmentVariables( + anything(), + undefined, + undefined, + defaultShell?.shell, + ), + ).thenResolve(envVars); + + when(collection.replace(anything(), anything())).thenResolve(); + when(collection.delete(anything())).thenResolve(); + + await terminalEnvVarCollectionService._applyCollection(undefined, customShell); + + verify(collection.replace('CONDA_PREFIX', 'prefix/to/conda')).once(); + verify(collection.delete(anything())).never(); + }); + + test('If no activated variables are returned for default shell, clear collection', async () => { + when( + environmentActivationService.getActivatedEnvironmentVariables( + anything(), + undefined, + undefined, + defaultShell?.shell, + ), + ).thenResolve(undefined); + + when(collection.replace(anything(), anything())).thenResolve(); + when(collection.delete(anything())).thenResolve(); + + await terminalEnvVarCollectionService._applyCollection(undefined, defaultShell?.shell); + + verify(collection.clear()).once(); + }); +}); From 396cfb9df6549ef5b18eaa671ae73368d1595aa8 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 8 Mar 2023 10:58:28 -0800 Subject: [PATCH 26/27] Add tests for activation --- src/client/common/experiments/helpers.ts | 3 +- ...rminalEnvVarCollectionService.unit.test.ts | 66 ++++++++++++++++++- 2 files changed, 65 insertions(+), 4 deletions(-) diff --git a/src/client/common/experiments/helpers.ts b/src/client/common/experiments/helpers.ts index ee3504e15a2f..4aed04da3fd0 100644 --- a/src/client/common/experiments/helpers.ts +++ b/src/client/common/experiments/helpers.ts @@ -4,11 +4,12 @@ 'use strict'; import { workspace } from 'vscode'; +import { isTestExecution } from '../constants'; import { IExperimentService } from '../types'; import { TerminalEnvVarActivation } from './groups'; export function inTerminalEnvVarExperiment(experimentService: IExperimentService): boolean { - if (workspace.workspaceFile) { + if (workspace.workspaceFile && !isTestExecution()) { // Don't run experiment in multi-root workspaces for now, requires work on VSCode: // https://github.com/microsoft/vscode/issues/171173 return false; diff --git a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts index 0db93887c483..4ac04cf1ee22 100644 --- a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts +++ b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts @@ -3,14 +3,15 @@ 'use strict'; -import { expect } from 'chai'; +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 } from 'vscode'; +import { EnvironmentVariableCollection, ProgressLocation, Uri } from 'vscode'; import { IApplicationShell, IApplicationEnvironment } from '../../../client/common/application/types'; import { TerminalEnvVarActivation } from '../../../client/common/experiments/groups'; import { IPlatformService } from '../../../client/common/platform/types'; -import { IExtensionContext, IExperimentService } from '../../../client/common/types'; +import { IExtensionContext, IExperimentService, Resource } from '../../../client/common/types'; import { Interpreters } from '../../../client/common/utils/localize'; import { getOSType } from '../../../client/common/utils/platform'; import { defaultShells } from '../../../client/interpreter/activation/service'; @@ -68,6 +69,65 @@ suite('Terminal Environment Variable Collection Service', () => { ); }); + teardown(() => { + sinon.restore(); + }); + + test('Apply activated variables to the collection on activation', async () => { + const applyCollectionStub = sinon.stub(terminalEnvVarCollectionService, '_applyCollection'); + applyCollectionStub.resolves(); + when(interpreterService.onDidChangeInterpreter(anything(), anything(), anything())).thenReturn(); + when(applicationEnvironment.onDidChangeShell(anything(), anything(), anything())).thenReturn(); + await terminalEnvVarCollectionService.activate(); + assert(applyCollectionStub.calledOnce, 'Collection not applied on activation'); + }); + + test('When not in experiment, do not apply activated variables to the collection and clear it instead', async () => { + reset(experimentService); + when(experimentService.inExperimentSync(TerminalEnvVarActivation.experiment)).thenReturn(false); + const applyCollectionStub = sinon.stub(terminalEnvVarCollectionService, '_applyCollection'); + applyCollectionStub.resolves(); + when(interpreterService.onDidChangeInterpreter(anything(), anything(), anything())).thenReturn(); + when(applicationEnvironment.onDidChangeShell(anything(), anything(), anything())).thenReturn(); + + await terminalEnvVarCollectionService.activate(); + + verify(interpreterService.onDidChangeInterpreter(anything(), anything(), anything())).never(); + verify(applicationEnvironment.onDidChangeShell(anything(), anything(), anything())).never(); + assert(applyCollectionStub.notCalled, 'Collection should not be applied on activation'); + + verify(collection.clear()).once(); + }); + + test('When interpreter changes, apply new activated variables to the collection', async () => { + const applyCollectionStub = sinon.stub(terminalEnvVarCollectionService, '_applyCollection'); + applyCollectionStub.resolves(); + const resource = Uri.file('x'); + let callback: (resource: Resource) => Promise; + when(interpreterService.onDidChangeInterpreter(anything(), anything(), anything())).thenCall((cb) => { + callback = cb; + }); + when(applicationEnvironment.onDidChangeShell(anything(), anything(), anything())).thenReturn(); + await terminalEnvVarCollectionService.activate(); + + await callback!(resource); + assert(applyCollectionStub.calledWithExactly(resource)); + }); + + test('When selected shell changes, apply new activated variables to the collection', async () => { + const applyCollectionStub = sinon.stub(terminalEnvVarCollectionService, '_applyCollection'); + applyCollectionStub.resolves(); + let callback: (shell: string) => Promise; + when(applicationEnvironment.onDidChangeShell(anything(), anything(), anything())).thenCall((cb) => { + callback = cb; + }); + when(interpreterService.onDidChangeInterpreter(anything(), anything(), anything())).thenReturn(); + await terminalEnvVarCollectionService.activate(); + + await callback!(customShell); + assert(applyCollectionStub.calledWithExactly(undefined, customShell)); + }); + test('If activated variables are returned for custom shell, apply it correctly to the collection', async () => { const envVars: NodeJS.ProcessEnv = { CONDA_PREFIX: 'prefix/to/conda', ..._normCaseKeys(process.env) }; delete envVars.PATH; From 1dfb78502bc64f8de5a826b0be07ffa770f16132 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 8 Mar 2023 13:11:55 -0800 Subject: [PATCH 27/27] Update vscode engine --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 0cde0318b0df..3211ae25b8cd 100644 --- a/package.json +++ b/package.json @@ -41,7 +41,7 @@ "theme": "dark" }, "engines": { - "vscode": "^1.75.0-20230123" + "vscode": "^1.76.0" }, "keywords": [ "python",