From bfe68ea296e465756b379ca154455a95930d5efe Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 28 Mar 2023 10:38:43 -0700 Subject: [PATCH 1/9] Add diagnostic to validate ComSpec --- .../diagnostics/checks/pythonInterpreter.ts | 64 ++++++++++++++++++- .../application/diagnostics/constants.ts | 1 + .../common/process/pythonEnvironment.ts | 4 +- .../common/process/pythonExecutionFactory.ts | 2 +- src/client/common/process/types.ts | 4 +- src/client/common/utils/localize.ts | 1 + .../pythonEnvironments/info/executable.ts | 7 +- 7 files changed, 74 insertions(+), 9 deletions(-) diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index 4f5133d9dcbe..df43112051da 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -28,6 +28,10 @@ import { EventName } from '../../../telemetry/constants'; import { IExtensionSingleActivationService } from '../../../activation/types'; import { cache } from '../../../common/utils/decorators'; import { noop } from '../../../common/utils/misc'; +import { IPythonExecutionFactory } from '../../../common/process/types'; +import { getOSType, OSType } from '../../../common/utils/platform'; +import { IFileSystem } from '../../../common/platform/types'; +import { traceError } from '../../../logging'; const messages = { [DiagnosticCodes.NoPythonInterpretersDiagnostic]: l10n.t( @@ -36,6 +40,9 @@ const messages = { [DiagnosticCodes.InvalidPythonInterpreterDiagnostic]: l10n.t( 'An Invalid Python interpreter is selected{0}, please try changing it to enable features such as IntelliSense, linting, and debugging. See output for more details regarding why the interpreter is invalid.', ), + [DiagnosticCodes.InvalidComspecDiagnostic]: l10n.t( + "The environment variable 'Comspec' seems to be set to an invalid value. Please correct it to carry valid path to Command Prompt to enable features such as IntelliSense, linting, and debugging. See instructions which might help.", + ), }; export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic { @@ -61,6 +68,12 @@ export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic { } } +export class DefaultShellDiagnostic extends BaseDiagnostic { + constructor(code: DiagnosticCodes.InvalidComspecDiagnostic, resource: Resource, scope = DiagnosticScope.Global) { + super(code, messages[code], DiagnosticSeverity.Error, scope, resource, undefined, 'always'); + } +} + export const InvalidPythonInterpreterServiceId = 'InvalidPythonInterpreterServiceId'; @injectable() @@ -73,7 +86,11 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService @inject(IDisposableRegistry) disposableRegistry: IDisposableRegistry, ) { super( - [DiagnosticCodes.NoPythonInterpretersDiagnostic, DiagnosticCodes.InvalidPythonInterpreterDiagnostic], + [ + DiagnosticCodes.NoPythonInterpretersDiagnostic, + DiagnosticCodes.InvalidPythonInterpreterDiagnostic, + DiagnosticCodes.InvalidComspecDiagnostic, + ], serviceContainer, disposableRegistry, false, @@ -103,6 +120,13 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService public async _manualDiagnose(resource: Resource): Promise { const workspaceService = this.serviceContainer.get(IWorkspaceService); const interpreterService = this.serviceContainer.get(IInterpreterService); + const currentInterpreter = await interpreterService.getActiveInterpreter(resource); + if (!currentInterpreter) { + const diagnostics = await this.diagnoseDefaultShell(resource); + if (diagnostics.length) { + return diagnostics; + } + } const hasInterpreters = await interpreterService.hasInterpreters(); const interpreterPathService = this.serviceContainer.get(IInterpreterPathService); const isInterpreterSetToDefault = interpreterPathService.get(resource) === 'python'; @@ -118,7 +142,6 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService ]; } - const currentInterpreter = await interpreterService.getActiveInterpreter(resource); if (!currentInterpreter) { return [ new InvalidPythonInterpreterDiagnostic( @@ -163,6 +186,17 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService private getCommandPrompts(diagnostic: IDiagnostic): { prompt: string; command?: IDiagnosticCommand }[] { const commandFactory = this.serviceContainer.get(IDiagnosticsCommandFactory); + if (diagnostic.code === DiagnosticCodes.InvalidComspecDiagnostic) { + return [ + { + prompt: Common.instructions, + command: commandFactory.createCommand(diagnostic, { + type: 'launch', + options: 'https://aka.ms/AAk3djo', + }), + }, + ]; + } const prompts = [ { prompt: Common.selectPythonInterpreter, @@ -183,6 +217,32 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService } return prompts; } + + private async diagnoseDefaultShell(resource: Resource): Promise { + if (getOSType() !== OSType.Windows) { + return []; + } + const executionFactory = this.serviceContainer.get(IPythonExecutionFactory); + const executionService = await executionFactory.create({ resource }); + try { + await executionService.getExecutablePath({ throwOnError: true }); + } catch (ex) { + if ((ex as Error).message?.includes('4058')) { + // ENOENT (-4058) error is thrown by Node when the default shell is invalid. + if (await this.isComspecInvalid()) { + traceError('ComSpec is set to an invalid value', process.env.ComSpec); + return [new DefaultShellDiagnostic(DiagnosticCodes.InvalidComspecDiagnostic, resource)]; + } + } + } + return []; + } + + private async isComspecInvalid() { + const comSpec = process.env.ComSpec ?? ''; + const fs = this.serviceContainer.get(IFileSystem); + return fs.fileExists(comSpec); + } } function getOnCloseHandler(diagnostic: IDiagnostic): IDiagnosticMessageOnCloseHandler | undefined { diff --git a/src/client/application/diagnostics/constants.ts b/src/client/application/diagnostics/constants.ts index 9fdd6ff13723..9d47d217d443 100644 --- a/src/client/application/diagnostics/constants.ts +++ b/src/client/application/diagnostics/constants.ts @@ -12,6 +12,7 @@ export enum DiagnosticCodes { InvalidPythonPathInDebuggerLaunchDiagnostic = 'InvalidPythonPathInDebuggerLaunchDiagnostic', EnvironmentActivationInPowerShellWithBatchFilesNotSupportedDiagnostic = 'EnvironmentActivationInPowerShellWithBatchFilesNotSupportedDiagnostic', InvalidPythonInterpreterDiagnostic = 'InvalidPythonInterpreterDiagnostic', + InvalidComspecDiagnostic = 'InvalidComspecDiagnostic', LSNotSupportedDiagnostic = 'LSNotSupportedDiagnostic', PythonPathDeprecatedDiagnostic = 'PythonPathDeprecatedDiagnostic', JustMyCodeDiagnostic = 'JustMyCodeDiagnostic', diff --git a/src/client/common/process/pythonEnvironment.ts b/src/client/common/process/pythonEnvironment.ts index f4c6f8954126..579ac015e547 100644 --- a/src/client/common/process/pythonEnvironment.ts +++ b/src/client/common/process/pythonEnvironment.ts @@ -47,7 +47,7 @@ class PythonEnvironment implements IPythonEnvironment { return this.cachedInterpreterInformation; } - public async getExecutablePath(): Promise { + public async getExecutablePath(options?: { throwOnError?: boolean }): Promise { // If we've passed the python file, then return the file. // This is because on mac if using the interpreter /usr/bin/python2.7 we can get a different value for the path if (await this.deps.isValidExecutable(this.pythonPath)) { @@ -59,7 +59,7 @@ class PythonEnvironment implements IPythonEnvironment { return result; } const python = this.getExecutionInfo(); - const promise = getExecutablePath(python, this.deps.shellExec); + const promise = getExecutablePath(python, this.deps.shellExec, options); cachedExecutablePath.set(this.pythonPath, promise); return promise; } diff --git a/src/client/common/process/pythonExecutionFactory.ts b/src/client/common/process/pythonExecutionFactory.ts index fc13e7f2346c..1120b61c9467 100644 --- a/src/client/common/process/pythonExecutionFactory.ts +++ b/src/client/common/process/pythonExecutionFactory.ts @@ -145,7 +145,7 @@ function createPythonService(procService: IProcessService, env: IPythonEnvironme const procs = createPythonProcessService(procService, env); return { getInterpreterInformation: () => env.getInterpreterInformation(), - getExecutablePath: () => env.getExecutablePath(), + getExecutablePath: (options?: { throwOnError?: boolean }) => env.getExecutablePath(options), isModuleInstalled: (m) => env.isModuleInstalled(m), getModuleVersion: (m) => env.getModuleVersion(m), getExecutionInfo: (a) => env.getExecutionInfo(a), diff --git a/src/client/common/process/types.ts b/src/client/common/process/types.ts index bcab76e66b09..16ffdc50919c 100644 --- a/src/client/common/process/types.ts +++ b/src/client/common/process/types.ts @@ -84,7 +84,7 @@ export const IPythonExecutionService = Symbol('IPythonExecutionService'); export interface IPythonExecutionService { getInterpreterInformation(): Promise; - getExecutablePath(): Promise; + getExecutablePath(options?: { throwOnError?: boolean }): Promise; isModuleInstalled(moduleName: string): Promise; getModuleVersion(moduleName: string): Promise; getExecutionInfo(pythonArgs?: string[]): PythonExecInfo; @@ -100,7 +100,7 @@ export interface IPythonExecutionService { export interface IPythonEnvironment { getInterpreterInformation(): Promise; getExecutionObservableInfo(pythonArgs?: string[], pythonExecutable?: string): PythonExecInfo; - getExecutablePath(): Promise; + getExecutablePath(options?: { throwOnError?: boolean }): Promise; isModuleInstalled(moduleName: string): Promise; getModuleVersion(moduleName: string): Promise; getExecutionInfo(pythonArgs?: string[], pythonExecutable?: string): PythonExecInfo; diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 8673fe7cc8cd..b8ce7d5fc623 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -49,6 +49,7 @@ export namespace Diagnostics { export namespace Common { export const allow = l10n.t('Allow'); + export const instructions = l10n.t('Instructions'); export const close = l10n.t('Close'); export const bannerLabelYes = l10n.t('Yes'); export const bannerLabelNo = l10n.t('No'); diff --git a/src/client/pythonEnvironments/info/executable.ts b/src/client/pythonEnvironments/info/executable.ts index 9b16d04f5753..c221a96741a2 100644 --- a/src/client/pythonEnvironments/info/executable.ts +++ b/src/client/pythonEnvironments/info/executable.ts @@ -17,7 +17,7 @@ import { copyPythonExecInfo, PythonExecInfo } from '../exec'; export async function getExecutablePath( python: PythonExecInfo, shellExec: ShellExecFunc, - timeout?: number, + options?: { throwOnError?: boolean }, ): Promise { try { const [args, parse] = getExecutable(); @@ -28,7 +28,7 @@ export async function getExecutablePath( (p, c) => (p ? `${p} ${c.toCommandArgumentForPythonExt()}` : `${c.toCommandArgumentForPythonExt()}`), '', ); - const result = await shellExec(quoted, { timeout: timeout ?? 15000 }); + const result = await shellExec(quoted, { timeout: 15000 }); const executable = parse(result.stdout.trim()); if (executable === '') { throw new Error(`${quoted} resulted in empty stdout`); @@ -36,6 +36,9 @@ export async function getExecutablePath( return executable; } catch (ex) { traceError(ex); + if (options?.throwOnError) { + throw ex; + } return undefined; } } From fc3295aac05eb547a8f44275ad9b70b9c60333d6 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 28 Mar 2023 13:26:31 -0700 Subject: [PATCH 2/9] Refactor --- .../diagnostics/checks/pythonInterpreter.ts | 53 ++++++++++++------- .../common/process/pythonEnvironment.ts | 4 +- .../common/process/pythonExecutionFactory.ts | 2 +- src/client/common/process/types.ts | 4 +- src/client/common/utils/localize.ts | 2 +- .../pythonEnvironments/info/executable.ts | 9 +--- 6 files changed, 42 insertions(+), 32 deletions(-) diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index df43112051da..58903c9e9768 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -6,7 +6,7 @@ import { inject, injectable } from 'inversify'; import { DiagnosticSeverity, l10n } from 'vscode'; import '../../../common/extensions'; import * as path from 'path'; -import { IDisposableRegistry, IInterpreterPathService, Resource } from '../../../common/types'; +import { IConfigurationService, IDisposableRegistry, IInterpreterPathService, Resource } from '../../../common/types'; import { IInterpreterService } from '../../../interpreter/contracts'; import { IServiceContainer } from '../../../ioc/types'; import { BaseDiagnostic, BaseDiagnosticsService } from '../base'; @@ -28,10 +28,11 @@ import { EventName } from '../../../telemetry/constants'; import { IExtensionSingleActivationService } from '../../../activation/types'; import { cache } from '../../../common/utils/decorators'; import { noop } from '../../../common/utils/misc'; -import { IPythonExecutionFactory } from '../../../common/process/types'; import { getOSType, OSType } from '../../../common/utils/platform'; import { IFileSystem } from '../../../common/platform/types'; import { traceError } from '../../../logging'; +import { getExecutable } from '../../../common/process/internal/python'; +import { shellExec } from '../../../common/process/rawProcessApis'; const messages = { [DiagnosticCodes.NoPythonInterpretersDiagnostic]: l10n.t( @@ -41,7 +42,7 @@ const messages = { 'An Invalid Python interpreter is selected{0}, please try changing it to enable features such as IntelliSense, linting, and debugging. See output for more details regarding why the interpreter is invalid.', ), [DiagnosticCodes.InvalidComspecDiagnostic]: l10n.t( - "The environment variable 'Comspec' seems to be set to an invalid value. Please correct it to carry valid path to Command Prompt to enable features such as IntelliSense, linting, and debugging. See instructions which might help.", + 'We detected an issue with one of your environment variables that breaks features such as IntelliSense, linting and debugging. Try setting the "ComSpec" variable to a valid Command Prompt path in your system to fix it.', ), }; @@ -112,20 +113,16 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService ); } - // eslint-disable-next-line class-methods-use-this - public async diagnose(_resource: Resource): Promise { - return []; + public async diagnose(resource: Resource): Promise { + return this.diagnoseDefaultShell(resource); } public async _manualDiagnose(resource: Resource): Promise { const workspaceService = this.serviceContainer.get(IWorkspaceService); const interpreterService = this.serviceContainer.get(IInterpreterService); - const currentInterpreter = await interpreterService.getActiveInterpreter(resource); - if (!currentInterpreter) { - const diagnostics = await this.diagnoseDefaultShell(resource); - if (diagnostics.length) { - return diagnostics; - } + const diagnostics = await this.diagnoseDefaultShell(resource); + if (diagnostics.length) { + return diagnostics; } const hasInterpreters = await interpreterService.hasInterpreters(); const interpreterPathService = this.serviceContainer.get(IInterpreterPathService); @@ -142,6 +139,7 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService ]; } + const currentInterpreter = await interpreterService.getActiveInterpreter(resource); if (!currentInterpreter) { return [ new InvalidPythonInterpreterDiagnostic( @@ -189,7 +187,7 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService if (diagnostic.code === DiagnosticCodes.InvalidComspecDiagnostic) { return [ { - prompt: Common.instructions, + prompt: Common.seeInstructions, command: commandFactory.createCommand(diagnostic, { type: 'launch', options: 'https://aka.ms/AAk3djo', @@ -222,12 +220,16 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService if (getOSType() !== OSType.Windows) { return []; } - const executionFactory = this.serviceContainer.get(IPythonExecutionFactory); - const executionService = await executionFactory.create({ resource }); + const interpreterService = this.serviceContainer.get(IInterpreterService); + const currentInterpreter = await interpreterService.getActiveInterpreter(resource); + if (currentInterpreter) { + return []; + } try { - await executionService.getExecutablePath({ throwOnError: true }); + await this.shellExecPython(); } catch (ex) { - if ((ex as Error).message?.includes('4058')) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + if ((ex as any).errno === -4058) { // ENOENT (-4058) error is thrown by Node when the default shell is invalid. if (await this.isComspecInvalid()) { traceError('ComSpec is set to an invalid value', process.env.ComSpec); @@ -241,7 +243,22 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService private async isComspecInvalid() { const comSpec = process.env.ComSpec ?? ''; const fs = this.serviceContainer.get(IFileSystem); - return fs.fileExists(comSpec); + return fs.fileExists(comSpec).then((exists) => !exists); + } + + @cache(-1, true) + // eslint-disable-next-line class-methods-use-this + private async shellExecPython() { + const configurationService = this.serviceContainer.get(IConfigurationService); + const { pythonPath } = configurationService.getSettings(); + const [args] = getExecutable(); + const argv = [pythonPath, ...args]; + // Concat these together to make a set of quoted strings + const quoted = argv.reduce( + (p, c) => (p ? `${p} ${c.toCommandArgumentForPythonExt()}` : `${c.toCommandArgumentForPythonExt()}`), + '', + ); + return shellExec(quoted, { timeout: 15000 }); } } diff --git a/src/client/common/process/pythonEnvironment.ts b/src/client/common/process/pythonEnvironment.ts index 579ac015e547..f4c6f8954126 100644 --- a/src/client/common/process/pythonEnvironment.ts +++ b/src/client/common/process/pythonEnvironment.ts @@ -47,7 +47,7 @@ class PythonEnvironment implements IPythonEnvironment { return this.cachedInterpreterInformation; } - public async getExecutablePath(options?: { throwOnError?: boolean }): Promise { + public async getExecutablePath(): Promise { // If we've passed the python file, then return the file. // This is because on mac if using the interpreter /usr/bin/python2.7 we can get a different value for the path if (await this.deps.isValidExecutable(this.pythonPath)) { @@ -59,7 +59,7 @@ class PythonEnvironment implements IPythonEnvironment { return result; } const python = this.getExecutionInfo(); - const promise = getExecutablePath(python, this.deps.shellExec, options); + const promise = getExecutablePath(python, this.deps.shellExec); cachedExecutablePath.set(this.pythonPath, promise); return promise; } diff --git a/src/client/common/process/pythonExecutionFactory.ts b/src/client/common/process/pythonExecutionFactory.ts index 1120b61c9467..fc13e7f2346c 100644 --- a/src/client/common/process/pythonExecutionFactory.ts +++ b/src/client/common/process/pythonExecutionFactory.ts @@ -145,7 +145,7 @@ function createPythonService(procService: IProcessService, env: IPythonEnvironme const procs = createPythonProcessService(procService, env); return { getInterpreterInformation: () => env.getInterpreterInformation(), - getExecutablePath: (options?: { throwOnError?: boolean }) => env.getExecutablePath(options), + getExecutablePath: () => env.getExecutablePath(), isModuleInstalled: (m) => env.isModuleInstalled(m), getModuleVersion: (m) => env.getModuleVersion(m), getExecutionInfo: (a) => env.getExecutionInfo(a), diff --git a/src/client/common/process/types.ts b/src/client/common/process/types.ts index 16ffdc50919c..bcab76e66b09 100644 --- a/src/client/common/process/types.ts +++ b/src/client/common/process/types.ts @@ -84,7 +84,7 @@ export const IPythonExecutionService = Symbol('IPythonExecutionService'); export interface IPythonExecutionService { getInterpreterInformation(): Promise; - getExecutablePath(options?: { throwOnError?: boolean }): Promise; + getExecutablePath(): Promise; isModuleInstalled(moduleName: string): Promise; getModuleVersion(moduleName: string): Promise; getExecutionInfo(pythonArgs?: string[]): PythonExecInfo; @@ -100,7 +100,7 @@ export interface IPythonExecutionService { export interface IPythonEnvironment { getInterpreterInformation(): Promise; getExecutionObservableInfo(pythonArgs?: string[], pythonExecutable?: string): PythonExecInfo; - getExecutablePath(options?: { throwOnError?: boolean }): Promise; + getExecutablePath(): Promise; isModuleInstalled(moduleName: string): Promise; getModuleVersion(moduleName: string): Promise; getExecutionInfo(pythonArgs?: string[], pythonExecutable?: string): PythonExecInfo; diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index b8ce7d5fc623..0aaf80c92b35 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -49,7 +49,7 @@ export namespace Diagnostics { export namespace Common { export const allow = l10n.t('Allow'); - export const instructions = l10n.t('Instructions'); + export const seeInstructions = l10n.t('See Instructions'); export const close = l10n.t('Close'); export const bannerLabelYes = l10n.t('Yes'); export const bannerLabelNo = l10n.t('No'); diff --git a/src/client/pythonEnvironments/info/executable.ts b/src/client/pythonEnvironments/info/executable.ts index c221a96741a2..70c74329c49b 100644 --- a/src/client/pythonEnvironments/info/executable.ts +++ b/src/client/pythonEnvironments/info/executable.ts @@ -14,11 +14,7 @@ import { copyPythonExecInfo, PythonExecInfo } from '../exec'; * @param python - the information to use when running Python * @param shellExec - the function to use to run Python */ -export async function getExecutablePath( - python: PythonExecInfo, - shellExec: ShellExecFunc, - options?: { throwOnError?: boolean }, -): Promise { +export async function getExecutablePath(python: PythonExecInfo, shellExec: ShellExecFunc): Promise { try { const [args, parse] = getExecutable(); const info = copyPythonExecInfo(python, args); @@ -36,9 +32,6 @@ export async function getExecutablePath( return executable; } catch (ex) { traceError(ex); - if (options?.throwOnError) { - throw ex; - } return undefined; } } From b7a14296e4d2796f406f458d47976580c4737bf1 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 28 Mar 2023 14:06:26 -0700 Subject: [PATCH 3/9] Add diagnostic for incomplete path env var --- .../diagnostics/checks/pythonInterpreter.ts | 47 +++++++++++++++++-- .../application/diagnostics/constants.ts | 2 + 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index 58903c9e9768..ae56d600421c 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -28,11 +28,12 @@ import { EventName } from '../../../telemetry/constants'; import { IExtensionSingleActivationService } from '../../../activation/types'; import { cache } from '../../../common/utils/decorators'; import { noop } from '../../../common/utils/misc'; -import { getOSType, OSType } from '../../../common/utils/platform'; +import { getEnvironmentVariable, getOSType, OSType } from '../../../common/utils/platform'; import { IFileSystem } from '../../../common/platform/types'; import { traceError } from '../../../logging'; import { getExecutable } from '../../../common/process/internal/python'; import { shellExec } from '../../../common/process/rawProcessApis'; +import { getSearchPathEnvVarNames } from '../../../common/utils/exec'; const messages = { [DiagnosticCodes.NoPythonInterpretersDiagnostic]: l10n.t( @@ -44,6 +45,9 @@ const messages = { [DiagnosticCodes.InvalidComspecDiagnostic]: l10n.t( 'We detected an issue with one of your environment variables that breaks features such as IntelliSense, linting and debugging. Try setting the "ComSpec" variable to a valid Command Prompt path in your system to fix it.', ), + [DiagnosticCodes.IncompletePathVarDiagnostic]: l10n.t( + 'We detected an issue with "Path" environment variable that breaks features such as IntelliSense, linting and debugging. Please edit it to make sure it contains the "SystemRoot" subdirectories.', + ), }; export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic { @@ -70,7 +74,11 @@ export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic { } export class DefaultShellDiagnostic extends BaseDiagnostic { - constructor(code: DiagnosticCodes.InvalidComspecDiagnostic, resource: Resource, scope = DiagnosticScope.Global) { + constructor( + code: DiagnosticCodes.InvalidComspecDiagnostic | DiagnosticCodes.IncompletePathVarDiagnostic, + resource: Resource, + scope = DiagnosticScope.Global, + ) { super(code, messages[code], DiagnosticSeverity.Error, scope, resource, undefined, 'always'); } } @@ -91,6 +99,7 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService DiagnosticCodes.NoPythonInterpretersDiagnostic, DiagnosticCodes.InvalidPythonInterpreterDiagnostic, DiagnosticCodes.InvalidComspecDiagnostic, + DiagnosticCodes.IncompletePathVarDiagnostic, ], serviceContainer, disposableRegistry, @@ -121,7 +130,7 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService const workspaceService = this.serviceContainer.get(IWorkspaceService); const interpreterService = this.serviceContainer.get(IInterpreterService); const diagnostics = await this.diagnoseDefaultShell(resource); - if (diagnostics.length) { + if (diagnostics.length > 0) { return diagnostics; } const hasInterpreters = await interpreterService.hasInterpreters(); @@ -195,6 +204,17 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService }, ]; } + if (diagnostic.code === DiagnosticCodes.IncompletePathVarDiagnostic) { + return [ + { + prompt: Common.seeInstructions, + command: commandFactory.createCommand(diagnostic, { + type: 'launch', + options: 'https://aka.ms/AAk744c', + }), + }, + ]; + } const prompts = [ { prompt: Common.selectPythonInterpreter, @@ -232,20 +252,37 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService if ((ex as any).errno === -4058) { // ENOENT (-4058) error is thrown by Node when the default shell is invalid. if (await this.isComspecInvalid()) { - traceError('ComSpec is set to an invalid value', process.env.ComSpec); + traceError('ComSpec is set to an invalid value', getEnvironmentVariable('ComSpec')); return [new DefaultShellDiagnostic(DiagnosticCodes.InvalidComspecDiagnostic, resource)]; } + if (this.isPathVarIncomplete()) { + traceError('PATH env var appears to be incomplete', process.env.Path, process.env.PATH); + return [new DefaultShellDiagnostic(DiagnosticCodes.IncompletePathVarDiagnostic, resource)]; + } } } return []; } private async isComspecInvalid() { - const comSpec = process.env.ComSpec ?? ''; + const comSpec = getEnvironmentVariable('ComSpec') ?? ''; const fs = this.serviceContainer.get(IFileSystem); return fs.fileExists(comSpec).then((exists) => !exists); } + // eslint-disable-next-line class-methods-use-this + private isPathVarIncomplete() { + const envVars = getSearchPathEnvVarNames(); + const systemRoot = getEnvironmentVariable('SystemRoot') ?? 'C:\\WINDOWS'; + for (const envVar of envVars) { + const value = getEnvironmentVariable(envVar); + if (value?.includes(systemRoot)) { + return false; + } + } + return true; + } + @cache(-1, true) // eslint-disable-next-line class-methods-use-this private async shellExecPython() { diff --git a/src/client/application/diagnostics/constants.ts b/src/client/application/diagnostics/constants.ts index 9d47d217d443..ca2867fc4f49 100644 --- a/src/client/application/diagnostics/constants.ts +++ b/src/client/application/diagnostics/constants.ts @@ -13,6 +13,8 @@ export enum DiagnosticCodes { EnvironmentActivationInPowerShellWithBatchFilesNotSupportedDiagnostic = 'EnvironmentActivationInPowerShellWithBatchFilesNotSupportedDiagnostic', InvalidPythonInterpreterDiagnostic = 'InvalidPythonInterpreterDiagnostic', InvalidComspecDiagnostic = 'InvalidComspecDiagnostic', + IncompletePathVarDiagnostic = 'IncompletePathVarDiagnostic', + DefaultShellErrorDiagnostic = 'DefaultShellErrorDiagnostic', LSNotSupportedDiagnostic = 'LSNotSupportedDiagnostic', PythonPathDeprecatedDiagnostic = 'PythonPathDeprecatedDiagnostic', JustMyCodeDiagnostic = 'JustMyCodeDiagnostic', From c04de203c00a91122e0599b88b4435e44639fcc5 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 28 Mar 2023 14:18:38 -0700 Subject: [PATCH 4/9] Add diagnostic if we're unable to determine the exact cause --- .../diagnostics/checks/pythonInterpreter.ts | 144 ++++++++++-------- 1 file changed, 81 insertions(+), 63 deletions(-) diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index ae56d600421c..e1f51f5b3d12 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -48,6 +48,9 @@ const messages = { [DiagnosticCodes.IncompletePathVarDiagnostic]: l10n.t( 'We detected an issue with "Path" environment variable that breaks features such as IntelliSense, linting and debugging. Please edit it to make sure it contains the "SystemRoot" subdirectories.', ), + [DiagnosticCodes.DefaultShellErrorDiagnostic]: l10n.t( + 'We detected an issue with your default shell that breaks features such as IntelliSense, linting and debugging. Try resetting "Comspec" and "Path" environment variables to fix it.', + ), }; export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic { @@ -75,7 +78,10 @@ export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic { export class DefaultShellDiagnostic extends BaseDiagnostic { constructor( - code: DiagnosticCodes.InvalidComspecDiagnostic | DiagnosticCodes.IncompletePathVarDiagnostic, + code: + | DiagnosticCodes.InvalidComspecDiagnostic + | DiagnosticCodes.IncompletePathVarDiagnostic + | DiagnosticCodes.DefaultShellErrorDiagnostic, resource: Resource, scope = DiagnosticScope.Global, ) { @@ -170,6 +176,69 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService return false; } + private async diagnoseDefaultShell(resource: Resource): Promise { + if (getOSType() !== OSType.Windows) { + return []; + } + const interpreterService = this.serviceContainer.get(IInterpreterService); + const currentInterpreter = await interpreterService.getActiveInterpreter(resource); + if (currentInterpreter) { + return []; + } + try { + await this.shellExecPython(); + } catch (ex) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + if ((ex as any).errno === -4058) { + // ENOENT (-4058) error is thrown by Node when the default shell is invalid. + if (await this.isComspecInvalid()) { + traceError('ComSpec is set to an invalid value', getEnvironmentVariable('ComSpec')); + return [new DefaultShellDiagnostic(DiagnosticCodes.InvalidComspecDiagnostic, resource)]; + } + if (this.isPathVarIncomplete()) { + traceError('PATH env var appears to be incomplete', process.env.Path, process.env.PATH); + return [new DefaultShellDiagnostic(DiagnosticCodes.IncompletePathVarDiagnostic, resource)]; + } + return [new DefaultShellDiagnostic(DiagnosticCodes.DefaultShellErrorDiagnostic, resource)]; + } + } + return []; + } + + private async isComspecInvalid() { + const comSpec = getEnvironmentVariable('ComSpec') ?? ''; + const fs = this.serviceContainer.get(IFileSystem); + return fs.fileExists(comSpec).then((exists) => !exists); + } + + // eslint-disable-next-line class-methods-use-this + private isPathVarIncomplete() { + const envVars = getSearchPathEnvVarNames(); + const systemRoot = getEnvironmentVariable('SystemRoot') ?? 'C:\\WINDOWS'; + for (const envVar of envVars) { + const value = getEnvironmentVariable(envVar); + if (value?.includes(systemRoot)) { + return false; + } + } + return true; + } + + @cache(-1, true) + // eslint-disable-next-line class-methods-use-this + private async shellExecPython() { + const configurationService = this.serviceContainer.get(IConfigurationService); + const { pythonPath } = configurationService.getSettings(); + const [args] = getExecutable(); + const argv = [pythonPath, ...args]; + // Concat these together to make a set of quoted strings + const quoted = argv.reduce( + (p, c) => (p ? `${p} ${c.toCommandArgumentForPythonExt()}` : `${c.toCommandArgumentForPythonExt()}`), + '', + ); + return shellExec(quoted, { timeout: 15000 }); + } + @cache(1000, true) // This is to handle throttling of multiple events. protected async onHandle(diagnostics: IDiagnostic[]): Promise { if (diagnostics.length === 0) { @@ -215,6 +284,17 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService }, ]; } + if (diagnostic.code === DiagnosticCodes.DefaultShellErrorDiagnostic) { + return [ + { + prompt: Common.seeInstructions, + command: commandFactory.createCommand(diagnostic, { + type: 'launch', + options: 'https://aka.ms/AAk744c', + }), + }, + ]; + } const prompts = [ { prompt: Common.selectPythonInterpreter, @@ -235,68 +315,6 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService } return prompts; } - - private async diagnoseDefaultShell(resource: Resource): Promise { - if (getOSType() !== OSType.Windows) { - return []; - } - const interpreterService = this.serviceContainer.get(IInterpreterService); - const currentInterpreter = await interpreterService.getActiveInterpreter(resource); - if (currentInterpreter) { - return []; - } - try { - await this.shellExecPython(); - } catch (ex) { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - if ((ex as any).errno === -4058) { - // ENOENT (-4058) error is thrown by Node when the default shell is invalid. - if (await this.isComspecInvalid()) { - traceError('ComSpec is set to an invalid value', getEnvironmentVariable('ComSpec')); - return [new DefaultShellDiagnostic(DiagnosticCodes.InvalidComspecDiagnostic, resource)]; - } - if (this.isPathVarIncomplete()) { - traceError('PATH env var appears to be incomplete', process.env.Path, process.env.PATH); - return [new DefaultShellDiagnostic(DiagnosticCodes.IncompletePathVarDiagnostic, resource)]; - } - } - } - return []; - } - - private async isComspecInvalid() { - const comSpec = getEnvironmentVariable('ComSpec') ?? ''; - const fs = this.serviceContainer.get(IFileSystem); - return fs.fileExists(comSpec).then((exists) => !exists); - } - - // eslint-disable-next-line class-methods-use-this - private isPathVarIncomplete() { - const envVars = getSearchPathEnvVarNames(); - const systemRoot = getEnvironmentVariable('SystemRoot') ?? 'C:\\WINDOWS'; - for (const envVar of envVars) { - const value = getEnvironmentVariable(envVar); - if (value?.includes(systemRoot)) { - return false; - } - } - return true; - } - - @cache(-1, true) - // eslint-disable-next-line class-methods-use-this - private async shellExecPython() { - const configurationService = this.serviceContainer.get(IConfigurationService); - const { pythonPath } = configurationService.getSettings(); - const [args] = getExecutable(); - const argv = [pythonPath, ...args]; - // Concat these together to make a set of quoted strings - const quoted = argv.reduce( - (p, c) => (p ? `${p} ${c.toCommandArgumentForPythonExt()}` : `${c.toCommandArgumentForPythonExt()}`), - '', - ); - return shellExec(quoted, { timeout: 15000 }); - } } function getOnCloseHandler(diagnostic: IDiagnostic): IDiagnosticMessageOnCloseHandler | undefined { From 606d501d7608e946863135d3393d0707c8d3b69e Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 28 Mar 2023 14:20:14 -0700 Subject: [PATCH 5/9] Update link --- src/client/application/diagnostics/checks/pythonInterpreter.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index e1f51f5b3d12..4d94b4b72939 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -290,7 +290,7 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService prompt: Common.seeInstructions, command: commandFactory.createCommand(diagnostic, { type: 'launch', - options: 'https://aka.ms/AAk744c', + options: 'https://aka.ms/AAk7qix', }), }, ]; From de7de5f305ec5ac51b93dcd8ff92360dd7bdc228 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 28 Mar 2023 14:27:48 -0700 Subject: [PATCH 6/9] Refactor --- .../diagnostics/checks/pythonInterpreter.ts | 52 +++++++------------ 1 file changed, 19 insertions(+), 33 deletions(-) diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index 4d94b4b72939..a82738afaade 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -76,15 +76,13 @@ export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic { } } +type DefaultShellDiagnostics = + | DiagnosticCodes.InvalidComspecDiagnostic + | DiagnosticCodes.IncompletePathVarDiagnostic + | DiagnosticCodes.DefaultShellErrorDiagnostic; + export class DefaultShellDiagnostic extends BaseDiagnostic { - constructor( - code: - | DiagnosticCodes.InvalidComspecDiagnostic - | DiagnosticCodes.IncompletePathVarDiagnostic - | DiagnosticCodes.DefaultShellErrorDiagnostic, - resource: Resource, - scope = DiagnosticScope.Global, - ) { + constructor(code: DefaultShellDiagnostics, resource: Resource, scope = DiagnosticScope.Global) { super(code, messages[code], DiagnosticSeverity.Error, scope, resource, undefined, 'always'); } } @@ -106,6 +104,7 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService DiagnosticCodes.InvalidPythonInterpreterDiagnostic, DiagnosticCodes.InvalidComspecDiagnostic, DiagnosticCodes.IncompletePathVarDiagnostic, + DiagnosticCodes.DefaultShellErrorDiagnostic, ], serviceContainer, disposableRegistry, @@ -191,8 +190,8 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService // eslint-disable-next-line @typescript-eslint/no-explicit-any if ((ex as any).errno === -4058) { // ENOENT (-4058) error is thrown by Node when the default shell is invalid. + traceError('ComSpec is likely set to an invalid value', getEnvironmentVariable('ComSpec')); if (await this.isComspecInvalid()) { - traceError('ComSpec is set to an invalid value', getEnvironmentVariable('ComSpec')); return [new DefaultShellDiagnostic(DiagnosticCodes.InvalidComspecDiagnostic, resource)]; } if (this.isPathVarIncomplete()) { @@ -262,35 +261,22 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService private getCommandPrompts(diagnostic: IDiagnostic): { prompt: string; command?: IDiagnosticCommand }[] { const commandFactory = this.serviceContainer.get(IDiagnosticsCommandFactory); - if (diagnostic.code === DiagnosticCodes.InvalidComspecDiagnostic) { - return [ - { - prompt: Common.seeInstructions, - command: commandFactory.createCommand(diagnostic, { - type: 'launch', - options: 'https://aka.ms/AAk3djo', - }), - }, - ]; - } - if (diagnostic.code === DiagnosticCodes.IncompletePathVarDiagnostic) { - return [ - { - prompt: Common.seeInstructions, - command: commandFactory.createCommand(diagnostic, { - type: 'launch', - options: 'https://aka.ms/AAk744c', - }), - }, - ]; - } - if (diagnostic.code === DiagnosticCodes.DefaultShellErrorDiagnostic) { + if ( + diagnostic.code === DiagnosticCodes.InvalidComspecDiagnostic || + diagnostic.code === DiagnosticCodes.IncompletePathVarDiagnostic || + diagnostic.code === DiagnosticCodes.DefaultShellErrorDiagnostic + ) { + const links: Record = { + InvalidComspecDiagnostic: 'https://aka.ms/AAk3djo', + IncompletePathVarDiagnostic: 'https://aka.ms/AAk744c', + DefaultShellErrorDiagnostic: 'https://aka.ms/AAk7qix', + }; return [ { prompt: Common.seeInstructions, command: commandFactory.createCommand(diagnostic, { type: 'launch', - options: 'https://aka.ms/AAk7qix', + options: links[diagnostic.code], }), }, ]; From 41803ff6d03e2afcbac6b032c9c3f8a9763e4a9a Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 28 Mar 2023 15:55:49 -0700 Subject: [PATCH 7/9] Add tests --- .../diagnostics/checks/pythonInterpreter.ts | 6 +- .../checks/pythonInterpreter.unit.test.ts | 218 ++++++++++++++++-- 2 files changed, 205 insertions(+), 19 deletions(-) diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index a82738afaade..9de40ae43401 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -32,8 +32,8 @@ import { getEnvironmentVariable, getOSType, OSType } from '../../../common/utils import { IFileSystem } from '../../../common/platform/types'; import { traceError } from '../../../logging'; import { getExecutable } from '../../../common/process/internal/python'; -import { shellExec } from '../../../common/process/rawProcessApis'; import { getSearchPathEnvVarNames } from '../../../common/utils/exec'; +import { IProcessServiceFactory } from '../../../common/process/types'; const messages = { [DiagnosticCodes.NoPythonInterpretersDiagnostic]: l10n.t( @@ -235,7 +235,9 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService (p, c) => (p ? `${p} ${c.toCommandArgumentForPythonExt()}` : `${c.toCommandArgumentForPythonExt()}`), '', ); - return shellExec(quoted, { timeout: 15000 }); + const processServiceFactory = this.serviceContainer.get(IProcessServiceFactory); + const service = await processServiceFactory.create(); + return service.shellExec(quoted, { timeout: 15000 }); } @cache(1000, true) // This is to handle throttling of multiple events. diff --git a/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts b/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts index ea9bc9ae62d5..340f1b9f5bfa 100644 --- a/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts +++ b/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts @@ -9,6 +9,7 @@ import { EventEmitter, Uri } from 'vscode'; import { BaseDiagnosticsService } from '../../../../client/application/diagnostics/base'; import { InvalidLaunchJsonDebuggerDiagnostic } from '../../../../client/application/diagnostics/checks/invalidLaunchJsonDebugger'; import { + DefaultShellDiagnostic, InvalidPythonInterpreterDiagnostic, InvalidPythonInterpreterService, } from '../../../../client/application/diagnostics/checks/pythonInterpreter'; @@ -27,11 +28,18 @@ import { import { CommandsWithoutArgs } from '../../../../client/common/application/commands'; import { ICommandManager, IWorkspaceService } from '../../../../client/common/application/types'; import { Commands } from '../../../../client/common/constants'; -import { IPlatformService } from '../../../../client/common/platform/types'; -import { IDisposable, IDisposableRegistry, IInterpreterPathService, Resource } from '../../../../client/common/types'; +import { IFileSystem, IPlatformService } from '../../../../client/common/platform/types'; +import { IProcessService, IProcessServiceFactory } from '../../../../client/common/process/types'; +import { + IConfigurationService, + IDisposable, + IDisposableRegistry, + IInterpreterPathService, + Resource, +} from '../../../../client/common/types'; import { Common } from '../../../../client/common/utils/localize'; import { noop } from '../../../../client/common/utils/misc'; -import { IInterpreterHelper, IInterpreterService } from '../../../../client/interpreter/contracts'; +import { IInterpreterService } from '../../../../client/interpreter/contracts'; import { IServiceContainer } from '../../../../client/ioc/types'; import { EnvironmentType, PythonEnvironment } from '../../../../client/pythonEnvironments/info'; import { sleep } from '../../../core'; @@ -44,13 +52,28 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { let platformService: typemoq.IMock; let workspaceService: typemoq.IMock; let commandManager: typemoq.IMock; - let helper: typemoq.IMock; + let configService: typemoq.IMock; + let fs: typemoq.IMock; let serviceContainer: typemoq.IMock; + let processService: typemoq.IMock; let interpreterPathService: typemoq.IMock; + const oldComSpec = process.env.ComSpec; + const oldPath = process.env.Path; function createContainer() { + fs = typemoq.Mock.ofType(); + fs.setup((f) => f.fileExists(process.env.ComSpec ?? 'exists')).returns(() => Promise.resolve(true)); serviceContainer = typemoq.Mock.ofType(); + processService = typemoq.Mock.ofType(); + const processServiceFactory = typemoq.Mock.ofType(); + processServiceFactory.setup((p) => p.create()).returns(() => Promise.resolve(processService.object)); + serviceContainer + .setup((s) => s.get(typemoq.It.isValue(IProcessServiceFactory))) + .returns(() => processServiceFactory.object); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + processService.setup((p) => (p as any).then).returns(() => undefined); workspaceService = typemoq.Mock.ofType(); commandManager = typemoq.Mock.ofType(); + serviceContainer.setup((s) => s.get(typemoq.It.isValue(IFileSystem))).returns(() => fs.object); serviceContainer.setup((s) => s.get(typemoq.It.isValue(ICommandManager))).returns(() => commandManager.object); workspaceService.setup((w) => w.workspaceFile).returns(() => undefined); serviceContainer @@ -82,8 +105,11 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { serviceContainer .setup((s) => s.get(typemoq.It.isValue(IInterpreterPathService))) .returns(() => interpreterPathService.object); - helper = typemoq.Mock.ofType(); - serviceContainer.setup((s) => s.get(typemoq.It.isValue(IInterpreterHelper))).returns(() => helper.object); + configService = typemoq.Mock.ofType(); + configService.setup((c) => c.getSettings()).returns(() => ({ pythonPath: 'pythonPath' } as any)); + serviceContainer + .setup((s) => s.get(typemoq.It.isValue(IConfigurationService))) + .returns(() => configService.object); serviceContainer.setup((s) => s.get(typemoq.It.isValue(IDisposableRegistry))).returns(() => []); return serviceContainer.object; } @@ -102,6 +128,11 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { (diagnosticService as any)._clear(); }); + teardown(() => { + process.env.ComSpec = oldComSpec; + process.env.Path = oldPath; + }); + test('Registers command to trigger environment prompts', async () => { let triggerFunction: ((resource: Resource) => Promise) | undefined; commandManager @@ -191,7 +222,61 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { 'not the same', ); }); - test('Should return invalid diagnostics if there are interpreters but no current interpreter', async () => { + test('Should return comspec diagnostics if comspec is configured incorrectly', async () => { + // No interpreter should exist if comspec is incorrectly configured. + interpreterService + .setup((i) => i.getActiveInterpreter(typemoq.It.isAny())) + .returns(() => { + return Promise.resolve(undefined); + }); + // Should fail with this error code if comspec is incorrectly configured. + processService + .setup((p) => p.shellExec(typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => Promise.reject({ errno: -4058 })); + // Should be set to an invalid value in this case. + process.env.ComSpec = 'doesNotExist'; + fs.setup((f) => f.fileExists('doesNotExist')).returns(() => Promise.resolve(false)); + + const diagnostics = await diagnosticService._manualDiagnose(undefined); + expect(diagnostics).to.be.deep.equal( + [new DefaultShellDiagnostic(DiagnosticCodes.InvalidComspecDiagnostic, undefined)], + 'not the same', + ); + }); + test('Should return incomplete path diagnostics if `Path` variable is incomplete and execution fails', async () => { + // No interpreter should exist if execution is failing. + interpreterService + .setup((i) => i.getActiveInterpreter(typemoq.It.isAny())) + .returns(() => { + return Promise.resolve(undefined); + }); + processService + .setup((p) => p.shellExec(typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => Promise.reject({ errno: -4058 })); + process.env.Path = 'SystemRootDoesNotExist'; + const diagnostics = await diagnosticService._manualDiagnose(undefined); + expect(diagnostics).to.be.deep.equal( + [new DefaultShellDiagnostic(DiagnosticCodes.IncompletePathVarDiagnostic, undefined)], + 'not the same', + ); + }); + test('Should return default shell error diagnostic if execution fails but we do not identify the cause', async () => { + // No interpreter should exist if execution is failing. + interpreterService + .setup((i) => i.getActiveInterpreter(typemoq.It.isAny())) + .returns(() => { + return Promise.resolve(undefined); + }); + processService + .setup((p) => p.shellExec(typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => Promise.reject({ errno: -4058 })); + const diagnostics = await diagnosticService._manualDiagnose(undefined); + expect(diagnostics).to.be.deep.equal( + [new DefaultShellDiagnostic(DiagnosticCodes.DefaultShellErrorDiagnostic, undefined)], + 'not the same', + ); + }); + test('Should return invalid interpreter diagnostics if there are interpreters but no current interpreter', async () => { interpreterService .setup((i) => i.hasInterpreters()) .returns(() => Promise.resolve(true)) @@ -200,8 +285,7 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { .setup((i) => i.getActiveInterpreter(typemoq.It.isAny())) .returns(() => { return Promise.resolve(undefined); - }) - .verifiable(typemoq.Times.once()); + }); const diagnostics = await diagnosticService._manualDiagnose(undefined); expect(diagnostics).to.be.deep.equal( @@ -214,24 +298,124 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { ], 'not the same', ); - interpreterService.verifyAll(); }); test('Should return empty diagnostics if there are interpreters and a current interpreter', async () => { - interpreterService - .setup((i) => i.hasInterpreters()) - .returns(() => Promise.resolve(true)) - .verifiable(typemoq.Times.once()); + interpreterService.setup((i) => i.hasInterpreters()).returns(() => Promise.resolve(true)); interpreterService .setup((i) => i.getActiveInterpreter(typemoq.It.isAny())) .returns(() => { return Promise.resolve({ envType: EnvironmentType.Unknown } as any); - }) - .verifiable(typemoq.Times.once()); + }); const diagnostics = await diagnosticService._manualDiagnose(undefined); expect(diagnostics).to.be.deep.equal([], 'not the same'); - interpreterService.verifyAll(); }); + + test('Handling comspec diagnostic should launch expected browser link', async () => { + const diagnostic = new DefaultShellDiagnostic(DiagnosticCodes.InvalidComspecDiagnostic, undefined); + const cmd = ({} as any) as IDiagnosticCommand; + let messagePrompt: MessageCommandPrompt | undefined; + messageHandler + .setup((i) => i.handle(typemoq.It.isValue(diagnostic), typemoq.It.isAny())) + .callback((_d, p: MessageCommandPrompt) => (messagePrompt = p)) + .returns(() => Promise.resolve()) + .verifiable(typemoq.Times.once()); + commandFactory + .setup((f) => + f.createCommand( + typemoq.It.isAny(), + typemoq.It.isObjectWith>({ + type: 'launch', + options: 'https://aka.ms/AAk3djo', + }), + ), + ) + .returns(() => cmd) + .verifiable(typemoq.Times.once()); + + await diagnosticService.handle([diagnostic]); + + messageHandler.verifyAll(); + commandFactory.verifyAll(); + expect(messagePrompt).not.be.equal(undefined, 'Message prompt not set'); + expect(messagePrompt!.commandPrompts).to.be.deep.equal([ + { + prompt: Common.seeInstructions, + command: cmd, + }, + ]); + }); + + test('Handling incomplete path diagnostic should launch expected browser link', async () => { + const diagnostic = new DefaultShellDiagnostic(DiagnosticCodes.IncompletePathVarDiagnostic, undefined); + const cmd = ({} as any) as IDiagnosticCommand; + let messagePrompt: MessageCommandPrompt | undefined; + messageHandler + .setup((i) => i.handle(typemoq.It.isValue(diagnostic), typemoq.It.isAny())) + .callback((_d, p: MessageCommandPrompt) => (messagePrompt = p)) + .returns(() => Promise.resolve()) + .verifiable(typemoq.Times.once()); + commandFactory + .setup((f) => + f.createCommand( + typemoq.It.isAny(), + typemoq.It.isObjectWith>({ + type: 'launch', + options: 'https://aka.ms/AAk744c', + }), + ), + ) + .returns(() => cmd) + .verifiable(typemoq.Times.once()); + + await diagnosticService.handle([diagnostic]); + + messageHandler.verifyAll(); + commandFactory.verifyAll(); + expect(messagePrompt).not.be.equal(undefined, 'Message prompt not set'); + expect(messagePrompt!.commandPrompts).to.be.deep.equal([ + { + prompt: Common.seeInstructions, + command: cmd, + }, + ]); + }); + + test('Handling default shell error diagnostic should launch expected browser link', async () => { + const diagnostic = new DefaultShellDiagnostic(DiagnosticCodes.DefaultShellErrorDiagnostic, undefined); + const cmd = ({} as any) as IDiagnosticCommand; + let messagePrompt: MessageCommandPrompt | undefined; + messageHandler + .setup((i) => i.handle(typemoq.It.isValue(diagnostic), typemoq.It.isAny())) + .callback((_d, p: MessageCommandPrompt) => (messagePrompt = p)) + .returns(() => Promise.resolve()) + .verifiable(typemoq.Times.once()); + commandFactory + .setup((f) => + f.createCommand( + typemoq.It.isAny(), + typemoq.It.isObjectWith>({ + type: 'launch', + options: 'https://aka.ms/AAk7qix', + }), + ), + ) + .returns(() => cmd) + .verifiable(typemoq.Times.once()); + + await diagnosticService.handle([diagnostic]); + + messageHandler.verifyAll(); + commandFactory.verifyAll(); + expect(messagePrompt).not.be.equal(undefined, 'Message prompt not set'); + expect(messagePrompt!.commandPrompts).to.be.deep.equal([ + { + prompt: Common.seeInstructions, + command: cmd, + }, + ]); + }); + test('Handling no interpreters diagnostic should return select interpreter cmd', async () => { const diagnostic = new InvalidPythonInterpreterDiagnostic( DiagnosticCodes.NoPythonInterpretersDiagnostic, From 8ab01933bd48d65db29d4fb43d0acffc26bf9a11 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 28 Mar 2023 16:00:35 -0700 Subject: [PATCH 8/9] Add platform checks --- .../checks/pythonInterpreter.unit.test.ts | 42 +++++++++++++++++-- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts b/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts index 340f1b9f5bfa..dec9ad41298e 100644 --- a/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts +++ b/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts @@ -42,6 +42,7 @@ import { noop } from '../../../../client/common/utils/misc'; import { IInterpreterService } from '../../../../client/interpreter/contracts'; import { IServiceContainer } from '../../../../client/ioc/types'; import { EnvironmentType, PythonEnvironment } from '../../../../client/pythonEnvironments/info'; +import { getOSType, OSType } from '../../../common'; import { sleep } from '../../../core'; suite('Application Diagnostics - Checks Python Interpreter', () => { @@ -222,7 +223,10 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { 'not the same', ); }); - test('Should return comspec diagnostics if comspec is configured incorrectly', async () => { + test('Should return comspec diagnostics if comspec is configured incorrectly', async function () { + if (getOSType() !== OSType.Windows) { + return this.skip(); + } // No interpreter should exist if comspec is incorrectly configured. interpreterService .setup((i) => i.getActiveInterpreter(typemoq.It.isAny())) @@ -243,7 +247,10 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { 'not the same', ); }); - test('Should return incomplete path diagnostics if `Path` variable is incomplete and execution fails', async () => { + test('Should return incomplete path diagnostics if `Path` variable is incomplete and execution fails', async function () { + if (getOSType() !== OSType.Windows) { + return this.skip(); + } // No interpreter should exist if execution is failing. interpreterService .setup((i) => i.getActiveInterpreter(typemoq.It.isAny())) @@ -260,7 +267,10 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { 'not the same', ); }); - test('Should return default shell error diagnostic if execution fails but we do not identify the cause', async () => { + test('Should return default shell error diagnostic if execution fails but we do not identify the cause', async function () { + if (getOSType() !== OSType.Windows) { + return this.skip(); + } // No interpreter should exist if execution is failing. interpreterService .setup((i) => i.getActiveInterpreter(typemoq.It.isAny())) @@ -276,6 +286,32 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { 'not the same', ); }); + test('Should return invalid interpreter diagnostics on non-Windows if there is no current interpreter and execution fails', async function () { + if (getOSType() === OSType.Windows) { + return this.skip(); + } + interpreterService.setup((i) => i.hasInterpreters()).returns(() => Promise.resolve(false)); + // No interpreter should exist if execution is failing. + interpreterService + .setup((i) => i.getActiveInterpreter(typemoq.It.isAny())) + .returns(() => { + return Promise.resolve(undefined); + }); + processService + .setup((p) => p.shellExec(typemoq.It.isAny(), typemoq.It.isAny())) + .returns(() => Promise.reject({ errno: -4058 })); + const diagnostics = await diagnosticService._manualDiagnose(undefined); + expect(diagnostics).to.be.deep.equal( + [ + new InvalidPythonInterpreterDiagnostic( + DiagnosticCodes.InvalidPythonInterpreterDiagnostic, + undefined, + workspaceService.object, + ), + ], + 'not the same', + ); + }); test('Should return invalid interpreter diagnostics if there are interpreters but no current interpreter', async () => { interpreterService .setup((i) => i.hasInterpreters()) From 461dd2e82e2c8ba454504985ba1e6c77ce6d771c Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 28 Mar 2023 16:39:06 -0700 Subject: [PATCH 9/9] Code review --- src/client/application/diagnostics/checks/pythonInterpreter.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index 9de40ae43401..8fc1ce0c0a90 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -49,7 +49,7 @@ const messages = { 'We detected an issue with "Path" environment variable that breaks features such as IntelliSense, linting and debugging. Please edit it to make sure it contains the "SystemRoot" subdirectories.', ), [DiagnosticCodes.DefaultShellErrorDiagnostic]: l10n.t( - 'We detected an issue with your default shell that breaks features such as IntelliSense, linting and debugging. Try resetting "Comspec" and "Path" environment variables to fix it.', + 'We detected an issue with your default shell that breaks features such as IntelliSense, linting and debugging. Try resetting "ComSpec" and "Path" environment variables to fix it.', ), }; @@ -176,6 +176,7 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService } private async diagnoseDefaultShell(resource: Resource): Promise { + await this.isPathVarIncomplete(); if (getOSType() !== OSType.Windows) { return []; }