Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add diagnostic to validate ComSpec #20927

Merged
merged 9 commits into from
Mar 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 126 additions & 5 deletions src/client/application/diagnostics/checks/pythonInterpreter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -28,6 +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 { 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 { getSearchPathEnvVarNames } from '../../../common/utils/exec';
import { IProcessServiceFactory } from '../../../common/process/types';

const messages = {
[DiagnosticCodes.NoPythonInterpretersDiagnostic]: l10n.t(
Expand All @@ -36,6 +42,15 @@ 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(
'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.',
),
[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 {
Expand All @@ -61,6 +76,17 @@ export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic {
}
}

type DefaultShellDiagnostics =
| DiagnosticCodes.InvalidComspecDiagnostic
| DiagnosticCodes.IncompletePathVarDiagnostic
| DiagnosticCodes.DefaultShellErrorDiagnostic;

export class DefaultShellDiagnostic extends BaseDiagnostic {
constructor(code: DefaultShellDiagnostics, resource: Resource, scope = DiagnosticScope.Global) {
super(code, messages[code], DiagnosticSeverity.Error, scope, resource, undefined, 'always');
}
}

export const InvalidPythonInterpreterServiceId = 'InvalidPythonInterpreterServiceId';

@injectable()
Expand All @@ -73,7 +99,13 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService
@inject(IDisposableRegistry) disposableRegistry: IDisposableRegistry,
) {
super(
[DiagnosticCodes.NoPythonInterpretersDiagnostic, DiagnosticCodes.InvalidPythonInterpreterDiagnostic],
[
DiagnosticCodes.NoPythonInterpretersDiagnostic,
DiagnosticCodes.InvalidPythonInterpreterDiagnostic,
DiagnosticCodes.InvalidComspecDiagnostic,
DiagnosticCodes.IncompletePathVarDiagnostic,
DiagnosticCodes.DefaultShellErrorDiagnostic,
],
serviceContainer,
disposableRegistry,
false,
Expand All @@ -95,14 +127,17 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService
);
}

// eslint-disable-next-line class-methods-use-this
public async diagnose(_resource: Resource): Promise<IDiagnostic[]> {
return [];
public async diagnose(resource: Resource): Promise<IDiagnostic[]> {
return this.diagnoseDefaultShell(resource);
}

public async _manualDiagnose(resource: Resource): Promise<IDiagnostic[]> {
const workspaceService = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
const interpreterService = this.serviceContainer.get<IInterpreterService>(IInterpreterService);
const diagnostics = await this.diagnoseDefaultShell(resource);
if (diagnostics.length > 0) {
return diagnostics;
}
const hasInterpreters = await interpreterService.hasInterpreters();
const interpreterPathService = this.serviceContainer.get<IInterpreterPathService>(IInterpreterPathService);
const isInterpreterSetToDefault = interpreterPathService.get(resource) === 'python';
Expand Down Expand Up @@ -140,6 +175,72 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService
return false;
}

private async diagnoseDefaultShell(resource: Resource): Promise<IDiagnostic[]> {
await this.isPathVarIncomplete();
if (getOSType() !== OSType.Windows) {
return [];
}
const interpreterService = this.serviceContainer.get<IInterpreterService>(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) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note we only show these diagnostics if a very particular error code is observed.

// 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()) {
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>(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>(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()}`),
'',
);
const processServiceFactory = this.serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory);
const service = await processServiceFactory.create();
return service.shellExec(quoted, { timeout: 15000 });
}

@cache(1000, true) // This is to handle throttling of multiple events.
protected async onHandle(diagnostics: IDiagnostic[]): Promise<void> {
if (diagnostics.length === 0) {
Expand All @@ -163,6 +264,26 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService

private getCommandPrompts(diagnostic: IDiagnostic): { prompt: string; command?: IDiagnosticCommand }[] {
const commandFactory = this.serviceContainer.get<IDiagnosticsCommandFactory>(IDiagnosticsCommandFactory);
if (
diagnostic.code === DiagnosticCodes.InvalidComspecDiagnostic ||
diagnostic.code === DiagnosticCodes.IncompletePathVarDiagnostic ||
diagnostic.code === DiagnosticCodes.DefaultShellErrorDiagnostic
) {
const links: Record<DefaultShellDiagnostics, string> = {
InvalidComspecDiagnostic: 'https://aka.ms/AAk3djo',
IncompletePathVarDiagnostic: 'https://aka.ms/AAk744c',
DefaultShellErrorDiagnostic: 'https://aka.ms/AAk7qix',
Comment on lines +273 to +275
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add info to wiki page. I am a bit worried about issues getting moved and comments getting lost,

What do you think? @luabud @cwebster-99

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to wiki page

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because this is aka.ms link, I don't think the PR should be blocked on the wiki page being created.
And let me know if you'd like help with the wiki page!

};
return [
{
prompt: Common.seeInstructions,
command: commandFactory.createCommand(diagnostic, {
type: 'launch',
options: links[diagnostic.code],
}),
},
];
}
const prompts = [
{
prompt: Common.selectPythonInterpreter,
Expand Down
3 changes: 3 additions & 0 deletions src/client/application/diagnostics/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ export enum DiagnosticCodes {
InvalidPythonPathInDebuggerLaunchDiagnostic = 'InvalidPythonPathInDebuggerLaunchDiagnostic',
EnvironmentActivationInPowerShellWithBatchFilesNotSupportedDiagnostic = 'EnvironmentActivationInPowerShellWithBatchFilesNotSupportedDiagnostic',
InvalidPythonInterpreterDiagnostic = 'InvalidPythonInterpreterDiagnostic',
InvalidComspecDiagnostic = 'InvalidComspecDiagnostic',
IncompletePathVarDiagnostic = 'IncompletePathVarDiagnostic',
DefaultShellErrorDiagnostic = 'DefaultShellErrorDiagnostic',
LSNotSupportedDiagnostic = 'LSNotSupportedDiagnostic',
PythonPathDeprecatedDiagnostic = 'PythonPathDeprecatedDiagnostic',
JustMyCodeDiagnostic = 'JustMyCodeDiagnostic',
Expand Down
1 change: 1 addition & 0 deletions src/client/common/utils/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export namespace Diagnostics {

export namespace Common {
export const allow = l10n.t('Allow');
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');
Expand Down
8 changes: 2 additions & 6 deletions src/client/pythonEnvironments/info/executable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
timeout?: number,
): Promise<string | undefined> {
export async function getExecutablePath(python: PythonExecInfo, shellExec: ShellExecFunc): Promise<string | undefined> {
try {
const [args, parse] = getExecutable();
const info = copyPythonExecInfo(python, args);
Expand All @@ -28,7 +24,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`);
Expand Down
Loading