From 547706a6db46052140fda190c268fc6b24e96a61 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 30 Aug 2023 23:38:56 +0000 Subject: [PATCH 1/4] Prefer prepending to PATH instead of replacing it --- .../terminalEnvVarCollectionService.ts | 16 +++++++++- ...rminalEnvVarCollectionService.unit.test.ts | 32 +++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts index 3849160b7e4b..4f3ea9ea6cbf 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,20 @@ 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, + }); + 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 2e2327bd181c..64a0e92b3694 100644 --- a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts +++ b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts @@ -216,6 +216,38 @@ suite('Terminal Environment Variable Collection Service', () => { assert.deepEqual(opts, { applyAtProcessCreation: false, applyAtShellIntegration: true }); }); + test('Prepend PATH instead of replacing it, where possible', 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('Verify envs are not applied if env activation is disabled', async () => { const envVars: NodeJS.ProcessEnv = { CONDA_PREFIX: 'prefix/to/conda', ...process.env }; when( From cb66272971eff45c2fe798a6eafb7306ca800fed Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 30 Aug 2023 23:53:43 +0000 Subject: [PATCH 2/4] Never replace --- .vscode/launch.json | 2 +- ...rminalEnvVarCollectionService.unit.test.ts | 34 ++++++++++++++++++- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 82981a93305d..ba8900b0a5d0 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -160,7 +160,7 @@ "--ui=tdd", "--recursive", "--colors", - //"--grep", "", + "--grep", "Terminal Environment Variable Collection", "--timeout=300000" ], "outFiles": ["${workspaceFolder}/out/**/*.js", "!${workspaceFolder}/**/node_modules**/*"], diff --git a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts index 64a0e92b3694..d0d0bcd03159 100644 --- a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts +++ b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts @@ -216,7 +216,7 @@ suite('Terminal Environment Variable Collection Service', () => { assert.deepEqual(opts, { applyAtProcessCreation: false, applyAtShellIntegration: true }); }); - test('Prepend PATH instead of replacing it, where possible', async () => { + 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( @@ -248,6 +248,38 @@ suite('Terminal Environment Variable Collection Service', () => { 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( From 99a87992cfe9ef39b4c7ac8f6ee38439877c1a7a Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 30 Aug 2023 23:54:12 +0000 Subject: [PATCH 3/4] sa --- .vscode/launch.json | 2 +- .../activation/terminalEnvVarCollectionService.ts | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index ba8900b0a5d0..12c7c3ce5dc0 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -160,7 +160,7 @@ "--ui=tdd", "--recursive", "--colors", - "--grep", "Terminal Environment Variable Collection", + //"--grep", "Terminal Environment Variable Collection Pr", "--timeout=300000" ], "outFiles": ["${workspaceFolder}/out/**/*.js", "!${workspaceFolder}/**/node_modules**/*"], diff --git a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts index 4f3ea9ea6cbf..81edb852dda4 100644 --- a/src/client/interpreter/activation/terminalEnvVarCollectionService.ts +++ b/src/client/interpreter/activation/terminalEnvVarCollectionService.ts @@ -191,8 +191,14 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ applyAtShellIntegration: true, applyAtProcessCreation: true, }); - return; + } 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, { From 38649ed6614babef8a03f4b53f7a323e627462dc Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 30 Aug 2023 23:54:41 +0000 Subject: [PATCH 4/4] df --- .vscode/launch.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 12c7c3ce5dc0..82981a93305d 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -160,7 +160,7 @@ "--ui=tdd", "--recursive", "--colors", - //"--grep", "Terminal Environment Variable Collection Pr", + //"--grep", "", "--timeout=300000" ], "outFiles": ["${workspaceFolder}/out/**/*.js", "!${workspaceFolder}/**/node_modules**/*"],