Skip to content

Commit

Permalink
Prepend PATH both at shell integration and process creation (#22905)
Browse files Browse the repository at this point in the history
  • Loading branch information
Kartik Raj committed Feb 13, 2024
1 parent 5174d5c commit 5f971ae
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 13 deletions.
14 changes: 9 additions & 5 deletions src/client/terminals/envCollectionActivation/service.ts
View file
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
3 changes: 2 additions & 1 deletion src/client/terminals/envCollectionActivation/shellIntegrationService.ts
View file
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 src/client/terminals/types.ts
View file
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
12 changes: 6 additions & 6 deletions src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts
View file
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

0 comments on commit 5f971ae

Please sign in to comment.