Skip to content

Commit

Permalink
Merge pull request #152266 from microsoft/merogge/shell-int-failure-r…
Browse files Browse the repository at this point in the history
…elaunch

add warning when shell integration fails and create a terminal
  • Loading branch information
Tyriar committed Jun 21, 2022
2 parents 2a84b0f + 9089f9c commit b64ba3d
Show file tree
Hide file tree
Showing 10 changed files with 62 additions and 27 deletions.
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
4 changes: 3 additions & 1 deletion 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
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ __vsc_command_complete() {
fi
__vsc_update_cwd
}

__vsc_update_prompt() {
# in command execution
if [ "$__vsc_in_command_execution" = "1" ]; then
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
59 changes: 39 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 All @@ -83,6 +83,7 @@ import { IWorkbenchLayoutService, Position } from 'vs/workbench/services/layout/
import { IPathService } from 'vs/workbench/services/path/common/pathService';
import { IPreferencesService } from 'vs/workbench/services/preferences/common/preferences';
import type { ITerminalAddon, Terminal as XTermTerminal } from 'xterm';
import { IOpenerService } from 'vs/platform/opener/common/opener';

const enum Constants {
/**
Expand Down Expand Up @@ -210,6 +211,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 @@ -372,7 +374,8 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
@IEditorService private readonly _editorService: IEditorService,
@IWorkspaceTrustRequestService private readonly _workspaceTrustRequestService: IWorkspaceTrustRequestService,
@IHistoryService private readonly _historyService: IHistoryService,
@ITelemetryService private readonly _telemetryService: ITelemetryService
@ITelemetryService private readonly _telemetryService: ITelemetryService,
@IOpenerService private readonly _openerService: IOpenerService
) {
super();

Expand Down Expand Up @@ -1513,6 +1516,9 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
case ProcessPropertyType.HasChildProcesses:
this._onDidChangeHasChildProcesses.fire(value);
break;
case ProcessPropertyType.UsedShellIntegrationInjection:
this._usedShellIntegrationInjection = true;
break;
}
});

Expand Down Expand Up @@ -1564,11 +1570,10 @@ 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 => {
if (error) {
this._onProcessExit(error, error.code === ShellIntegrationExitCode);
this._onProcessExit(error);
}
});
if (this.xterm?.shellIntegration) {
Expand Down Expand Up @@ -1604,18 +1609,25 @@ 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;
}

const parsedExitResult = parseExitResult(exitCodeOrError, this.shellLaunchConfig, this._processManager.processState, this._initialCwd);

if (this._usedShellIntegrationInjection && (this._processManager.processState === ProcessState.KilledDuringLaunch || this._processManager.processState === ProcessState.KilledByProcess)) {
this._relaunchWithShellIntegrationDisabled(parsedExitResult?.message);
this._onExit.fire(exitCodeOrError);
return;
}

this._isExiting = true;

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);
this._exitCode = parsedExitResult?.code;
const exitMessage = parsedExitResult?.message;

Expand Down Expand Up @@ -1663,10 +1675,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 All @@ -1676,6 +1684,25 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
}
}

private _relaunchWithShellIntegrationDisabled(exitMessage: string | undefined): void {
this._shellLaunchConfig.ignoreShellIntegration = true;
this.relaunch();
this.statusList.add({
id: TerminalStatus.ShellIntegrationAttentionNeeded,
severity: Severity.Warning,
icon: Codicon.warning,
tooltip: (`${exitMessage} ` ?? '') + nls.localize('launchFailed.exitCodeOnlyShellIntegration', 'Disabling shell integration with {0} might help.', '`terminal.integrated.shellIntegration.enabled`'),
hoverActions: [{
commandId: TerminalCommandId.ShellIntegrationLearnMore,
label: nls.localize('shellIntegration.learnMore', "Learn more"),
run: () => {
this._openerService.open('https://code.visualstudio.com/docs/editor/integrated-terminal#_shell-integration');
}
}]
});
this._telemetryService.publicLog2<{}, { owner: 'meganrogge'; comment: 'Indicates the process exited when created with shell integration args' }>('terminal/shellIntegrationFailureProcessExit');
}

/**
* Ensure write calls to xterm.js have finished before resolving.
*/
Expand Down Expand Up @@ -1748,7 +1775,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 @@ -2616,8 +2642,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 @@ -2639,13 +2664,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;
}
1 change: 1 addition & 0 deletions src/vs/workbench/contrib/terminal/common/terminal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,7 @@ export const enum TerminalCommandId {
ConfigureTerminalSettings = 'workbench.action.terminal.openSettings',
OpenDetectedLink = 'workbench.action.terminal.openDetectedLink',
OpenWordLink = 'workbench.action.terminal.openWordLink',
ShellIntegrationLearnMore = 'workbench.action.terminal.learnMore',
OpenFileLink = 'workbench.action.terminal.openFileLink',
OpenWebLink = 'workbench.action.terminal.openUrlLink',
RunRecentCommand = 'workbench.action.terminal.runRecentCommand',
Expand Down
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

0 comments on commit b64ba3d

Please sign in to comment.