From d9b9c88f3e555eab540aef5af0f47619c749322e Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 31 Aug 2023 10:23:21 -0700 Subject: [PATCH] Always prepend to PATH instead of replacing it (#21906) For #20822 #11039 Replacing as-is has its problems, for eg. pyenv asks their users to manipulate `PATH` in their init script: https://github.com/pyenv/pyenv#set-up-your-shell-environment-for-pyenv, which we could end up replacing. ![image](https://github.com/microsoft/vscode-python/assets/13199757/cc904f76-8d42-47e1-a6c8-6cfff6543db8) Particularly for pyenv, it mean users not being able to find pyenv: ![image](https://github.com/microsoft/vscode-python/assets/13199757/26100328-c227-435b-a4f2-ec168099f4c1) Prepending solves it for cases where initial PATH value is suffix of the final value: ![image](https://github.com/microsoft/vscode-python/assets/13199757/a95e4ffd-68dc-4e73-905e-504b3051324f) But, in other cases, this means that we end up with the whole `PATH` thrice. This is because it prepends it twice: - Once in shell integration script - Once when creating a process So the final value could be: ``` PATH= ``` where `` refers to value of `PATH` environment variable post activation. eg. ![image](https://github.com/microsoft/vscode-python/assets/13199757/7e771f62-eb53-49be-b261-d259096008f3) --- .../terminalEnvVarCollectionService.ts | 22 ++++++- ...rminalEnvVarCollectionService.unit.test.ts | 64 +++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts index fa949ff69fad..9015dd7b9388 100644 --- a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts +++ b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts @@ -167,7 +167,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ if (shouldSkip(key)) { return; } - const value = env[key]; + let value = env[key]; const prevValue = processEnv[key]; if (prevValue !== value) { if (value !== undefined) { @@ -180,6 +180,26 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ }); return; } + if (key === 'PATH') { + 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.) + const prependedPart = env.PATH.slice(0, -processEnv.PATH.length); + value = prependedPart; + traceVerbose(`Prepending environment variable ${key} in collection with ${value}`); + envVarCollection.prepend(key, value, { + applyAtShellIntegration: true, + applyAtProcessCreation: true, + }); + } else { + traceVerbose(`Prepending environment variable ${key} in collection to ${value}`); + envVarCollection.prepend(key, value, { + applyAtShellIntegration: true, + applyAtProcessCreation: true, + }); + } + return; + } traceVerbose(`Setting environment variable ${key} in collection to ${value}`); envVarCollection.replace(key, value, { applyAtShellIntegration: true, diff --git a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts index b3a017031765..1513be676ee4 100644 --- a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts +++ b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts @@ -216,6 +216,70 @@ suite('Terminal Environment Variable Collection Service', () => { assert.deepEqual(opts, { applyAtProcessCreation: false, applyAtShellIntegration: true }); }); + test('Prepend only "prepend portion of PATH" where applicable', async () => { + const processEnv = { PATH: 'hello/1/2/3' }; + reset(environmentActivationService); + when(environmentActivationService.getProcessEnvironmentVariables(anything(), anything())).thenResolve( + processEnv, + ); + const prependedPart = 'path/to/activate/dir:'; + const envVars: NodeJS.ProcessEnv = { PATH: `${prependedPart}${processEnv.PATH}` }; + when( + environmentActivationService.getActivatedEnvironmentVariables( + anything(), + undefined, + undefined, + customShell, + ), + ).thenResolve(envVars); + + when(collection.replace(anything(), anything(), anything())).thenResolve(); + when(collection.delete(anything())).thenResolve(); + let opts: EnvironmentVariableMutatorOptions | undefined; + when(collection.prepend('PATH', anything(), anything())).thenCall((_, _v, o) => { + opts = o; + }); + + await terminalEnvVarCollectionService._applyCollection(undefined, customShell); + + verify(collection.clear()).once(); + verify(collection.prepend('PATH', prependedPart, anything())).once(); + verify(collection.replace('PATH', anything(), anything())).never(); + assert.deepEqual(opts, { applyAtProcessCreation: true, applyAtShellIntegration: true }); + }); + + test('Prepend full PATH otherwise', async () => { + const processEnv = { PATH: 'hello/1/2/3' }; + reset(environmentActivationService); + when(environmentActivationService.getProcessEnvironmentVariables(anything(), anything())).thenResolve( + processEnv, + ); + const finalPath = 'hello/3/2/1'; + const envVars: NodeJS.ProcessEnv = { PATH: finalPath }; + when( + environmentActivationService.getActivatedEnvironmentVariables( + anything(), + undefined, + undefined, + customShell, + ), + ).thenResolve(envVars); + + when(collection.replace(anything(), anything(), anything())).thenResolve(); + when(collection.delete(anything())).thenResolve(); + let opts: EnvironmentVariableMutatorOptions | undefined; + when(collection.prepend('PATH', anything(), anything())).thenCall((_, _v, o) => { + opts = o; + }); + + await terminalEnvVarCollectionService._applyCollection(undefined, customShell); + + verify(collection.clear()).once(); + verify(collection.prepend('PATH', finalPath, anything())).once(); + verify(collection.replace('PATH', anything(), anything())).never(); + assert.deepEqual(opts, { applyAtProcessCreation: true, applyAtShellIntegration: true }); + }); + test('Verify envs are not applied if env activation is disabled', async () => { const envVars: NodeJS.ProcessEnv = { CONDA_PREFIX: 'prefix/to/conda', ...process.env }; when(