Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Prepend PATH both at shell integration and process creation #22905

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

Merged
merged 2 commits into from
Feb 13, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
14 changes: 9 additions & 5 deletions 14 src/client/terminals/envCollectionActivation/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
this.disposables,
);
const { shell } = this.applicationEnvironment;
const isActive = await this.shellIntegrationService.isWorking(shell);
const isActive = await this.shellIntegrationService.isWorking();
const shellType = identifyShellFromShellPath(shell);
if (!isActive && shellType !== TerminalShellType.commandPrompt) {
traceWarn(
Expand Down Expand Up @@ -218,6 +218,10 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
return;
}
if (key === 'PATH') {
const options = {
applyAtShellIntegration: true,
applyAtProcessCreation: true,
};
if (processEnv.PATH && env.PATH?.endsWith(processEnv.PATH)) {
// Prefer prepending to PATH instead of replacing it, as we do not want to replace any
// changes to PATH users might have made it in their init scripts (~/.bashrc etc.)
Expand All @@ -226,7 +230,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
value = `${deactivate}${this.separator}${value}`;
}
traceVerbose(`Prepending environment variable ${key} in collection with ${value}`);
envVarCollection.prepend(key, value, prependOptions);
envVarCollection.prepend(key, value, options);
} else {
if (!value.endsWith(this.separator)) {
value = value.concat(this.separator);
Expand All @@ -235,7 +239,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
value = `${deactivate}${this.separator}${value}`;
}
traceVerbose(`Prepending environment variable ${key} in collection to ${value}`);
envVarCollection.prepend(key, value, prependOptions);
envVarCollection.prepend(key, value, options);
}
return;
}
Expand Down Expand Up @@ -300,7 +304,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
// PS1 should be set but no PS1 was set.
return;
}
const config = await this.shellIntegrationService.isWorking(shell);
const config = await this.shellIntegrationService.isWorking();
if (!config) {
traceVerbose('PS1 is not set when shell integration is disabled.');
return;
Expand Down Expand Up @@ -356,7 +360,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
}

private async getPrependOptions(): Promise<EnvironmentVariableMutatorOptions> {
const isActive = await this.shellIntegrationService.isWorking(this.applicationEnvironment.shell);
const isActive = await this.shellIntegrationService.isWorking();
// Ideally we would want to prepend exactly once, either at shell integration or process creation.
// TODO: Stop prepending altogether once https://github.com/microsoft/vscode/issues/145234 is available.
return isActive
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ export class ShellIntegrationService implements IShellIntegrationService {

public readonly onDidChangeStatus = this.didChange.event;

public async isWorking(shell: string): Promise<boolean> {
public async isWorking(): Promise<boolean> {
const { shell } = this.appEnvironment;
return this._isWorking(shell).catch((ex) => {
traceError(`Failed to determine if shell supports shell integration`, shell, ex);
return false;
Expand Down
2 changes: 1 addition & 1 deletion 2 src/client/terminals/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export interface ITerminalEnvVarCollectionService {
export const IShellIntegrationService = Symbol('IShellIntegrationService');
export interface IShellIntegrationService {
onDidChangeStatus: Event<void>;
isWorking(shell: string): Promise<boolean>;
isWorking(): Promise<boolean>;
}

export const ITerminalDeactivateService = Symbol('ITerminalDeactivateService');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ suite('Terminal Environment Variable Collection Service', () => {
context = mock<IExtensionContext>();
shell = mock<IApplicationShell>();
shellIntegrationService = mock<IShellIntegrationService>();
when(shellIntegrationService.isWorking(anything())).thenResolve(true);
when(shellIntegrationService.isWorking()).thenResolve(true);
globalCollection = mock<GlobalEnvironmentVariableCollection>();
collection = mock<EnvironmentVariableCollection>();
when(context.environmentVariableCollection).thenReturn(instance(globalCollection));
Expand Down Expand Up @@ -336,7 +336,7 @@ suite('Terminal Environment Variable Collection Service', () => {
verify(collection.clear()).once();
verify(collection.prepend('PATH', prependedPart, anything())).once();
verify(collection.replace('PATH', anything(), anything())).never();
assert.deepEqual(opts, { applyAtProcessCreation: false, applyAtShellIntegration: true });
assert.deepEqual(opts, { applyAtProcessCreation: true, applyAtShellIntegration: true });
});

test('Also prepend deactivate script location if available', async () => {
Expand Down Expand Up @@ -372,7 +372,7 @@ suite('Terminal Environment Variable Collection Service', () => {
const separator = getOSType() === OSType.Windows ? ';' : ':';
verify(collection.prepend('PATH', `scriptLocation${separator}${prependedPart}`, anything())).once();
verify(collection.replace('PATH', anything(), anything())).never();
assert.deepEqual(opts, { applyAtProcessCreation: false, applyAtShellIntegration: true });
assert.deepEqual(opts, { applyAtProcessCreation: true, applyAtShellIntegration: true });
});

test('Prepend full PATH with separator otherwise', async () => {
Expand Down Expand Up @@ -405,7 +405,7 @@ suite('Terminal Environment Variable Collection Service', () => {
verify(collection.clear()).once();
verify(collection.prepend('PATH', `${finalPath}${separator}`, anything())).once();
verify(collection.replace('PATH', anything(), anything())).never();
assert.deepEqual(opts, { applyAtProcessCreation: false, applyAtShellIntegration: true });
assert.deepEqual(opts, { applyAtProcessCreation: true, applyAtShellIntegration: true });
});

test('Prepend full PATH with separator otherwise', async () => {
Expand Down Expand Up @@ -441,7 +441,7 @@ suite('Terminal Environment Variable Collection Service', () => {
verify(collection.clear()).once();
verify(collection.prepend('PATH', `scriptLocation${separator}${finalPath}${separator}`, anything())).once();
verify(collection.replace('PATH', anything(), anything())).never();
assert.deepEqual(opts, { applyAtProcessCreation: false, applyAtShellIntegration: true });
assert.deepEqual(opts, { applyAtProcessCreation: true, applyAtShellIntegration: true });
});

test('Verify envs are not applied if env activation is disabled', async () => {
Expand Down Expand Up @@ -523,7 +523,7 @@ suite('Terminal Environment Variable Collection Service', () => {

test('Correct track that prompt was set for PS1 if shell integration is disabled', async () => {
reset(shellIntegrationService);
when(shellIntegrationService.isWorking(anything())).thenResolve(false);
when(shellIntegrationService.isWorking()).thenResolve(false);
when(platform.osType).thenReturn(OSType.Linux);
const envVars: NodeJS.ProcessEnv = { VIRTUAL_ENV: 'prefix/to/venv', PS1: '(.venv)', ...process.env };
const ps1Shell = 'bash';
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.