Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding PYTHONSTARTUP with shell integration to environment variable collection #24111

Merged
merged 18 commits into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,12 @@
"scope": "resource",
"type": "array"
},
"python.terminal.enable Shell Integration in Python Terminal REPL": {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I get feedback on the setting name and description? @cwebster-99
This is for allowing user to opt out of using shell integration for REPL when they launch the terminal REPL via typing out "python" in terminal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good. My one suggestion for the description is to be a bit more descriptive on what shell integration might enable or disable from the user perspective so users may better understand what might change in their experience.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! Made some changes here: 8e8dd18
So it would look like:
Screenshot 2024-09-16 at 10 58 35 AM

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debating between

Python Startup For Shell Integration 

vs

Python Start up For Shell Integration 

vs

PYTHONSTARTUP For Shell Integration   (PYTHONSTARTUP is actual Python official name of the file we are injecting to enable the feature)

vs

Python Start Up Script

for the title

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like the one that is there currently (option 1)!

"default": true,
"description": "%python.terminal.enableSITerminal.description%",
"scope": "resource",
"type": "boolean"
},
"python.REPL.enableREPLSmartSend": {
"default": true,
"description": "%python.EnableREPLSmartSend.description%",
Expand Down
1 change: 1 addition & 0 deletions package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
"python.tensorBoard.logDirectory.description": "Set this setting to your preferred TensorBoard log directory to skip log directory prompt when starting TensorBoard.",
"python.tensorBoard.logDirectory.markdownDeprecationMessage": "Tensorboard support has been moved to the extension [Tensorboard extension](https://marketplace.visualstudio.com/items?itemName=ms-toolsai.tensorboard). Instead use the setting `tensorBoard.logDirectory`.",
"python.tensorBoard.logDirectory.deprecationMessage": "Tensorboard support has been moved to the extension Tensorboard extension. Instead use the setting `tensorBoard.logDirectory`.",
"python.terminal.enableSITerminal.description": "Enable Shell Integration for Python Terminal REPL.",
"python.terminal.activateEnvInCurrentTerminal.description": "Activate Python Environment in the current Terminal on load of the Extension.",
"python.terminal.activateEnvironment.description": "Activate Python Environment in all Terminals created.",
"python.terminal.executeInFileDir.description": "When executing a file in the terminal, whether to use execute in the file's directory, instead of the current open folder.",
Expand Down
1 change: 1 addition & 0 deletions src/client/common/configSettings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,7 @@ export class PythonSettings implements IPythonSettings {
launchArgs: [],
activateEnvironment: true,
activateEnvInCurrentTerminal: false,
pythonrcStartup: true,
};

this.REPL = pythonSettings.get<IREPLSettings>('REPL')!;
Expand Down
4 changes: 4 additions & 0 deletions src/client/common/terminal/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ export class TerminalService implements ITerminalService, Disposable {
}
this.terminal!.sendText(text);
}

public async executeCommand(commandLine: string): Promise<ITerminalExecutedCommand | undefined> {
const terminal = this.terminal!;
if (!this.options?.hideFromUser) {
Expand Down Expand Up @@ -125,6 +126,9 @@ export class TerminalService implements ITerminalService, Disposable {
this.terminal!.show(preserveFocus);
}
}
// When terminal is launched
// Copy pythonrc into <userdata>/pythonrc.py
// Update environment variable collection to include PYTHONSTARTUP=<userdata>/pythonrc.py
// TODO: Debt switch to Promise<Terminal> ---> breaks 20 tests
public async ensureTerminal(preserveFocus: boolean = true): Promise<void> {
if (this.terminal) {
Expand Down
1 change: 1 addition & 0 deletions src/client/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ export interface ITerminalSettings {
readonly launchArgs: string[];
readonly activateEnvironment: boolean;
readonly activateEnvInCurrentTerminal: boolean;
readonly pythonrcStartup: boolean;
}

export interface IREPLSettings {
Expand Down
9 changes: 8 additions & 1 deletion src/client/extensionActivation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,12 @@ import { TerminalProvider } from './providers/terminalProvider';
import { setExtensionInstallTelemetryProperties } from './telemetry/extensionInstallTelemetry';
import { registerTypes as tensorBoardRegisterTypes } from './tensorBoard/serviceRegistry';
import { registerTypes as commonRegisterTerminalTypes } from './terminals/serviceRegistry';
import { ICodeExecutionHelper, ICodeExecutionManager, ITerminalAutoActivation } from './terminals/types';
import {
ICodeExecutionHelper,
ICodeExecutionManager,
IPythonStartupEnvVarService,
ITerminalAutoActivation,
} from './terminals/types';
import { registerTypes as unitTestsRegisterTypes } from './testing/serviceRegistry';

// components
Expand Down Expand Up @@ -177,6 +182,8 @@ async function activateLegacy(ext: ExtensionState, startupStopWatch: StopWatch):

serviceManager.get<ITerminalAutoActivation>(ITerminalAutoActivation).register();

serviceManager.get<IPythonStartupEnvVarService>(IPythonStartupEnvVarService).register();

serviceManager.get<ICodeExecutionManager>(ICodeExecutionManager).registerCommands();

disposables.push(new ReplProvider(serviceContainer));
Expand Down
9 changes: 7 additions & 2 deletions src/client/terminals/envCollectionActivation/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,11 @@ import { TerminalShellType } from '../../common/terminal/types';
import { OSType } from '../../common/utils/platform';

import { PythonEnvType } from '../../pythonEnvironments/base/info';
import { IShellIntegrationService, ITerminalDeactivateService, ITerminalEnvVarCollectionService } from '../types';
import {
IShellIntegrationDetectionService,
ITerminalDeactivateService,
ITerminalEnvVarCollectionService,
} from '../types';
import { ProgressService } from '../../common/application/progressService';

@injectable()
Expand Down Expand Up @@ -80,7 +84,8 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
@inject(IConfigurationService) private readonly configurationService: IConfigurationService,
@inject(ITerminalDeactivateService) private readonly terminalDeactivateService: ITerminalDeactivateService,
@inject(IPathUtils) private readonly pathUtils: IPathUtils,
@inject(IShellIntegrationService) private readonly shellIntegrationService: IShellIntegrationService,
@inject(IShellIntegrationDetectionService)
private readonly shellIntegrationService: IShellIntegrationDetectionService,
@inject(IEnvironmentVariablesProvider)
private readonly environmentVariablesProvider: IEnvironmentVariablesProvider,
) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { TerminalShellType } from '../../common/terminal/types';
import { IDisposableRegistry, IPersistentStateFactory } from '../../common/types';
import { sleep } from '../../common/utils/async';
import { traceError, traceVerbose } from '../../logging';
import { IShellIntegrationService } from '../types';
import { IShellIntegrationDetectionService } from '../types';

/**
* This is a list of shells which support shell integration:
Expand All @@ -33,7 +33,7 @@ export enum isShellIntegrationWorking {
}

@injectable()
export class ShellIntegrationService implements IShellIntegrationService {
export class ShellIntegrationDetectionService implements IShellIntegrationDetectionService {
private isWorkingForShell = new Set<TerminalShellType>();

private readonly didChange = new EventEmitter<void>();
Expand Down
43 changes: 43 additions & 0 deletions src/client/terminals/pythonStartupEnvVar/service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import * as path from 'path';
import { injectable, inject } from 'inversify';
import { Uri, workspace } from 'vscode';
import { IPythonStartupEnvVarService } from '../types';
import { IConfigurationService, IExtensionContext } from '../../common/types';
import { EXTENSION_ROOT_DIR } from '../../constants';

@injectable()
export class PythonStartupEnvVarService implements IPythonStartupEnvVarService {
constructor(
@inject(IExtensionContext) private context: IExtensionContext,
@inject(IConfigurationService) private configurationService: IConfigurationService,
) {}

public async register(): Promise<void> {
const storageUri = this.context.storageUri || this.context.globalStorageUri;
try {
await workspace.fs.createDirectory(storageUri);
} catch {
// already exists, most likely
}
const destPath = Uri.joinPath(storageUri, 'pythonrc.py');

// TODO: Only do this when we have a setting
// Rollout strategy:
// Stage 1. Opt-in setting in stable/insiders
// Stage 2. Out-out setting in insiders
// Stage 3. Out-out setting in stable (or experiment?)
const sourcePath = path.join(EXTENSION_ROOT_DIR, 'python_files', 'pythonrc.py');

await workspace.fs.copy(Uri.file(sourcePath), destPath, { overwrite: true });
const pythonrcSetting = this.configurationService.getSettings().terminal.pythonrcStartup;
// TODO: Is there better place to set this?
if (pythonrcSetting) {
this.context.environmentVariableCollection.replace('PYTHONSTARTUP', destPath.fsPath);
} else {
this.context.environmentVariableCollection.delete('PYTHONSTARTUP');
}
}
}
13 changes: 10 additions & 3 deletions src/client/terminals/serviceRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,18 @@ import {
ICodeExecutionHelper,
ICodeExecutionManager,
ICodeExecutionService,
IShellIntegrationService,
IPythonStartupEnvVarService,
IShellIntegrationDetectionService,
ITerminalAutoActivation,
ITerminalDeactivateService,
ITerminalEnvVarCollectionService,
} from './types';
import { TerminalEnvVarCollectionService } from './envCollectionActivation/service';
import { IExtensionActivationService, IExtensionSingleActivationService } from '../activation/types';
import { TerminalIndicatorPrompt } from './envCollectionActivation/indicatorPrompt';
import { ShellIntegrationService } from './envCollectionActivation/shellIntegrationService';
import { TerminalDeactivateService } from './envCollectionActivation/deactivateService';
import { ShellIntegrationDetectionService } from './envCollectionActivation/shellIntegrationService';
import { PythonStartupEnvVarService } from './pythonStartupEnvVar/service';

export function registerTypes(serviceManager: IServiceManager): void {
serviceManager.addSingleton<ICodeExecutionHelper>(ICodeExecutionHelper, CodeExecutionHelper);
Expand Down Expand Up @@ -50,6 +52,11 @@ export function registerTypes(serviceManager: IServiceManager): void {
IExtensionSingleActivationService,
TerminalIndicatorPrompt,
);
serviceManager.addSingleton<IShellIntegrationService>(IShellIntegrationService, ShellIntegrationService);
serviceManager.addSingleton<IShellIntegrationDetectionService>(
IShellIntegrationDetectionService,
ShellIntegrationDetectionService,
);
serviceManager.addSingleton<IPythonStartupEnvVarService>(IPythonStartupEnvVarService, PythonStartupEnvVarService);

serviceManager.addBinding(ITerminalEnvVarCollectionService, IExtensionActivationService);
}
9 changes: 7 additions & 2 deletions src/client/terminals/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ export interface ITerminalEnvVarCollectionService {
isTerminalPromptSetCorrectly(resource?: Resource): boolean;
}

export const IShellIntegrationService = Symbol('IShellIntegrationService');
export interface IShellIntegrationService {
export const IShellIntegrationDetectionService = Symbol('IShellIntegrationDetectionService');
export interface IShellIntegrationDetectionService {
onDidChangeStatus: Event<void>;
isWorking(): Promise<boolean>;
}
Expand All @@ -53,3 +53,8 @@ export interface ITerminalDeactivateService {
initializeScriptParams(shell: string): Promise<void>;
getScriptLocation(shell: string, resource: Resource): Promise<string | undefined>;
}

export const IPythonStartupEnvVarService = Symbol('IPythonStartupEnvVarService');
export interface IPythonStartupEnvVarService {
register(): void;
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +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, ITerminalDeactivateService } from '../../../client/terminals/types';
import { IShellIntegrationDetectionService, ITerminalDeactivateService } from '../../../client/terminals/types';
import { IEnvironmentVariablesProvider } from '../../../client/common/variables/types';

suite('Terminal Environment Variable Collection Service', () => {
Expand All @@ -58,7 +58,7 @@ suite('Terminal Environment Variable Collection Service', () => {
title: Interpreters.activatingTerminals,
};
let configService: IConfigurationService;
let shellIntegrationService: IShellIntegrationService;
let shellIntegrationService: IShellIntegrationDetectionService;
const displayPath = 'display/path';
const customShell = 'powershell';
const defaultShell = defaultShells[getOSType()];
Expand All @@ -76,7 +76,7 @@ suite('Terminal Environment Variable Collection Service', () => {
context = mock<IExtensionContext>();
shell = mock<IApplicationShell>();
const envVarProvider = mock<IEnvironmentVariablesProvider>();
shellIntegrationService = mock<IShellIntegrationService>();
shellIntegrationService = mock<IShellIntegrationDetectionService>();
when(shellIntegrationService.isWorking()).thenResolve(true);
globalCollection = mock<GlobalEnvironmentVariableCollection>();
collection = mock<EnvironmentVariableCollection>();
Expand Down
10 changes: 7 additions & 3 deletions src/test/terminals/serviceRegistry.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,20 @@ import { TerminalCodeExecutionProvider } from '../../client/terminals/codeExecut
import { TerminalDeactivateService } from '../../client/terminals/envCollectionActivation/deactivateService';
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,
IPythonStartupEnvVarService,
IShellIntegrationDetectionService,
ITerminalAutoActivation,
ITerminalDeactivateService,
ITerminalEnvVarCollectionService,
} from '../../client/terminals/types';
import { ShellIntegrationDetectionService } from '../../client/terminals/envCollectionActivation/shellIntegrationService';
import { PythonStartupEnvVarService } from '../../client/terminals/pythonStartupEnvVar/service';

suite('Terminal - Service Registry', () => {
test('Ensure all services get registered', () => {
Expand All @@ -39,7 +42,8 @@ suite('Terminal - Service Registry', () => {
[ITerminalEnvVarCollectionService, TerminalEnvVarCollectionService],
[IExtensionSingleActivationService, TerminalIndicatorPrompt],
[ITerminalDeactivateService, TerminalDeactivateService],
[IShellIntegrationService, ShellIntegrationService],
[IShellIntegrationDetectionService, ShellIntegrationDetectionService],
[IPythonStartupEnvVarService, PythonStartupEnvVarService],
].forEach((args) => {
if (args.length === 2) {
services
Expand Down
Loading