From 3e7e0d244b18079767a3f68b72cf855abe375429 Mon Sep 17 00:00:00 2001 From: Anthony Kim <62267334+anthonykim1@users.noreply.github.com> Date: Mon, 4 Nov 2024 13:03:28 -0800 Subject: [PATCH] dont automatically inject PYTHONSTARTUP (#24346) Resolves https://github.com/microsoft/vscode-python/issues/24345 and https://github.com/microsoft/vscode-python/issues/24290 and https://github.com/microsoft/vscode-python/issues/24105 remove automatic injection from before so only thing that allows shell integration to user for Python terminal REPL is the setting itself. --- src/client/common/terminal/service.ts | 43 ++---- .../common/terminal/syncTerminalService.ts | 6 +- src/client/common/terminal/types.ts | 7 +- src/test/common/terminals/helper.unit.test.ts | 24 ++++ src/test/smoke/smartSend.smoke.test.ts | 131 +++++++++--------- src/test/smokeTest.ts | 1 - 6 files changed, 107 insertions(+), 105 deletions(-) diff --git a/src/client/common/terminal/service.ts b/src/client/common/terminal/service.ts index 511135fa8b2f..45ce9afac47e 100644 --- a/src/client/common/terminal/service.ts +++ b/src/client/common/terminal/service.ts @@ -2,8 +2,7 @@ // Licensed under the MIT License. import { inject, injectable } from 'inversify'; -import * as path from 'path'; -import { CancellationToken, Disposable, Event, EventEmitter, Terminal } from 'vscode'; +import { CancellationToken, Disposable, Event, EventEmitter, Terminal, TerminalShellExecution } from 'vscode'; import '../../common/extensions'; import { IInterpreterService } from '../../interpreter/contracts'; import { IServiceContainer } from '../../ioc/types'; @@ -11,7 +10,6 @@ import { captureTelemetry } from '../../telemetry'; import { EventName } from '../../telemetry/constants'; import { ITerminalAutoActivation } from '../../terminals/types'; import { ITerminalManager } from '../application/types'; -import { EXTENSION_ROOT_DIR } from '../constants'; import { _SCRIPTS_DIR } from '../process/internal/scripts/constants'; import { IConfigurationService, IDisposableRegistry } from '../types'; import { @@ -20,7 +18,6 @@ import { ITerminalService, TerminalCreationOptions, TerminalShellType, - ITerminalExecutedCommand, } from './types'; import { traceVerbose } from '../../logging'; @@ -33,11 +30,12 @@ export class TerminalService implements ITerminalService, Disposable { private terminalHelper: ITerminalHelper; private terminalActivator: ITerminalActivator; private terminalAutoActivator: ITerminalAutoActivation; - private readonly envVarScript = path.join(EXTENSION_ROOT_DIR, 'python_files', 'pythonrc.py'); private readonly executeCommandListeners: Set = new Set(); + private _terminalFirstLaunched: boolean = true; public get onDidCloseTerminal(): Event { return this.terminalClosed.event.bind(this.terminalClosed); } + constructor( @inject(IServiceContainer) private serviceContainer: IServiceContainer, private readonly options?: TerminalCreationOptions, @@ -76,24 +74,24 @@ export class TerminalService implements ITerminalService, Disposable { } this.terminal!.sendText(text); } - public async executeCommand(commandLine: string): Promise { + public async executeCommand(commandLine: string): Promise { const terminal = this.terminal!; if (!this.options?.hideFromUser) { terminal.show(true); } // If terminal was just launched, wait some time for shell integration to onDidChangeShellIntegration. - if (!terminal.shellIntegration) { + if (!terminal.shellIntegration && this._terminalFirstLaunched) { + this._terminalFirstLaunched = false; const promise = new Promise((resolve) => { - const shellIntegrationChangeEventListener = this.terminalManager.onDidChangeTerminalShellIntegration( - () => { - this.executeCommandListeners.delete(shellIntegrationChangeEventListener); - resolve(true); - }, - ); + const disposable = this.terminalManager.onDidChangeTerminalShellIntegration(() => { + clearTimeout(timer); + disposable.dispose(); + resolve(true); + }); const TIMEOUT_DURATION = 500; - setTimeout(() => { - this.executeCommandListeners.add(shellIntegrationChangeEventListener); + const timer = setTimeout(() => { + disposable.dispose(); resolve(true); }, TIMEOUT_DURATION); }); @@ -102,18 +100,8 @@ export class TerminalService implements ITerminalService, Disposable { if (terminal.shellIntegration) { const execution = terminal.shellIntegration.executeCommand(commandLine); - return await new Promise((resolve) => { - const listener = this.terminalManager.onDidEndTerminalShellExecution((e) => { - if (e.execution === execution) { - this.executeCommandListeners.delete(listener); - resolve({ execution, exitCode: e.exitCode }); - } - }); - if (listener) { - this.executeCommandListeners.add(listener); - } - traceVerbose(`Shell Integration is enabled, executeCommand: ${commandLine}`); - }); + traceVerbose(`Shell Integration is enabled, executeCommand: ${commandLine}`); + return execution; } else { terminal.sendText(commandLine); traceVerbose(`Shell Integration is disabled, sendText: ${commandLine}`); @@ -136,7 +124,6 @@ export class TerminalService implements ITerminalService, Disposable { this.terminalShellType = this.terminalHelper.identifyTerminalShell(this.terminal); this.terminal = this.terminalManager.createTerminal({ name: this.options?.title || 'Python', - env: { PYTHONSTARTUP: this.envVarScript }, hideFromUser: this.options?.hideFromUser, }); this.terminalAutoActivator.disableAutoActivation(this.terminal); diff --git a/src/client/common/terminal/syncTerminalService.ts b/src/client/common/terminal/syncTerminalService.ts index e5b120a11110..60f8ed7a6847 100644 --- a/src/client/common/terminal/syncTerminalService.ts +++ b/src/client/common/terminal/syncTerminalService.ts @@ -4,7 +4,7 @@ 'use strict'; import { inject } from 'inversify'; -import { CancellationToken, Disposable, Event } from 'vscode'; +import { CancellationToken, Disposable, Event, TerminalShellExecution } from 'vscode'; import { IInterpreterService } from '../../interpreter/contracts'; import { traceVerbose } from '../../logging'; import { PythonEnvironment } from '../../pythonEnvironments/info'; @@ -14,7 +14,7 @@ import * as internalScripts from '../process/internal/scripts'; import { createDeferred, Deferred } from '../utils/async'; import { noop } from '../utils/misc'; import { TerminalService } from './service'; -import { ITerminalService, ITerminalExecutedCommand } from './types'; +import { ITerminalService } from './types'; enum State { notStarted = 0, @@ -145,7 +145,7 @@ export class SynchronousTerminalService implements ITerminalService, Disposable public sendText(text: string): Promise { return this.terminalService.sendText(text); } - public executeCommand(commandLine: string): Promise { + public executeCommand(commandLine: string): Promise { return this.terminalService.executeCommand(commandLine); } public show(preserveFocus?: boolean | undefined): Promise { diff --git a/src/client/common/terminal/types.ts b/src/client/common/terminal/types.ts index f8ae38f5d403..db2b7f80e4b1 100644 --- a/src/client/common/terminal/types.ts +++ b/src/client/common/terminal/types.ts @@ -54,15 +54,10 @@ export interface ITerminalService extends IDisposable { ): Promise; /** @deprecated */ sendText(text: string): Promise; - executeCommand(commandLine: string): Promise; + executeCommand(commandLine: string): Promise; show(preserveFocus?: boolean): Promise; } -export interface ITerminalExecutedCommand { - execution: TerminalShellExecution; - exitCode: number | undefined; -} - export const ITerminalServiceFactory = Symbol('ITerminalServiceFactory'); export type TerminalCreationOptions = { diff --git a/src/test/common/terminals/helper.unit.test.ts b/src/test/common/terminals/helper.unit.test.ts index 864188b7c7b4..0d130b573408 100644 --- a/src/test/common/terminals/helper.unit.test.ts +++ b/src/test/common/terminals/helper.unit.test.ts @@ -96,6 +96,30 @@ suite('Terminal Service helpers', () => { teardown(() => shellDetectorIdentifyTerminalShell.restore()); suite('Misc', () => { setup(doSetup); + test('Creating terminal should not automatically contain PYTHONSTARTUP', () => { + const theTitle = 'Hello'; + const terminal = 'Terminal Created'; + when(terminalManager.createTerminal(anything())).thenReturn(terminal as any); + const term = helper.createTerminal(theTitle); + const args = capture(terminalManager.createTerminal).first()[0]; + expect(term).to.be.deep.equal(terminal); + const terminalOptions = args.env; + const safeTerminalOptions = terminalOptions || {}; + expect(safeTerminalOptions).to.not.have.property('PYTHONSTARTUP'); + }); + + test('Env should be undefined if not explicitly passed in ', () => { + const theTitle = 'Hello'; + const terminal = 'Terminal Created'; + when(terminalManager.createTerminal(anything())).thenReturn(terminal as any); + + const term = helper.createTerminal(theTitle); + + verify(terminalManager.createTerminal(anything())).once(); + const args = capture(terminalManager.createTerminal).first()[0]; + expect(term).to.be.deep.equal(terminal); + expect(args.env).to.be.deep.equal(undefined); + }); test('Create terminal without a title', () => { const terminal = 'Terminal Created'; diff --git a/src/test/smoke/smartSend.smoke.test.ts b/src/test/smoke/smartSend.smoke.test.ts index 7f894df923ee..dc1f07f047e7 100644 --- a/src/test/smoke/smartSend.smoke.test.ts +++ b/src/test/smoke/smartSend.smoke.test.ts @@ -6,81 +6,78 @@ import { EXTENSION_ROOT_DIR_FOR_TESTS, IS_SMOKE_TEST } from '../constants'; import { closeActiveWindows, initialize, initializeTest } from '../initialize'; import { openFile, waitForCondition } from '../common'; -// TODO: This test is being flaky for windows, need to investigate why only fails on windows -if (process.platform !== 'win32') { - suite('Smoke Test: Run Smart Selection and Advance Cursor', () => { - suiteSetup(async function () { - if (!IS_SMOKE_TEST) { - return this.skip(); - } - await initialize(); - return undefined; - }); +suite('Smoke Test: Run Smart Selection and Advance Cursor', async () => { + suiteSetup(async function () { + if (!IS_SMOKE_TEST) { + return this.skip(); + } + await initialize(); + return undefined; + }); - setup(initializeTest); - suiteTeardown(closeActiveWindows); - teardown(closeActiveWindows); + setup(initializeTest); + suiteTeardown(closeActiveWindows); + teardown(closeActiveWindows); - test('Smart Send', async () => { - const file = path.join( - EXTENSION_ROOT_DIR_FOR_TESTS, - 'src', - 'testMultiRootWkspc', - 'smokeTests', - 'create_delete_file.py', - ); - const outputFile = path.join( - EXTENSION_ROOT_DIR_FOR_TESTS, - 'src', - 'testMultiRootWkspc', - 'smokeTests', - 'smart_send_smoke.txt', - ); + test('Smart Send', async () => { + const file = path.join( + EXTENSION_ROOT_DIR_FOR_TESTS, + 'src', + 'testMultiRootWkspc', + 'smokeTests', + 'create_delete_file.py', + ); + const outputFile = path.join( + EXTENSION_ROOT_DIR_FOR_TESTS, + 'src', + 'testMultiRootWkspc', + 'smokeTests', + 'smart_send_smoke.txt', + ); - await fs.remove(outputFile); + await fs.remove(outputFile); - const textDocument = await openFile(file); + const textDocument = await openFile(file); - if (vscode.window.activeTextEditor) { - const myPos = new vscode.Position(0, 0); - vscode.window.activeTextEditor!.selections = [new vscode.Selection(myPos, myPos)]; - } - await vscode.commands - .executeCommand('python.execSelectionInTerminal', textDocument.uri) - .then(undefined, (err) => { - assert.fail(`Something went wrong running the Python file in the terminal: ${err}`); - }); + if (vscode.window.activeTextEditor) { + const myPos = new vscode.Position(0, 0); + vscode.window.activeTextEditor!.selections = [new vscode.Selection(myPos, myPos)]; + } + await vscode.commands + .executeCommand('python.execSelectionInTerminal', textDocument.uri) + .then(undefined, (err) => { + assert.fail(`Something went wrong running the Python file in the terminal: ${err}`); + }); - const checkIfFileHasBeenCreated = () => fs.pathExists(outputFile); - await waitForCondition(checkIfFileHasBeenCreated, 10_000, `"${outputFile}" file not created`); + const checkIfFileHasBeenCreated = () => fs.pathExists(outputFile); + await waitForCondition(checkIfFileHasBeenCreated, 20_000, `"${outputFile}" file not created`); - await vscode.commands - .executeCommand('python.execSelectionInTerminal', textDocument.uri) - .then(undefined, (err) => { - assert.fail(`Something went wrong running the Python file in the terminal: ${err}`); - }); - await vscode.commands - .executeCommand('python.execSelectionInTerminal', textDocument.uri) - .then(undefined, (err) => { - assert.fail(`Something went wrong running the Python file in the terminal: ${err}`); - }); + await vscode.commands + .executeCommand('python.execSelectionInTerminal', textDocument.uri) + .then(undefined, (err) => { + assert.fail(`Something went wrong running the Python file in the terminal: ${err}`); + }); + await vscode.commands + .executeCommand('python.execSelectionInTerminal', textDocument.uri) + .then(undefined, (err) => { + assert.fail(`Something went wrong running the Python file in the terminal: ${err}`); + }); - async function wait() { - return new Promise((resolve) => { - setTimeout(() => { - resolve(); - }, 10000); - }); - } + async function wait() { + return new Promise((resolve) => { + setTimeout(() => { + resolve(); + }, 10000); + }); + } - await wait(); + await wait(); - const deletedFile = !(await fs.pathExists(outputFile)); - if (deletedFile) { - assert.ok(true, `"${outputFile}" file has been deleted`); - } else { - assert.fail(`"${outputFile}" file still exists`); - } - }); + const deletedFile = !(await fs.pathExists(outputFile)); + if (deletedFile) { + assert.ok(true, `"${outputFile}" file has been deleted`); + } else { + assert.fail(`"${outputFile}" file still exists`); + } }); -} +}); diff --git a/src/test/smokeTest.ts b/src/test/smokeTest.ts index bcd70c0e3417..a101e961e03d 100644 --- a/src/test/smokeTest.ts +++ b/src/test/smokeTest.ts @@ -5,7 +5,6 @@ // Must always be on top to setup expected env. process.env.VSC_PYTHON_SMOKE_TEST = '1'; - import { spawn } from 'child_process'; import * as fs from '../client/common/platform/fs-paths'; import * as glob from 'glob';