From 3f78ccb7a7dbc8a7b9d6cc56c6d855d5fd6179d7 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Tue, 9 Feb 2021 15:21:52 -0800 Subject: [PATCH 01/11] 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 02/11] 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 03/11] 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 04/11] 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 05/11] 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 06/11] 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 07/11] 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 08/11] 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 09/11] 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 10/11] 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 11/11] 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.