Skip to content

Commit

Permalink
Revert "Revert "Reliably detect whether shell integration is working" (
Browse files Browse the repository at this point in the history
…#22445)"

This reverts commit 2fc9fea.
  • Loading branch information
Kartik Raj committed Nov 8, 2023
1 parent 2fc9fea commit 934f46d
Show file tree
Hide file tree
Showing 11 changed files with 183 additions and 44 deletions.
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
"testObserver",
"quickPickItemTooltip",
"saveEditor",
"terminalDataWriteEvent"
"terminalDataWriteEvent",
"terminalExecuteCommandEvent"
],
"author": {
"name": "Microsoft Corporation"
Expand Down
10 changes: 9 additions & 1 deletion src/client/common/application/applicationShell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ import {
WorkspaceFolderPickOptions,
} from 'vscode';
import { traceError } from '../../logging';
import { IApplicationShell, TerminalDataWriteEvent } from './types';
import { IApplicationShell, TerminalDataWriteEvent, TerminalExecutedCommand } from './types';

@injectable()
export class ApplicationShell implements IApplicationShell {
Expand Down Expand Up @@ -182,4 +182,12 @@ export class ApplicationShell implements IApplicationShell {
return new EventEmitter<TerminalDataWriteEvent>().event;
}
}
public get onDidExecuteTerminalCommand(): Event<TerminalExecutedCommand> | undefined {
try {
return window.onDidExecuteTerminalCommand;
} catch (ex) {
traceError('Failed to get proposed API TerminalExecutedCommand', ex);
return undefined;
}
}
}
34 changes: 34 additions & 0 deletions src/client/common/application/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,42 @@ export interface TerminalDataWriteEvent {
readonly data: string;
}

export interface TerminalExecutedCommand {
/**
* The {@link Terminal} the command was executed in.
*/
terminal: Terminal;
/**
* The full command line that was executed, including both the command and the arguments.
*/
commandLine: string | undefined;
/**
* The current working directory that was reported by the shell. This will be a {@link Uri}
* if the string reported by the shell can reliably be mapped to the connected machine.
*/
cwd: Uri | string | undefined;
/**
* The exit code reported by the shell.
*/
exitCode: number | undefined;
/**
* The output of the command when it has finished executing. This is the plain text shown in
* the terminal buffer and does not include raw escape sequences. Depending on the shell
* setup, this may include the command line as part of the output.
*/
output: string | undefined;
}

export const IApplicationShell = Symbol('IApplicationShell');
export interface IApplicationShell {
/**
* An event that is emitted when a terminal with shell integration activated has completed
* executing a command.
*
* Note that this event will not fire if the executed command exits the shell, listen to
* {@link onDidCloseTerminal} to handle that case.
*/
readonly onDidExecuteTerminalCommand: Event<TerminalExecutedCommand> | undefined;
/**
* An [event](#Event) which fires when the focus state of the current window
* changes. The value of the event represents whether the window is focused.
Expand Down
29 changes: 7 additions & 22 deletions src/client/terminals/envCollectionActivation/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ import { TerminalShellType } from '../../common/terminal/types';
import { OSType } from '../../common/utils/platform';
import { normCase } from '../../common/platform/fs-paths';
import { PythonEnvType } from '../../pythonEnvironments/base/info';
import { ITerminalEnvVarCollectionService } from '../types';
import { ShellIntegrationShells } from './shellIntegration';
import { IShellIntegrationService, ITerminalEnvVarCollectionService } from '../types';
import { ProgressService } from '../../common/application/progressService';

@injectable()
Expand Down Expand Up @@ -80,6 +79,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
@inject(IWorkspaceService) private workspaceService: IWorkspaceService,
@inject(IConfigurationService) private readonly configurationService: IConfigurationService,
@inject(IPathUtils) private readonly pathUtils: IPathUtils,
@inject(IShellIntegrationService) private readonly shellIntegrationService: IShellIntegrationService,
) {
this.separator = platform.osType === OSType.Windows ? ';' : ':';
this.progressService = new ProgressService(this.shell);
Expand Down Expand Up @@ -121,7 +121,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
this.disposables,
);
const { shell } = this.applicationEnvironment;
const isActive = this.isShellIntegrationActive(shell);
const isActive = this.shellIntegrationService.isWorking(shell);
const shellType = identifyShellFromShellPath(shell);
if (!isActive && shellType !== TerminalShellType.commandPrompt) {
traceWarn(`Shell integration is not active, environment activated maybe overriden by the shell.`);
Expand Down Expand Up @@ -184,7 +184,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ

// PS1 in some cases is a shell variable (not an env variable) so "env" might not contain it, calculate it in that case.
env.PS1 = await this.getPS1(shell, resource, env);
const prependOptions = this.getPrependOptions(shell);
const prependOptions = await this.getPrependOptions(shell);

// Clear any previously set env vars from collection
envVarCollection.clear();
Expand Down Expand Up @@ -277,7 +277,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
// PS1 should be set but no PS1 was set.
return;
}
const config = this.isShellIntegrationActive(shell);
const config = await this.shellIntegrationService.isWorking(shell);
if (!config) {
traceVerbose('PS1 is not set when shell integration is disabled.');
return;
Expand Down Expand Up @@ -332,8 +332,8 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
}
}

private getPrependOptions(shell: string): EnvironmentVariableMutatorOptions {
const isActive = this.isShellIntegrationActive(shell);
private async getPrependOptions(shell: string): Promise<EnvironmentVariableMutatorOptions> {
const isActive = await this.shellIntegrationService.isWorking(shell);
// Ideally we would want to prepend exactly once, either at shell integration or process creation.
// TODO: Stop prepending altogether once https://github.com/microsoft/vscode/issues/145234 is available.
return isActive
Expand All @@ -347,21 +347,6 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
};
}

private isShellIntegrationActive(shell: string): boolean {
const isEnabled = this.workspaceService
.getConfiguration('terminal')
.get<boolean>('integrated.shellIntegration.enabled')!;
if (isEnabled && ShellIntegrationShells.includes(identifyShellFromShellPath(shell))) {
// Unfortunately shell integration could still've failed in remote scenarios, we can't know for sure:
// https://code.visualstudio.com/docs/terminal/shell-integration#_automatic-script-injection
return true;
}
if (!isEnabled) {
traceVerbose('Shell integrated is disabled in user settings.');
}
return false;
}

private getEnvironmentVariableCollection(scope: EnvironmentVariableScope = {}) {
const envVarCollection = this.context.environmentVariableCollection as GlobalEnvironmentVariableCollection;
return envVarCollection.getScoped(scope);
Expand Down
13 changes: 0 additions & 13 deletions src/client/terminals/envCollectionActivation/shellIntegration.ts

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import { injectable, inject } from 'inversify';
import { IApplicationShell, ITerminalManager, IWorkspaceService } from '../../common/application/types';
import { identifyShellFromShellPath } from '../../common/terminal/shellDetectors/baseShellDetector';
import { TerminalShellType } from '../../common/terminal/types';
import { createDeferred, sleep } from '../../common/utils/async';
import { cache } from '../../common/utils/decorators';
import { traceVerbose } from '../../logging';
import { IShellIntegrationService } from '../types';

/**
* This is a list of shells which support shell integration:
* https://code.visualstudio.com/docs/terminal/shell-integration
*/
const ShellIntegrationShells = [
TerminalShellType.powershell,
TerminalShellType.powershellCore,
TerminalShellType.bash,
TerminalShellType.zsh,
TerminalShellType.fish,
];

@injectable()
export class ShellIntegrationService implements IShellIntegrationService {
constructor(
@inject(ITerminalManager) private readonly terminalManager: ITerminalManager,
@inject(IApplicationShell) private readonly appShell: IApplicationShell,
@inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService,
) {}

@cache(-1, true)
public async isWorking(shell: string): Promise<boolean> {
const isEnabled = this.workspaceService
.getConfiguration('terminal')
.get<boolean>('integrated.shellIntegration.enabled')!;
if (!isEnabled) {
traceVerbose('Shell integrated is disabled in user settings.');
}
const isSupposedToWork = isEnabled && ShellIntegrationShells.includes(identifyShellFromShellPath(shell));
if (!isSupposedToWork) {
return false;
}
const deferred = createDeferred<void>();
const timestamp = new Date().getTime();
const name = `Python ${timestamp}`;
const onDidExecuteTerminalCommand = this.appShell.onDidExecuteTerminalCommand?.bind(this.appShell);
if (!onDidExecuteTerminalCommand) {
// Proposed API is not available, assume shell integration is working at this point.
return true;
}
const disposable = onDidExecuteTerminalCommand((e) => {
if (e.terminal.name === name) {
deferred.resolve();
}
});
const terminal = this.terminalManager.createTerminal({
name,
shellPath: shell,
hideFromUser: true,
});
terminal.sendText(`echo ${shell}`);
const success = await Promise.race([sleep(3000).then(() => false), deferred.promise.then(() => true)]);
disposable.dispose();
return success;
}
}
3 changes: 3 additions & 0 deletions src/client/terminals/serviceRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@ import {
ICodeExecutionHelper,
ICodeExecutionManager,
ICodeExecutionService,
IShellIntegrationService,
ITerminalAutoActivation,
ITerminalEnvVarCollectionService,
} from './types';
import { TerminalEnvVarCollectionService } from './envCollectionActivation/service';
import { IExtensionActivationService, IExtensionSingleActivationService } from '../activation/types';
import { TerminalDeactivateLimitationPrompt } from './envCollectionActivation/deactivatePrompt';
import { TerminalIndicatorPrompt } from './envCollectionActivation/indicatorPrompt';
import { ShellIntegrationService } from './envCollectionActivation/shellIntegrationService';

export function registerTypes(serviceManager: IServiceManager): void {
serviceManager.addSingleton<ICodeExecutionHelper>(ICodeExecutionHelper, CodeExecutionHelper);
Expand Down Expand Up @@ -50,5 +52,6 @@ export function registerTypes(serviceManager: IServiceManager): void {
IExtensionSingleActivationService,
TerminalDeactivateLimitationPrompt,
);
serviceManager.addSingleton<IShellIntegrationService>(IShellIntegrationService, ShellIntegrationService);
serviceManager.addBinding(ITerminalEnvVarCollectionService, IExtensionActivationService);
}
5 changes: 5 additions & 0 deletions src/client/terminals/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,8 @@ export interface ITerminalEnvVarCollectionService {
*/
isTerminalPromptSetCorrectly(resource?: Resource): boolean;
}

export const IShellIntegrationService = Symbol('IShellIntegrationService');
export interface IShellIntegrationService {
isWorking(shell: string): Promise<boolean>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
GlobalEnvironmentVariableCollection,
ProgressLocation,
Uri,
WorkspaceConfiguration,
WorkspaceFolder,
} from 'vscode';
import {
Expand All @@ -38,6 +37,7 @@ import { IInterpreterService } from '../../../client/interpreter/contracts';
import { PathUtils } from '../../../client/common/platform/pathUtils';
import { PythonEnvType } from '../../../client/pythonEnvironments/base/info';
import { PythonEnvironment } from '../../../client/pythonEnvironments/info';
import { IShellIntegrationService } from '../../../client/terminals/types';

suite('Terminal Environment Variable Collection Service', () => {
let platform: IPlatformService;
Expand All @@ -50,29 +50,28 @@ suite('Terminal Environment Variable Collection Service', () => {
let applicationEnvironment: IApplicationEnvironment;
let environmentActivationService: IEnvironmentActivationService;
let workspaceService: IWorkspaceService;
let workspaceConfig: WorkspaceConfiguration;
let terminalEnvVarCollectionService: TerminalEnvVarCollectionService;
const progressOptions = {
location: ProgressLocation.Window,
title: Interpreters.activatingTerminals,
};
let configService: IConfigurationService;
let shellIntegrationService: IShellIntegrationService;
const displayPath = 'display/path';
const customShell = 'powershell';
const defaultShell = defaultShells[getOSType()];

setup(() => {
workspaceService = mock<IWorkspaceService>();
workspaceConfig = mock<WorkspaceConfiguration>();
when(workspaceService.getWorkspaceFolder(anything())).thenReturn(undefined);
when(workspaceService.workspaceFolders).thenReturn(undefined);
when(workspaceService.getConfiguration('terminal')).thenReturn(instance(workspaceConfig));
when(workspaceConfig.get<boolean>('integrated.shellIntegration.enabled')).thenReturn(true);
platform = mock<IPlatformService>();
when(platform.osType).thenReturn(getOSType());
interpreterService = mock<IInterpreterService>();
context = mock<IExtensionContext>();
shell = mock<IApplicationShell>();
shellIntegrationService = mock<IShellIntegrationService>();
when(shellIntegrationService.isWorking(anything())).thenResolve(true);
globalCollection = mock<GlobalEnvironmentVariableCollection>();
collection = mock<EnvironmentVariableCollection>();
when(context.environmentVariableCollection).thenReturn(instance(globalCollection));
Expand Down Expand Up @@ -108,6 +107,7 @@ suite('Terminal Environment Variable Collection Service', () => {
instance(workspaceService),
instance(configService),
new PathUtils(getOSType() === OSType.Windows),
instance(shellIntegrationService),
);
});

Expand Down Expand Up @@ -445,8 +445,8 @@ suite('Terminal Environment Variable Collection Service', () => {
});

test('Correct track that prompt was set for PS1 if shell integration is disabled', async () => {
reset(workspaceConfig);
when(workspaceConfig.get<boolean>('integrated.shellIntegration.enabled')).thenReturn(false);
reset(shellIntegrationService);
when(shellIntegrationService.isWorking(anything())).thenResolve(false);
when(platform.osType).thenReturn(OSType.Linux);
const envVars: NodeJS.ProcessEnv = { VIRTUAL_ENV: 'prefix/to/venv', PS1: '(.venv)', ...process.env };
const ps1Shell = 'bash';
Expand Down
3 changes: 3 additions & 0 deletions src/test/terminals/serviceRegistry.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@ import { TerminalCodeExecutionProvider } from '../../client/terminals/codeExecut
import { TerminalDeactivateLimitationPrompt } from '../../client/terminals/envCollectionActivation/deactivatePrompt';
import { TerminalIndicatorPrompt } from '../../client/terminals/envCollectionActivation/indicatorPrompt';
import { TerminalEnvVarCollectionService } from '../../client/terminals/envCollectionActivation/service';
import { ShellIntegrationService } from '../../client/terminals/envCollectionActivation/shellIntegrationService';
import { registerTypes } from '../../client/terminals/serviceRegistry';
import {
ICodeExecutionHelper,
ICodeExecutionManager,
ICodeExecutionService,
IShellIntegrationService,
ITerminalAutoActivation,
ITerminalEnvVarCollectionService,
} from '../../client/terminals/types';
Expand All @@ -35,6 +37,7 @@ suite('Terminal - Service Registry', () => {
[ITerminalEnvVarCollectionService, TerminalEnvVarCollectionService],
[IExtensionSingleActivationService, TerminalIndicatorPrompt],
[IExtensionSingleActivationService, TerminalDeactivateLimitationPrompt],
[IShellIntegrationService, ShellIntegrationService],
].forEach((args) => {
if (args.length === 2) {
services
Expand Down
Loading

0 comments on commit 934f46d

Please sign in to comment.