Skip to content

Commit

Permalink
dont automatically inject PYTHONSTARTUP (#24346)
Browse files Browse the repository at this point in the history 
Resolves #24345 and
#24290 and
#24105
remove automatic injection from before so only thing that allows shell
integration to user for Python terminal REPL is the setting itself.
  • Loading branch information
@anthonykim1
anthonykim1 authored Nov 4, 2024
1 parent 622653e commit 3e7e0d2
Show file tree
Hide file tree
Showing 6 changed files with 107 additions and 105 deletions.
43 changes: 15 additions & 28 deletions src/client/common/terminal/service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,14 @@
// 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';
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 {
Expand All @@ -20,7 +18,6 @@ import {
ITerminalService,
TerminalCreationOptions,
TerminalShellType,
ITerminalExecutedCommand,
} from './types';
import { traceVerbose } from '../../logging';

Expand All @@ -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<Disposable> = new Set();
private _terminalFirstLaunched: boolean = true;
public get onDidCloseTerminal(): Event<void> {
return this.terminalClosed.event.bind(this.terminalClosed);
}

constructor(
@inject(IServiceContainer) private serviceContainer: IServiceContainer,
private readonly options?: TerminalCreationOptions,
Expand Down Expand Up @@ -76,24 +74,24 @@ export class TerminalService implements ITerminalService, Disposable {
}
this.terminal!.sendText(text);
}
public async executeCommand(commandLine: string): Promise<ITerminalExecutedCommand | undefined> {
public async executeCommand(commandLine: string): Promise<TerminalShellExecution | undefined> {
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<boolean>((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);
});
Expand All @@ -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}`);
Expand All @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions src/client/common/terminal/syncTerminalService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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,
Expand Down Expand Up @@ -145,7 +145,7 @@ export class SynchronousTerminalService implements ITerminalService, Disposable
public sendText(text: string): Promise<void> {
return this.terminalService.sendText(text);
}
public executeCommand(commandLine: string): Promise<ITerminalExecutedCommand | undefined> {
public executeCommand(commandLine: string): Promise<TerminalShellExecution | undefined> {
return this.terminalService.executeCommand(commandLine);
}
public show(preserveFocus?: boolean | undefined): Promise<void> {
Expand Down
7 changes: 1 addition & 6 deletions src/client/common/terminal/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,10 @@ export interface ITerminalService extends IDisposable {
): Promise<void>;
/** @deprecated */
sendText(text: string): Promise<void>;
executeCommand(commandLine: string): Promise<ITerminalExecutedCommand | undefined>;
executeCommand(commandLine: string): Promise<TerminalShellExecution | undefined>;
show(preserveFocus?: boolean): Promise<void>;
}

export interface ITerminalExecutedCommand {
execution: TerminalShellExecution;
exitCode: number | undefined;
}

export const ITerminalServiceFactory = Symbol('ITerminalServiceFactory');

export type TerminalCreationOptions = {
Expand Down
24 changes: 24 additions & 0 deletions src/test/common/terminals/helper.unit.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
131 changes: 64 additions & 67 deletions src/test/smoke/smartSend.smoke.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>('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<void>('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<void>('python.execSelectionInTerminal', textDocument.uri)
.then(undefined, (err) => {
assert.fail(`Something went wrong running the Python file in the terminal: ${err}`);
});
await vscode.commands
.executeCommand<void>('python.execSelectionInTerminal', textDocument.uri)
.then(undefined, (err) => {
assert.fail(`Something went wrong running the Python file in the terminal: ${err}`);
});
await vscode.commands
.executeCommand<void>('python.execSelectionInTerminal', textDocument.uri)
.then(undefined, (err) => {
assert.fail(`Something went wrong running the Python file in the terminal: ${err}`);
});
await vscode.commands
.executeCommand<void>('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<void>((resolve) => {
setTimeout(() => {
resolve();
}, 10000);
});
}
async function wait() {
return new Promise<void>((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`);
}
});
}
});
1 change: 0 additions & 1 deletion src/test/smokeTest.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down

0 comments on commit 3e7e0d2

Please sign in to comment.