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 warning when shell integration fails and create a terminal #152266

Merged
merged 14 commits into from
Jun 21, 2022
Merged
9 changes: 8 additions & 1 deletion src/vs/platform/terminal/common/terminal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,8 @@ export const enum ProcessPropertyType {
HasChildProcesses = 'hasChildProcesses',
ResolvedShellLaunchConfig = 'resolvedShellLaunchConfig',
OverrideDimensions = 'overrideDimensions',
FailedShellIntegrationActivation = 'failedShellIntegrationActivation'
FailedShellIntegrationActivation = 'failedShellIntegrationActivation',
UsedShellIntegrationInjection = 'usedShellIntegrationInjection'
}

export interface IProcessProperty<T extends ProcessPropertyType> {
Expand All @@ -234,6 +235,7 @@ export interface IProcessPropertyMap {
[ProcessPropertyType.ResolvedShellLaunchConfig]: IShellLaunchConfig;
[ProcessPropertyType.OverrideDimensions]: ITerminalDimensionsOverride | undefined;
[ProcessPropertyType.FailedShellIntegrationActivation]: boolean | undefined;
[ProcessPropertyType.UsedShellIntegrationInjection]: boolean | undefined;
}

export interface IFixedTerminalDimensions {
Expand Down Expand Up @@ -528,6 +530,11 @@ export interface IShellLaunchConfig {
* Opt-out of the default terminal persistence on restart and reload
*/
isTransient?: boolean;

/**
* Create a terminal without shell integration even when it's enabled
*/
ignoreShellIntegration?: boolean;
}

export interface ICreateContributedTerminalProfileOptions {
Expand Down
2 changes: 1 addition & 1 deletion src/vs/platform/terminal/node/terminalEnvironment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export function getShellIntegrationInjection(
// - The global setting is disabled
// - There is no executable (not sure what script to run)
// - The terminal is used by a feature like tasks or debugging
if (!options.enabled || !shellLaunchConfig.executable || shellLaunchConfig.isFeatureTerminal || shellLaunchConfig.hideFromUser) {
if (!options.enabled || !shellLaunchConfig.executable || shellLaunchConfig.isFeatureTerminal || shellLaunchConfig.hideFromUser || shellLaunchConfig.ignoreShellIntegration) {
return undefined;
}

Expand Down
5 changes: 3 additions & 2 deletions src/vs/platform/terminal/node/terminalProcess.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess
hasChildProcesses: true,
resolvedShellLaunchConfig: {},
overrideDimensions: undefined,
failedShellIntegrationActivation: false
failedShellIntegrationActivation: false,
usedShellIntegrationInjection: undefined
};
private static _lastKillOrStart = 0;
private _exitCode: number | undefined;
Expand Down Expand Up @@ -202,6 +203,7 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess
if (this._options.shellIntegration.enabled) {
injection = getShellIntegrationInjection(this.shellLaunchConfig, this._options.shellIntegration, this._logService);
if (injection) {
this._onDidChangeProperty.fire({ type: ProcessPropertyType.UsedShellIntegrationInjection, value: true });
if (injection.envMixin) {
for (const [key, value] of Object.entries(injection.envMixin)) {
this._ptyOptions.env ||= {};
Expand Down Expand Up @@ -315,7 +317,6 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess
this._childProcessMonitor?.handleOutput();
});
ptyProcess.onExit(e => {
this._exitCode = e.exitCode;
meganrogge marked this conversation as resolved.
Show resolved Hide resolved
this._queueProcessExit();
});
this._sendProcessId(ptyProcess.pid);
Expand Down
3 changes: 2 additions & 1 deletion src/vs/workbench/contrib/terminal/browser/remotePty.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ export class RemotePty extends Disposable implements ITerminalChildProcess {
hasChildProcesses: true,
resolvedShellLaunchConfig: {},
overrideDimensions: undefined,
failedShellIntegrationActivation: false
failedShellIntegrationActivation: false,
usedShellIntegrationInjection: undefined
};

get id(): number { return this._id; }
Expand Down
44 changes: 24 additions & 20 deletions src/vs/workbench/contrib/terminal/browser/terminalInstance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ import { XtermTerminal } from 'vs/workbench/contrib/terminal/browser/xterm/xterm
import { IEnvironmentVariableCollection, IEnvironmentVariableInfo } from 'vs/workbench/contrib/terminal/common/environmentVariable';
import { deserializeEnvironmentVariableCollections } from 'vs/workbench/contrib/terminal/common/environmentVariableShared';
import { getCommandHistory, getDirectoryHistory } from 'vs/workbench/contrib/terminal/common/history';
import { DEFAULT_COMMANDS_TO_SKIP_SHELL, INavigationMode, ITerminalBackend, ITerminalProcessManager, ITerminalProfileResolverService, ProcessState, ShellIntegrationExitCode, TerminalCommandId, TERMINAL_CREATION_COMMANDS, TERMINAL_VIEW_ID } from 'vs/workbench/contrib/terminal/common/terminal';
import { DEFAULT_COMMANDS_TO_SKIP_SHELL, INavigationMode, ITerminalBackend, ITerminalProcessManager, ITerminalProfileResolverService, ProcessState, TerminalCommandId, TERMINAL_CREATION_COMMANDS, TERMINAL_VIEW_ID } from 'vs/workbench/contrib/terminal/common/terminal';
import { TerminalContextKeys } from 'vs/workbench/contrib/terminal/common/terminalContextKey';
import { formatMessageForTerminal } from 'vs/platform/terminal/common/terminalStrings';
import { terminalStrings } from 'vs/workbench/contrib/terminal/common/terminalStrings';
Expand Down Expand Up @@ -209,6 +209,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
private _hasScrollBar?: boolean;
private _target?: TerminalLocation | undefined;
private _disableShellIntegrationReporting: boolean | undefined;
private _usedShellIntegrationInjection: boolean = false;

readonly capabilities = new TerminalCapabilityStoreMultiplexer();
readonly statusList: ITerminalStatusList;
Expand Down Expand Up @@ -1511,6 +1512,10 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
case ProcessPropertyType.HasChildProcesses:
this._onDidChangeHasChildProcesses.fire(value);
break;
case ProcessPropertyType.UsedShellIntegrationInjection:
this._usedShellIntegrationInjection = true;
console.log('used shell integration injection');
meganrogge marked this conversation as resolved.
Show resolved Hide resolved
break;
}
});

Expand Down Expand Up @@ -1562,11 +1567,22 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
this._initDimensions();
this.xterm?.raw.resize(this._cols || Constants.DefaultCols, this._rows || Constants.DefaultRows);
}

const originalIcon = this.shellLaunchConfig.icon;
await this._processManager.createProcess(this._shellLaunchConfig, this._cols || Constants.DefaultCols, this._rows || Constants.DefaultRows, this._accessibilityService.isScreenReaderOptimized()).then(error => {
await this._processManager.createProcess(this._shellLaunchConfig, this._cols || Constants.DefaultCols, this._rows || Constants.DefaultRows, this._accessibilityService.isScreenReaderOptimized()).then(async error => {
meganrogge marked this conversation as resolved.
Show resolved Hide resolved
if (error) {
this._onProcessExit(error, error.code === ShellIntegrationExitCode);
if (this._usedShellIntegrationInjection) {
await this.reuseTerminal({ ignoreShellIntegration: true });
meganrogge marked this conversation as resolved.
Show resolved Hide resolved
this.statusList.add({
id: TerminalStatus.ShellIntegrationAttentionNeeded,
severity: Severity.Warning,
icon: Codicon.warning,
tooltip: nls.localize('launchFailed.exitCodeOnlyShellIntegration', "The terminal process failed to launch (exit code: {0}). Disabling shell integration with `terminal.integrated.shellIntegration.enabled` might help.", error.code),
meganrogge marked this conversation as resolved.
Show resolved Hide resolved
meganrogge marked this conversation as resolved.
Show resolved Hide resolved
hoverActions: []
});
this._telemetryService.publicLog2<{}, { owner: 'meganrogge'; comment: 'Indicates the process exited when created with shell integration args' }>('terminal/shellIntegrationFailureProcessExit');
} else {
this._onProcessExit(error);
}
}
});
if (this.xterm?.shellIntegration) {
Expand Down Expand Up @@ -1602,7 +1618,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
* @param exitCode The exit code of the process, this is undefined when the terminal was exited
* through user action.
*/
private async _onProcessExit(exitCodeOrError?: number | ITerminalLaunchError, failedShellIntegrationInjection?: boolean): Promise<void> {
private async _onProcessExit(exitCodeOrError?: number | ITerminalLaunchError): Promise<void> {
// Prevent dispose functions being triggered multiple times
if (this._isExiting) {
return;
Expand All @@ -1613,7 +1629,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
await this._flushXtermData();
this._logService.debug(`Terminal process exit (instanceId: ${this.instanceId}) with code ${this._exitCode}`);

const parsedExitResult = parseExitResult(exitCodeOrError, this.shellLaunchConfig, this._processManager.processState, this._initialCwd, failedShellIntegrationInjection);
const parsedExitResult = parseExitResult(exitCodeOrError, this.shellLaunchConfig, this._processManager.processState, this._initialCwd);
this._exitCode = parsedExitResult?.code;
const exitMessage = parsedExitResult?.message;

Expand Down Expand Up @@ -1654,10 +1670,6 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
}
}

if (failedShellIntegrationInjection) {
this._telemetryService.publicLog2<{}, { owner: 'meganrogge'; comment: 'Indicates the process exited when created with shell integration args' }>('terminal/shellIntegrationFailureProcessExit');
}

// First onExit to consumers, this can happen after the terminal has already been disposed.
this._onExit.fire(exitCodeOrError);

Expand Down Expand Up @@ -1737,7 +1749,6 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {

// Set the new shell launch config
this._shellLaunchConfig = shell; // Must be done before calling _createProcess()

await this._processManager.relaunch(this._shellLaunchConfig, this._cols || Constants.DefaultCols, this._rows || Constants.DefaultRows, this._accessibilityService.isScreenReaderOptimized(), reset).then(error => {
if (error) {
this._onProcessExit(error);
Expand Down Expand Up @@ -2605,8 +2616,7 @@ export function parseExitResult(
exitCodeOrError: ITerminalLaunchError | number | undefined,
shellLaunchConfig: IShellLaunchConfig,
processState: ProcessState,
initialCwd: string | undefined,
failedShellIntegrationInjection?: boolean
initialCwd: string | undefined
): { code: number | undefined; message: string | undefined } | undefined {
// Only return a message if the exit code is non-zero
if (exitCodeOrError === undefined || exitCodeOrError === 0) {
Expand All @@ -2628,13 +2638,7 @@ export function parseExitResult(
commandLine += shellLaunchConfig.args.map(a => ` '${a}'`).join();
}
}
if (failedShellIntegrationInjection) {
if (commandLine) {
message = nls.localize('launchFailed.exitCodeAndCommandLineShellIntegration', "The terminal process \"{0}\" failed to launch (exit code: {1}). Disabling shell integration with `terminal.integrated.shellIntegration.enabled` might help.", commandLine, code);
} else {
message = nls.localize('launchFailed.exitCodeOnlyShellIntegration', "The terminal process failed to launch (exit code: {0}). Disabling shell integration with `terminal.integrated.shellIntegration.enabled` might help.", code);
}
} else if (processState === ProcessState.KilledDuringLaunch) {
if (processState === ProcessState.KilledDuringLaunch) {
if (commandLine) {
message = nls.localize('launchFailed.exitCodeAndCommandLine', "The terminal process \"{0}\" failed to launch (exit code: {1}).", commandLine, code);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export const enum TerminalStatus {
Bell = 'bell',
Disconnected = 'disconnected',
RelaunchNeeded = 'relaunch-needed',
ShellIntegrationAttentionNeeded = 'shell-integration-attention-needed'
}

export interface ITerminalStatus {
Expand Down
6 changes: 5 additions & 1 deletion src/vs/workbench/contrib/terminal/browser/terminalTooltip.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ export function getShellIntegrationTooltip(instance: ITerminalInstance, markdown
if (shellIntegrationCapabilities.length > 0) {
shellIntegrationString += `${markdown ? '\n\n---\n\n' : '\n\n'} ${localize('shellIntegration.enabled', "Shell integration activated")}`;
} else {
shellIntegrationString += `${markdown ? '\n\n---\n\n' : '\n\n'} ${localize('shellIntegration.activationFailed', "Shell integration failed to activate")}`;
if (instance.shellLaunchConfig.ignoreShellIntegration) {
shellIntegrationString += `${markdown ? '\n\n---\n\n' : '\n\n'} ${localize('launchFailed.exitCodeOnlyShellIntegration', "The terminal process failed to launch. Disabling shell integration with terminal.integrated.shellIntegration.enabled might help.")}`;
} else {
shellIntegrationString += `${markdown ? '\n\n---\n\n' : '\n\n'} ${localize('shellIntegration.activationFailed', "Shell integration failed to activate")}`;
}
}
return shellIntegrationString;
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ export class LocalPty extends Disposable implements ITerminalChildProcess {
hasChildProcesses: true,
resolvedShellLaunchConfig: {},
overrideDimensions: undefined,
failedShellIntegrationActivation: false
failedShellIntegrationActivation: false,
usedShellIntegrationInjection: undefined
};
private readonly _onProcessData = this._register(new Emitter<IProcessDataEvent | string>());
readonly onProcessData = this._onProcessData.event;
Expand Down