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.

Sign up for GitHub

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

remove python extension override of PATH variable #5354

Merged
merged 6 commits into from
Nov 14, 2024
Merged

remove python extension override of PATH variable #5354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b2414a9
do not use GlobalEnvironmentVariableCollection
isabelizimm Nov 13, 2024
c5ccca8
lint
isabelizimm Nov 13, 2024
2072c9f
update tests to pass preCompile
isabelizimm Nov 13, 2024
f8876ea
Merge branch 'main' into quarto-on-PATH
isabelizimm Nov 13, 2024
b2ef837
relint after merge
isabelizimm Nov 13, 2024
34fa70d
remove incorrect comment
isabelizimm Nov 13, 2024
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
41 changes: 31 additions & 10 deletions extensions/positron-python/src/client/terminals/envCollectionActivation/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@ import { inject, injectable } from 'inversify';
import {
MarkdownString,
WorkspaceFolder,
GlobalEnvironmentVariableCollection,
EnvironmentVariableScope,
// --- Start Positron ---
// GlobalEnvironmentVariableCollection,
// EnvironmentVariableScope,
EnvironmentVariableCollection,
// --- End Positron ---
EnvironmentVariableMutatorOptions,
ProgressLocation,
} from 'vscode';
Expand Down Expand Up @@ -169,7 +172,10 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
private async _applyCollectionImpl(resource: Resource, shell = this.applicationEnvironment.shell): Promise<void> {
const workspaceFolder = this.getWorkspaceFolder(resource);
const settings = this.configurationService.getSettings(resource);
const envVarCollection = this.getEnvironmentVariableCollection({ workspaceFolder });
// --- Start Positron ---
// remove workspace folder scope to avoid overwriting other extensions' env vars
const envVarCollection = this.getEnvironmentVariableCollection();
// --- End Positron ---
if (!settings.terminal.activateEnvironment) {
envVarCollection.clear();
traceVerbose('Activating environments in terminal is disabled for', resource?.fsPath);
Expand Down Expand Up @@ -365,9 +371,12 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
private async handleMicroVenv(resource: Resource) {
try {
const settings = this.configurationService.getSettings(resource);
const workspaceFolder = this.getWorkspaceFolder(resource);
// --- Start Positron ---
// remove workspace folder scope to avoid overwriting other extensions' env vars
// const workspaceFolder = this.getWorkspaceFolder(resource);
if (!settings.terminal.activateEnvironment) {
this.getEnvironmentVariableCollection({ workspaceFolder }).clear();
this.getEnvironmentVariableCollection().clear();
// --- End Positron ---
traceVerbose(
'Do not activate microvenv as activating environments in terminal is disabled for',
resource?.fsPath,
Expand All @@ -378,7 +387,9 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
if (interpreter?.envType === EnvironmentType.Venv) {
const activatePath = path.join(path.dirname(interpreter.path), 'activate');
if (!(await pathExists(activatePath))) {
const envVarCollection = this.getEnvironmentVariableCollection({ workspaceFolder });
// --- Start Positron ---
// remove workspace folder scope to avoid overwriting other extensions' env vars
const envVarCollection = this.getEnvironmentVariableCollection();
const pathVarName = getSearchPathEnvVarNames()[0];
envVarCollection.replace(
'PATH',
Expand All @@ -387,7 +398,8 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
);
return;
}
this.getEnvironmentVariableCollection({ workspaceFolder }).clear();
this.getEnvironmentVariableCollection().clear();
// --- End Positron ---
}
} catch (ex) {
traceWarn(`Microvenv failed as it is using proposed API which is constantly changing`, ex);
Expand All @@ -409,9 +421,18 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
};
}

private getEnvironmentVariableCollection(scope: EnvironmentVariableScope = {}) {
const envVarCollection = this.context.environmentVariableCollection as GlobalEnvironmentVariableCollection;
return envVarCollection.getScoped(scope);
// --- Start Positron ---
// Global Environment Variable Collections will overwrite other additions to the same environment variable
// from other extensions, eg, Quarto

private getEnvironmentVariableCollection() {
// private getEnvironmentVariableCollection(scope: EnvironmentVariableScope = {}) {
// const envVarCollection = this.context.environmentVariableCollection as GlobalEnvironmentVariableCollection;
// return envVarCollection.getScoped(scope);

const envVarCollection = this.context.environmentVariableCollection as EnvironmentVariableCollection;
isabelizimm marked this conversation as resolved.
Show resolved Hide resolved
return envVarCollection;
// --- End Positron ---
}

private getWorkspaceFolder(resource: Resource): WorkspaceFolder | undefined {
Expand Down
16 changes: 11 additions & 5 deletions ...tron-python/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import { mock, instance, when, anything, verify, reset } from 'ts-mockito';
import {
EnvironmentVariableCollection,
EnvironmentVariableMutatorOptions,
GlobalEnvironmentVariableCollection,
// --- Start Positron ---
// GlobalEnvironmentVariableCollection,
// --- End Positron ---
ProgressLocation,
Uri,
WorkspaceFolder,
Expand Down Expand Up @@ -47,7 +49,9 @@ suite('Terminal Environment Variable Collection Service', () => {
let shell: IApplicationShell;
let experimentService: IExperimentService;
let collection: EnvironmentVariableCollection;
let globalCollection: GlobalEnvironmentVariableCollection;
// --- Start Positron ---
// let globalCollection: GlobalEnvironmentVariableCollection;
// --- End Positron ---
let applicationEnvironment: IApplicationEnvironment;
let environmentActivationService: IEnvironmentActivationService;
let workspaceService: IWorkspaceService;
Expand Down Expand Up @@ -78,10 +82,12 @@ suite('Terminal Environment Variable Collection Service', () => {
const envVarProvider = mock<IEnvironmentVariablesProvider>();
shellIntegrationService = mock<IShellIntegrationService>();
when(shellIntegrationService.isWorking()).thenResolve(true);
globalCollection = mock<GlobalEnvironmentVariableCollection>();
// --- Start Positron ---
// globalCollection = mock<GlobalEnvironmentVariableCollection>();
collection = mock<EnvironmentVariableCollection>();
when(context.environmentVariableCollection).thenReturn(instance(globalCollection));
when(globalCollection.getScoped(anything())).thenReturn(instance(collection));
when(context.environmentVariableCollection as EnvironmentVariableCollection).thenReturn(instance(collection));
// when(globalCollection.getScoped(anything())).thenReturn(instance(collection));
// --- End Positron ---
experimentService = mock<IExperimentService>();
when(experimentService.inExperimentSync(TerminalEnvVarActivation.experiment)).thenReturn(true);
applicationEnvironment = mock<IApplicationEnvironment>();
Expand Down
Loading