From 7352a0a5d6ce73138f541ae63ace966c7014fc83 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Tue, 9 Feb 2021 15:11:12 -0800 Subject: [PATCH 01/14] Cherry pick fix for native notebook support into release branch (#15369) * Fix problem with notebook apis not being used. (#15366) * Update changelog * Remove news file --- CHANGELOG.md | 2 ++ src/client/common/application/notebook.ts | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 290fb2d196f3..93d944df5799 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,8 @@ ([#15232](https://github.com/Microsoft/vscode-python/issues/15232)) 1. Ensure target environment is activated in the terminal when running install scripts. ([#15285](https://github.com/Microsoft/vscode-python/issues/15285)) +1. Allow support for using notebook APIs in the VS code stable build. + ([#15364](https://github.com/Microsoft/vscode-python/issues/15364)) ### Code Health diff --git a/src/client/common/application/notebook.ts b/src/client/common/application/notebook.ts index 52557d820184..bd5a873d7361 100644 --- a/src/client/common/application/notebook.ts +++ b/src/client/common/application/notebook.ts @@ -85,7 +85,7 @@ export class VSCodeNotebook implements IVSCodeNotebook { @inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry, @inject(IApplicationEnvironment) readonly env: IApplicationEnvironment, ) { - if (this.useProposedApi && this.env.channel === 'insiders') { + if (this.useProposedApi) { this.addEventHandlers(); this.canUseNotebookApi = true; } From 3f78ccb7a7dbc8a7b9d6cc56c6d855d5fd6179d7 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Tue, 9 Feb 2021 15:21:52 -0800 Subject: [PATCH 02/14] Add extraPaths support to JediLSP (#15365) * Add extraPaths support * Remove duplicate opt option * Eslint cleanup * Fix tests --- .eslintignore | 1 - package.json | 2 - src/client/activation/jedi/analysisOptions.ts | 42 ++++++++- src/client/providers/jediProxy.ts | 88 +++++++++++++----- .../jedi/jediAnalysisOptions.unit.test.ts | 93 +++++++++++++++++++ 5 files changed, 198 insertions(+), 28 deletions(-) create mode 100644 src/test/activation/jedi/jediAnalysisOptions.unit.test.ts diff --git a/.eslintignore b/.eslintignore index d7240625a999..55d41fe9132c 100644 --- a/.eslintignore +++ b/.eslintignore @@ -383,7 +383,6 @@ src/client/providers/docStringFoldingProvider.ts src/client/providers/linterProvider.ts src/client/providers/simpleRefactorProvider.ts src/client/providers/completionProvider.ts -src/client/providers/jediProxy.ts src/client/providers/definitionProvider.ts src/client/providers/referenceProvider.ts src/client/providers/terminalProvider.ts diff --git a/package.json b/package.json index cdd48b18e0fb..68e97e23b293 100644 --- a/package.json +++ b/package.json @@ -1044,7 +1044,6 @@ "DeprecatePythonPath - experiment", "RunByLine - experiment", "tryPylance", - "jediLSP", "debuggerDataViewer", "pythonSendEntireLineToREPL", "pythonTensorboardExperiment", @@ -1074,7 +1073,6 @@ "DeprecatePythonPath - experiment", "RunByLine - experiment", "tryPylance", - "jediLSP", "debuggerDataViewer", "pythonSendEntireLineToREPL", "pythonTensorboardExperiment", diff --git a/src/client/activation/jedi/analysisOptions.ts b/src/client/activation/jedi/analysisOptions.ts index f2ba1555fcd8..6eb2e296c31c 100644 --- a/src/client/activation/jedi/analysisOptions.ts +++ b/src/client/activation/jedi/analysisOptions.ts @@ -1,8 +1,13 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. import { inject, injectable } from 'inversify'; +import * as path from 'path'; +import { WorkspaceFolder } from 'vscode'; +import { IWorkspaceService } from '../../common/application/types'; +import { IConfigurationService, Resource } from '../../common/types'; import { IEnvironmentVariablesProvider } from '../../common/variables/types'; +import { PythonEnvironment } from '../../pythonEnvironments/info'; import { LanguageServerAnalysisOptionsWithEnv } from '../common/analysisOptions'; import { ILanguageServerOutputChannel } from '../types'; @@ -10,15 +15,47 @@ import { ILanguageServerOutputChannel } from '../types'; @injectable() export class JediLanguageServerAnalysisOptions extends LanguageServerAnalysisOptionsWithEnv { - // eslint-disable-next-line @typescript-eslint/no-useless-constructor + private resource: Resource | undefined; + constructor( @inject(IEnvironmentVariablesProvider) envVarsProvider: IEnvironmentVariablesProvider, @inject(ILanguageServerOutputChannel) lsOutputChannel: ILanguageServerOutputChannel, + @inject(IConfigurationService) private readonly configurationService: IConfigurationService, + @inject(IWorkspaceService) private readonly workspace: IWorkspaceService, ) { super(envVarsProvider, lsOutputChannel); + this.resource = undefined; + } + + public async initialize(resource: Resource, interpreter: PythonEnvironment | undefined) { + this.resource = resource; + return super.initialize(resource, interpreter); + } + + protected getWorkspaceFolder(): WorkspaceFolder | undefined { + return this.workspace.getWorkspaceFolder(this.resource); } protected async getInitializationOptions() { + const pythonSettings = this.configurationService.getSettings(this.resource); + const workspacePath = this.getWorkspaceFolder()?.uri.fsPath; + const extraPaths = pythonSettings.autoComplete + ? pythonSettings.autoComplete.extraPaths.map((extraPath) => { + if (path.isAbsolute(extraPath)) { + return extraPath; + } + return workspacePath ? path.join(workspacePath, extraPath) : ''; + }) + : []; + + if (workspacePath) { + extraPaths.unshift(workspacePath); + } + + const distinctExtraPaths = extraPaths + .filter((value) => value.length > 0) + .filter((value, index, self) => self.indexOf(value) === index); + return { markupKindPreferred: 'markdown', completion: { @@ -31,6 +68,9 @@ export class JediLanguageServerAnalysisOptions extends LanguageServerAnalysisOpt didSave: true, didChange: true, }, + workspace: { + extraPaths: distinctExtraPaths, + }, }; } } diff --git a/src/client/providers/jediProxy.ts b/src/client/providers/jediProxy.ts index e3564a8b5e79..b76418e75940 100644 --- a/src/client/providers/jediProxy.ts +++ b/src/client/providers/jediProxy.ts @@ -1,9 +1,11 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +/* eslint-disable max-classes-per-file, @typescript-eslint/no-explicit-any */ + import { ChildProcess } from 'child_process'; import * as path from 'path'; -// @ts-ignore +// @ts-ignore pidusage does not work in strict mode import * as pidusage from 'pidusage'; import { CancellationToken, CancellationTokenSource, CompletionItemKind, Disposable, SymbolKind, Uri } from 'vscode'; import '../common/extensions'; @@ -21,7 +23,7 @@ import { IServiceContainer } from '../ioc/types'; import { PythonEnvironment } from '../pythonEnvironments/info'; import { sendTelemetryEvent } from '../telemetry'; import { EventName } from '../telemetry/constants'; -import { traceError, traceWarning } from './../common/logger'; +import { traceError, traceWarning } from '../common/logger'; const pythonVSCodeTypeMappings = new Map(); pythonVSCodeTypeMappings.set('none', CompletionItemKind.Value); @@ -153,23 +155,41 @@ type JediProxyPayload = { export class JediProxy implements Disposable { private proc?: ChildProcess; + private pythonSettings: IPythonSettings; - private cmdId: number = 0; + + private cmdId = 0; + private lastKnownPythonInterpreter: string; + private previousData = ''; + private commands = new Map>(); + private commandQueue: number[] = []; + private spawnRetryAttempts = 0; + private additionalAutoCompletePaths: string[] = []; + private workspacePath: string; + private languageServerStarted!: Deferred; + private initialized: Deferred; + private environmentVariablesProvider!: IEnvironmentVariablesProvider; - private ignoreJediMemoryFootprint: boolean = false; + + private ignoreJediMemoryFootprint = false; + private pidUsageFailures = { timer: new StopWatch(), counter: 0 }; + private lastCmdIdProcessed?: number; + private lastCmdIdProcessedForPidUsage?: number; + private readonly disposables: Disposable[] = []; + private timer?: NodeJS.Timer | number; public constructor( @@ -188,11 +208,11 @@ export class JediProxy implements Disposable { this.checkJediMemoryFootprint().ignoreErrors(); } - private static getProperty(o: object, name: string): T { - return (o as any)[name]; + private static getProperty(o: Record, name: string): T { + return o[name]; } - public dispose() { + public dispose(): void { while (this.disposables.length > 0) { const disposable = this.disposables.pop(); if (disposable) { @@ -200,6 +220,7 @@ export class JediProxy implements Disposable { } } if (this.timer) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any clearTimeout(this.timer as any); } this.killProcess(); @@ -227,7 +248,7 @@ export class JediProxy implements Disposable { this.commandQueue.push(executionCmd.id); } catch (ex) { traceError(ex); - //If 'This socket is closed.' that means process didn't start at all (at least not properly). + // If 'This socket is closed.' that means process didn't start at all (at least not properly). if (ex.message === 'This socket is closed.') { this.killProcess(); } else { @@ -247,6 +268,7 @@ export class JediProxy implements Disposable { this.handleError('spawnProcess', ex); }); } + private shouldCheckJediMemoryFootprint() { if (this.ignoreJediMemoryFootprint || this.pythonSettings.jediMemoryLimit === -1) { return false; @@ -262,6 +284,7 @@ export class JediProxy implements Disposable { } return true; } + private async checkJediMemoryFootprint() { // Check memory footprint periodically. Do not check on every request due to // the performance impact. See https://github.com/soyuka/pidusage - on Windows @@ -276,6 +299,7 @@ export class JediProxy implements Disposable { } this.timer = setTimeout(() => this.checkJediMemoryFootprint(), 15 * 1000); } + private async checkJediMemoryFootprintImpl(): Promise { if (!this.proc || this.proc.killed) { return; @@ -321,24 +345,27 @@ export class JediProxy implements Disposable { deferred.resolve(); }); + // eslint-disable-next-line consistent-return return deferred.promise; } // @debounce(1500) @swallowExceptions('JediProxy') private async environmentVariablesChangeHandler() { - const newAutoComletePaths = await this.buildAutoCompletePaths(); - if (this.additionalAutoCompletePaths.join(',') !== newAutoComletePaths.join(',')) { - this.additionalAutoCompletePaths = newAutoComletePaths; + const newAutoCompletePaths = await this.buildAutoCompletePaths(); + if (this.additionalAutoCompletePaths.join(',') !== newAutoCompletePaths.join(',')) { + this.additionalAutoCompletePaths = newAutoCompletePaths; this.restartLanguageServer().ignoreErrors(); } } + @swallowExceptions('JediProxy') private async startLanguageServer(): Promise { const newAutoComletePaths = await this.buildAutoCompletePaths(); this.additionalAutoCompletePaths = newAutoComletePaths; return this.restartLanguageServer(); } + private restartLanguageServer(): Promise { this.killProcess(); this.clearPendingRequests(); @@ -360,11 +387,14 @@ export class JediProxy implements Disposable { if (this.proc) { this.proc.kill(); } - } catch (ex) {} + } catch (ex) { + // intentionally left blank + } this.proc = undefined; } - private handleError(source: string, errorMessage: string) { + // eslint-disable-next-line class-methods-use-this + private handleError(source: string, errorMessage: string): void { traceError(`${source} jediProxy`, `Error (${source}) ${errorMessage}`); } @@ -412,7 +442,8 @@ export class JediProxy implements Disposable { const data = output.out; // Possible there was an exception in parsing the data returned, // so append the data and then parse it. - const dataStr = (this.previousData = `${this.previousData}${data}`); + this.previousData = `${this.previousData}${data}`; + const dataStr = this.previousData; let responses: any[]; try { @@ -444,7 +475,7 @@ export class JediProxy implements Disposable { return; } this.lastCmdIdProcessed = cmd.id; - if (JediProxy.getProperty(response, 'arguments')) { + if (JediProxy.getProperty(response, 'arguments')) { this.commandQueue.splice(this.commandQueue.indexOf(cmd.id), 1); return; } @@ -473,9 +504,10 @@ export class JediProxy implements Disposable { (error) => this.handleError('subscription.error', `${error}`), ); } + private getCommandHandler( command: CommandType, - ): undefined | ((command: IExecutionCommand, response: object) => void) { + ): undefined | ((command: IExecutionCommand, response: Record) => void) { switch (command) { case CommandType.Completions: return this.onCompletion; @@ -490,10 +522,11 @@ export class JediProxy implements Disposable { case CommandType.Arguments: return this.onArguments; default: - return; } + return undefined; } - private onCompletion(command: IExecutionCommand, response: object): void { + + private onCompletion(command: IExecutionCommand, response: Record): void { let results = JediProxy.getProperty(response, 'results'); results = Array.isArray(results) ? results : []; results.forEach((item) => { @@ -509,7 +542,7 @@ export class JediProxy implements Disposable { this.safeResolve(command, completionResult); } - private onDefinition(command: IExecutionCommand, response: object): void { + private onDefinition(command: IExecutionCommand, response: Record): void { const defs = JediProxy.getProperty(response, 'results'); const defResult: IDefinitionResult = { requestId: command.id, @@ -537,7 +570,7 @@ export class JediProxy implements Disposable { this.safeResolve(command, defResult); } - private onHover(command: IExecutionCommand, response: object): void { + private onHover(command: IExecutionCommand, response: Record): void { const defs = JediProxy.getProperty(response, 'results'); const defResult: IHoverResult = { requestId: command.id, @@ -554,7 +587,7 @@ export class JediProxy implements Disposable { this.safeResolve(command, defResult); } - private onSymbols(command: IExecutionCommand, response: object): void { + private onSymbols(command: IExecutionCommand, response: Record): void { let defs = JediProxy.getProperty(response, 'results'); defs = Array.isArray(defs) ? defs : []; const defResults: ISymbolResult = { @@ -581,7 +614,7 @@ export class JediProxy implements Disposable { this.safeResolve(command, defResults); } - private onUsages(command: IExecutionCommand, response: object): void { + private onUsages(command: IExecutionCommand, response: Record): void { let defs = JediProxy.getProperty(response, 'results'); defs = Array.isArray(defs) ? defs : []; const refResult: IReferenceResult = { @@ -599,7 +632,7 @@ export class JediProxy implements Disposable { this.safeResolve(command, refResult); } - private onArguments(command: IExecutionCommand, response: object): void { + private onArguments(command: IExecutionCommand, response: Record): void { const defs = JediProxy.getProperty(response, 'results'); this.safeResolve(command, { @@ -617,6 +650,7 @@ export class JediProxy implements Disposable { try { this.safeResolve(cmd1, undefined); } catch (ex) { + // Intentionally left blank } finally { this.commands.delete(id); } @@ -663,6 +697,7 @@ export class JediProxy implements Disposable { return ''; } } + private async buildAutoCompletePaths(): Promise { const filePathPromises = [ // Sysprefix. @@ -703,6 +738,7 @@ export class JediProxy implements Disposable { return []; } } + private getEnvironmentVariablesProvider() { if (!this.environmentVariablesProvider) { this.environmentVariablesProvider = this.serviceContainer.get( @@ -714,6 +750,7 @@ export class JediProxy implements Disposable { } return this.environmentVariablesProvider; } + private getConfig(): JediProxyConfig { // Add support for paths relative to workspace. const extraPaths = this.pythonSettings.autoComplete @@ -747,6 +784,7 @@ export class JediProxy implements Disposable { }; } + // eslint-disable-next-line class-methods-use-this private safeResolve( command: IExecutionCommand | undefined | null, result: ICommandResult | PromiseLike | undefined, @@ -827,6 +865,7 @@ export interface IAutoCompleteItem { kind: SymbolKind; text: string; description: string; + // eslint-disable-next-line camelcase raw_docstring: string; rightLabel: string; } @@ -865,12 +904,13 @@ export class JediProxyHandler implements Disposable { return this.jediProxy; } - public dispose() { + public dispose(): void { if (this.jediProxy) { this.jediProxy.dispose(); } } + // eslint-disable-next-line @typescript-eslint/no-unused-vars public sendCommand(cmd: ICommand, _token?: CancellationToken): Promise { const executionCmd = >cmd; executionCmd.id = executionCmd.id || this.jediProxy.getNextCommandId(); diff --git a/src/test/activation/jedi/jediAnalysisOptions.unit.test.ts b/src/test/activation/jedi/jediAnalysisOptions.unit.test.ts new file mode 100644 index 000000000000..e7a1f5011edb --- /dev/null +++ b/src/test/activation/jedi/jediAnalysisOptions.unit.test.ts @@ -0,0 +1,93 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { expect } from 'chai'; +import * as path from 'path'; +import { anything, instance, mock, when } from 'ts-mockito'; +import { EventEmitter, Uri, WorkspaceFolder } from 'vscode'; +import { JediLanguageServerAnalysisOptions } from '../../../client/activation/jedi/analysisOptions'; +import { ILanguageServerAnalysisOptions, ILanguageServerOutputChannel } from '../../../client/activation/types'; +import { IWorkspaceService } from '../../../client/common/application/types'; +import { WorkspaceService } from '../../../client/common/application/workspace'; +import { ConfigurationService } from '../../../client/common/configuration/service'; +import { IConfigurationService } from '../../../client/common/types'; +import { IEnvironmentVariablesProvider } from '../../../client/common/variables/types'; + +suite('Jedi LSP - analysis Options', () => { + const workspacePath = path.join('this', 'is', 'fake', 'workspace', 'path'); + const expectedWorkspacePath = path.sep + workspacePath; + + let envVarsProvider: IEnvironmentVariablesProvider; + let lsOutputChannel: ILanguageServerOutputChannel; + let configurationService: IConfigurationService; + let workspaceService: IWorkspaceService; + + let analysisOptions: ILanguageServerAnalysisOptions; + + class MockWorkspaceFolder implements WorkspaceFolder { + public uri: Uri; + + public name: string; + + public ownedResources = new Set(); + + constructor(folder: string, public index: number = 0) { + this.uri = Uri.file(folder); + this.name = folder; + } + } + + setup(() => { + envVarsProvider = mock(IEnvironmentVariablesProvider); + lsOutputChannel = mock(ILanguageServerOutputChannel); + configurationService = mock(ConfigurationService); + workspaceService = mock(WorkspaceService); + + const onDidChangeEnvVariables = new EventEmitter(); + when(envVarsProvider.onDidEnvironmentVariablesChange).thenReturn(onDidChangeEnvVariables.event); + + analysisOptions = new JediLanguageServerAnalysisOptions( + instance(envVarsProvider), + instance(lsOutputChannel), + instance(configurationService), + instance(workspaceService), + ); + }); + test('Without extraPaths provided and no workspace', async () => { + when(workspaceService.getWorkspaceFolder(anything())).thenReturn(undefined); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + when(configurationService.getSettings(anything())).thenReturn({} as any); + analysisOptions.initialize(undefined, undefined); + + const result = await analysisOptions.getAnalysisOptions(); + expect(result.initializationOptions.workspace.extraPaths).to.deep.equal([]); + }); + + test('Without extraPaths provided', async () => { + when(workspaceService.getWorkspaceFolder(anything())).thenReturn(new MockWorkspaceFolder(workspacePath)); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + when(configurationService.getSettings(anything())).thenReturn({} as any); + analysisOptions.initialize(undefined, undefined); + + const result = await analysisOptions.getAnalysisOptions(); + expect(result.initializationOptions.workspace.extraPaths).to.deep.equal([expectedWorkspacePath]); + }); + + test('With extraPaths provided', async () => { + when(workspaceService.getWorkspaceFolder(anything())).thenReturn(new MockWorkspaceFolder(workspacePath)); + when(configurationService.getSettings(anything())).thenReturn({ + // We expect a distinct set of paths back, using __dirname to test absolute path + autoComplete: { extraPaths: [__dirname, 'relative/pathB', 'relative/pathB'] }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any); + analysisOptions.initialize(undefined, undefined); + + const result = await analysisOptions.getAnalysisOptions(); + + expect(result.initializationOptions.workspace.extraPaths).to.deep.equal([ + expectedWorkspacePath, + __dirname, + path.join(expectedWorkspacePath, 'relative/pathB'), + ]); + }); +}); From 525ec7873bf95a8e9a51ed3afdcdab412a4d56a2 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Tue, 9 Feb 2021 15:30:42 -0800 Subject: [PATCH 03/14] Ignore directories with the same name as python binaries (#15367) * Ignore directories with the same name as pyhton binaries * Use withFileTypes instead of lstat --- .../pythonEnvironments/common/commonUtils.ts | 6 +++--- .../pythonEnvironments/common/posixUtils.ts | 2 +- .../services/posixKnownPathsLocator.ts | 21 ++++++++++--------- .../posixroot/location3/python3.9/empty | 1 + 4 files changed, 16 insertions(+), 14 deletions(-) create mode 100644 src/test/pythonEnvironments/common/envlayouts/posixroot/location3/python3.9/empty diff --git a/src/client/pythonEnvironments/common/commonUtils.ts b/src/client/pythonEnvironments/common/commonUtils.ts index 825ca783bc51..95467aff687e 100644 --- a/src/client/pythonEnvironments/common/commonUtils.ts +++ b/src/client/pythonEnvironments/common/commonUtils.ts @@ -12,10 +12,10 @@ import { comparePythonVersionSpecificity } from '../base/info/env'; import { parseVersion } from '../base/info/pythonVersion'; import { getPythonVersionFromConda } from '../discovery/locators/services/conda'; import { getPythonVersionFromPyvenvCfg } from '../discovery/locators/services/virtualEnvironmentIdentifier'; -import { isPosixPythonBin } from './posixUtils'; +import { isPosixPythonBinPattern } from './posixUtils'; import { isWindowsPythonExe } from './windowsUtils'; -const matchPythonExecutable = getOSType() === OSType.Windows ? isWindowsPythonExe : isPosixPythonBin; +const matchPythonExecutable = getOSType() === OSType.Windows ? isWindowsPythonExe : isPosixPythonBinPattern; type FileFilterFunc = (filename: string) => boolean; @@ -33,7 +33,7 @@ export async function* findInterpretersInDir( ): AsyncIterableIterator { // "checkBin" is a local variable rather than global // so we can stub it out during unit testing. - const checkBin = getOSType() === OSType.Windows ? isWindowsPythonExe : isPosixPythonBin; + const checkBin = getOSType() === OSType.Windows ? isWindowsPythonExe : isPosixPythonBinPattern; const cfg = { ignoreErrors, filterSubDir, diff --git a/src/client/pythonEnvironments/common/posixUtils.ts b/src/client/pythonEnvironments/common/posixUtils.ts index 181976fddf3d..62227fbcb2c1 100644 --- a/src/client/pythonEnvironments/common/posixUtils.ts +++ b/src/client/pythonEnvironments/common/posixUtils.ts @@ -10,7 +10,7 @@ import { getSearchPathEntries } from '../../common/utils/exec'; * @param {string} interpreterPath : Path to python interpreter. * @returns {boolean} : Returns true if the path matches pattern for windows python executable. */ -export function isPosixPythonBin(interpreterPath: string): boolean { +export function isPosixPythonBinPattern(interpreterPath: string): boolean { /** * This Reg-ex matches following file names: * python diff --git a/src/client/pythonEnvironments/discovery/locators/services/posixKnownPathsLocator.ts b/src/client/pythonEnvironments/discovery/locators/services/posixKnownPathsLocator.ts index 87a4e76d0b16..8dc0e1dc095b 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/posixKnownPathsLocator.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/posixKnownPathsLocator.ts @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import * as fsapi from 'fs-extra'; +import * as fs from 'fs'; import * as path from 'path'; import { traceError, traceInfo } from '../../../../common/logger'; @@ -11,23 +11,24 @@ import { buildEnvInfo } from '../../../base/info/env'; import { parseVersion } from '../../../base/info/pythonVersion'; import { IPythonEnvsIterator, Locator } from '../../../base/locator'; import { getFileInfo, resolveSymbolicLink } from '../../../common/externalDependencies'; -import { commonPosixBinPaths, isPosixPythonBin } from '../../../common/posixUtils'; +import { commonPosixBinPaths, isPosixPythonBinPattern } from '../../../common/posixUtils'; async function getPythonBinFromKnownPaths(): Promise { - const knownPaths = await commonPosixBinPaths(); + const knownDirs = await commonPosixBinPaths(); const pythonBins: Set = new Set(); - for (const knownPath of knownPaths) { - const files = (await fsapi.readdir(knownPath)) - .map((filename: string) => path.join(knownPath, filename)) - .filter(isPosixPythonBin); + for (const dirname of knownDirs) { + const paths = (await fs.promises.readdir(dirname, { withFileTypes: true })) + .filter((dirent: fs.Dirent) => !dirent.isDirectory()) + .map((dirent: fs.Dirent) => path.join(dirname, dirent.name)) + .filter(isPosixPythonBinPattern); - for (const file of files) { + for (const filepath of paths) { // Ensure that we have a collection of unique global binaries by // resolving all symlinks to the target binaries. try { - const resolvedBin = await resolveSymbolicLink(file); + const resolvedBin = await resolveSymbolicLink(filepath); pythonBins.add(resolvedBin); - traceInfo(`Found: ${file} --> ${resolvedBin}`); + traceInfo(`Found: ${filepath} --> ${resolvedBin}`); } catch (ex) { traceError('Failed to resolve symbolic link: ', ex); } diff --git a/src/test/pythonEnvironments/common/envlayouts/posixroot/location3/python3.9/empty b/src/test/pythonEnvironments/common/envlayouts/posixroot/location3/python3.9/empty new file mode 100644 index 000000000000..0c6fe8957e8a --- /dev/null +++ b/src/test/pythonEnvironments/common/envlayouts/posixroot/location3/python3.9/empty @@ -0,0 +1 @@ +this is intentionally empty From 338af8951e190c2e55c0ea69c90d7c045fb976ea Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 9 Feb 2021 11:06:03 -0800 Subject: [PATCH 04/14] Correct pipenv activation provider when in discovery experiment (#15319) * Correct pipenv activation provider when in discovery experiment * Fix ESLint errors --- .eslintignore | 2 - .../pipEnvActivationProvider.ts | 40 +++++++++++-------- src/client/pythonEnvironments/legacyIOC.ts | 8 +--- .../pipEnvActivationProvider.unit.test.ts | 11 ++++- 4 files changed, 34 insertions(+), 27 deletions(-) diff --git a/.eslintignore b/.eslintignore index 55d41fe9132c..9c9908ede444 100644 --- a/.eslintignore +++ b/.eslintignore @@ -186,7 +186,6 @@ src/test/common/terminals/service.unit.test.ts src/test/common/terminals/helper.unit.test.ts src/test/common/terminals/activation.unit.test.ts src/test/common/terminals/shellDetectors/shellDetectors.unit.test.ts -src/test/common/terminals/environmentActivationProviders/pipEnvActivationProvider.unit.test.ts src/test/common/terminals/environmentActivationProviders/terminalActivation.testvirtualenvs.ts src/test/common/socketStream.test.ts src/test/common/configSettings.test.ts @@ -548,7 +547,6 @@ src/client/common/terminal/shellDetectors/vscEnvironmentShellDetector.ts src/client/common/terminal/shellDetectors/terminalNameShellDetector.ts src/client/common/terminal/shellDetectors/settingsShellDetector.ts src/client/common/terminal/shellDetectors/baseShellDetector.ts -src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts src/client/common/terminal/environmentActivationProviders/baseActivationProvider.ts src/client/common/terminal/environmentActivationProviders/commandPrompt.ts src/client/common/terminal/environmentActivationProviders/bash.ts diff --git a/src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts b/src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts index 1589bd43ce54..49e60e109a03 100644 --- a/src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts +++ b/src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts @@ -5,13 +5,15 @@ import { inject, injectable, named } from 'inversify'; import { Uri } from 'vscode'; -import '../../../common/extensions'; +import '../../extensions'; import { IInterpreterService } from '../../../interpreter/contracts'; +import { isPipenvEnvironmentRelatedToFolder } from '../../../pythonEnvironments/discovery/locators/services/pipEnvHelper'; import { EnvironmentType } from '../../../pythonEnvironments/info'; import { IWorkspaceService } from '../../application/types'; +import { inDiscoveryExperiment } from '../../experiments/helpers'; import { IFileSystem } from '../../platform/types'; -import { IToolExecutionPath, ToolExecutionPath } from '../../types'; -import { ITerminalActivationCommandProvider, TerminalShellType } from '../types'; +import { IExperimentService, IToolExecutionPath, ToolExecutionPath } from '../../types'; +import { ITerminalActivationCommandProvider } from '../types'; @injectable() export class PipEnvActivationCommandProvider implements ITerminalActivationCommandProvider { @@ -22,37 +24,41 @@ export class PipEnvActivationCommandProvider implements ITerminalActivationComma private readonly pipEnvExecution: IToolExecutionPath, @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, @inject(IFileSystem) private readonly fs: IFileSystem, + @inject(IExperimentService) private readonly experimentService: IExperimentService, ) {} - public isShellSupported(_targetShell: TerminalShellType): boolean { + // eslint-disable-next-line class-methods-use-this + public isShellSupported(): boolean { return false; } - public async getActivationCommands(resource: Uri | undefined, _: TerminalShellType): Promise { + public async getActivationCommands(resource: Uri | undefined): Promise { const interpreter = await this.interpreterService.getActiveInterpreter(resource); if (!interpreter || interpreter.envType !== EnvironmentType.Pipenv) { - return; + return undefined; } // Activate using `pipenv shell` only if the current folder relates pipenv environment. const workspaceFolder = resource ? this.workspaceService.getWorkspaceFolder(resource) : undefined; - if ( - workspaceFolder && - interpreter.pipEnvWorkspaceFolder && - !this.fs.arePathsSame(workspaceFolder.uri.fsPath, interpreter.pipEnvWorkspaceFolder) - ) { - return; + if (workspaceFolder) { + if (await inDiscoveryExperiment(this.experimentService)) { + if (!(await isPipenvEnvironmentRelatedToFolder(interpreter.path, workspaceFolder?.uri.fsPath))) { + return undefined; + } + } else if ( + interpreter.pipEnvWorkspaceFolder && + !this.fs.arePathsSame(workspaceFolder.uri.fsPath, interpreter.pipEnvWorkspaceFolder) + ) { + return undefined; + } } const execName = this.pipEnvExecution.executable; return [`${execName.fileToCommandArgument()} shell`]; } - public async getActivationCommandsForInterpreter( - pythonPath: string, - _targetShell: TerminalShellType, - ): Promise { + public async getActivationCommandsForInterpreter(pythonPath: string): Promise { const interpreter = await this.interpreterService.getInterpreterDetails(pythonPath); if (!interpreter || interpreter.envType !== EnvironmentType.Pipenv) { - return; + return undefined; } const execName = this.pipEnvExecution.executable; diff --git a/src/client/pythonEnvironments/legacyIOC.ts b/src/client/pythonEnvironments/legacyIOC.ts index 3c1239b1ff32..e1f09632e33c 100644 --- a/src/client/pythonEnvironments/legacyIOC.ts +++ b/src/client/pythonEnvironments/legacyIOC.ts @@ -85,7 +85,7 @@ const convertedKinds = new Map( ); function convertEnvInfo(info: PythonEnvInfo): PythonEnvironment { - const { name, location, executable, arch, kind, searchLocation, version, distro } = info; + const { name, location, executable, arch, kind, version, distro } = info; const { filename, sysPrefix } = executable; const env: PythonEnvironment = { sysPrefix, @@ -102,12 +102,6 @@ function convertEnvInfo(info: PythonEnvInfo): PythonEnvironment { } // Otherwise it stays Unknown. - if (searchLocation !== undefined) { - if (kind === PythonEnvKind.Pipenv) { - env.pipEnvWorkspaceFolder = searchLocation.fsPath; - } - } - if (version !== undefined) { const { release, sysVersion } = version; if (release === undefined) { diff --git a/src/test/common/terminals/environmentActivationProviders/pipEnvActivationProvider.unit.test.ts b/src/test/common/terminals/environmentActivationProviders/pipEnvActivationProvider.unit.test.ts index 1e0e5d506043..89bc04005eab 100644 --- a/src/test/common/terminals/environmentActivationProviders/pipEnvActivationProvider.unit.test.ts +++ b/src/test/common/terminals/environmentActivationProviders/pipEnvActivationProvider.unit.test.ts @@ -9,11 +9,12 @@ import * as TypeMoq from 'typemoq'; import { Uri } from 'vscode'; import { IWorkspaceService } from '../../../../client/common/application/types'; import { WorkspaceService } from '../../../../client/common/application/workspace'; +import { DiscoveryVariants } from '../../../../client/common/experiments/groups'; import { FileSystem } from '../../../../client/common/platform/fileSystem'; import { IFileSystem } from '../../../../client/common/platform/types'; import { PipEnvActivationCommandProvider } from '../../../../client/common/terminal/environmentActivationProviders/pipEnvActivationProvider'; import { ITerminalActivationCommandProvider, TerminalShellType } from '../../../../client/common/terminal/types'; -import { IToolExecutionPath } from '../../../../client/common/types'; +import { IExperimentService, IToolExecutionPath } from '../../../../client/common/types'; import { getNamesAndValues } from '../../../../client/common/utils/enum'; import { IInterpreterService } from '../../../../client/interpreter/contracts'; import { InterpreterService } from '../../../../client/interpreter/interpreterService'; @@ -28,17 +29,22 @@ suite('Terminals Activation - Pipenv', () => { let pipEnvExecution: TypeMoq.IMock; let workspaceService: IWorkspaceService; let fs: IFileSystem; + let experimentService: IExperimentService; setup(() => { interpreterService = mock(InterpreterService); fs = mock(FileSystem); workspaceService = mock(WorkspaceService); interpreterService = mock(InterpreterService); + experimentService = mock(); + when(experimentService.inExperiment(DiscoveryVariants.discoverWithFileWatching)).thenResolve(false); + when(experimentService.inExperiment(DiscoveryVariants.discoveryWithoutFileWatching)).thenResolve(false); pipEnvExecution = TypeMoq.Mock.ofType(); activationProvider = new PipEnvActivationCommandProvider( instance(interpreterService), pipEnvExecution.object, instance(workspaceService), instance(fs), + instance(experimentService), ); pipEnvExecution.setup((p) => p.executable).returns(() => pipenvExecFile); @@ -60,6 +66,7 @@ suite('Terminals Activation - Pipenv', () => { for (const interpreterType of nonPipInterpreterTypes) { when(interpreterService.getActiveInterpreter(resource)).thenResolve({ type: interpreterType, + // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any); for (const shell of getNamesAndValues(TerminalShellType)) { @@ -72,6 +79,7 @@ suite('Terminals Activation - Pipenv', () => { test('pipenv shell is returned for pipenv interpeter', async () => { when(interpreterService.getActiveInterpreter(resource)).thenResolve({ envType: EnvironmentType.Pipenv, + // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any); for (const shell of getNamesAndValues(TerminalShellType)) { @@ -84,6 +92,7 @@ suite('Terminals Activation - Pipenv', () => { pipenvExecFile = 'my pipenv'; when(interpreterService.getActiveInterpreter(resource)).thenResolve({ envType: EnvironmentType.Pipenv, + // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any); for (const shell of getNamesAndValues(TerminalShellType)) { From b47cac4d9943d24cc407ec58f6ef4e3f811deedf Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 9 Feb 2021 14:04:51 -0700 Subject: [PATCH 05/14] In PythonEnvsReducer.resolveEnv(), always fall back to the wrapped locator. (#15350) fixes #15118 --- .../locators/composite/environmentsReducer.ts | 6 ++++-- .../composite/environmentsReducer.unit.test.ts | 18 +++++++++++------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/client/pythonEnvironments/base/locators/composite/environmentsReducer.ts b/src/client/pythonEnvironments/base/locators/composite/environmentsReducer.ts index 8bbb91a5b8e7..af7495490986 100644 --- a/src/client/pythonEnvironments/base/locators/composite/environmentsReducer.ts +++ b/src/client/pythonEnvironments/base/locators/composite/environmentsReducer.ts @@ -22,9 +22,11 @@ export class PythonEnvsReducer implements ILocator { public async resolveEnv(env: string | PythonEnvInfo): Promise { const environments = await getEnvs(this.iterEnvs()); - const environment = environments.find((e) => areSameEnv(e, env)); + let environment: string | PythonEnvInfo | undefined = environments.find((e) => areSameEnv(e, env)); if (!environment) { - return undefined; + // It isn't one we've reduced, but fall back + // to the wrapped locator anyway. + environment = env; } return this.parentLocator.resolveEnv(environment); } diff --git a/src/test/pythonEnvironments/base/locators/composite/environmentsReducer.unit.test.ts b/src/test/pythonEnvironments/base/locators/composite/environmentsReducer.unit.test.ts index 2f1e0310e8b8..3865572ac780 100644 --- a/src/test/pythonEnvironments/base/locators/composite/environmentsReducer.unit.test.ts +++ b/src/test/pythonEnvironments/base/locators/composite/environmentsReducer.unit.test.ts @@ -248,7 +248,7 @@ suite('Python envs locator - Environments Reducer', () => { assert.deepEqual(resolved, expected); }); - test("If the reducer isn't able to resolve environment, return undefined", async () => { + test("If the reducer isn't able to resolve environment, fall back to the wrapped locator", async () => { const env1 = createNamedEnv('env', '3.8', PythonEnvKind.Conda, path.join('path', 'to', 'exec')); const env2 = createNamedEnv('env2', '2.7', PythonEnvKind.System, path.join('path', 'to', 'exec3')); const env3 = createNamedEnv('env', '3.8.1b1', PythonEnvKind.System, path.join('path', 'to', 'exec')); @@ -257,20 +257,24 @@ suite('Python envs locator - Environments Reducer', () => { const env6 = createNamedEnv('env', '3.8.1', PythonEnvKind.Unknown, path.join('path', 'to', 'exec')); const environmentsToBeIterated = [env1, env2, env3, env4, env5, env6]; // env1 env3 env6 are same - const env136 = createNamedEnv('env', '3.8.1b1', PythonEnvKind.Conda, path.join('path', 'to', 'exec')); + const filename1 = path.join('resolved', 'path', 'to', 'execNeverSeenBefore'); + const filename2 = path.join('resolved', 'path', 'to', 'execAlsoNeverSeenBefore'); + const expected = createNamedEnv('resolvedEnv', '3.8.1', PythonEnvKind.Conda, filename1); const parentLocator = new SimpleLocator(environmentsToBeIterated, { resolve: async (e: PythonEnvInfo) => { - if (isEqual(e, env136)) { - return createNamedEnv('resolvedEnv', '3.8.1', PythonEnvKind.Conda, 'resolved/path/to/exec'); + if (e.executable.filename === expected.executable.filename) { + return expected; } - throw new Error('Incorrect environment sent to the resolve'); + return undefined; }, }); const reducer = new PythonEnvsReducer(parentLocator); - const expected = await reducer.resolveEnv(path.join('path', 'to', 'execNeverSeenBefore')); + const resolved1 = await reducer.resolveEnv(filename1); + const resolved2 = await reducer.resolveEnv(filename2); - assert.deepEqual(expected, undefined); + assert.deepEqual(resolved1, expected); + assert.equal(resolved2, undefined); }); }); }); From 9622cd0c9cff99818dfc7be2e58c62e51d263c2f Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 10 Feb 2021 16:14:04 -0800 Subject: [PATCH 06/14] Fix issue with missing interpreter info for some cases (#15376) * Use child process apis directly. * Use raw API in process service * Handle process cleanup * Address sonar * Refactor process service by pulling the raw process APIs out of the class * Address comments --- src/client/common/process/proc.ts | 210 +-------------- src/client/common/process/rawProcessApis.ts | 251 ++++++++++++++++++ .../base/info/interpreter.ts | 6 +- .../common/externalDependencies.ts | 40 ++- .../discovery/locators/services/conda.ts | 14 +- .../info/environmentInfoService.ts | 25 +- 6 files changed, 324 insertions(+), 222 deletions(-) create mode 100644 src/client/common/process/rawProcessApis.ts diff --git a/src/client/common/process/proc.ts b/src/client/common/process/proc.ts index 667eafa5d1ee..61e06ed811d1 100644 --- a/src/client/common/process/proc.ts +++ b/src/client/common/process/proc.ts @@ -1,23 +1,17 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { exec, execSync, spawn } from 'child_process'; import { EventEmitter } from 'events'; -import { Observable } from 'rxjs/Observable'; -import { Readable } from 'stream'; import { IDisposable } from '../types'; -import { createDeferred } from '../utils/async'; import { EnvironmentVariables } from '../variables/types'; -import { DEFAULT_ENCODING } from './constants'; +import { execObservable, killPid, plainExec, shellExec } from './rawProcessApis'; import { ExecutionResult, IBufferDecoder, IProcessService, ObservableExecutionResult, - Output, ShellOptions, SpawnOptions, - StdErrError, } from './types'; export class ProcessService extends EventEmitter implements IProcessService { @@ -37,16 +31,7 @@ export class ProcessService extends EventEmitter implements IProcessService { } public static kill(pid: number): void { - try { - if (process.platform === 'win32') { - // Windows doesn't support SIGTERM, so execute taskkill to kill the process - execSync(`taskkill /pid ${pid} /T /F`); // NOSONAR - } else { - process.kill(pid); - } - } catch { - // Ignore. - } + killPid(pid); } public dispose(): void { @@ -61,199 +46,18 @@ export class ProcessService extends EventEmitter implements IProcessService { } public execObservable(file: string, args: string[], options: SpawnOptions = {}): ObservableExecutionResult { - const spawnOptions = this.getDefaultOptions(options); - const encoding = spawnOptions.encoding ? spawnOptions.encoding : 'utf8'; - const proc = spawn(file, args, spawnOptions); - let procExited = false; - const disposable: IDisposable = { - dispose() { - if (proc && !proc.killed && !procExited) { - ProcessService.kill(proc.pid); - } - if (proc) { - proc.unref(); - } - }, - }; - this.processesToKill.add(disposable); - - const output = new Observable>((subscriber) => { - const disposables: IDisposable[] = []; - - // eslint-disable-next-line @typescript-eslint/ban-types - const on = (ee: Readable | null, name: string, fn: Function) => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ee?.on(name, fn as any); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - disposables.push({ dispose: () => ee?.removeListener(name, fn as any) as any }); - }; - - if (options.token) { - disposables.push( - options.token.onCancellationRequested(() => { - if (!procExited && !proc.killed) { - proc.kill(); - procExited = true; - } - }), - ); - } - - const sendOutput = (source: 'stdout' | 'stderr', data: Buffer) => { - const out = this.decoder.decode([data], encoding); - if (source === 'stderr' && options.throwOnStdErr) { - subscriber.error(new StdErrError(out)); - } else { - subscriber.next({ source, out }); - } - }; - - on(proc.stdout, 'data', (data: Buffer) => sendOutput('stdout', data)); - on(proc.stderr, 'data', (data: Buffer) => sendOutput('stderr', data)); - - proc.once('close', () => { - procExited = true; - subscriber.complete(); - disposables.forEach((d) => d.dispose()); - }); - proc.once('exit', () => { - procExited = true; - subscriber.complete(); - disposables.forEach((d) => d.dispose()); - }); - proc.once('error', (ex) => { - procExited = true; - subscriber.error(ex); - disposables.forEach((d) => d.dispose()); - }); - }); - + const result = execObservable(file, args, options, this.decoder, this.env, this.processesToKill); this.emit('exec', file, args, options); - - return { - proc, - out: output, - dispose: disposable.dispose, - }; + return result; } public exec(file: string, args: string[], options: SpawnOptions = {}): Promise> { - const spawnOptions = this.getDefaultOptions(options); - const encoding = spawnOptions.encoding ? spawnOptions.encoding : 'utf8'; - const proc = spawn(file, args, spawnOptions); - const deferred = createDeferred>(); - const disposable: IDisposable = { - dispose: () => { - if (!proc.killed && !deferred.completed) { - proc.kill(); - } - }, - }; - this.processesToKill.add(disposable); - const disposables: IDisposable[] = []; - - // eslint-disable-next-line @typescript-eslint/ban-types - const on = (ee: Readable | null, name: string, fn: Function) => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ee?.on(name, fn as any); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - disposables.push({ dispose: () => ee?.removeListener(name, fn as any) as any }); - }; - - if (options.token) { - disposables.push(options.token.onCancellationRequested(disposable.dispose)); - } - - const stdoutBuffers: Buffer[] = []; - on(proc.stdout, 'data', (data: Buffer) => stdoutBuffers.push(data)); - const stderrBuffers: Buffer[] = []; - on(proc.stderr, 'data', (data: Buffer) => { - if (options.mergeStdOutErr) { - stdoutBuffers.push(data); - stderrBuffers.push(data); - } else { - stderrBuffers.push(data); - } - }); - - proc.once('close', () => { - if (deferred.completed) { - return; - } - const stderr: string | undefined = - stderrBuffers.length === 0 ? undefined : this.decoder.decode(stderrBuffers, encoding); - if (stderr && stderr.length > 0 && options.throwOnStdErr) { - deferred.reject(new StdErrError(stderr)); - } else { - const stdout = this.decoder.decode(stdoutBuffers, encoding); - deferred.resolve({ stdout, stderr }); - } - disposables.forEach((d) => d.dispose()); - }); - proc.once('error', (ex) => { - deferred.reject(ex); - disposables.forEach((d) => d.dispose()); - }); - + const promise = plainExec(file, args, options, this.decoder, this.env, this.processesToKill); this.emit('exec', file, args, options); - - return deferred.promise; + return promise; } public shellExec(command: string, options: ShellOptions = {}): Promise> { - const shellOptions = this.getDefaultOptions(options); - return new Promise((resolve, reject) => { - const proc = exec(command, shellOptions, (e, stdout, stderr) => { - if (e && e !== null) { - reject(e); - } else if (shellOptions.throwOnStdErr && stderr && stderr.length) { - reject(new Error(stderr)); - } else { - // Make sure stderr is undefined if we actually had none. This is checked - // elsewhere because that's how exec behaves. - resolve({ stderr: stderr && stderr.length > 0 ? stderr : undefined, stdout }); - } - }); // NOSONAR - const disposable: IDisposable = { - dispose: () => { - if (!proc.killed) { - proc.kill(); - } - }, - }; - this.processesToKill.add(disposable); - }); - } - - private getDefaultOptions(options: T): T { - const defaultOptions = { ...options }; - const execOptions = defaultOptions as SpawnOptions; - if (execOptions) { - execOptions.encoding = - typeof execOptions.encoding === 'string' && execOptions.encoding.length > 0 - ? execOptions.encoding - : DEFAULT_ENCODING; - const { encoding } = execOptions; - delete execOptions.encoding; - execOptions.encoding = encoding; - } - if (!defaultOptions.env || Object.keys(defaultOptions.env).length === 0) { - const env = this.env ? this.env : process.env; - defaultOptions.env = { ...env }; - } else { - defaultOptions.env = { ...defaultOptions.env }; - } - - if (execOptions && execOptions.extraVariables) { - defaultOptions.env = { ...defaultOptions.env, ...execOptions.extraVariables }; - } - - // Always ensure we have unbuffered output. - defaultOptions.env.PYTHONUNBUFFERED = '1'; - if (!defaultOptions.env.PYTHONIOENCODING) { - defaultOptions.env.PYTHONIOENCODING = 'utf-8'; - } - - return defaultOptions; + return shellExec(command, options, this.env, this.processesToKill); } } diff --git a/src/client/common/process/rawProcessApis.ts b/src/client/common/process/rawProcessApis.ts new file mode 100644 index 000000000000..9cb0c40b9bca --- /dev/null +++ b/src/client/common/process/rawProcessApis.ts @@ -0,0 +1,251 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { exec, execSync, spawn } from 'child_process'; +import { Readable } from 'stream'; +import { Observable } from 'rxjs/Observable'; +import { IDisposable } from '../types'; +import { createDeferred } from '../utils/async'; +import { EnvironmentVariables } from '../variables/types'; +import { DEFAULT_ENCODING } from './constants'; +import { + ExecutionResult, + IBufferDecoder, + ObservableExecutionResult, + Output, + ShellOptions, + SpawnOptions, + StdErrError, +} from './types'; + +export function getDefaultOptions( + options: T, + defaultEnv?: EnvironmentVariables, +): T { + const defaultOptions = { ...options }; + const execOptions = defaultOptions as SpawnOptions; + if (execOptions) { + execOptions.encoding = + typeof execOptions.encoding === 'string' && execOptions.encoding.length > 0 + ? execOptions.encoding + : DEFAULT_ENCODING; + const { encoding } = execOptions; + delete execOptions.encoding; + execOptions.encoding = encoding; + } + if (!defaultOptions.env || Object.keys(defaultOptions.env).length === 0) { + const env = defaultEnv || process.env; + defaultOptions.env = { ...env }; + } else { + defaultOptions.env = { ...defaultOptions.env }; + } + + if (execOptions && execOptions.extraVariables) { + defaultOptions.env = { ...defaultOptions.env, ...execOptions.extraVariables }; + } + + // Always ensure we have unbuffered output. + defaultOptions.env.PYTHONUNBUFFERED = '1'; + if (!defaultOptions.env.PYTHONIOENCODING) { + defaultOptions.env.PYTHONIOENCODING = 'utf-8'; + } + + return defaultOptions; +} + +export function shellExec( + command: string, + options: ShellOptions = {}, + defaultEnv?: EnvironmentVariables, + disposables?: Set, +): Promise> { + const shellOptions = getDefaultOptions(options, defaultEnv); + return new Promise((resolve, reject) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const callback = (e: any, stdout: any, stderr: any) => { + if (e && e !== null) { + reject(e); + } else if (shellOptions.throwOnStdErr && stderr && stderr.length) { + reject(new Error(stderr)); + } else { + // Make sure stderr is undefined if we actually had none. This is checked + // elsewhere because that's how exec behaves. + resolve({ stderr: stderr && stderr.length > 0 ? stderr : undefined, stdout }); + } + }; + const proc = exec(command, shellOptions, callback); // NOSONAR + const disposable: IDisposable = { + dispose: () => { + if (!proc.killed) { + proc.kill(); + } + }, + }; + if (disposables) { + disposables.add(disposable); + } + }); +} + +export function plainExec( + file: string, + args: string[], + options: SpawnOptions = {}, + decoder?: IBufferDecoder, + defaultEnv?: EnvironmentVariables, + disposables?: Set, +): Promise> { + const spawnOptions = getDefaultOptions(options, defaultEnv); + const encoding = spawnOptions.encoding ? spawnOptions.encoding : 'utf8'; + const proc = spawn(file, args, spawnOptions); + const deferred = createDeferred>(); + const disposable: IDisposable = { + dispose: () => { + if (!proc.killed && !deferred.completed) { + proc.kill(); + } + }, + }; + disposables?.add(disposable); + const internalDisposables: IDisposable[] = []; + + // eslint-disable-next-line @typescript-eslint/ban-types + const on = (ee: Readable | null, name: string, fn: Function) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + ee?.on(name, fn as any); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + internalDisposables.push({ dispose: () => ee?.removeListener(name, fn as any) as any }); + }; + + if (options.token) { + internalDisposables.push(options.token.onCancellationRequested(disposable.dispose)); + } + + const stdoutBuffers: Buffer[] = []; + on(proc.stdout, 'data', (data: Buffer) => stdoutBuffers.push(data)); + const stderrBuffers: Buffer[] = []; + on(proc.stderr, 'data', (data: Buffer) => { + if (options.mergeStdOutErr) { + stdoutBuffers.push(data); + stderrBuffers.push(data); + } else { + stderrBuffers.push(data); + } + }); + + proc.once('close', () => { + if (deferred.completed) { + return; + } + const stderr: string | undefined = + stderrBuffers.length === 0 ? undefined : decoder?.decode(stderrBuffers, encoding); + if (stderr && stderr.length > 0 && options.throwOnStdErr) { + deferred.reject(new StdErrError(stderr)); + } else { + const stdout = decoder ? decoder.decode(stdoutBuffers, encoding) : ''; + deferred.resolve({ stdout, stderr }); + } + internalDisposables.forEach((d) => d.dispose()); + }); + proc.once('error', (ex) => { + deferred.reject(ex); + internalDisposables.forEach((d) => d.dispose()); + }); + + return deferred.promise; +} + +export function execObservable( + file: string, + args: string[], + options: SpawnOptions = {}, + decoder?: IBufferDecoder, + defaultEnv?: EnvironmentVariables, + disposables?: Set, +): ObservableExecutionResult { + const spawnOptions = getDefaultOptions(options, defaultEnv); + const encoding = spawnOptions.encoding ? spawnOptions.encoding : 'utf8'; + const proc = spawn(file, args, spawnOptions); + let procExited = false; + const disposable: IDisposable = { + dispose() { + if (proc && !proc.killed && !procExited) { + killPid(proc.pid); + } + if (proc) { + proc.unref(); + } + }, + }; + disposables?.add(disposable); + + const output = new Observable>((subscriber) => { + const internalDisposables: IDisposable[] = []; + + // eslint-disable-next-line @typescript-eslint/ban-types + const on = (ee: Readable | null, name: string, fn: Function) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + ee?.on(name, fn as any); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + internalDisposables.push({ dispose: () => ee?.removeListener(name, fn as any) as any }); + }; + + if (options.token) { + internalDisposables.push( + options.token.onCancellationRequested(() => { + if (!procExited && !proc.killed) { + proc.kill(); + procExited = true; + } + }), + ); + } + + const sendOutput = (source: 'stdout' | 'stderr', data: Buffer) => { + const out = decoder ? decoder.decode([data], encoding) : ''; + if (source === 'stderr' && options.throwOnStdErr) { + subscriber.error(new StdErrError(out)); + } else { + subscriber.next({ source, out }); + } + }; + + on(proc.stdout, 'data', (data: Buffer) => sendOutput('stdout', data)); + on(proc.stderr, 'data', (data: Buffer) => sendOutput('stderr', data)); + + proc.once('close', () => { + procExited = true; + subscriber.complete(); + internalDisposables.forEach((d) => d.dispose()); + }); + proc.once('exit', () => { + procExited = true; + subscriber.complete(); + internalDisposables.forEach((d) => d.dispose()); + }); + proc.once('error', (ex) => { + procExited = true; + subscriber.error(ex); + internalDisposables.forEach((d) => d.dispose()); + }); + }); + + return { + proc, + out: output, + dispose: disposable.dispose, + }; +} + +export function killPid(pid: number): void { + try { + if (process.platform === 'win32') { + // Windows doesn't support SIGTERM, so execute taskkill to kill the process + execSync(`taskkill /pid ${pid} /T /F`); // NOSONAR + } else { + process.kill(pid); + } + } catch { + // Ignore. + } +} diff --git a/src/client/pythonEnvironments/base/info/interpreter.ts b/src/client/pythonEnvironments/base/info/interpreter.ts index 8c436b6329ae..4e639fbaa4a4 100644 --- a/src/client/pythonEnvironments/base/info/interpreter.ts +++ b/src/client/pythonEnvironments/base/info/interpreter.ts @@ -6,6 +6,7 @@ import { interpreterInfo as getInterpreterInfoCommand, InterpreterInfoJson, } from '../../../common/process/internal/scripts'; +import { IDisposable } from '../../../common/types'; import { Architecture } from '../../../common/utils/platform'; import { copyPythonExecInfo, PythonExecInfo } from '../../exec'; import { parseVersion } from './pythonVersion'; @@ -45,7 +46,7 @@ type ShellExecResult = { stdout: string; stderr?: string; }; -type ShellExecFunc = (command: string, timeout: number) => Promise; +type ShellExecFunc = (command: string, timeout: number, disposables?: Set) => Promise; type Logger = { info(msg: string): void; @@ -64,6 +65,7 @@ export async function getInterpreterInfo( python: PythonExecInfo, shellExec: ShellExecFunc, logger?: Logger, + disposables?: Set, ): Promise { const [args, parse] = getInterpreterInfoCommand(); const info = copyPythonExecInfo(python, args); @@ -78,7 +80,7 @@ export async function getInterpreterInfo( // See these two bugs: // https://github.com/microsoft/vscode-python/issues/7569 // https://github.com/microsoft/vscode-python/issues/7760 - const result = await shellExec(quoted, 15000); + const result = await shellExec(quoted, 15000, disposables); if (result.stderr) { if (logger) { logger.error(`Failed to parse interpreter information for ${argv} stderr: ${result.stderr}`); diff --git a/src/client/pythonEnvironments/common/externalDependencies.ts b/src/client/pythonEnvironments/common/externalDependencies.ts index 219cdecac503..062a8923245c 100644 --- a/src/client/pythonEnvironments/common/externalDependencies.ts +++ b/src/client/pythonEnvironments/common/externalDependencies.ts @@ -4,13 +4,14 @@ import * as fsapi from 'fs-extra'; import * as path from 'path'; import * as vscode from 'vscode'; -import { ExecutionResult, IProcessServiceFactory, SpawnOptions } from '../../common/process/types'; -import { IExperimentService } from '../../common/types'; +import { ExecutionResult, SpawnOptions } from '../../common/process/types'; +import { IExperimentService, IDisposable } from '../../common/types'; import { chain, iterable } from '../../common/utils/async'; import { normalizeFilename } from '../../common/utils/filesystem'; import { getOSType, OSType } from '../../common/utils/platform'; -import { IDisposable } from '../../common/utils/resourceLifecycle'; import { IServiceContainer } from '../../ioc/types'; +import { plainExec, shellExec } from '../../common/process/rawProcessApis'; +import { BufferDecoder } from '../../common/process/decoder'; let internalServiceContainer: IServiceContainer; export function initializeExternalDependencies(serviceContainer: IServiceContainer): void { @@ -19,18 +20,31 @@ export function initializeExternalDependencies(serviceContainer: IServiceContain // processes -function getProcessFactory(): IProcessServiceFactory { - return internalServiceContainer.get(IProcessServiceFactory); -} - -export async function shellExecute(command: string, timeout: number): Promise> { - const proc = await getProcessFactory().create(); - return proc.shellExec(command, { timeout }); +/** + * Specialized version of the more generic shellExecute function to use only in + * cases where we don't need to pass custom environment variables read from env + * files or execution options. + */ +export async function shellExecute( + command: string, + timeout: number, + disposables?: Set, +): Promise> { + return shellExec(command, { timeout }, undefined, disposables); } -export async function exec(file: string, args: string[], options: SpawnOptions = {}): Promise> { - const proc = await getProcessFactory().create(); - return proc.exec(file, args, options); +/** + * Specialized version of the more generic exec function to use only in + * cases where we don't need to pass custom environment variables read from + * env files. + */ +export async function exec( + file: string, + args: string[], + options: SpawnOptions = {}, + disposables?: Set, +): Promise> { + return plainExec(file, args, options, new BufferDecoder(), undefined, disposables); } // filesystem diff --git a/src/client/pythonEnvironments/discovery/locators/services/conda.ts b/src/client/pythonEnvironments/discovery/locators/services/conda.ts index be23c549456d..537be256bd2f 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/conda.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/conda.ts @@ -9,6 +9,7 @@ import { parseVersion } from '../../../base/info/pythonVersion'; import { getRegistryInterpreters } from '../../../common/windowsUtils'; import { EnvironmentType, PythonEnvironment } from '../../../info'; +import { IDisposable } from '../../../../common/types'; export const AnacondaCompanyNames = ['Anaconda, Inc.', 'Continuum Analytics, Inc.']; @@ -326,8 +327,19 @@ export class Conda { * Corresponds to "conda info --json". */ public async getInfo(): Promise { - const result = await exec(this.command, ['info', '--json']); + const disposables = new Set(); + const result = await exec(this.command, ['info', '--json'], {}, disposables); traceVerbose(`conda info --json: ${result.stdout}`); + + // Ensure the process we started is cleaned up. + disposables.forEach((p) => { + try { + p.dispose(); + } catch { + // ignore. + } + }); + return JSON.parse(result.stdout); } diff --git a/src/client/pythonEnvironments/info/environmentInfoService.ts b/src/client/pythonEnvironments/info/environmentInfoService.ts index 09715b888b8f..675739245e58 100644 --- a/src/client/pythonEnvironments/info/environmentInfoService.ts +++ b/src/client/pythonEnvironments/info/environmentInfoService.ts @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +import { traceVerbose } from '../../common/logger'; +import { IDisposable } from '../../common/types'; import { createDeferred, Deferred } from '../../common/utils/async'; import { createRunningWorkerPool, IWorkerPool, QueuePosition } from '../../common/utils/workerPool'; import { getInterpreterInfo, InterpreterInformation } from '../base/info/interpreter'; @@ -21,9 +23,26 @@ export interface IEnvironmentInfoService { } async function buildEnvironmentInfo(interpreterPath: string): Promise { - const interpreterInfo = await getInterpreterInfo(buildPythonExecInfo(interpreterPath), shellExecute).catch( - () => undefined, - ); + const disposables = new Set(); + const interpreterInfo = await getInterpreterInfo( + buildPythonExecInfo(interpreterPath), + shellExecute, + undefined, + disposables, + ).catch((reason) => { + traceVerbose(reason); + return undefined; + }); + + // Ensure the process we started is cleaned up. + disposables.forEach((p) => { + try { + p.dispose(); + } catch { + // ignore. + } + }); + if (interpreterInfo === undefined || interpreterInfo.version === undefined) { return undefined; } From fda25050849de768cb6379380f84a813a56553ab Mon Sep 17 00:00:00 2001 From: Jim Griesmer Date: Thu, 11 Feb 2021 09:47:16 -0800 Subject: [PATCH 07/14] Add reference to CVE-2020-16977 fixed in Oct 2020. (#15381) --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 93d944df5799..be0cfc9fd710 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -653,6 +653,8 @@ part of! ([#14182](https://github.com/Microsoft/vscode-python/issues/14182)) 1. Fix exporting from the interactive window. ([#14210](https://github.com/Microsoft/vscode-python/issues/14210)) +1. Fix for CVE-2020-16977 + ([CVE-2020-16977](https://msrc.microsoft.com/update-guide/vulnerability/CVE-2020-16977)) ### Thanks From 3e56c969100ebc485297a1e35ca7282b212430fc Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 9 Feb 2021 11:01:15 -0800 Subject: [PATCH 08/14] Fix CI failing with the number of constructor arguments error (#15347) --- src/test/serviceRegistry.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/serviceRegistry.ts b/src/test/serviceRegistry.ts index 2e84c6355335..4764653c2272 100644 --- a/src/test/serviceRegistry.ts +++ b/src/test/serviceRegistry.ts @@ -175,7 +175,7 @@ export class IocContainer { private disposables: Disposable[] = []; constructor() { - const cont = new Container(); + const cont = new Container({ skipBaseClassChecks: true }); this.serviceManager = new ServiceManager(cont); this.serviceContainer = new ServiceContainer(cont); From 6db5c5eb0c6f2c82b6e1f947c722dde2cd80c4a8 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 9 Feb 2021 09:59:13 -0800 Subject: [PATCH 09/14] Fix Command 'workbench.action.closeAllEditors' timed out failure on CI (#15322) --- src/test/initialize.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/initialize.ts b/src/test/initialize.ts index f1542f9c3baa..cba7529693f8 100644 --- a/src/test/initialize.ts +++ b/src/test/initialize.ts @@ -86,7 +86,7 @@ async function closeWindowsInteral() { // Lets not waste too much time. const timer = setTimeout(() => { reject(new Error("Command 'workbench.action.closeAllEditors' timed out")); - }, 2000); + }, 15000); vscode.commands.executeCommand('workbench.action.closeAllEditors').then( () => { clearTimeout(timer); From d619de6ca23961b117ec04be81816023a20976bc Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 10 Feb 2021 16:25:09 -0800 Subject: [PATCH 10/14] Ensure environment variables provider can be created using standard classes (#15377) * Ensure environment variables provider can be created using standard classes * Fix unit tests --- .../variables/environmentVariablesProvider.ts | 17 +++++--- src/client/extensionActivation.ts | 2 - src/client/extensionInit.ts | 2 + .../envVarsProvider.multiroot.test.ts | 3 -- .../environmentVariablesProvider.unit.test.ts | 41 +++++++------------ 5 files changed, 29 insertions(+), 36 deletions(-) diff --git a/src/client/common/variables/environmentVariablesProvider.ts b/src/client/common/variables/environmentVariablesProvider.ts index 0a6b7a962f4a..20ddff4b9fc0 100644 --- a/src/client/common/variables/environmentVariablesProvider.ts +++ b/src/client/common/variables/environmentVariablesProvider.ts @@ -5,10 +5,12 @@ import { inject, injectable, optional } from 'inversify'; import { ConfigurationChangeEvent, Disposable, Event, EventEmitter, FileSystemWatcher, Uri } from 'vscode'; import { sendFileCreationTelemetry } from '../../telemetry/envFileTelemetry'; import { IWorkspaceService } from '../application/types'; +import { PythonSettings } from '../configSettings'; import { traceVerbose } from '../logger'; import { IPlatformService } from '../platform/types'; -import { IConfigurationService, ICurrentProcess, IDisposableRegistry } from '../types'; +import { ICurrentProcess, IDisposableRegistry } from '../types'; import { InMemoryCache } from '../utils/cacheUtils'; +import { SystemVariables } from './systemVariables'; import { EnvironmentVariables, IEnvironmentVariablesProvider, IEnvironmentVariablesService } from './types'; const CACHE_DURATION = 60 * 60 * 1000; @@ -25,7 +27,6 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid @inject(IDisposableRegistry) disposableRegistry: Disposable[], @inject(IPlatformService) private platformService: IPlatformService, @inject(IWorkspaceService) private workspaceService: IWorkspaceService, - @inject(IConfigurationService) private readonly configurationService: IConfigurationService, @inject(ICurrentProcess) private process: ICurrentProcess, @optional() private cacheDuration: number = CACHE_DURATION, ) { @@ -86,11 +87,17 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid return mergedVars; } public async getCustomEnvironmentVariables(resource?: Uri): Promise { - const settings = this.configurationService.getSettings(resource); + const systemVariables: SystemVariables = new SystemVariables( + undefined, + PythonSettings.getSettingsUriAndTarget(resource, this.workspaceService).uri?.fsPath, + this.workspaceService, + ); + const envFileSetting = this.workspaceService.getConfiguration('python', resource).get('envFile'); + const envFile = systemVariables.resolveAny(envFileSetting)!; const workspaceFolderUri = this.getWorkspaceFolderUri(resource); this.trackedWorkspaceFolders.add(workspaceFolderUri ? workspaceFolderUri.fsPath : ''); - this.createFileWatcher(settings.envFile, workspaceFolderUri); - return this.envVarsService.parseFile(settings.envFile, this.process.env); + this.createFileWatcher(envFile, workspaceFolderUri); + return this.envVarsService.parseFile(envFile, this.process.env); } public configurationChanged(e: ConfigurationChangeEvent) { this.trackedWorkspaceFolders.forEach((item) => { diff --git a/src/client/extensionActivation.ts b/src/client/extensionActivation.ts index b0d0686de25c..816a0d400333 100644 --- a/src/client/extensionActivation.ts +++ b/src/client/extensionActivation.ts @@ -30,7 +30,6 @@ import { IOutputChannel, } from './common/types'; import { noop } from './common/utils/misc'; -import { registerTypes as variableRegisterTypes } from './common/variables/serviceRegistry'; import { DebuggerTypeName } from './debugger/constants'; import { DebugSessionEventDispatcher } from './debugger/extension/hooks/eventHandlerDispatcher'; import { IDebugSessionEventHandlers } from './debugger/extension/hooks/types'; @@ -112,7 +111,6 @@ async function activateLegacy(ext: ExtensionState): Promise { const { enableProposedApi } = applicationEnv.packageJson; serviceManager.addSingletonInstance(UseProposedApi, enableProposedApi); // Feature specific registrations. - variableRegisterTypes(serviceManager); unitTestsRegisterTypes(serviceManager); lintersRegisterTypes(serviceManager); interpretersRegisterTypes(serviceManager); diff --git a/src/client/extensionInit.ts b/src/client/extensionInit.ts index 60caffbb1617..1f825d9210b6 100644 --- a/src/client/extensionInit.ts +++ b/src/client/extensionInit.ts @@ -17,6 +17,7 @@ import { IOutputChannel, WORKSPACE_MEMENTO, } from './common/types'; +import { registerTypes as variableRegisterTypes } from './common/variables/serviceRegistry'; import { OutputChannelNames } from './common/utils/localize'; import { ExtensionState } from './components'; import { ServiceContainer } from './ioc/container'; @@ -69,6 +70,7 @@ export function initializeStandard(ext: ExtensionState): void { const { serviceManager } = ext.legacyIOC; // Core registrations (non-feature specific). commonRegisterTypes(serviceManager); + variableRegisterTypes(serviceManager); platformRegisterTypes(serviceManager); processRegisterTypes(serviceManager); diff --git a/src/test/common/variables/envVarsProvider.multiroot.test.ts b/src/test/common/variables/envVarsProvider.multiroot.test.ts index 56509cd4761a..a62dff0cb8af 100644 --- a/src/test/common/variables/envVarsProvider.multiroot.test.ts +++ b/src/test/common/variables/envVarsProvider.multiroot.test.ts @@ -7,7 +7,6 @@ import * as path from 'path'; import { anything, instance, mock, when } from 'ts-mockito'; import { ConfigurationTarget, Disposable, Uri, workspace } from 'vscode'; import { WorkspaceService } from '../../../client/common/application/workspace'; -import { ConfigurationService } from '../../../client/common/configuration/service'; import { PlatformService } from '../../../client/common/platform/platformService'; import { IFileSystem } from '../../../client/common/platform/types'; import { IDisposableRegistry, IPathUtils } from '../../../client/common/types'; @@ -76,14 +75,12 @@ suite('Multiroot Environment Variables Provider', () => { const variablesService = new EnvironmentVariablesService(pathUtils, fs); const disposables = ioc.serviceContainer.get(IDisposableRegistry); ioc.serviceManager.addSingletonInstance(IInterpreterAutoSelectionService, new MockAutoSelectionService()); - const cfgService = new ConfigurationService(ioc.serviceContainer); const workspaceService = new WorkspaceService(); return new EnvironmentVariablesProvider( variablesService, disposables, new PlatformService(), workspaceService, - cfgService, mockProcess, ); } diff --git a/src/test/common/variables/environmentVariablesProvider.unit.test.ts b/src/test/common/variables/environmentVariablesProvider.unit.test.ts index 44cfabe35c62..f55d128e48fc 100644 --- a/src/test/common/variables/environmentVariablesProvider.unit.test.ts +++ b/src/test/common/variables/environmentVariablesProvider.unit.test.ts @@ -11,12 +11,10 @@ import * as typemoq from 'typemoq'; import { ConfigurationChangeEvent, FileSystemWatcher, Uri } from 'vscode'; import { IWorkspaceService } from '../../../client/common/application/types'; import { WorkspaceService } from '../../../client/common/application/workspace'; -import { PythonSettings } from '../../../client/common/configSettings'; -import { ConfigurationService } from '../../../client/common/configuration/service'; import { PlatformService } from '../../../client/common/platform/platformService'; import { IPlatformService } from '../../../client/common/platform/types'; import { CurrentProcess } from '../../../client/common/process/currentProcess'; -import { IConfigurationService, ICurrentProcess, IPythonSettings } from '../../../client/common/types'; +import { ICurrentProcess } from '../../../client/common/types'; import { sleep } from '../../../client/common/utils/async'; import { EnvironmentVariablesService } from '../../../client/common/variables/environment'; import { EnvironmentVariablesProvider } from '../../../client/common/variables/environmentVariablesProvider'; @@ -29,26 +27,27 @@ suite('Multiroot Environment Variables Provider', () => { let envVarsService: IEnvironmentVariablesService; let platform: IPlatformService; let workspace: IWorkspaceService; - let configuration: IConfigurationService; let currentProcess: ICurrentProcess; - let settings: IPythonSettings; + let envFile: string; setup(() => { + envFile = ''; envVarsService = mock(EnvironmentVariablesService); platform = mock(PlatformService); workspace = mock(WorkspaceService); - configuration = mock(ConfigurationService); currentProcess = mock(CurrentProcess); - settings = mock(PythonSettings); - when(configuration.getSettings(anything())).thenReturn(instance(settings)); when(workspace.onDidChangeConfiguration).thenReturn(noop as any); + when(workspace.getConfiguration('python', anything())).thenReturn({ + get: (settingName: string) => { + return settingName === 'envFile' ? envFile : ''; + }, + } as any); provider = new EnvironmentVariablesProvider( instance(envVarsService), [], instance(platform), instance(workspace), - instance(configuration), instance(currentProcess), ); @@ -198,12 +197,11 @@ suite('Multiroot Environment Variables Provider', () => { }); test(`Getting environment variables (without an envfile, without PATH in current env, without PYTHONPATH in current env) & ${workspaceTitle}`, async () => { - const envFile = path.join('a', 'b', 'env.file'); + envFile = path.join('a', 'b', 'env.file'); const workspaceFolder = workspaceUri ? { name: '', index: 0, uri: workspaceUri } : undefined; const currentProcEnv = { SOMETHING: 'wow' }; when(currentProcess.env).thenReturn(currentProcEnv); - when(settings.envFile).thenReturn(envFile); when(workspace.getWorkspaceFolder(workspaceUri)).thenReturn(workspaceFolder); when(envVarsService.parseFile(envFile, currentProcEnv)).thenResolve(undefined); when(platform.pathVariableName).thenReturn('PATH'); @@ -211,20 +209,18 @@ suite('Multiroot Environment Variables Provider', () => { const vars = await provider.getEnvironmentVariables(workspaceUri); verify(currentProcess.env).atLeast(1); - verify(settings.envFile).atLeast(1); verify(envVarsService.parseFile(envFile, currentProcEnv)).atLeast(1); verify(envVarsService.mergeVariables(deepEqual(currentProcEnv), deepEqual({}))).once(); verify(platform.pathVariableName).atLeast(1); assert.deepEqual(vars, {}); }); test(`Getting environment variables (with an envfile, without PATH in current env, without PYTHONPATH in current env) & ${workspaceTitle}`, async () => { - const envFile = path.join('a', 'b', 'env.file'); + envFile = path.join('a', 'b', 'env.file'); const workspaceFolder = workspaceUri ? { name: '', index: 0, uri: workspaceUri } : undefined; const currentProcEnv = { SOMETHING: 'wow' }; const envFileVars = { MY_FILE: '1234' }; when(currentProcess.env).thenReturn(currentProcEnv); - when(settings.envFile).thenReturn(envFile); when(workspace.getWorkspaceFolder(workspaceUri)).thenReturn(workspaceFolder); when(envVarsService.parseFile(envFile, currentProcEnv)).thenCall(async () => ({ ...envFileVars })); when(platform.pathVariableName).thenReturn('PATH'); @@ -232,20 +228,18 @@ suite('Multiroot Environment Variables Provider', () => { const vars = await provider.getEnvironmentVariables(workspaceUri); verify(currentProcess.env).atLeast(1); - verify(settings.envFile).atLeast(1); verify(envVarsService.parseFile(envFile, currentProcEnv)).atLeast(1); verify(envVarsService.mergeVariables(deepEqual(currentProcEnv), deepEqual(envFileVars))).once(); verify(platform.pathVariableName).atLeast(1); assert.deepEqual(vars, envFileVars); }); test(`Getting environment variables (with an envfile, with PATH in current env, with PYTHONPATH in current env) & ${workspaceTitle}`, async () => { - const envFile = path.join('a', 'b', 'env.file'); + envFile = path.join('a', 'b', 'env.file'); const workspaceFolder = workspaceUri ? { name: '', index: 0, uri: workspaceUri } : undefined; const currentProcEnv = { SOMETHING: 'wow', PATH: 'some path value', PYTHONPATH: 'some python path value' }; const envFileVars = { MY_FILE: '1234' }; when(currentProcess.env).thenReturn(currentProcEnv); - when(settings.envFile).thenReturn(envFile); when(workspace.getWorkspaceFolder(workspaceUri)).thenReturn(workspaceFolder); when(envVarsService.parseFile(envFile, currentProcEnv)).thenCall(async () => ({ ...envFileVars })); when(platform.pathVariableName).thenReturn('PATH'); @@ -253,7 +247,6 @@ suite('Multiroot Environment Variables Provider', () => { const vars = await provider.getEnvironmentVariables(workspaceUri); verify(currentProcess.env).atLeast(1); - verify(settings.envFile).atLeast(1); verify(envVarsService.parseFile(envFile, currentProcEnv)).atLeast(1); verify(envVarsService.mergeVariables(deepEqual(currentProcEnv), deepEqual(envFileVars))).once(); verify(envVarsService.appendPath(deepEqual(envFileVars), currentProcEnv.PATH)).once(); @@ -263,12 +256,11 @@ suite('Multiroot Environment Variables Provider', () => { }); test(`Getting environment variables which are already cached does not reinvoke the method ${workspaceTitle}`, async () => { - const envFile = path.join('a', 'b', 'env.file'); + envFile = path.join('a', 'b', 'env.file'); const workspaceFolder = workspaceUri ? { name: '', index: 0, uri: workspaceUri } : undefined; const currentProcEnv = { SOMETHING: 'wow' }; when(currentProcess.env).thenReturn(currentProcEnv); - when(settings.envFile).thenReturn(envFile); when(workspace.getWorkspaceFolder(workspaceUri)).thenReturn(workspaceFolder); when(envVarsService.parseFile(envFile, currentProcEnv)).thenResolve(undefined); when(platform.pathVariableName).thenReturn('PATH'); @@ -280,7 +272,7 @@ suite('Multiroot Environment Variables Provider', () => { await provider.getEnvironmentVariables(workspaceUri); // Verify that the contents of `_getEnvironmentVariables()` method are only invoked once - verify(configuration.getSettings(anything())).once(); + verify(workspace.getConfiguration('python', anything())).once(); assert.deepEqual(vars, {}); }); @@ -290,7 +282,6 @@ suite('Multiroot Environment Variables Provider', () => { const currentProcEnv = { SOMETHING: 'wow' }; when(currentProcess.env).thenReturn(currentProcEnv); - when(settings.envFile).thenReturn(envFile); when(workspace.getWorkspaceFolder(workspaceUri)).thenReturn(workspaceFolder); when(envVarsService.parseFile(envFile, currentProcEnv)).thenResolve(undefined); when(platform.pathVariableName).thenReturn('PATH'); @@ -300,7 +291,6 @@ suite('Multiroot Environment Variables Provider', () => { [], instance(platform), instance(workspace), - instance(configuration), instance(currentProcess), 100, ); @@ -312,14 +302,14 @@ suite('Multiroot Environment Variables Provider', () => { await provider.getEnvironmentVariables(workspaceUri); // Verify that the contents of `_getEnvironmentVariables()` method are invoked twice - verify(configuration.getSettings(anything())).twice(); + verify(workspace.getConfiguration('python', anything())).twice(); assert.deepEqual(vars, {}); }); test(`Environment variables are updated when env file changes ${workspaceTitle}`, async () => { const root = workspaceUri?.fsPath ?? ''; const sourceDir = path.join(root, 'a', 'b'); - const envFile = path.join(sourceDir, 'env.file'); + envFile = path.join(sourceDir, 'env.file'); const sourceFile = path.join(sourceDir, 'main.py'); const workspaceFolder = workspaceUri ? { name: '', index: 0, uri: workspaceUri } : undefined; @@ -339,7 +329,6 @@ suite('Multiroot Environment Variables Provider', () => { when(workspace.createFileSystemWatcher(envFile)).thenReturn(fileSystemWatcher.object); when(currentProcess.env).thenReturn(currentProcEnv); - when(settings.envFile).thenReturn(envFile); when(workspace.getWorkspaceFolder(anything())).thenReturn(workspaceFolder); when(envVarsService.parseFile(envFile, currentProcEnv)).thenCall(async () => ({ ...envFileVars })); when(platform.pathVariableName).thenReturn('PATH'); From 18c263728516cfae642dd483a113bc523f63149e Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 11 Feb 2021 00:11:18 -0800 Subject: [PATCH 11/14] Fix ESLint errors for environment variable provider (#15383) --- .eslintignore | 3 - .../variables/environmentVariablesProvider.ts | 17 ++++-- .../envVarsProvider.multiroot.test.ts | 4 +- .../environmentVariablesProvider.unit.test.ts | 59 +++++++++++++------ 4 files changed, 57 insertions(+), 26 deletions(-) diff --git a/.eslintignore b/.eslintignore index 9c9908ede444..806ca4428bef 100644 --- a/.eslintignore +++ b/.eslintignore @@ -209,8 +209,6 @@ src/test/common/featureDeprecationManager.unit.test.ts src/test/common/serviceRegistry.unit.test.ts src/test/common/extensions.unit.test.ts src/test/common/variables/envVarsService.unit.test.ts -src/test/common/variables/environmentVariablesProvider.unit.test.ts -src/test/common/variables/envVarsProvider.multiroot.test.ts src/test/common/nuget/azureBobStoreRepository.unit.test.ts src/test/common/helpers.test.ts src/test/common/application/commands/reloadCommand.unit.test.ts @@ -575,7 +573,6 @@ src/client/common/constants.ts src/client/common/variables/serviceRegistry.ts src/client/common/variables/environment.ts src/client/common/variables/types.ts -src/client/common/variables/environmentVariablesProvider.ts src/client/common/variables/sysTypes.ts src/client/common/variables/systemVariables.ts src/client/common/nuget/azureBlobStoreNugetRepository.ts diff --git a/src/client/common/variables/environmentVariablesProvider.ts b/src/client/common/variables/environmentVariablesProvider.ts index 20ddff4b9fc0..2736a2dd8856 100644 --- a/src/client/common/variables/environmentVariablesProvider.ts +++ b/src/client/common/variables/environmentVariablesProvider.ts @@ -17,9 +17,13 @@ const CACHE_DURATION = 60 * 60 * 1000; @injectable() export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvider, Disposable { public trackedWorkspaceFolders = new Set(); + private fileWatchers = new Map(); + private disposables: Disposable[] = []; + private changeEventEmitter: EventEmitter; + private readonly envVarCaches = new Map>(); constructor( @@ -40,7 +44,7 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid return this.changeEventEmitter.event; } - public dispose() { + public dispose(): void { this.changeEventEmitter.dispose(); this.fileWatchers.forEach((watcher) => { if (watcher) { @@ -70,6 +74,7 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid cache.data = { ...vars }; return vars; } + public async _getEnvironmentVariables(resource?: Uri): Promise { let mergedVars = await this.getCustomEnvironmentVariables(resource); if (!mergedVars) { @@ -86,6 +91,7 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid } return mergedVars; } + public async getCustomEnvironmentVariables(resource?: Uri): Promise { const systemVariables: SystemVariables = new SystemVariables( undefined, @@ -99,7 +105,8 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid this.createFileWatcher(envFile, workspaceFolderUri); return this.envVarsService.parseFile(envFile, this.process.env); } - public configurationChanged(e: ConfigurationChangeEvent) { + + public configurationChanged(e: ConfigurationChangeEvent): void { this.trackedWorkspaceFolders.forEach((item) => { const uri = item && item.length > 0 ? Uri.file(item) : undefined; if (e.affectsConfiguration('python.envFile', uri)) { @@ -107,7 +114,8 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid } }); } - public createFileWatcher(envFile: string, workspaceFolderUri?: Uri) { + + public createFileWatcher(envFile: string, workspaceFolderUri?: Uri): void { if (this.fileWatchers.has(envFile)) { return; } @@ -119,9 +127,10 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid this.disposables.push(envFileWatcher.onDidDelete(() => this.onEnvironmentFileChanged(workspaceFolderUri))); } } + private getWorkspaceFolderUri(resource?: Uri): Uri | undefined { if (!resource) { - return; + return undefined; } const workspaceFolder = this.workspaceService.getWorkspaceFolder(resource!); return workspaceFolder ? workspaceFolder.uri : undefined; diff --git a/src/test/common/variables/envVarsProvider.multiroot.test.ts b/src/test/common/variables/envVarsProvider.multiroot.test.ts index a62dff0cb8af..63a3dd8e411d 100644 --- a/src/test/common/variables/envVarsProvider.multiroot.test.ts +++ b/src/test/common/variables/envVarsProvider.multiroot.test.ts @@ -34,7 +34,7 @@ suite('Multiroot Environment Variables Provider', () => { const pathVariableName = getSearchPathEnvVarNames()[0]; suiteSetup(async function () { if (!IS_MULTI_ROOT_TEST) { - return this.skip(); + this.skip(); } await clearPythonPathInWorkspaceFolder(workspace4Path); await updateSetting('envFile', undefined, workspace4PyFile, ConfigurationTarget.WorkspaceFolder); @@ -178,7 +178,7 @@ suite('Multiroot Environment Variables Provider', () => { // this test is flaky on windows (likely the value of the path property // has incorrect path separator chars). Tracked by GH #4756 if (isOs(OSType.Windows)) { - return this.skip(); + this.skip(); } await updateSetting('envFile', '${workspaceRoot}/.env', workspace4PyFile, ConfigurationTarget.WorkspaceFolder); diff --git a/src/test/common/variables/environmentVariablesProvider.unit.test.ts b/src/test/common/variables/environmentVariablesProvider.unit.test.ts index f55d128e48fc..3ea7036e249d 100644 --- a/src/test/common/variables/environmentVariablesProvider.unit.test.ts +++ b/src/test/common/variables/environmentVariablesProvider.unit.test.ts @@ -1,3 +1,4 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. @@ -65,7 +66,9 @@ suite('Multiroot Environment Variables Provider', () => { provider.trackedWorkspaceFolders.add(workspaceFolder1Uri.fsPath); provider.trackedWorkspaceFolders.add(workspaceFolder2Uri.fsPath); - provider.onDidEnvironmentVariablesChange((uri) => (affectedWorkspace = uri)); + provider.onDidEnvironmentVariablesChange((uri) => { + affectedWorkspace = uri; + }); const changedEvent: ConfigurationChangeEvent = { affectsConfiguration(setting: string, uri?: Uri) { return setting === 'python.envFile' && uri!.fsPath === workspaceFolder1Uri.fsPath; @@ -82,9 +85,11 @@ suite('Multiroot Environment Variables Provider', () => { const workspaceFolderUri = Uri.file('workspace1'); provider.trackedWorkspaceFolders.add(workspaceFolderUri.fsPath); - provider.onDidEnvironmentVariablesChange((uri) => (affectedWorkspace = uri)); + provider.onDidEnvironmentVariablesChange((uri) => { + affectedWorkspace = uri; + }); const changedEvent: ConfigurationChangeEvent = { - affectsConfiguration(_setting: string, _uri?: Uri) { + affectsConfiguration() { return false; }, }; @@ -95,9 +100,11 @@ suite('Multiroot Environment Variables Provider', () => { }); test('Event is not fired when workspace is not tracked', () => { let affectedWorkspace: Uri | undefined; - provider.onDidEnvironmentVariablesChange((uri) => (affectedWorkspace = uri)); + provider.onDidEnvironmentVariablesChange((uri) => { + affectedWorkspace = uri; + }); const changedEvent: ConfigurationChangeEvent = { - affectsConfiguration(_setting: string, _uri?: Uri) { + affectsConfiguration() { return true; }, }; @@ -110,17 +117,22 @@ suite('Multiroot Environment Variables Provider', () => { const workspaceTitle = workspaceUri ? '(with a workspace)' : '(without a workspace)'; test(`Event is fired when the environment file is modified ${workspaceTitle}`, () => { let affectedWorkspace: Uri | undefined = Uri.file('dummy value'); - const envFile = path.join('a', 'b', 'env.file'); + envFile = path.join('a', 'b', 'env.file'); const fileSystemWatcher = typemoq.Mock.ofType(); + // eslint-disable-next-line @typescript-eslint/ban-types let onChangeHandler: undefined | ((resource?: Uri) => Function); fileSystemWatcher .setup((fs) => fs.onDidChange(typemoq.It.isAny())) - .callback((cb) => (onChangeHandler = cb)) + .callback((cb) => { + onChangeHandler = cb; + }) .verifiable(typemoq.Times.once()); when(workspace.createFileSystemWatcher(envFile)).thenReturn(fileSystemWatcher.object); - provider.onDidEnvironmentVariablesChange((uri) => (affectedWorkspace = uri)); + provider.onDidEnvironmentVariablesChange((uri) => { + affectedWorkspace = uri; + }); provider.createFileWatcher(envFile, workspaceUri); @@ -133,17 +145,22 @@ suite('Multiroot Environment Variables Provider', () => { }); test(`Event is fired when the environment file is deleted ${workspaceTitle}`, () => { let affectedWorkspace: Uri | undefined = Uri.file('dummy value'); - const envFile = path.join('a', 'b', 'env.file'); + envFile = path.join('a', 'b', 'env.file'); const fileSystemWatcher = typemoq.Mock.ofType(); + // eslint-disable-next-line @typescript-eslint/ban-types let onDeleted: undefined | ((resource?: Uri) => Function); fileSystemWatcher .setup((fs) => fs.onDidDelete(typemoq.It.isAny())) - .callback((cb) => (onDeleted = cb)) + .callback((cb) => { + onDeleted = cb; + }) .verifiable(typemoq.Times.once()); when(workspace.createFileSystemWatcher(envFile)).thenReturn(fileSystemWatcher.object); - provider.onDidEnvironmentVariablesChange((uri) => (affectedWorkspace = uri)); + provider.onDidEnvironmentVariablesChange((uri) => { + affectedWorkspace = uri; + }); provider.createFileWatcher(envFile, workspaceUri); @@ -156,17 +173,22 @@ suite('Multiroot Environment Variables Provider', () => { }); test(`Event is fired when the environment file is created ${workspaceTitle}`, () => { let affectedWorkspace: Uri | undefined = Uri.file('dummy value'); - const envFile = path.join('a', 'b', 'env.file'); + envFile = path.join('a', 'b', 'env.file'); const fileSystemWatcher = typemoq.Mock.ofType(); + // eslint-disable-next-line @typescript-eslint/ban-types let onCreated: undefined | ((resource?: Uri) => Function); fileSystemWatcher .setup((fs) => fs.onDidCreate(typemoq.It.isAny())) - .callback((cb) => (onCreated = cb)) + .callback((cb) => { + onCreated = cb; + }) .verifiable(typemoq.Times.once()); when(workspace.createFileSystemWatcher(envFile)).thenReturn(fileSystemWatcher.object); - provider.onDidEnvironmentVariablesChange((uri) => (affectedWorkspace = uri)); + provider.onDidEnvironmentVariablesChange((uri) => { + affectedWorkspace = uri; + }); provider.createFileWatcher(envFile, workspaceUri); @@ -178,7 +200,7 @@ suite('Multiroot Environment Variables Provider', () => { assert.equal(affectedWorkspace, workspaceUri); }); test(`File system watcher event handlers are added once ${workspaceTitle}`, () => { - const envFile = path.join('a', 'b', 'env.file'); + envFile = path.join('a', 'b', 'env.file'); const fileSystemWatcher = typemoq.Mock.ofType(); fileSystemWatcher.setup((fs) => fs.onDidChange(typemoq.It.isAny())).verifiable(typemoq.Times.once()); @@ -277,7 +299,7 @@ suite('Multiroot Environment Variables Provider', () => { }); test(`Cache result must be cleared when cache expires ${workspaceTitle}`, async () => { - const envFile = path.join('a', 'b', 'env.file'); + envFile = path.join('a', 'b', 'env.file'); const workspaceFolder = workspaceUri ? { name: '', index: 0, uri: workspaceUri } : undefined; const currentProcEnv = { SOMETHING: 'wow' }; @@ -319,12 +341,15 @@ suite('Multiroot Environment Variables Provider', () => { }; const envFileVars = { MY_FILE: '1234', PYTHONPATH: `./foo${path.delimiter}./bar` }; + // eslint-disable-next-line @typescript-eslint/ban-types let onChangeHandler: undefined | ((resource?: Uri) => Function); const fileSystemWatcher = typemoq.Mock.ofType(); fileSystemWatcher .setup((fs) => fs.onDidChange(typemoq.It.isAny())) - .callback((cb) => (onChangeHandler = cb)) + .callback((cb) => { + onChangeHandler = cb; + }) .verifiable(typemoq.Times.once()); when(workspace.createFileSystemWatcher(envFile)).thenReturn(fileSystemWatcher.object); From ca8cbdfc05042c7fda24578062ca5b8d98baea8a Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Tue, 9 Feb 2021 14:21:57 -0800 Subject: [PATCH 12/14] Fix problem with notebook apis not being used. (#15366) --- news/2 Fixes/15364.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/2 Fixes/15364.md diff --git a/news/2 Fixes/15364.md b/news/2 Fixes/15364.md new file mode 100644 index 000000000000..3d49c2ac9734 --- /dev/null +++ b/news/2 Fixes/15364.md @@ -0,0 +1 @@ +Allow support for using notebook APIs in the VS code stable build. From 55539535b1d8f6c171cfd5149202b1d20ebba6c8 Mon Sep 17 00:00:00 2001 From: Joyce Er Date: Thu, 11 Feb 2021 14:11:46 -0800 Subject: [PATCH 13/14] Show Python: Launch TensorBoard command in command palette (#15382) (#15386) * Always register Launch TensorBoard command * Put usage tracking behind experiment --- package.json | 3 +- .../tensorBoard/tensorBoardSessionProvider.ts | 43 +++++++------------ .../tensorBoard/tensorBoardUsageTracker.ts | 7 ++- .../tensorBoard/tensorBoardSession.test.ts | 5 --- .../tensorBoardUsageTracker.unit.test.ts | 13 +++++- 5 files changed, 35 insertions(+), 36 deletions(-) diff --git a/package.json b/package.json index 68e97e23b293..5e71bed1cd5a 100644 --- a/package.json +++ b/package.json @@ -488,8 +488,7 @@ }, { "command": "python.launchTensorBoard", - "category": "Python", - "when": "python.isInNativeTensorBoardExperiment" + "category": "Python" } ], "view/title": [ diff --git a/src/client/tensorBoard/tensorBoardSessionProvider.ts b/src/client/tensorBoard/tensorBoardSessionProvider.ts index 0ac4a7be3468..3067110f5b86 100644 --- a/src/client/tensorBoard/tensorBoardSessionProvider.ts +++ b/src/client/tensorBoard/tensorBoardSessionProvider.ts @@ -5,11 +5,9 @@ import { inject, injectable } from 'inversify'; import { IExtensionSingleActivationService } from '../activation/types'; import { IApplicationShell, ICommandManager, IWorkspaceService } from '../common/application/types'; import { Commands } from '../common/constants'; -import { ContextKey } from '../common/contextKey'; -import { NativeTensorBoard } from '../common/experiments/groups'; import { traceError, traceInfo } from '../common/logger'; import { IProcessServiceFactory } from '../common/process/types'; -import { IDisposableRegistry, IExperimentService, IInstaller } from '../common/types'; +import { IDisposableRegistry, IInstaller } from '../common/types'; import { TensorBoard } from '../common/utils/localize'; import { IInterpreterService } from '../interpreter/contracts'; import { sendTelemetryEvent } from '../telemetry'; @@ -26,34 +24,25 @@ export class TensorBoardSessionProvider implements IExtensionSingleActivationSer @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, @inject(ICommandManager) private readonly commandManager: ICommandManager, @inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry, - @inject(IExperimentService) private readonly experimentService: IExperimentService, @inject(IProcessServiceFactory) private readonly processServiceFactory: IProcessServiceFactory, ) {} public async activate(): Promise { - this.activateInternal().ignoreErrors(); - } - - private async activateInternal() { - if (await this.experimentService.inExperiment(NativeTensorBoard.experiment)) { - this.disposables.push( - this.commandManager.registerCommand( - Commands.LaunchTensorBoard, - ( - entrypoint: TensorBoardEntrypoint = TensorBoardEntrypoint.palette, - trigger: TensorBoardEntrypointTrigger = TensorBoardEntrypointTrigger.palette, - ) => { - sendTelemetryEvent(EventName.TENSORBOARD_SESSION_LAUNCH, undefined, { - trigger, - entrypoint, - }); - return this.createNewSession(); - }, - ), - ); - const contextKey = new ContextKey('python.isInNativeTensorBoardExperiment', this.commandManager); - contextKey.set(true).ignoreErrors(); - } + this.disposables.push( + this.commandManager.registerCommand( + Commands.LaunchTensorBoard, + ( + entrypoint: TensorBoardEntrypoint = TensorBoardEntrypoint.palette, + trigger: TensorBoardEntrypointTrigger = TensorBoardEntrypointTrigger.palette, + ) => { + sendTelemetryEvent(EventName.TENSORBOARD_SESSION_LAUNCH, undefined, { + trigger, + entrypoint, + }); + return this.createNewSession(); + }, + ), + ); } private async createNewSession(): Promise { diff --git a/src/client/tensorBoard/tensorBoardUsageTracker.ts b/src/client/tensorBoard/tensorBoardUsageTracker.ts index 750e6c1e835f..90f6ef7e9b32 100644 --- a/src/client/tensorBoard/tensorBoardUsageTracker.ts +++ b/src/client/tensorBoard/tensorBoardUsageTracker.ts @@ -7,7 +7,8 @@ import { TextEditor } from 'vscode'; import { IExtensionSingleActivationService } from '../activation/types'; import { IDocumentManager } from '../common/application/types'; import { isTestExecution } from '../common/constants'; -import { IDisposableRegistry } from '../common/types'; +import { NativeTensorBoard } from '../common/experiments/groups'; +import { IDisposableRegistry, IExperimentService } from '../common/types'; import { getDocumentLines } from '../telemetry/importTracker'; import { TensorBoardEntrypointTrigger } from './constants'; import { containsTensorBoardImport } from './helpers'; @@ -23,9 +24,13 @@ export class TensorBoardUsageTracker implements IExtensionSingleActivationServic @inject(IDocumentManager) private documentManager: IDocumentManager, @inject(IDisposableRegistry) private disposables: IDisposableRegistry, @inject(TensorBoardPrompt) private prompt: TensorBoardPrompt, + @inject(IExperimentService) private experimentService: IExperimentService, ) {} public async activate(): Promise { + if (!(await this.experimentService.inExperiment(NativeTensorBoard.experiment))) { + return; + } if (testExecution) { await this.activateInternal(); } else { diff --git a/src/test/tensorBoard/tensorBoardSession.test.ts b/src/test/tensorBoard/tensorBoardSession.test.ts index b66549687c26..fbbf535839db 100644 --- a/src/test/tensorBoard/tensorBoardSession.test.ts +++ b/src/test/tensorBoard/tensorBoardSession.test.ts @@ -7,7 +7,6 @@ import { TensorBoard } from '../../client/common/utils/localize'; import { IServiceManager } from '../../client/ioc/types'; import { TensorBoardEntrypoint, TensorBoardEntrypointTrigger } from '../../client/tensorBoard/constants'; import { TensorBoardSession } from '../../client/tensorBoard/tensorBoardSession'; -import { TensorBoardSessionProvider } from '../../client/tensorBoard/tensorBoardSessionProvider'; import { closeActiveWindows, initialize } from '../initialize'; import * as ExperimentHelpers from '../../client/common/experiments/helpers'; import { IInterpreterService } from '../../client/interpreter/contracts'; @@ -31,7 +30,6 @@ suite('TensorBoard session creation', async () => { let serviceManager: IServiceManager; let errorMessageStub: Sinon.SinonStub; let sandbox: Sinon.SinonSandbox; - let provider: TensorBoardSessionProvider; let applicationShell: IApplicationShell; let commandManager: ICommandManager; @@ -59,9 +57,6 @@ suite('TensorBoard session creation', async () => { const interpreterService = serviceManager.get(IInterpreterService); sandbox.stub(interpreterService, 'getActiveInterpreter').resolves(interpreter); - // Create tensorboard session provider - provider = serviceManager.get(TensorBoardSessionProvider); - await provider.activate(); applicationShell = serviceManager.get(IApplicationShell); errorMessageStub = sandbox.stub(applicationShell, 'showErrorMessage'); commandManager = serviceManager.get(ICommandManager); diff --git a/src/test/tensorBoard/tensorBoardUsageTracker.unit.test.ts b/src/test/tensorBoard/tensorBoardUsageTracker.unit.test.ts index a267c5cb8569..a29af0207623 100644 --- a/src/test/tensorBoard/tensorBoardUsageTracker.unit.test.ts +++ b/src/test/tensorBoard/tensorBoardUsageTracker.unit.test.ts @@ -1,21 +1,32 @@ import { assert } from 'chai'; import * as sinon from 'sinon'; +import { mock, when, instance } from 'ts-mockito'; import { TensorBoardUsageTracker } from '../../client/tensorBoard/tensorBoardUsageTracker'; import { TensorBoardPrompt } from '../../client/tensorBoard/tensorBoardPrompt'; import { MockDocumentManager } from '../startPage/mockDocumentManager'; import { createTensorBoardPromptWithMocks } from './helpers'; +import { NativeTensorBoard } from '../../client/common/experiments/groups'; +import { ExperimentService } from '../../client/common/experiments/service'; suite('TensorBoard usage tracker', () => { let documentManager: MockDocumentManager; let tensorBoardImportTracker: TensorBoardUsageTracker; let prompt: TensorBoardPrompt; + let experimentService: ExperimentService; let showNativeTensorBoardPrompt: sinon.SinonSpy; setup(() => { documentManager = new MockDocumentManager(); prompt = createTensorBoardPromptWithMocks(); showNativeTensorBoardPrompt = sinon.spy(prompt, 'showNativeTensorBoardPrompt'); - tensorBoardImportTracker = new TensorBoardUsageTracker(documentManager, [], prompt); + experimentService = mock(ExperimentService); + when(experimentService.inExperiment(NativeTensorBoard.experiment)).thenResolve(true); + tensorBoardImportTracker = new TensorBoardUsageTracker( + documentManager, + [], + prompt, + instance(experimentService), + ); }); test('Simple tensorboard import in Python file', async () => { From 2179b3014c2bfa57f199906a202e89a195231648 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 17 Feb 2021 12:24:17 -0800 Subject: [PATCH 14/14] Cherry picks from main to release (#15421) * Do not call activate the discovery component before registering all the classes (#15379) * Do not attempt to activate discovery component before registering all the classes * Add clarification comment * Code reviews * Skip windows store and shims paths when using known path locators (#15388) * Skip windows store and shims paths when using known path locators * Clean up and comments * Tests * Handle cases where envs variables might not be set * Typo Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> * Change "Pylance not installed" prompt to allow reverting to Jedi (#15420) * Allow on suggestion refresh by default (#15430) Co-authored-by: Kartik Raj Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com> --- package.nls.json | 4 +- package.nls.ru.json | 1 - package.nls.zh-cn.json | 1 - src/client/activation/activationService.ts | 2 + src/client/activation/common/activatorBase.ts | 2 +- .../common/languageServerChangeHandler.ts | 36 +++-- src/client/activation/node/activator.ts | 7 +- src/client/common/utils/localize.ts | 11 +- src/client/extensionActivation.ts | 13 +- .../lowLevel/windowsKnownPathsLocator.ts | 13 ++ .../services/posixKnownPathsLocator.ts | 7 +- .../locators/services/pyenvLocator.ts | 13 +- .../locators/services/windowsStoreLocator.ts | 88 +++++++++--- src/client/pythonEnvironments/legacyIOC.ts | 2 +- .../activation/node/activator.unit.test.ts | 27 ++-- .../languageServerChangeHandler.unit.test.ts | 131 +++++++++++++++--- .../common/environmentIdentifier.unit.test.ts | 17 ++- .../locators/pyenvLocator.unit.test.ts | 22 +++ .../locators/windowsStoreLocator.unit.test.ts | 10 ++ 19 files changed, 332 insertions(+), 75 deletions(-) diff --git a/package.nls.json b/package.nls.json index 3c903570d2b9..e86c191091a9 100644 --- a/package.nls.json +++ b/package.nls.json @@ -49,9 +49,11 @@ "Pylance.proposePylanceMessage": "Try out a new faster, feature-rich language server for Python by Microsoft, Pylance! Install the extension now.", "Pylance.tryItNow": "Try it now", "Pylance.remindMeLater": "Remind me later", - "Pylance.installPylanceMessage": "Pylance extension is not installed. Click Yes to open Pylance installation page.", "Pylance.pylanceNotInstalledMessage": "Pylance extension is not installed.", "Pylance.pylanceInstalledReloadPromptMessage": "Pylance extension is now installed. Reload window to activate?", + "Pylance.pylanceRevertToJediPrompt": "The Pylance extension is not installed but the python.languageServer value is set to \"Pylance\". Would you like to install the Pylance extension to use Pylance, or revert back to Jedi?", + "Pylance.pylanceInstallPylance": "Install Pylance", + "Pylance.pylanceRevertToJedi": "Revert to Jedi", "Experiments.inGroup": "User belongs to experiment group '{0}'", "Experiments.optedOutOf": "User opted out of experiment group '{0}'", "Interpreters.RefreshingInterpreters": "Refreshing Python Interpreters", diff --git a/package.nls.ru.json b/package.nls.ru.json index 9b6b0a5661da..0a80260e6644 100644 --- a/package.nls.ru.json +++ b/package.nls.ru.json @@ -34,7 +34,6 @@ "Pylance.proposePylanceMessage": "Попробуйте новый языковый сервер для Python от Microsoft: Pylance! Установите расширение Pylance.", "Pylance.tryItNow": "Да, хочу", "Pylance.remindMeLater": "Напомните позже", - "Pylance.installPylanceMessage": "Расширение Pylance не установлено. Нажмите Да чтобы открыть страницу установки Pylance.", "Pylance.pylanceNotInstalledMessage": "Расширение Pylance не установлено.", "Pylance.pylanceInstalledReloadPromptMessage": "Расширение Pylance установлено. Перезагрузить окно для его активации?", "LanguageService.reloadAfterLanguageServerChange": "Пожалуйста, перезагрузите окно после смены типа языкового сервера." diff --git a/package.nls.zh-cn.json b/package.nls.zh-cn.json index 823b1d6a9268..14b9701851f8 100644 --- a/package.nls.zh-cn.json +++ b/package.nls.zh-cn.json @@ -49,7 +49,6 @@ "Pylance.proposePylanceMessage": "试试微软新的更快的、功能丰富的语言服务器 Pylance! ", "Pylance.tryItNow": "立即安装", "Pylance.remindMeLater": "稍后提醒", - "Pylance.installPylanceMessage": "Pylance 扩展未安装。点击 \"是\" 打开 Pylance 安装页面。", "Pylance.pylanceNotInstalledMessage": "Pylance 扩展未安装。", "Pylance.pylanceInstalledReloadPromptMessage": "Pylance 扩展未安装。重新加载窗口以激活？", "Experiments.inGroup": "用户属于 '{0}' 实验组", diff --git a/src/client/activation/activationService.ts b/src/client/activation/activationService.ts index 82bc3eebc33b..b63721acd392 100644 --- a/src/client/activation/activationService.ts +++ b/src/client/activation/activationService.ts @@ -86,6 +86,8 @@ export class LanguageServerExtensionActivationService this.serviceContainer.get(IExtensions), this.serviceContainer.get(IApplicationShell), this.serviceContainer.get(ICommandManager), + this.serviceContainer.get(IWorkspaceService), + this.serviceContainer.get(IConfigurationService), ); disposables.push(this.languageServerChangeHandler); } diff --git a/src/client/activation/common/activatorBase.ts b/src/client/activation/common/activatorBase.ts index 7dcc80455291..3de6b41c2e98 100644 --- a/src/client/activation/common/activatorBase.ts +++ b/src/client/activation/common/activatorBase.ts @@ -42,7 +42,7 @@ export abstract class LanguageServerActivatorBase implements ILanguageServerActi protected resource?: Resource; constructor( protected readonly manager: ILanguageServerManager, - private readonly workspace: IWorkspaceService, + protected readonly workspace: IWorkspaceService, protected readonly fs: IFileSystem, protected readonly configurationService: IConfigurationService, ) {} diff --git a/src/client/activation/common/languageServerChangeHandler.ts b/src/client/activation/common/languageServerChangeHandler.ts index d81f2cfdc723..22c325b6d6e5 100644 --- a/src/client/activation/common/languageServerChangeHandler.ts +++ b/src/client/activation/common/languageServerChangeHandler.ts @@ -1,10 +1,10 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { Disposable } from 'vscode'; -import { IApplicationShell, ICommandManager } from '../../common/application/types'; +import { ConfigurationTarget, Disposable } from 'vscode'; +import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../common/application/types'; import { PYLANCE_EXTENSION_ID } from '../../common/constants'; -import { IExtensions } from '../../common/types'; +import { IConfigurationService, IExtensions } from '../../common/types'; import { createDeferred } from '../../common/utils/async'; import { Common, LanguageService, Pylance } from '../../common/utils/localize'; import { LanguageServerType } from '../types'; @@ -12,16 +12,32 @@ import { LanguageServerType } from '../types'; export async function promptForPylanceInstall( appShell: IApplicationShell, commandManager: ICommandManager, + workspace: IWorkspaceService, + configService: IConfigurationService, ): Promise { - // If not installed, point user to Pylance at the store. const response = await appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ); - if (response === Common.bannerLabelYes()) { + if (response === Pylance.pylanceInstallPylance()) { commandManager.executeCommand('extension.open', PYLANCE_EXTENSION_ID); + } else if (response === Pylance.pylanceRevertToJedi()) { + const inspection = workspace.getConfiguration('python').inspect('languageServer'); + + let target: ConfigurationTarget | undefined; + if (inspection?.workspaceValue) { + target = ConfigurationTarget.Workspace; + } else if (inspection?.globalValue) { + target = ConfigurationTarget.Global; + } + + if (target) { + await configService.updateSetting('languageServer', LanguageServerType.Jedi, undefined, target); + commandManager.executeCommand('workbench.action.reloadWindow'); + } } } @@ -37,6 +53,8 @@ export class LanguageServerChangeHandler implements Disposable { private readonly extensions: IExtensions, private readonly appShell: IApplicationShell, private readonly commands: ICommandManager, + private readonly workspace: IWorkspaceService, + private readonly configService: IConfigurationService, ) { this.pylanceInstalled = this.isPylanceInstalled(); this.disposables.push( @@ -70,7 +88,7 @@ export class LanguageServerChangeHandler implements Disposable { let response: string | undefined; if (lsType === LanguageServerType.Node && !this.isPylanceInstalled()) { // If not installed, point user to Pylance at the store. - await promptForPylanceInstall(this.appShell, this.commands); + await promptForPylanceInstall(this.appShell, this.commands, this.workspace, this.configService); // At this point Pylance is not yet installed. Skip reload prompt // since we are going to show it when Pylance becomes available. } else { diff --git a/src/client/activation/node/activator.ts b/src/client/activation/node/activator.ts index 1d5766c1856e..fa3a99e2df01 100644 --- a/src/client/activation/node/activator.ts +++ b/src/client/activation/node/activator.ts @@ -47,7 +47,12 @@ export class NodeLanguageServerActivator extends LanguageServerActivatorBase { // Pylance is not yet installed. Throw will cause activator to use Jedi // temporarily. Language server installation tracker will prompt for window // reload when Pylance becomes available. - await promptForPylanceInstall(this.appShell, this.commandManager); + await promptForPylanceInstall( + this.appShell, + this.commandManager, + this.workspace, + this.configurationService, + ); throw new Error(Pylance.pylanceNotInstalledMessage()); } } diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 65423b36f133..ca090b39afff 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -114,10 +114,6 @@ export namespace Pylance { export const tryItNow = localize('Pylance.tryItNow', 'Try it now'); export const remindMeLater = localize('Pylance.remindMeLater', 'Remind me later'); - export const installPylanceMessage = localize( - 'Pylance.installPylanceMessage', - 'Pylance extension is not installed. Click Yes to open Pylance installation page.', - ); export const pylanceNotInstalledMessage = localize( 'Pylance.pylanceNotInstalledMessage', 'Pylance extension is not installed.', @@ -126,6 +122,13 @@ export namespace Pylance { 'Pylance.pylanceInstalledReloadPromptMessage', 'Pylance extension is now installed. Reload window to activate?', ); + + export const pylanceRevertToJediPrompt = localize( + 'Pylance.pylanceRevertToJediPrompt', + 'The Pylance extension is not installed but the python.languageServer value is set to "Pylance". Would you like to install the Pylance extension to use Pylance, or revert back to Jedi?', + ); + export const pylanceInstallPylance = localize('Pylance.pylanceInstallPylance', 'Install Pylance'); + export const pylanceRevertToJedi = localize('Pylance.pylanceRevertToJedi', 'Revert to Jedi'); } export namespace Jupyter { diff --git a/src/client/extensionActivation.ts b/src/client/extensionActivation.ts index 816a0d400333..0652602250d0 100644 --- a/src/client/extensionActivation.ts +++ b/src/client/extensionActivation.ts @@ -78,12 +78,19 @@ export async function activateComponents( // `Promise.all()`, etc.) will flatten nested promises. Thus // activation resolves `ActivationResult`, which can safely wrap // the "inner" promise. + + // TODO: As of now activateLegacy() registers various classes which might + // be required while activating components. Once registration from + // activateLegacy() are moved before we activate other components, we can + // activate them parallelly with the other components. + // https://github.com/microsoft/vscode-python/issues/15380 + // These will go away eventually once everything is refactored into components. + const legacyActivationResult = await activateLegacy(ext); const promises: Promise[] = [ + // More component activations will go here pythonEnvironments.activate(components.pythonEnvs), - // These will go away eventually. - activateLegacy(ext), ]; - return Promise.all(promises); + return Promise.all([legacyActivationResult, ...promises]); } /// ////////////////////////// diff --git a/src/client/pythonEnvironments/base/locators/lowLevel/windowsKnownPathsLocator.ts b/src/client/pythonEnvironments/base/locators/lowLevel/windowsKnownPathsLocator.ts index c5f1bde4733e..d9e986db45de 100644 --- a/src/client/pythonEnvironments/base/locators/lowLevel/windowsKnownPathsLocator.ts +++ b/src/client/pythonEnvironments/base/locators/lowLevel/windowsKnownPathsLocator.ts @@ -9,6 +9,8 @@ import { Event } from 'vscode'; import { getSearchPathEntries } from '../../../../common/utils/exec'; import { Disposables, IDisposable } from '../../../../common/utils/resourceLifecycle'; import { isStandardPythonBinary } from '../../../common/commonUtils'; +import { isPyenvShimDir } from '../../../discovery/locators/services/pyenvLocator'; +import { isWindowsStoreDir } from '../../../discovery/locators/services/windowsStoreLocator'; import { PythonEnvInfo, PythonEnvKind, PythonEnvSource } from '../../info'; import { ILocator, IPythonEnvsIterator, PythonLocatorQuery } from '../../locator'; import { Locators } from '../../locators'; @@ -31,6 +33,17 @@ export class WindowsPathEnvVarLocator implements ILocator, IDisposable { constructor() { const dirLocators: (ILocator & IDisposable)[] = getSearchPathEntries() + .filter((dirname) => { + // Filter out following directories: + // 1. Windows Store app directories: We have a store app locator that handles this. The + // python.exe available in these directories might not be python. It can be a store + // install shortcut that takes you to windows store. + // + // 2. Filter out pyenv shims: They are not actual python binaries, they are used to launch + // the binaries specified in .python-version file in the cwd. We should not be reporting + // those binaries as environments. + return !isWindowsStoreDir(dirname) && !isPyenvShimDir(dirname); + }) // Build a locator for each directory. .map((dirname) => getDirFilesLocator(dirname, PythonEnvKind.Unknown)); this.disposables.push(...dirLocators); diff --git a/src/client/pythonEnvironments/discovery/locators/services/posixKnownPathsLocator.ts b/src/client/pythonEnvironments/discovery/locators/services/posixKnownPathsLocator.ts index 8dc0e1dc095b..e7017089280d 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/posixKnownPathsLocator.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/posixKnownPathsLocator.ts @@ -4,7 +4,6 @@ import * as fs from 'fs'; import * as path from 'path'; import { traceError, traceInfo } from '../../../../common/logger'; - import { Architecture } from '../../../../common/utils/platform'; import { PythonEnvInfo, PythonEnvKind, PythonEnvSource, PythonReleaseLevel, PythonVersion } from '../../../base/info'; import { buildEnvInfo } from '../../../base/info/env'; @@ -12,9 +11,13 @@ import { parseVersion } from '../../../base/info/pythonVersion'; import { IPythonEnvsIterator, Locator } from '../../../base/locator'; import { getFileInfo, resolveSymbolicLink } from '../../../common/externalDependencies'; import { commonPosixBinPaths, isPosixPythonBinPattern } from '../../../common/posixUtils'; +import { isPyenvShimDir } from './pyenvLocator'; async function getPythonBinFromKnownPaths(): Promise { - const knownDirs = await commonPosixBinPaths(); + // Filter out pyenv shims. They are not actual python binaries, they are used to launch + // the binaries specified in .python-version file in the cwd. We should not be reporting + // those binaries as environments. + const knownDirs = (await commonPosixBinPaths()).filter((dirname) => !isPyenvShimDir(dirname)); const pythonBins: Set = new Set(); for (const dirname of knownDirs) { const paths = (await fs.promises.readdir(dirname, { withFileTypes: true })) diff --git a/src/client/pythonEnvironments/discovery/locators/services/pyenvLocator.ts b/src/client/pythonEnvironments/discovery/locators/services/pyenvLocator.ts index 9fcbde3bd2dc..ddac1574932d 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/pyenvLocator.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/pyenvLocator.ts @@ -13,7 +13,7 @@ import { getInterpreterPathFromDir, getPythonVersionFromPath, } from '../../../common/commonUtils'; -import { getFileInfo, getSubDirs, pathExists } from '../../../common/externalDependencies'; +import { arePathsSame, getFileInfo, getSubDirs, pathExists } from '../../../common/externalDependencies'; function getPyenvDir(): string { // Check if the pyenv environment variables exist: PYENV on Windows, PYENV_ROOT on Unix. @@ -37,6 +37,17 @@ function getPyenvVersionsDir(): string { return path.join(getPyenvDir(), 'versions'); } +/** + * Checks if a given directory path is same as `pyenv` shims path. This checks + * `~/.pyenv/shims` on posix and `~/.pyenv/pyenv-win/shims` on windows. + * @param {string} dirPath: Absolute path to any directory + * @returns {boolean}: Returns true if the patch is same as `pyenv` shims directory. + */ +export function isPyenvShimDir(dirPath: string): boolean { + const shimPath = path.join(getPyenvDir(), 'shims'); + return arePathsSame(shimPath, dirPath) || arePathsSame(`${shimPath}${path.sep}`, dirPath); +} + /** * Checks if the given interpreter belongs to a pyenv based environment. * @param {string} interpreterPath: Absolute path to the python interpreter. diff --git a/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts b/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts index 571c415551a2..daa46800dc0d 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts @@ -11,7 +11,7 @@ import { buildEnvInfo } from '../../../base/info/env'; import { getPythonVersionFromPath } from '../../../base/info/pythonVersion'; import { IPythonEnvsIterator } from '../../../base/locator'; import { FSWatchingLocator } from '../../../base/locators/lowLevel/fsWatchingLocator'; -import { getFileInfo } from '../../../common/externalDependencies'; +import { getFileInfo, pathExists } from '../../../common/externalDependencies'; import { PythonEnvStructure } from '../../../common/pythonBinariesWatcher'; /** @@ -26,16 +26,59 @@ export function getWindowsStoreAppsRoot(): string { /** * Checks if a given path is under the forbidden windows store directory. - * @param {string} interpreterPath : Absolute path to the python interpreter. + * @param {string} absPath : Absolute path to a file or directory. * @returns {boolean} : Returns true if `interpreterPath` is under * `%ProgramFiles%/WindowsApps`. */ -export function isForbiddenStorePath(interpreterPath: string): boolean { +export function isForbiddenStorePath(absPath: string): boolean { const programFilesStorePath = path .join(getEnvironmentVariable('ProgramFiles') || 'Program Files', 'WindowsApps') .normalize() .toUpperCase(); - return path.normalize(interpreterPath).toUpperCase().includes(programFilesStorePath); + return path.normalize(absPath).toUpperCase().includes(programFilesStorePath); +} + +/** + * Checks if a given directory is any one of the possible windows store directories, or + * its sub-directory. + * @param {string} dirPath : Absolute path to a directory. + * + * Remarks: + * These locations are tested: + * 1. %LOCALAPPDATA%/Microsoft/WindowsApps + * 2. %ProgramFiles%/WindowsApps + */ +export function isWindowsStoreDir(dirPath: string): boolean { + const storeRootPath = path.normalize(getWindowsStoreAppsRoot()).toUpperCase(); + return path.normalize(dirPath).toUpperCase().includes(storeRootPath) || isForbiddenStorePath(dirPath); +} + +/** + * Checks if store python is installed. + * @param {string} interpreterPath : Absolute path to a interpreter. + * Remarks: + * If store python was never installed then the store apps directory will not + * have idle.exe or pip.exe. We can use this as a way to identify the python.exe + * found in the store apps directory is a real python or a store install shortcut. + */ +async function isStorePythonInstalled(interpreterPath?: string): Promise { + let results = await Promise.all([ + pathExists(path.join(getWindowsStoreAppsRoot(), 'idle.exe')), + pathExists(path.join(getWindowsStoreAppsRoot(), 'pip.exe')), + ]); + + if (results.includes(true)) { + return true; + } + + if (interpreterPath) { + results = await Promise.all([ + pathExists(path.join(path.dirname(interpreterPath), 'idle.exe')), + pathExists(path.join(path.dirname(interpreterPath), 'pip.exe')), + ]); + return results.includes(true); + } + return false; } /** @@ -71,17 +114,19 @@ export function isForbiddenStorePath(interpreterPath: string): boolean { * */ export async function isWindowsStoreEnvironment(interpreterPath: string): Promise { - const pythonPathToCompare = path.normalize(interpreterPath).toUpperCase(); - const localAppDataStorePath = path.normalize(getWindowsStoreAppsRoot()).toUpperCase(); - if (pythonPathToCompare.includes(localAppDataStorePath)) { - return true; - } + if (await isStorePythonInstalled(interpreterPath)) { + const pythonPathToCompare = path.normalize(interpreterPath).toUpperCase(); + const localAppDataStorePath = path.normalize(getWindowsStoreAppsRoot()).toUpperCase(); + if (pythonPathToCompare.includes(localAppDataStorePath)) { + return true; + } - // Program Files store path is a forbidden path. Only admins and system has access this path. - // We should never have to look at this path or even execute python from this path. - if (isForbiddenStorePath(pythonPathToCompare)) { - traceWarning('isWindowsStoreEnvironment called with Program Files store path.'); - return true; + // Program Files store path is a forbidden path. Only admins and system has access this path. + // We should never have to look at this path or even execute python from this path. + if (isForbiddenStorePath(pythonPathToCompare)) { + traceWarning('isWindowsStoreEnvironment called with Program Files store path.'); + return true; + } } return false; } @@ -107,7 +152,7 @@ const pythonExeGlob = 'python3.{[0-9],[0-9][0-9]}.exe'; * @param {string} interpreterPath : Path to python interpreter. * @returns {boolean} : Returns true if the path matches pattern for windows python executable. */ -export function isWindowsStorePythonExe(interpreterPath: string): boolean { +export function isWindowsStorePythonExePattern(interpreterPath: string): boolean { return minimatch(path.basename(interpreterPath), pythonExeGlob, { nocase: true }); } @@ -128,11 +173,16 @@ export function isWindowsStorePythonExe(interpreterPath: string): boolean { * that location. */ export async function getWindowsStorePythonExes(): Promise { - const windowsAppsRoot = getWindowsStoreAppsRoot(); + if (await isStorePythonInstalled()) { + const windowsAppsRoot = getWindowsStoreAppsRoot(); - // Collect python*.exe directly under %LOCALAPPDATA%/Microsoft/WindowsApps - const files = await fsapi.readdir(windowsAppsRoot); - return files.map((filename: string) => path.join(windowsAppsRoot, filename)).filter(isWindowsStorePythonExe); + // Collect python*.exe directly under %LOCALAPPDATA%/Microsoft/WindowsApps + const files = await fsapi.readdir(windowsAppsRoot); + return files + .map((filename: string) => path.join(windowsAppsRoot, filename)) + .filter(isWindowsStorePythonExePattern); + } + return []; } export class WindowsStoreLocator extends FSWatchingLocator { diff --git a/src/client/pythonEnvironments/legacyIOC.ts b/src/client/pythonEnvironments/legacyIOC.ts index e1f09632e33c..c8c2d1ad736d 100644 --- a/src/client/pythonEnvironments/legacyIOC.ts +++ b/src/client/pythonEnvironments/legacyIOC.ts @@ -142,7 +142,7 @@ class ComponentAdapter implements IComponentAdapter, IExtensionSingleActivationS private readonly refreshed = new vscode.EventEmitter(); - private allowOnSuggestionRefresh = false; + private allowOnSuggestionRefresh = true; constructor( // The adapter only wraps one thing: the component API. diff --git a/src/test/activation/node/activator.unit.test.ts b/src/test/activation/node/activator.unit.test.ts index bfc3bfa3cb0f..1207526d1f09 100644 --- a/src/test/activation/node/activator.unit.test.ts +++ b/src/test/activation/node/activator.unit.test.ts @@ -17,7 +17,7 @@ import { PYLANCE_EXTENSION_ID } from '../../../client/common/constants'; import { FileSystem } from '../../../client/common/platform/fileSystem'; import { IFileSystem } from '../../../client/common/platform/types'; import { IConfigurationService, IExtensions, IPythonSettings } from '../../../client/common/types'; -import { Common, Pylance } from '../../../client/common/utils/localize'; +import { Pylance } from '../../../client/common/utils/localize'; suite('Pylance Language Server - Activator', () => { let activator: NodeLanguageServerActivator; @@ -92,20 +92,22 @@ suite('Pylance Language Server - Activator', () => { test('When Pylance is not installed activator should show install prompt ', async () => { when( appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ), - ).thenReturn(Promise.resolve(Common.bannerLabelNo())); + ).thenReturn(Promise.resolve(Pylance.remindMeLater())); try { await activator.start(undefined); } catch {} verify( appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ), ).once(); verify(commandManager.executeCommand('extension.open', PYLANCE_EXTENSION_ID)).never(); @@ -114,11 +116,12 @@ suite('Pylance Language Server - Activator', () => { test('When Pylance is not installed activator should open Pylance install page if users clicks Yes', async () => { when( appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ), - ).thenReturn(Promise.resolve(Common.bannerLabelYes())); + ).thenReturn(Promise.resolve(Pylance.pylanceInstallPylance())); try { await activator.start(undefined); diff --git a/src/test/activation/node/languageServerChangeHandler.unit.test.ts b/src/test/activation/node/languageServerChangeHandler.unit.test.ts index 09c96206708c..d66b4e574f0f 100644 --- a/src/test/activation/node/languageServerChangeHandler.unit.test.ts +++ b/src/test/activation/node/languageServerChangeHandler.unit.test.ts @@ -3,13 +3,13 @@ 'use strict'; -import { anyString, instance, mock, verify, when } from 'ts-mockito'; -import { EventEmitter, Extension } from 'vscode'; +import { anyString, instance, mock, verify, when, anything } from 'ts-mockito'; +import { ConfigurationTarget, EventEmitter, Extension, WorkspaceConfiguration } from 'vscode'; import { LanguageServerChangeHandler } from '../../../client/activation/common/languageServerChangeHandler'; import { LanguageServerType } from '../../../client/activation/types'; -import { IApplicationShell, ICommandManager } from '../../../client/common/application/types'; +import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../../client/common/application/types'; import { PYLANCE_EXTENSION_ID } from '../../../client/common/constants'; -import { IExtensions } from '../../../client/common/types'; +import { IConfigurationService, IExtensions } from '../../../client/common/types'; import { Common, LanguageService, Pylance } from '../../../client/common/utils/localize'; suite('Language Server - Change Handler', () => { @@ -19,11 +19,16 @@ suite('Language Server - Change Handler', () => { let extensionsChangedEvent: EventEmitter; let handler: LanguageServerChangeHandler; + let workspace: IWorkspaceService; + let configService: IConfigurationService; + let pylanceExtension: Extension; setup(() => { extensions = mock(); appShell = mock(); commands = mock(); + workspace = mock(); + configService = mock(); pylanceExtension = mock>(); @@ -84,9 +89,10 @@ suite('Language Server - Change Handler', () => { test('Handler should prompt for install when language server changes to Pylance and Pylance is not installed', async () => { when( appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ), ).thenReturn(Promise.resolve(undefined)); @@ -98,9 +104,10 @@ suite('Language Server - Change Handler', () => { ).never(); verify( appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ), ).once(); }); @@ -108,11 +115,12 @@ suite('Language Server - Change Handler', () => { test('Handler should open Pylance store page when language server changes to Pylance, Pylance is not installed and user clicks Yes', async () => { when( appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ), - ).thenReturn(Promise.resolve(Common.bannerLabelYes())); + ).thenReturn(Promise.resolve(Pylance.pylanceInstallPylance())); handler = makeHandler(undefined); await handler.handleLanguageServerChange(LanguageServerType.Node); @@ -124,11 +132,12 @@ suite('Language Server - Change Handler', () => { test('Handler should not open Pylance store page when language server changes to Pylance, Pylance is not installed and user clicks No', async () => { when( appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ), - ).thenReturn(Promise.resolve(Common.bannerLabelNo())); + ).thenReturn(Promise.resolve(Pylance.remindMeLater())); handler = makeHandler(undefined); await handler.handleLanguageServerChange(LanguageServerType.Node); @@ -171,12 +180,98 @@ suite('Language Server - Change Handler', () => { verify(commands.executeCommand('workbench.action.reloadWindow')).never(); }); + [ConfigurationTarget.Global, ConfigurationTarget.Workspace].forEach((target) => { + const targetName = target === ConfigurationTarget.Global ? 'global' : 'workspace'; + test(`Revert to Jedi with setting in ${targetName} config`, async () => { + const configuration = mock(); + + when( + appShell.showWarningMessage( + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), + ), + ).thenReturn(Promise.resolve(Pylance.pylanceRevertToJedi())); + + when(workspace.getConfiguration('python')).thenReturn(instance(configuration)); + + const inspection = { + key: 'python.languageServer', + workspaceValue: target === ConfigurationTarget.Workspace ? LanguageServerType.Node : undefined, + globalValue: target === ConfigurationTarget.Global ? LanguageServerType.Node : undefined, + }; + + when(configuration.inspect('languageServer')).thenReturn(inspection); + + handler = makeHandler(undefined); + await handler.handleLanguageServerChange(LanguageServerType.Node); + + verify( + appShell.showInformationMessage(LanguageService.reloadAfterLanguageServerChange(), Common.reload()), + ).never(); + verify( + appShell.showWarningMessage( + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), + ), + ).once(); + verify(configService.updateSetting('languageServer', LanguageServerType.Jedi, undefined, target)).once(); + }); + }); + + [ConfigurationTarget.WorkspaceFolder, undefined].forEach((target) => { + const targetName = target === ConfigurationTarget.WorkspaceFolder ? 'workspace folder' : 'missing'; + test(`Revert to Jedi with ${targetName} setting does nothing`, async () => { + const configuration = mock(); + + when( + appShell.showWarningMessage( + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), + ), + ).thenReturn(Promise.resolve(Pylance.pylanceRevertToJedi())); + + when(workspace.getConfiguration('python')).thenReturn(instance(configuration)); + + const inspection = { + key: 'python.languageServer', + workspaceFolderValue: + target === ConfigurationTarget.WorkspaceFolder ? LanguageServerType.Node : undefined, + }; + + when(configuration.inspect('languageServer')).thenReturn(inspection); + + handler = makeHandler(undefined); + await handler.handleLanguageServerChange(LanguageServerType.Node); + + verify( + appShell.showInformationMessage(LanguageService.reloadAfterLanguageServerChange(), Common.reload()), + ).never(); + verify( + appShell.showWarningMessage( + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), + ), + ).once(); + verify(configService.updateSetting(anything(), anything(), anything(), anything())).never(); + }); + }); + function makeHandler(initialLSType: LanguageServerType | undefined): LanguageServerChangeHandler { return new LanguageServerChangeHandler( initialLSType, instance(extensions), instance(appShell), instance(commands), + instance(workspace), + instance(configService), ); } }); diff --git a/src/test/pythonEnvironments/common/environmentIdentifier.unit.test.ts b/src/test/pythonEnvironments/common/environmentIdentifier.unit.test.ts index c258b0d04481..0a87b1331e6c 100644 --- a/src/test/pythonEnvironments/common/environmentIdentifier.unit.test.ts +++ b/src/test/pythonEnvironments/common/environmentIdentifier.unit.test.ts @@ -81,6 +81,7 @@ suite('Environment Identifier', () => { suite('Windows Store', () => { let getEnvVar: sinon.SinonStub; + let pathExists: sinon.SinonStub; const fakeLocalAppDataPath = path.join(TEST_LAYOUT_ROOT, 'storeApps'); const fakeProgramFilesPath = 'X:\\Program Files'; const executable = ['python.exe', 'python3.exe', 'python3.8.exe']; @@ -88,17 +89,23 @@ suite('Environment Identifier', () => { getEnvVar = sinon.stub(platformApis, 'getEnvironmentVariable'); getEnvVar.withArgs('LOCALAPPDATA').returns(fakeLocalAppDataPath); getEnvVar.withArgs('ProgramFiles').returns(fakeProgramFilesPath); + + pathExists = sinon.stub(externalDependencies, 'pathExists'); + pathExists.withArgs(path.join(fakeLocalAppDataPath, 'Microsoft', 'WindowsApps', 'idle.exe')).resolves(true); }); suiteTeardown(() => { getEnvVar.restore(); + pathExists.restore(); }); executable.forEach((exe) => { test(`Path to local app data windows store interpreter (${exe})`, async () => { + getEnvVar.withArgs('LOCALAPPDATA').returns(fakeLocalAppDataPath); const interpreterPath = path.join(fakeLocalAppDataPath, 'Microsoft', 'WindowsApps', exe); const envType: EnvironmentType = await identifyEnvironment(interpreterPath); assert.deepEqual(envType, EnvironmentType.WindowsStore); }); test(`Path to local app data windows store interpreter app sub-directory (${exe})`, async () => { + getEnvVar.withArgs('LOCALAPPDATA').returns(fakeLocalAppDataPath); const interpreterPath = path.join( fakeLocalAppDataPath, 'Microsoft', @@ -126,13 +133,15 @@ suite('Environment Identifier', () => { assert.deepEqual(envType, EnvironmentType.WindowsStore); }); test(`Program files app data not set (${exe})`, async () => { - getEnvVar.withArgs('ProgramFiles').returns(undefined); const interpreterPath = path.join( fakeProgramFilesPath, 'WindowsApps', 'PythonSoftwareFoundation.Python.3.8_qbz5n2kfra8p0', exe, ); + getEnvVar.withArgs('ProgramFiles').returns(undefined); + pathExists.withArgs(path.join(path.dirname(interpreterPath), 'idle.exe')).resolves(true); + const envType: EnvironmentType = await identifyEnvironment(interpreterPath); assert.deepEqual(envType, EnvironmentType.WindowsStore); }); @@ -147,6 +156,12 @@ suite('Environment Identifier', () => { const interpreterPath = path .join(fakeLocalAppDataPath, 'Microsoft', 'WindowsApps', exe) .replace('\\', '/'); + pathExists.callsFake((p: string) => { + if (p.endsWith('idle.exe')) { + return Promise.resolve(true); + } + return Promise.resolve(false); + }); const envType: EnvironmentType = await identifyEnvironment(`\\\\?\\${interpreterPath}`); assert.deepEqual(envType, EnvironmentType.WindowsStore); }); diff --git a/src/test/pythonEnvironments/discovery/locators/pyenvLocator.unit.test.ts b/src/test/pythonEnvironments/discovery/locators/pyenvLocator.unit.test.ts index 74c5b912f20f..1e6819aa1a6f 100644 --- a/src/test/pythonEnvironments/discovery/locators/pyenvLocator.unit.test.ts +++ b/src/test/pythonEnvironments/discovery/locators/pyenvLocator.unit.test.ts @@ -9,6 +9,7 @@ import * as fileUtils from '../../../../client/pythonEnvironments/common/externa import { IPyenvVersionStrings, isPyenvEnvironment, + isPyenvShimDir, parsePyenvVersion, } from '../../../../client/pythonEnvironments/discovery/locators/services/pyenvLocator'; @@ -269,3 +270,24 @@ suite('Pyenv Versions Parser Test', () => { }); }); }); + +suite('Pyenv Shims Dir filter tests', () => { + let getEnvVariableStub: sinon.SinonStub; + const pyenvRoot = path.join('path', 'to', 'pyenv', 'root'); + + setup(() => { + getEnvVariableStub = sinon.stub(platformUtils, 'getEnvironmentVariable'); + getEnvVariableStub.withArgs('PYENV_ROOT').returns(pyenvRoot); + }); + + teardown(() => { + getEnvVariableStub.restore(); + }); + + test('isPyenvShimDir: valid case', () => { + assert.deepStrictEqual(isPyenvShimDir(path.join(pyenvRoot, 'shims')), true); + }); + test('isPyenvShimDir: invalid case', () => { + assert.deepStrictEqual(isPyenvShimDir(__dirname), false); + }); +}); diff --git a/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.unit.test.ts b/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.unit.test.ts index fc065aa6096b..995082bb4192 100644 --- a/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.unit.test.ts +++ b/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.unit.test.ts @@ -20,6 +20,7 @@ import { parseVersion } from '../../../../client/pythonEnvironments/base/info/py import * as externalDep from '../../../../client/pythonEnvironments/common/externalDependencies'; import { getWindowsStorePythonExes, + isWindowsStoreDir, WindowsStoreLocator, } from '../../../../client/pythonEnvironments/discovery/locators/services/windowsStoreLocator'; import { getEnvs } from '../../base/common'; @@ -50,6 +51,15 @@ suite('Windows Store', () => { const actual = await getWindowsStorePythonExes(); assert.deepEqual(actual, expected); }); + + test('isWindowsStoreDir: valid case', () => { + assert.deepStrictEqual(isWindowsStoreDir(testStoreAppRoot), true); + assert.deepStrictEqual(isWindowsStoreDir(testStoreAppRoot + path.sep), true); + }); + + test('isWindowsStoreDir: invalid case', () => { + assert.deepStrictEqual(isWindowsStoreDir(__dirname), false); + }); }); suite('Locator', () => {