Skip to content

Commit 0f1c112

Browse files
Kartik Rajanthonykim1
Kartik Raj
authored andcommitted
Always prepend to PATH instead of replacing it (microsoft#21906)
For microsoft#20822 microsoft#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=<activated_full_path><activated_full_path><original_path> ``` where `<activated_full_path>` refers to value of `PATH` environment variable post activation. eg. ![image](https://github.com/microsoft/vscode-python/assets/13199757/7e771f62-eb53-49be-b261-d259096008f3)
1 parent c1e9b1e commit 0f1c112

File tree

2 files changed

+85
-1
lines changed

2 files changed

+85
-1
lines changed

src/client/interpreter/activation/terminalEnvVarCollectionService.ts

+21-1
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
167167
if (shouldSkip(key)) {
168168
return;
169169
}
170-
const value = env[key];
170+
let value = env[key];
171171
const prevValue = processEnv[key];
172172
if (prevValue !== value) {
173173
if (value !== undefined) {
@@ -180,6 +180,26 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
180180
});
181181
return;
182182
}
183+
if (key === 'PATH') {
184+
if (processEnv.PATH && env.PATH?.endsWith(processEnv.PATH)) {
185+
// Prefer prepending to PATH instead of replacing it, as we do not want to replace any
186+
// changes to PATH users might have made it in their init scripts (~/.bashrc etc.)
187+
const prependedPart = env.PATH.slice(0, -processEnv.PATH.length);
188+
value = prependedPart;
189+
traceVerbose(`Prepending environment variable ${key} in collection with ${value}`);
190+
envVarCollection.prepend(key, value, {
191+
applyAtShellIntegration: true,
192+
applyAtProcessCreation: true,
193+
});
194+
} else {
195+
traceVerbose(`Prepending environment variable ${key} in collection to ${value}`);
196+
envVarCollection.prepend(key, value, {
197+
applyAtShellIntegration: true,
198+
applyAtProcessCreation: true,
199+
});
200+
}
201+
return;
202+
}
183203
traceVerbose(`Setting environment variable ${key} in collection to ${value}`);
184204
envVarCollection.replace(key, value, {
185205
applyAtShellIntegration: true,

src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts

+64
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,70 @@ suite('Terminal Environment Variable Collection Service', () => {
216216
assert.deepEqual(opts, { applyAtProcessCreation: false, applyAtShellIntegration: true });
217217
});
218218

219+
test('Prepend only "prepend portion of PATH" where applicable', async () => {
220+
const processEnv = { PATH: 'hello/1/2/3' };
221+
reset(environmentActivationService);
222+
when(environmentActivationService.getProcessEnvironmentVariables(anything(), anything())).thenResolve(
223+
processEnv,
224+
);
225+
const prependedPart = 'path/to/activate/dir:';
226+
const envVars: NodeJS.ProcessEnv = { PATH: `${prependedPart}${processEnv.PATH}` };
227+
when(
228+
environmentActivationService.getActivatedEnvironmentVariables(
229+
anything(),
230+
undefined,
231+
undefined,
232+
customShell,
233+
),
234+
).thenResolve(envVars);
235+
236+
when(collection.replace(anything(), anything(), anything())).thenResolve();
237+
when(collection.delete(anything())).thenResolve();
238+
let opts: EnvironmentVariableMutatorOptions | undefined;
239+
when(collection.prepend('PATH', anything(), anything())).thenCall((_, _v, o) => {
240+
opts = o;
241+
});
242+
243+
await terminalEnvVarCollectionService._applyCollection(undefined, customShell);
244+
245+
verify(collection.clear()).once();
246+
verify(collection.prepend('PATH', prependedPart, anything())).once();
247+
verify(collection.replace('PATH', anything(), anything())).never();
248+
assert.deepEqual(opts, { applyAtProcessCreation: true, applyAtShellIntegration: true });
249+
});
250+
251+
test('Prepend full PATH otherwise', async () => {
252+
const processEnv = { PATH: 'hello/1/2/3' };
253+
reset(environmentActivationService);
254+
when(environmentActivationService.getProcessEnvironmentVariables(anything(), anything())).thenResolve(
255+
processEnv,
256+
);
257+
const finalPath = 'hello/3/2/1';
258+
const envVars: NodeJS.ProcessEnv = { PATH: finalPath };
259+
when(
260+
environmentActivationService.getActivatedEnvironmentVariables(
261+
anything(),
262+
undefined,
263+
undefined,
264+
customShell,
265+
),
266+
).thenResolve(envVars);
267+
268+
when(collection.replace(anything(), anything(), anything())).thenResolve();
269+
when(collection.delete(anything())).thenResolve();
270+
let opts: EnvironmentVariableMutatorOptions | undefined;
271+
when(collection.prepend('PATH', anything(), anything())).thenCall((_, _v, o) => {
272+
opts = o;
273+
});
274+
275+
await terminalEnvVarCollectionService._applyCollection(undefined, customShell);
276+
277+
verify(collection.clear()).once();
278+
verify(collection.prepend('PATH', finalPath, anything())).once();
279+
verify(collection.replace('PATH', anything(), anything())).never();
280+
assert.deepEqual(opts, { applyAtProcessCreation: true, applyAtShellIntegration: true });
281+
});
282+
219283
test('Verify envs are not applied if env activation is disabled', async () => {
220284
const envVars: NodeJS.ProcessEnv = { CONDA_PREFIX: 'prefix/to/conda', ...process.env };
221285
when(

0 commit comments

Comments
 (0)