Shell cmd should load executables from venv folder

.eslintrc.js

@@ -520,7 +520,6 @@ module.exports = {
         'src/client/common/dotnet/services/linuxCompatibilityService.ts',
         'src/client/common/dotnet/services/windowsCompatibilityService.ts',
         'src/client/common/variables/serviceRegistry.ts',
-        'src/client/common/variables/types.ts',
         'src/client/common/variables/sysTypes.ts',
         'src/client/common/variables/systemVariables.ts',
         'src/client/common/nuget/azureBlobStoreNugetRepository.ts',

news/2 Fixes/9089.md

@@ -0,0 +1,2 @@
+When running `shell commands`, ensure the kernel first looks for executables in Python Environment associated with the Kernel.
+E.g. commands such as `!pip` and `!python` will point to the `pip` and `python` executable associated with the kernel.

src/client/common/variables/environment.ts

@@ -46,19 +46,28 @@ export class EnvironmentVariablesService implements IEnvironmentVariablesService
     }
 
     public appendPythonPath(vars: EnvironmentVariables, ...pythonPaths: string[]) {
-        return this.appendPaths(vars, 'PYTHONPATH', ...pythonPaths);
+        return this.appendPaths(vars, 'PYTHONPATH', true, ...pythonPaths);
     }
 
     public appendPath(vars: EnvironmentVariables, ...paths: string[]) {
-        return this.appendPaths(vars, 'PATH', ...paths);
+        return this.appendPaths(vars, 'PATH', true, ...paths);
     }
 
-    private appendPaths(vars: EnvironmentVariables, variableName: 'PATH' | 'PYTHONPATH', ...pathsToAppend: string[]) {
-        const valueToAppend = pathsToAppend
+    public prependPath(vars: EnvironmentVariables, ...paths: string[]) {
+        return this.appendPaths(vars, 'PATH', false, ...paths);
+    }
+
+    private appendPaths(
+        vars: EnvironmentVariables,
+        variableName: 'PATH' | 'PYTHONPATH',
+        append: boolean,
+        ...pathsToAppend: string[]
+    ) {
+        const valueToAppendOrPrepend = pathsToAppend
             .filter((item) => typeof item === 'string' && item.trim().length > 0)
             .map((item) => item.trim())
             .join(path.delimiter);
-        if (valueToAppend.length === 0) {
+        if (valueToAppendOrPrepend.length === 0) {
             return vars;
         }
 
@@ -69,9 +78,13 @@ export class EnvironmentVariablesService implements IEnvironmentVariablesService
         const variable = vars && matchingKey ? vars[matchingKey] : undefined;
         const setKey = matchingKey || variableName;
         if (variable && typeof variable === 'string' && variable.length > 0) {
-            vars[setKey] = variable + path.delimiter + valueToAppend;
+            if (append) {
+                vars[setKey] = variable + path.delimiter + valueToAppendOrPrepend;
+            } else {
+                vars[setKey] = valueToAppendOrPrepend + path.delimiter + variable;
+            }
         } else {
-            vars[setKey] = valueToAppend;
+            vars[setKey] = valueToAppendOrPrepend;
         }
         return vars;
     }

src/client/common/variables/types.ts

@@ -19,6 +19,7 @@ export interface IEnvironmentVariablesService {
     mergeVariables(source: EnvironmentVariables, target: EnvironmentVariables): void;
     appendPythonPath(vars: EnvironmentVariables, ...pythonPaths: string[]): void;
     appendPath(vars: EnvironmentVariables, ...paths: string[]): void;
+    prependPath(vars: EnvironmentVariables, ...paths: string[]): void;
 }
 
 /**

src/client/datascience/jupyter/kernels/jupyterKernelService.ts

@@ -15,6 +15,7 @@ import { IFileSystem } from '../../../common/platform/types';
 
 import { ReadWrite, Resource } from '../../../common/types';
 import { noop } from '../../../common/utils/misc';
+import { IEnvironmentVariablesService } from '../../../common/variables/types';
 import { IEnvironmentActivationService } from '../../../interpreter/activation/types';
 import { ignoreLogging, logValue } from '../../../logging/trace';
 import { PythonEnvironment } from '../../../pythonEnvironments/info';
@@ -38,7 +39,8 @@ export class JupyterKernelService {
         @inject(IKernelDependencyService) private readonly kernelDependencyService: IKernelDependencyService,
         @inject(IFileSystem) private readonly fs: IFileSystem,
         @inject(IEnvironmentActivationService) private readonly activationHelper: IEnvironmentActivationService,
-        @inject(ILocalKernelFinder) private readonly kernelFinder: ILocalKernelFinder
+        @inject(ILocalKernelFinder) private readonly kernelFinder: ILocalKernelFinder,
+        @inject(IEnvironmentVariablesService) private readonly envVarsService: IEnvironmentVariablesService
     ) {}
 
     /**
@@ -256,6 +258,12 @@ export class JupyterKernelService {
                     .catch(noop)
                     // eslint-disable-next-line @typescript-eslint/no-explicit-any
                     .then((env) => (env || {}) as any);
+                // Ensure the python env folder is always at the top of the PATH, this way all executables from that env are used.
+                // This way shell commands such as `!pip`, `!python` end up pointing to the right executables.
+                // Also applies to `!java` where java could be an executable in the conda bin directory.
+                if (interpreter && specModel.env) {
+                    this.envVarsService.prependPath(specModel.env as {}, path.dirname(interpreter.path));
+                }
 
                 // Ensure global site_packages are not in the path.
                 // The global site_packages will be added to the path later.

src/client/datascience/kernel-launcher/kernelEnvVarsService.ts

@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 
 import { inject, injectable } from 'inversify';
+import * as path from 'path';
 import { traceError, traceInfo } from '../../common/logger';
 import { Resource } from '../../common/types';
 import { noop } from '../../common/utils/misc';
@@ -98,6 +99,13 @@ export class KernelEnvironmentVariablesService {
         if (kernelEnv.PYTHONPATH) {
             this.envVarsService.appendPythonPath(mergedVars, kernelEnv.PYTHONPATH);
         }
+        // Ensure the python env folder is always at the top of the PATH, this way all executables from that env are used.
+        // This way shell commands such as `!pip`, `!python` end up pointing to the right executables.
+        // Also applies to `!java` where java could be an executable in the conda bin directory.
+        if (interpreter) {
+            this.envVarsService.prependPath(mergedVars, path.dirname(interpreter.path));
+        }
+
         // Ensure global site_packages are not in the path.
         // The global site_packages will be added to the path later.
         // For more details see here https://github.com/microsoft/vscode-jupyter/issues/8553#issuecomment-997144591

src/test/common/variables/kernelEnvVarsService.unit.test.ts

@@ -73,7 +73,7 @@ suite('Kernel Environment Variables Service', () => {
             const processPath = Object.keys(process.env).find((k) => k.toLowerCase() == 'path');
             expect(processPath).to.not.be.undefined;
             expect(vars).to.not.be.undefined;
-            expect(vars![processPath!]).to.be.equal('foobar');
+            expect(vars![processPath!]).to.be.equal(`${path.dirname(interpreter.path)}${path.delimiter}foobar`);
         });
 
         test('Paths are merged', async () => {
@@ -90,7 +90,9 @@ suite('Kernel Environment Variables Service', () => {
             const processPath = Object.keys(process.env).find((k) => k.toLowerCase() == 'path');
             expect(processPath).to.not.be.undefined;
             expect(vars).to.not.be.undefined;
-            expect(vars![processPath!]).to.be.equal(`foobar${path.delimiter}foobaz`);
+            expect(vars![processPath!]).to.be.equal(
+                `${path.dirname(interpreter.path)}${path.delimiter}foobar${path.delimiter}foobaz`
+            );
         });
 
         test('KernelSpec interpreterPath used if interpreter is undefined', async () => {
@@ -113,7 +115,9 @@ suite('Kernel Environment Variables Service', () => {
             const processPath = Object.keys(process.env).find((k) => k.toLowerCase() == 'path');
             expect(processPath).to.not.be.undefined;
             expect(vars).to.not.be.undefined;
-            expect(vars![processPath!]).to.be.equal(`foobar${path.delimiter}foobaz`);
+            expect(vars![processPath!]).to.be.equal(
+                `${path.dirname(interpreter.path)}${path.delimiter}foobar${path.delimiter}foobaz`
+            );
         });
     });
 });

src/test/datascience/jupyter/kernels/jupyterKernelService.unit.test.ts

@@ -20,6 +20,7 @@ import * as path from 'path';
 import { arePathsSame } from '../../../common';
 import { DisplayOptions } from '../../../../client/datascience/displayOptions';
 import { CancellationTokenSource } from 'vscode';
+import { IEnvironmentVariablesService } from '../../../../client/common/variables/types';
 
 // eslint-disable-next-line
 suite('DataScience - JupyterKernelService', () => {
@@ -297,7 +298,8 @@ suite('DataScience - JupyterKernelService', () => {
             instance(kernelDependencyService),
             instance(fs),
             instance(appEnv),
-            instance(kernelFinder)
+            instance(kernelFinder),
+            instance(mock<IEnvironmentVariablesService>())
         );
     });
     test('Dependencies checked on all kernels with interpreters', async () => {

src/test/datascience/notebook/executionService.vscode.test.ts

@@ -39,7 +39,8 @@ import {
     workAroundVSCodeNotebookStartPages,
     waitForTextOutput,
     defaultNotebookTestTimeout,
-    waitForCellExecutionState
+    waitForCellExecutionState,
+    getCellOutputs
 } from './helper';
 import { ProductNames } from '../../../client/common/installer/productNames';
 import { openNotebook } from '../helpers';
@@ -51,6 +52,7 @@ import {
 } from '../../../client/datascience/notebook/helpers/helpers';
 import { getDisplayPath } from '../../../client/common/platform/fs-paths';
 import { IPYTHON_VERSION_CODE, IS_REMOTE_NATIVE_TEST } from '../../constants';
+import { areInterpreterPathsSame } from '../../../client/pythonEnvironments/info/interpreter';
 
 // eslint-disable-next-line @typescript-eslint/no-var-requires, @typescript-eslint/no-require-imports
 const expectedPromptMessageSuffix = `requires ${ProductNames.get(Product.ipykernel)!} to be installed.`;
@@ -376,6 +378,34 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () {
 
         await waitForExecutionCompletedSuccessfully(cell);
     });
+    test('Shell commands should give preference to executables in Python Environment', async function () {
+        if (IS_REMOTE_NATIVE_TEST) {
+            return this.skip();
+        }
+        await insertCodeCell('import sys', { index: 0 });
+        await insertCodeCell('import os', { index: 1 });
+        await insertCodeCell('print(os.getenv("PATH"))', { index: 2 });
+        await insertCodeCell('print(sys.executable)', { index: 3 });
+        const [, , cell3, cell4] = vscodeNotebook.activeNotebookEditor?.document.getCells()!;
+
+        // Basically anything such as `!which python` and the like should point to the right executable.
+        // For that to work, the first directory in the PATH must be the Python environment.
+
+        await Promise.all([
+            runAllCellsInActiveNotebook(),
+            waitForExecutionCompletedSuccessfully(cell4),
+            waitForCondition(async () => getCellOutputs(cell4).length > 0, defaultNotebookTestTimeout, 'No output')
+        ]);
+
+        const pathValue = getCellOutputs(cell3).split(path.delimiter);
+        const sysExecutable = getCellOutputs(cell4).trim().toLowerCase();
+
+        // First path in PATH must be the directory where executable is located.
+        assert.ok(
+            areInterpreterPathsSame(path.dirname(sysExecutable), pathValue[0].toLowerCase()),
+            `First entry in PATH (${pathValue[0]}) does not point to executable (${sysExecutable})`
+        );
+    });
     test('Testing streamed output', async () => {
         // Assume you are executing a cell that prints numbers 1-100.
         // When printing number 50, you click clear.

src/test/datascience/notebook/intellisense/gotodef.vscode.test.ts

@@ -89,7 +89,14 @@ suite('DataScience - VSCode Intellisense Notebook and Interactive Goto Definitio
         );
 
         // Verify we are in cell1
-        assert.equal(vscode.window.activeTextEditor?.document, cell1.document, 'Text editor did not switch');
+        await waitForCondition(
+            async () => {
+                assert.equal(vscode.window.activeTextEditor?.document, cell1.document, 'Text editor did not switch');
+                return true;
+            },
+            5_000,
+            'Text editor did not switch'
+        );
     });
 
     test('Import pandas and goto it', async () => {