Skip to content

Commit

Permalink
Fix: Don’t use executeCommand on python sub-shell for windows. (#24542)
Browse files Browse the repository at this point in the history
Related: #24418 
Further resolves:
#24422
Shell integration does not exist on windows python repl so dont use
execute command on python subshell in this case.

This will prevent keyboard interrupt from happening even when setting
for shell integration is enabled for python shell (which is currently
only supported on mac and linux)
  • Loading branch information
anthonykim1 authored Dec 4, 2024
1 parent eab8794 commit 4153f7c
Show file tree
Hide file tree
Showing 15 changed files with 46 additions and 22 deletions.
3 changes: 1 addition & 2 deletions src/client/common/configSettings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ import {
} from './types';
import { debounceSync } from './utils/decorators';
import { SystemVariables } from './variables/systemVariables';
import { getOSType, OSType } from './utils/platform';
import { isWindows } from './platform/platformService';
import { getOSType, OSType, isWindows } from './utils/platform';
import { untildify } from './helpers';

export class PythonSettings implements IPythonSettings {
Expand Down
2 changes: 1 addition & 1 deletion src/client/common/pipes/namedPipes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import * as path from 'path';
import * as rpc from 'vscode-jsonrpc/node';
import { CancellationError, CancellationToken, Disposable } from 'vscode';
import { traceVerbose } from '../../logging';
import { isWindows } from '../platform/platformService';
import { isWindows } from '../utils/platform';
import { createDeferred } from '../utils/async';

const { XDG_RUNTIME_DIR } = process.env;
Expand Down
6 changes: 1 addition & 5 deletions src/client/common/platform/platformService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { injectable } from 'inversify';
import * as os from 'os';
import { coerce, SemVer } from 'semver';
import { getSearchPathEnvVarNames } from '../utils/exec';
import { Architecture, getArchitecture, getOSType, OSType } from '../utils/platform';
import { Architecture, getArchitecture, getOSType, isWindows, OSType } from '../utils/platform';
import { parseSemVerSafe } from '../utils/version';
import { IPlatformService } from './types';

Expand Down Expand Up @@ -73,7 +73,3 @@ export class PlatformService implements IPlatformService {
return getArchitecture() === Architecture.x64;
}
}

export function isWindows(): boolean {
return getOSType() === OSType.Windows;
}
2 changes: 1 addition & 1 deletion src/client/common/serviceRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ import { Random } from './utils/random';
import { ContextKeyManager } from './application/contextKeyManager';
import { CreatePythonFileCommandHandler } from './application/commands/createPythonFile';
import { RequireJupyterPrompt } from '../jupyter/requireJupyterPrompt';
import { isWindows } from './platform/platformService';
import { isWindows } from './utils/platform';
import { PixiActivationCommandProvider } from './terminal/environmentActivationProviders/pixiActivationProvider';

export function registerTypes(serviceManager: IServiceManager): void {
Expand Down
3 changes: 2 additions & 1 deletion src/client/common/terminal/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
} from './types';
import { traceVerbose } from '../../logging';
import { getConfiguration } from '../vscodeApis/workspaceApis';
import { isWindows } from '../utils/platform';

@injectable()
export class TerminalService implements ITerminalService, Disposable {
Expand Down Expand Up @@ -104,7 +105,7 @@ export class TerminalService implements ITerminalService, Disposable {

const config = getConfiguration('python');
const pythonrcSetting = config.get<boolean>('terminal.shellIntegration.enabled');
if (isPythonShell && !pythonrcSetting) {
if ((isPythonShell && !pythonrcSetting) || (isPythonShell && isWindows())) {
// If user has explicitly disabled SI for Python, use sendText for inside Terminal REPL.
terminal.sendText(commandLine);
return undefined;
Expand Down
4 changes: 4 additions & 0 deletions src/client/common/utils/platform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,3 +67,7 @@ export function getUserHomeDir(): string | undefined {
}
return getEnvironmentVariable('HOME') || getEnvironmentVariable('HOMEPATH');
}

export function isWindows(): boolean {
return getOSType() === OSType.Windows;
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,14 @@ import * as path from 'path';
import * as rpc from 'vscode-jsonrpc/node';
import { PassThrough } from 'stream';
import * as fs from '../../../../common/platform/fs-paths';
import { isWindows } from '../../../../common/platform/platformService';
import { isWindows, getUserHomeDir } from '../../../../common/utils/platform';
import { EXTENSION_ROOT_DIR } from '../../../../constants';
import { createDeferred, createDeferredFrom } from '../../../../common/utils/async';
import { DisposableBase, DisposableStore } from '../../../../common/utils/resourceLifecycle';
import { noop } from '../../../../common/utils/misc';
import { getConfiguration, getWorkspaceFolderPaths, isTrusted } from '../../../../common/vscodeApis/workspaceApis';
import { CONDAPATH_SETTING_KEY } from '../../../common/environmentManagers/conda';
import { VENVFOLDERS_SETTING_KEY, VENVPATH_SETTING_KEY } from '../lowLevel/customVirtualEnvLocator';
import { getUserHomeDir } from '../../../../common/utils/platform';
import { createLogOutputChannel } from '../../../../common/vscodeApis/windowApis';
import { sendNativeTelemetry, NativePythonTelemetry } from './nativePythonTelemetry';
import { NativePythonEnvironmentKind } from './nativePythonUtils';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import { Disposable, Event, EventEmitter, GlobPattern, RelativePattern, Uri, WorkspaceFolder } from 'vscode';
import { createFileSystemWatcher, getWorkspaceFolder } from '../../../../common/vscodeApis/workspaceApis';
import { isWindows } from '../../../../common/platform/platformService';
import { isWindows } from '../../../../common/utils/platform';
import { arePathsSame } from '../../../common/externalDependencies';
import { FileChangeType } from '../../../../common/platform/fileSystemWatcher';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,11 @@
import * as path from 'path';
import { readJSON } from 'fs-extra';
import which from 'which';
import { getUserHomeDir } from '../../../common/utils/platform';
import { getUserHomeDir, isWindows } from '../../../common/utils/platform';
import { exec, getPythonSetting, onDidChangePythonSetting, pathExists } from '../externalDependencies';
import { cache } from '../../../common/utils/decorators';
import { traceVerbose, traceWarn } from '../../../logging';
import { OUTPUT_MARKER_SCRIPT } from '../../../common/process/internal/scripts';
import { isWindows } from '../../../common/platform/platformService';
import { IDisposableRegistry } from '../../../common/types';
import { getWorkspaceFolderPaths } from '../../../common/vscodeApis/workspaceApis';
import { isTestExecution } from '../../../common/constants';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { Commands } from '../../../common/constants';
import { Common } from '../../../common/utils/localize';
import { executeCommand } from '../../../common/vscodeApis/commandApis';
import { showErrorMessage } from '../../../common/vscodeApis/windowApis';
import { isWindows } from '../../../common/platform/platformService';
import { isWindows } from '../../../common/utils/platform';

export async function showErrorMessageWithLogs(message: string): Promise<void> {
const result = await showErrorMessage(message, Common.openOutputPanel, Common.selectPythonInterpreter);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
import { findFiles } from '../../../common/vscodeApis/workspaceApis';
import { traceError, traceVerbose } from '../../../logging';
import { Commands } from '../../../common/constants';
import { isWindows } from '../../../common/platform/platformService';
import { isWindows } from '../../../common/utils/platform';
import { getVenvPath, hasVenv } from '../common/commonUtils';
import { deleteEnvironmentNonWindows, deleteEnvironmentWindows } from './venvDeleteUtils';

Expand Down
2 changes: 1 addition & 1 deletion src/test/common/configSettings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import * as vscode from 'vscode';
import { SystemVariables } from '../../client/common/variables/systemVariables';
import { getExtensionSettings } from '../extensionSettings';
import { initialize } from './../initialize';
import { isWindows } from '../../client/common/platform/platformService';
import { isWindows } from '../../client/common/utils/platform';

const workspaceRoot = path.join(__dirname, '..', '..', '..', 'src', 'test');

Expand Down
28 changes: 27 additions & 1 deletion src/test/common/terminals/service.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { IServiceContainer } from '../../../client/ioc/types';
import { ITerminalAutoActivation } from '../../../client/terminals/types';
import { createPythonInterpreter } from '../../utils/interpreters';
import * as workspaceApis from '../../../client/common/vscodeApis/workspaceApis';
import * as platform from '../../../client/common/utils/platform';

suite('Terminal Service', () => {
let service: TerminalService;
Expand All @@ -42,6 +43,7 @@ suite('Terminal Service', () => {
let getConfigurationStub: sinon.SinonStub;
let pythonConfig: TypeMoq.IMock<WorkspaceConfiguration>;
let editorConfig: TypeMoq.IMock<WorkspaceConfiguration>;
let isWindowsStub: sinon.SinonStub;

setup(() => {
terminal = TypeMoq.Mock.ofType<VSCodeTerminal>();
Expand Down Expand Up @@ -94,6 +96,7 @@ suite('Terminal Service', () => {
mockServiceContainer.setup((c) => c.get(ITerminalActivator)).returns(() => terminalActivator.object);
mockServiceContainer.setup((c) => c.get(ITerminalAutoActivation)).returns(() => terminalAutoActivator.object);
getConfigurationStub = sinon.stub(workspaceApis, 'getConfiguration');
isWindowsStub = sinon.stub(platform, 'isWindows');
pythonConfig = TypeMoq.Mock.ofType<WorkspaceConfiguration>();
editorConfig = TypeMoq.Mock.ofType<WorkspaceConfiguration>();
getConfigurationStub.callsFake((section: string) => {
Expand Down Expand Up @@ -231,7 +234,8 @@ suite('Terminal Service', () => {
terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1));
});

test('Ensure sendText is NOT called when Python shell integration and terminal shell integration are both enabled', async () => {
test('Ensure sendText is NOT called when Python shell integration and terminal shell integration are both enabled - Mac, Linux', async () => {
isWindowsStub.returns(false);
pythonConfig
.setup((p) => p.get('terminal.shellIntegration.enabled'))
.returns(() => true)
Expand All @@ -252,6 +256,28 @@ suite('Terminal Service', () => {
terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.never());
});

test('Ensure sendText IS called even when Python shell integration and terminal shell integration are both enabled - Window', async () => {
isWindowsStub.returns(true);
pythonConfig
.setup((p) => p.get('terminal.shellIntegration.enabled'))
.returns(() => true)
.verifiable(TypeMoq.Times.once());

terminalHelper
.setup((helper) => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
.returns(() => Promise.resolve(undefined));
service = new TerminalService(mockServiceContainer.object);
const textToSend = 'Some Text';
terminalHelper.setup((h) => h.identifyTerminalShell(TypeMoq.It.isAny())).returns(() => TerminalShellType.bash);
terminalManager.setup((t) => t.createTerminal(TypeMoq.It.isAny())).returns(() => terminal.object);

service.ensureTerminal();
service.executeCommand(textToSend, true);

terminal.verify((t) => t.show(TypeMoq.It.isValue(true)), TypeMoq.Times.exactly(1));
terminal.verify((t) => t.sendText(TypeMoq.It.isValue(textToSend)), TypeMoq.Times.exactly(1));
});

test('Ensure terminal is not shown if `hideFromUser` option is set to `true`', async () => {
terminalHelper
.setup((helper) => helper.getEnvironmentActivationCommands(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
Expand Down
3 changes: 1 addition & 2 deletions src/test/pythonEnvironments/nativeAPI.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,8 @@ import {
NativeEnvManagerInfo,
NativePythonFinder,
} from '../../client/pythonEnvironments/base/locators/common/nativePythonFinder';
import { Architecture } from '../../client/common/utils/platform';
import { Architecture, isWindows } from '../../client/common/utils/platform';
import { PythonEnvInfo, PythonEnvKind, PythonEnvType } from '../../client/pythonEnvironments/base/info';
import { isWindows } from '../../client/common/platform/platformService';
import { NativePythonEnvironmentKind } from '../../client/pythonEnvironments/base/locators/common/nativePythonUtils';
import * as condaApi from '../../client/pythonEnvironments/common/environmentManagers/conda';
import * as pyenvApi from '../../client/pythonEnvironments/common/environmentManagers/pyenv';
Expand Down
3 changes: 2 additions & 1 deletion src/test/serviceRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import * as TypeMoq from 'typemoq';
import { Disposable, Memento } from 'vscode';
import { FileSystem } from '../client/common/platform/fileSystem';
import { PathUtils } from '../client/common/platform/pathUtils';
import { PlatformService, isWindows } from '../client/common/platform/platformService';
import { PlatformService } from '../client/common/platform/platformService';
import { isWindows } from '../client/common/utils/platform';
import { RegistryImplementation } from '../client/common/platform/registry';
import { registerTypes as platformRegisterTypes } from '../client/common/platform/serviceRegistry';
import { IFileSystem, IPlatformService, IRegistry } from '../client/common/platform/types';
Expand Down

0 comments on commit 4153f7c

Please sign in to comment.