From 2fc9fea46f073b43679ab32ba666ee48ec3a8530 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 8 Nov 2023 10:50:31 -0800 Subject: [PATCH] Revert "Reliably detect whether shell integration is working" (#22445) Reverts microsoft/vscode-python#22440 It seems reactivating never finishes after this, although this doesn't repro when debugging the extension, have to investigate further. --- package.json | 3 +- .../common/application/applicationShell.ts | 10 +-- src/client/common/application/types.ts | 34 ---------- .../envCollectionActivation/service.ts | 29 ++++++-- .../shellIntegration.ts | 13 ++++ .../shellIntegrationService.ts | 68 ------------------- src/client/terminals/serviceRegistry.ts | 3 - src/client/terminals/types.ts | 5 -- ...rminalEnvVarCollectionService.unit.test.ts | 14 ++-- .../terminals/serviceRegistry.unit.test.ts | 3 - ....proposed.terminalExecuteCommandEvent.d.ts | 45 ------------ 11 files changed, 44 insertions(+), 183 deletions(-) create mode 100644 src/client/terminals/envCollectionActivation/shellIntegration.ts delete mode 100644 src/client/terminals/envCollectionActivation/shellIntegrationService.ts delete mode 100644 typings/vscode-proposed/vscode.proposed.terminalExecuteCommandEvent.d.ts diff --git a/package.json b/package.json index 5c25b2b84832..b0a184c1c4ed 100644 --- a/package.json +++ b/package.json @@ -23,8 +23,7 @@ "testObserver", "quickPickItemTooltip", "saveEditor", - "terminalDataWriteEvent", - "terminalExecuteCommandEvent" + "terminalDataWriteEvent" ], "author": { "name": "Microsoft Corporation" diff --git a/src/client/common/application/applicationShell.ts b/src/client/common/application/applicationShell.ts index 8035d979efbd..aadf80186900 100644 --- a/src/client/common/application/applicationShell.ts +++ b/src/client/common/application/applicationShell.ts @@ -39,7 +39,7 @@ import { WorkspaceFolderPickOptions, } from 'vscode'; import { traceError } from '../../logging'; -import { IApplicationShell, TerminalDataWriteEvent, TerminalExecutedCommand } from './types'; +import { IApplicationShell, TerminalDataWriteEvent } from './types'; @injectable() export class ApplicationShell implements IApplicationShell { @@ -182,12 +182,4 @@ export class ApplicationShell implements IApplicationShell { return new EventEmitter().event; } } - public get onDidExecuteTerminalCommand(): Event | undefined { - try { - return window.onDidExecuteTerminalCommand; - } catch (ex) { - traceError('Failed to get proposed API TerminalExecutedCommand', ex); - return undefined; - } - } } diff --git a/src/client/common/application/types.ts b/src/client/common/application/types.ts index 6705331bf57d..863f5e4651b2 100644 --- a/src/client/common/application/types.ts +++ b/src/client/common/application/types.ts @@ -78,42 +78,8 @@ 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 | 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. diff --git a/src/client/terminals/envCollectionActivation/service.ts b/src/client/terminals/envCollectionActivation/service.ts index f1de0ee050b1..46e70d60d922 100644 --- a/src/client/terminals/envCollectionActivation/service.ts +++ b/src/client/terminals/envCollectionActivation/service.ts @@ -37,7 +37,8 @@ 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 { IShellIntegrationService, ITerminalEnvVarCollectionService } from '../types'; +import { ITerminalEnvVarCollectionService } from '../types'; +import { ShellIntegrationShells } from './shellIntegration'; import { ProgressService } from '../../common/application/progressService'; @injectable() @@ -79,7 +80,6 @@ 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); @@ -121,7 +121,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ this.disposables, ); const { shell } = this.applicationEnvironment; - const isActive = this.shellIntegrationService.isWorking(shell); + const isActive = this.isShellIntegrationActive(shell); const shellType = identifyShellFromShellPath(shell); if (!isActive && shellType !== TerminalShellType.commandPrompt) { traceWarn(`Shell integration is not active, environment activated maybe overriden by the shell.`); @@ -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 = await this.getPrependOptions(shell); + const prependOptions = this.getPrependOptions(shell); // Clear any previously set env vars from collection envVarCollection.clear(); @@ -277,7 +277,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ // PS1 should be set but no PS1 was set. return; } - const config = await this.shellIntegrationService.isWorking(shell); + const config = this.isShellIntegrationActive(shell); if (!config) { traceVerbose('PS1 is not set when shell integration is disabled.'); return; @@ -332,8 +332,8 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ } } - private async getPrependOptions(shell: string): Promise { - const isActive = await this.shellIntegrationService.isWorking(shell); + private getPrependOptions(shell: string): EnvironmentVariableMutatorOptions { + const isActive = this.isShellIntegrationActive(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 @@ -347,6 +347,21 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ }; } + private isShellIntegrationActive(shell: string): boolean { + const isEnabled = this.workspaceService + .getConfiguration('terminal') + .get('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); diff --git a/src/client/terminals/envCollectionActivation/shellIntegration.ts b/src/client/terminals/envCollectionActivation/shellIntegration.ts new file mode 100644 index 000000000000..1be2501595a4 --- /dev/null +++ b/src/client/terminals/envCollectionActivation/shellIntegration.ts @@ -0,0 +1,13 @@ +import { TerminalShellType } from '../../common/terminal/types'; + +/** + * This is a list of shells which support shell integration: + * https://code.visualstudio.com/docs/terminal/shell-integration + */ +export const ShellIntegrationShells = [ + TerminalShellType.powershell, + TerminalShellType.powershellCore, + TerminalShellType.bash, + TerminalShellType.zsh, + TerminalShellType.fish, +]; diff --git a/src/client/terminals/envCollectionActivation/shellIntegrationService.ts b/src/client/terminals/envCollectionActivation/shellIntegrationService.ts deleted file mode 100644 index 1c9024159438..000000000000 --- a/src/client/terminals/envCollectionActivation/shellIntegrationService.ts +++ /dev/null @@ -1,68 +0,0 @@ -// 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 { - const isEnabled = this.workspaceService - .getConfiguration('terminal') - .get('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(); - 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; - } -} diff --git a/src/client/terminals/serviceRegistry.ts b/src/client/terminals/serviceRegistry.ts index 86e2efb376e8..a9da776d011a 100644 --- a/src/client/terminals/serviceRegistry.ts +++ b/src/client/terminals/serviceRegistry.ts @@ -12,7 +12,6 @@ import { ICodeExecutionHelper, ICodeExecutionManager, ICodeExecutionService, - IShellIntegrationService, ITerminalAutoActivation, ITerminalEnvVarCollectionService, } from './types'; @@ -20,7 +19,6 @@ import { TerminalEnvVarCollectionService } from './envCollectionActivation/servi 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, CodeExecutionHelper); @@ -52,6 +50,5 @@ export function registerTypes(serviceManager: IServiceManager): void { IExtensionSingleActivationService, TerminalDeactivateLimitationPrompt, ); - serviceManager.addSingleton(IShellIntegrationService, ShellIntegrationService); serviceManager.addBinding(ITerminalEnvVarCollectionService, IExtensionActivationService); } diff --git a/src/client/terminals/types.ts b/src/client/terminals/types.ts index 02b038bd239d..ba30b8f6d47d 100644 --- a/src/client/terminals/types.ts +++ b/src/client/terminals/types.ts @@ -41,8 +41,3 @@ export interface ITerminalEnvVarCollectionService { */ isTerminalPromptSetCorrectly(resource?: Resource): boolean; } - -export const IShellIntegrationService = Symbol('IShellIntegrationService'); -export interface IShellIntegrationService { - isWorking(shell: string): Promise; -} diff --git a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts index 52efc3a45faa..88b9c978854c 100644 --- a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts +++ b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts @@ -12,6 +12,7 @@ import { GlobalEnvironmentVariableCollection, ProgressLocation, Uri, + WorkspaceConfiguration, WorkspaceFolder, } from 'vscode'; import { @@ -37,7 +38,6 @@ 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; @@ -50,28 +50,29 @@ 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(); + workspaceConfig = mock(); when(workspaceService.getWorkspaceFolder(anything())).thenReturn(undefined); when(workspaceService.workspaceFolders).thenReturn(undefined); + when(workspaceService.getConfiguration('terminal')).thenReturn(instance(workspaceConfig)); + when(workspaceConfig.get('integrated.shellIntegration.enabled')).thenReturn(true); platform = mock(); when(platform.osType).thenReturn(getOSType()); interpreterService = mock(); context = mock(); shell = mock(); - shellIntegrationService = mock(); - when(shellIntegrationService.isWorking(anything())).thenResolve(true); globalCollection = mock(); collection = mock(); when(context.environmentVariableCollection).thenReturn(instance(globalCollection)); @@ -107,7 +108,6 @@ suite('Terminal Environment Variable Collection Service', () => { instance(workspaceService), instance(configService), new PathUtils(getOSType() === OSType.Windows), - instance(shellIntegrationService), ); }); @@ -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(shellIntegrationService); - when(shellIntegrationService.isWorking(anything())).thenResolve(false); + reset(workspaceConfig); + when(workspaceConfig.get('integrated.shellIntegration.enabled')).thenReturn(false); when(platform.osType).thenReturn(OSType.Linux); const envVars: NodeJS.ProcessEnv = { VIRTUAL_ENV: 'prefix/to/venv', PS1: '(.venv)', ...process.env }; const ps1Shell = 'bash'; diff --git a/src/test/terminals/serviceRegistry.unit.test.ts b/src/test/terminals/serviceRegistry.unit.test.ts index 4abf77b8e794..816afa17cf88 100644 --- a/src/test/terminals/serviceRegistry.unit.test.ts +++ b/src/test/terminals/serviceRegistry.unit.test.ts @@ -13,13 +13,11 @@ 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'; @@ -37,7 +35,6 @@ suite('Terminal - Service Registry', () => { [ITerminalEnvVarCollectionService, TerminalEnvVarCollectionService], [IExtensionSingleActivationService, TerminalIndicatorPrompt], [IExtensionSingleActivationService, TerminalDeactivateLimitationPrompt], - [IShellIntegrationService, ShellIntegrationService], ].forEach((args) => { if (args.length === 2) { services diff --git a/typings/vscode-proposed/vscode.proposed.terminalExecuteCommandEvent.d.ts b/typings/vscode-proposed/vscode.proposed.terminalExecuteCommandEvent.d.ts deleted file mode 100644 index 7f503f1aa6da..000000000000 --- a/typings/vscode-proposed/vscode.proposed.terminalExecuteCommandEvent.d.ts +++ /dev/null @@ -1,45 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the MIT License. See License.txt in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -declare module 'vscode' { - // https://github.com/microsoft/vscode/issues/145234 - - 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 namespace window { - /** - * 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. - */ - export const onDidExecuteTerminalCommand: Event; - } -}