Ensure resolveEnvironment API resolves the latest details for conda envs without python (#20862)

Kartik Raj · web-flow · commit d16568eefa51 · 2023-03-16T16:32:21.000-07:00
Closes #20765 Change `resolveEnvironment` API to validate cache for conda envs without python before using it, it also making sure we fire a update event after resolving it and adding it to cache.
diff --git a/src/client/interpreter/interpreterService.ts b/src/client/interpreter/interpreterService.ts
@@ -9,6 +9,7 @@ import {
     ProgressLocation,
     ProgressOptions,
     Uri,
+    WorkspaceFolder,
 } from 'vscode';
 import '../common/extensions';
 import { IApplicationShell, IDocumentManager, IWorkspaceService } from '../common/application/types';
@@ -96,7 +97,13 @@ export class InterpreterService implements Disposable, IInterpreterService {
     public async refresh(resource?: Uri): Promise<void> {
         const interpreterDisplay = this.serviceContainer.get<IInterpreterDisplay>(IInterpreterDisplay);
         await interpreterDisplay.refresh(resource);
-        this.ensureEnvironmentContainsPython(this.configService.getSettings(resource).pythonPath).ignoreErrors();
+        const workspaceFolder = this.serviceContainer
+            .get<IWorkspaceService>(IWorkspaceService)
+            .getWorkspaceFolder(resource);
+        this.ensureEnvironmentContainsPython(
+            this.configService.getSettings(resource).pythonPath,
+            workspaceFolder,
+        ).ignoreErrors();
     }
 
     public initialize(): void {
@@ -227,18 +234,21 @@ export class InterpreterService implements Disposable, IInterpreterService {
         if (this._pythonPathSetting === '' || this._pythonPathSetting !== pySettings.pythonPath) {
             this._pythonPathSetting = pySettings.pythonPath;
             this.didChangeInterpreterEmitter.fire(resource);
+            const workspaceFolder = this.serviceContainer
+                .get<IWorkspaceService>(IWorkspaceService)
+                .getWorkspaceFolder(resource);
             reportActiveInterpreterChanged({
                 path: pySettings.pythonPath,
-                resource: this.serviceContainer.get<IWorkspaceService>(IWorkspaceService).getWorkspaceFolder(resource),
+                resource: workspaceFolder,
             });
             const interpreterDisplay = this.serviceContainer.get<IInterpreterDisplay>(IInterpreterDisplay);
             interpreterDisplay.refresh().catch((ex) => traceError('Python Extension: display.refresh', ex));
-            await this.ensureEnvironmentContainsPython(this._pythonPathSetting);
+            await this.ensureEnvironmentContainsPython(this._pythonPathSetting, workspaceFolder);
         }
     }
 
     @cache(-1, true)
-    private async ensureEnvironmentContainsPython(pythonPath: string) {
+    private async ensureEnvironmentContainsPython(pythonPath: string, workspaceFolder: WorkspaceFolder | undefined) {
         const installer = this.serviceContainer.get<IInstaller>(IInstaller);
         if (!(await installer.isInstalled(Product.python))) {
             // If Python is not installed into the environment, install it.
@@ -251,7 +261,18 @@ export class InterpreterService implements Disposable, IInterpreterService {
             traceLog('Conda envs without Python are known to not work well; fixing conda environment...');
             const promise = installer.install(Product.python, await this.getInterpreterDetails(pythonPath));
             shell.withProgress(progressOptions, () => promise);
-            promise.then(() => this.triggerRefresh().ignoreErrors());
+            promise
+                .then(async () => {
+                    // Fetch interpreter details so the cache is updated to include the newly installed Python.
+                    await this.getInterpreterDetails(pythonPath);
+                    // Fire an event as the executable for the environment has changed.
+                    this.didChangeInterpreterEmitter.fire(workspaceFolder?.uri);
+                    reportActiveInterpreterChanged({
+                        path: pythonPath,
+                        resource: workspaceFolder,
+                    });
+                })
+                .ignoreErrors();
         }
     }
 }
diff --git a/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts b/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts
@@ -5,13 +5,14 @@ import { Event } from 'vscode';
 import { isTestExecution } from '../../../../common/constants';
 import { traceInfo, traceVerbose } from '../../../../logging';
 import { arePathsSame, getFileInfo, pathExists } from '../../../common/externalDependencies';
-import { PythonEnvInfo } from '../../info';
+import { PythonEnvInfo, PythonEnvKind } from '../../info';
 import { areEnvsDeepEqual, areSameEnv, getEnvPath } from '../../info/env';
 import {
     BasicPythonEnvCollectionChangedEvent,
     PythonEnvCollectionChangedEvent,
     PythonEnvsWatcher,
 } from '../../watcher';
+import { getCondaInterpreterPath } from '../../../common/environmentManagers/conda';
 
 export interface IEnvsCollectionCache {
     /**
@@ -146,15 +147,18 @@ export class PythonEnvInfoCache extends PythonEnvsWatcher<PythonEnvCollectionCha
 
     public addEnv(env: PythonEnvInfo, hasLatestInfo?: boolean): void {
         const found = this.envs.find((e) => areSameEnv(e, env));
+        if (!found) {
+            this.envs.push(env);
+            this.fire({ new: env });
+        } else if (hasLatestInfo && !this.validatedEnvs.has(env.id!)) {
+            // Update cache if we have latest info and the env is not already validated.
+            this.updateEnv(found, env, true);
+        }
         if (hasLatestInfo) {
             traceVerbose(`Flushing env to cache ${env.id}`);
             this.validatedEnvs.add(env.id!);
             this.flush(env).ignoreErrors(); // If we have latest info, flush it so it can be saved.
         }
-        if (!found) {
-            this.envs.push(env);
-            this.fire({ new: env });
-        }
     }
 
     public updateEnv(oldValue: PythonEnvInfo, newValue: PythonEnvInfo | undefined, forceUpdate = false): void {
@@ -177,6 +181,20 @@ export class PythonEnvInfoCache extends PythonEnvsWatcher<PythonEnvCollectionCha
     public async getLatestInfo(path: string): Promise<PythonEnvInfo | undefined> {
         // `path` can either be path to environment or executable path
         const env = this.envs.find((e) => arePathsSame(e.location, path)) ?? this.envs.find((e) => areSameEnv(e, path));
+        if (
+            env?.kind === PythonEnvKind.Conda &&
+            getEnvPath(env.executable.filename, env.location).pathType === 'envFolderPath'
+        ) {
+            if (await pathExists(getCondaInterpreterPath(env.location))) {
+                // This is a conda env without python in cache which actually now has a valid python, so return
+                // `undefined` and delete value from cache as cached value is not the latest anymore.
+                this.validatedEnvs.delete(env.id!);
+                return undefined;
+            }
+            // Do not attempt to validate these envs as they lack an executable, and consider them as validated by default.
+            this.validatedEnvs.add(env.id!);
+            return env;
+        }
         if (env) {
             if (this.validatedEnvs.has(env.id!)) {
                 traceVerbose(`Found cached env for ${path}`);
diff --git a/src/test/pythonEnvironments/base/common.ts b/src/test/pythonEnvironments/base/common.ts
@@ -104,14 +104,15 @@ export class SimpleLocator<I = PythonEnvInfo> extends Locator<I> {
     constructor(
         private envs: I[],
         public callbacks: {
-            resolve?: null | ((env: PythonEnvInfo) => Promise<PythonEnvInfo | undefined>);
+            resolve?: null | ((env: PythonEnvInfo | string) => Promise<PythonEnvInfo | undefined>);
             before?(): Promise<void>;
             after?(): Promise<void>;
             onUpdated?: Event<PythonEnvUpdatedEvent<I> | ProgressNotificationEvent>;
             beforeEach?(e: I): Promise<void>;
             afterEach?(e: I): Promise<void>;
             onQuery?(query: PythonLocatorQuery | undefined, envs: I[]): Promise<I[]>;
         } = {},
+        private options?: { resolveAsString?: boolean },
     ) {
         super();
     }
@@ -172,7 +173,7 @@ export class SimpleLocator<I = PythonEnvInfo> extends Locator<I> {
         if (this.callbacks?.resolve === null) {
             return undefined;
         }
-        return this.callbacks.resolve(envInfo);
+        return this.callbacks.resolve(this.options?.resolveAsString ? env : envInfo);
     }
 }
 
diff --git a/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts b/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts
@@ -1,3 +1,4 @@
+/* eslint-disable @typescript-eslint/no-explicit-any */
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
@@ -22,13 +23,31 @@ import * as externalDependencies from '../../../../../client/pythonEnvironments/
 import { noop } from '../../../../core';
 import { TEST_LAYOUT_ROOT } from '../../../common/commonTestConstants';
 import { SimpleLocator } from '../../common';
-import { assertEnvEqual, assertEnvsEqual } from '../envTestUtils';
+import { assertEnvEqual, assertEnvsEqual, createFile, deleteFile } from '../envTestUtils';
+import { OSType, getOSType } from '../../../../common';
 
 suite('Python envs locator - Environments Collection', async () => {
     let collectionService: EnvsCollectionService;
     let storage: PythonEnvInfo[];
 
     const updatedName = 'updatedName';
+    const pathToCondaPython = getOSType() === OSType.Windows ? 'python.exe' : path.join('bin', 'python');
+    const condaEnvWithoutPython = createEnv(
+        'python',
+        undefined,
+        undefined,
+        path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython'),
+        PythonEnvKind.Conda,
+        path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython', pathToCondaPython),
+    );
+    const condaEnvWithPython = createEnv(
+        path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython', pathToCondaPython),
+        undefined,
+        undefined,
+        path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython'),
+        PythonEnvKind.Conda,
+        path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython', pathToCondaPython),
+    );
 
     function applyChangeEventToEnvList(envs: PythonEnvInfo[], event: PythonEnvCollectionChangedEvent) {
         const env = event.old ?? event.new;
@@ -49,8 +68,17 @@ suite('Python envs locator - Environments Collection', async () => {
         return envs;
     }
 
-    function createEnv(executable: string, searchLocation?: Uri, name?: string, location?: string) {
-        return buildEnvInfo({ executable, searchLocation, name, location });
+    function createEnv(
+        executable: string,
+        searchLocation?: Uri,
+        name?: string,
+        location?: string,
+        kind?: PythonEnvKind,
+        id?: string,
+    ) {
+        const env = buildEnvInfo({ executable, searchLocation, name, location, kind });
+        env.id = id ?? env.id;
+        return env;
     }
 
     function getLocatorEnvs() {
@@ -77,12 +105,7 @@ suite('Python envs locator - Environments Collection', async () => {
             path.join(TEST_LAYOUT_ROOT, 'pipenv', 'project1', '.venv', 'Scripts', 'python.exe'),
             Uri.file(TEST_LAYOUT_ROOT),
         );
-        const envCached3 = createEnv(
-            'python',
-            undefined,
-            undefined,
-            path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython'),
-        );
+        const envCached3 = condaEnvWithoutPython;
         return [cachedEnvForWorkspace, envCached1, envCached2, envCached3];
     }
 
@@ -123,7 +146,8 @@ suite('Python envs locator - Environments Collection', async () => {
         collectionService = new EnvsCollectionService(cache, parentLocator);
     });
 
-    teardown(() => {
+    teardown(async () => {
+        await deleteFile(condaEnvWithPython.executable.filename); //  Restore to the original state
         sinon.restore();
     });
 
@@ -404,7 +428,7 @@ suite('Python envs locator - Environments Collection', async () => {
         env.executable.mtime = 100;
         sinon.stub(externalDependencies, 'getFileInfo').resolves({ ctime: 100, mtime: 100 });
         const parentLocator = new SimpleLocator([], {
-            resolve: async (e: PythonEnvInfo) => {
+            resolve: async (e: any) => {
                 if (env.executable.filename === e.executable.filename) {
                     return resolvedViaLocator;
                 }
@@ -434,7 +458,7 @@ suite('Python envs locator - Environments Collection', async () => {
                 waitDeferred.resolve();
                 await deferred.promise;
             },
-            resolve: async (e: PythonEnvInfo) => {
+            resolve: async (e: any) => {
                 if (env.executable.filename === e.executable.filename) {
                     return resolvedViaLocator;
                 }
@@ -464,7 +488,7 @@ suite('Python envs locator - Environments Collection', async () => {
         env.executable.mtime = 90;
         sinon.stub(externalDependencies, 'getFileInfo').resolves({ ctime: 100, mtime: 100 });
         const parentLocator = new SimpleLocator([], {
-            resolve: async (e: PythonEnvInfo) => {
+            resolve: async (e: any) => {
                 if (env.executable.filename === e.executable.filename) {
                     return resolvedViaLocator;
                 }
@@ -483,7 +507,7 @@ suite('Python envs locator - Environments Collection', async () => {
     test('resolveEnv() adds env to cache after resolving using downstream locator', async () => {
         const resolvedViaLocator = buildEnvInfo({ executable: 'Resolved via locator' });
         const parentLocator = new SimpleLocator([], {
-            resolve: async (e: PythonEnvInfo) => {
+            resolve: async (e: any) => {
                 if (resolvedViaLocator.executable.filename === e.executable.filename) {
                     return resolvedViaLocator;
                 }
@@ -500,6 +524,49 @@ suite('Python envs locator - Environments Collection', async () => {
         assertEnvsEqual(envs, [resolved]);
     });
 
+    test('resolveEnv() uses underlying locator once conda envs without python get a python installed', async () => {
+        const cachedEnvs = [condaEnvWithoutPython];
+        const parentLocator = new SimpleLocator(
+            [],
+            {
+                resolve: async (e) => {
+                    if (condaEnvWithoutPython.location === (e as string)) {
+                        return condaEnvWithPython;
+                    }
+                    return undefined;
+                },
+            },
+            { resolveAsString: true },
+        );
+        const cache = await createCollectionCache({
+            get: () => cachedEnvs,
+            store: async () => noop(),
+        });
+        collectionService = new EnvsCollectionService(cache, parentLocator);
+        let resolved = await collectionService.resolveEnv(condaEnvWithoutPython.location);
+        assertEnvEqual(resolved, condaEnvWithoutPython); // Ensure cache is used to resolve such envs.
+
+        condaEnvWithPython.executable.ctime = 100;
+        condaEnvWithPython.executable.mtime = 100;
+        sinon.stub(externalDependencies, 'getFileInfo').resolves({ ctime: 100, mtime: 100 });
+
+        const events: PythonEnvCollectionChangedEvent[] = [];
+        collectionService.onChanged((e) => {
+            events.push(e);
+        });
+
+        await createFile(condaEnvWithPython.executable.filename); // Install Python into the env
+
+        resolved = await collectionService.resolveEnv(condaEnvWithoutPython.location);
+        assertEnvEqual(resolved, condaEnvWithPython); // Ensure it resolves latest info.
+
+        // Verify conda env without python in cache is replaced with updated info.
+        const envs = collectionService.getEnvs();
+        assertEnvsEqual(envs, [condaEnvWithPython]);
+
+        expect(events.length).to.equal(1, 'Update event should be fired');
+    });
+
     test('Ensure events from downstream locators do not trigger new refreshes if a refresh is already scheduled', async () => {
         const refreshDeferred = createDeferred();
         let refreshCount = 0;
diff --git a/src/test/pythonEnvironments/base/locators/envTestUtils.ts b/src/test/pythonEnvironments/base/locators/envTestUtils.ts
@@ -1,9 +1,10 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
+import * as fsapi from 'fs-extra';
 import * as assert from 'assert';
 import { exec } from 'child_process';
-import { zip } from 'lodash';
+import { cloneDeep, zip } from 'lodash';
 import { promisify } from 'util';
 import { PythonEnvInfo, PythonVersion, UNKNOWN_PYTHON_VERSION } from '../../../../client/pythonEnvironments/base/info';
 import { getEmptyVersion } from '../../../../client/pythonEnvironments/base/info/pythonVersion';
@@ -40,17 +41,30 @@ export function assertVersionsEqual(actual: PythonVersion | undefined, expected:
     assert.deepStrictEqual(actual, expected);
 }
 
+export async function createFile(filename: string, text = ''): Promise<string> {
+    await fsapi.writeFile(filename, text);
+    return filename;
+}
+
+export async function deleteFile(filename: string): Promise<void> {
+    await fsapi.remove(filename);
+}
+
 export function assertEnvEqual(actual: PythonEnvInfo | undefined, expected: PythonEnvInfo | undefined): void {
     assert.notStrictEqual(actual, undefined);
     assert.notStrictEqual(expected, undefined);
 
     if (actual) {
+        // Make sure to clone so we do not alter the original object
+        actual = cloneDeep(actual);
+        expected = cloneDeep(expected);
         // No need to match these, so reset them
         actual.executable.ctime = -1;
         actual.executable.mtime = -1;
-
         actual.version = normalizeVersion(actual.version);
         if (expected) {
+            expected.executable.ctime = -1;
+            expected.executable.mtime = -1;
             expected.version = normalizeVersion(expected.version);
             delete expected.id;
         }
diff --git a/src/test/pythonEnvironments/common/envlayouts/envsWithoutPython/condaLackingPython/bin/dummy b/src/test/pythonEnvironments/common/envlayouts/envsWithoutPython/condaLackingPython/bin/dummy

Original file line number	Diff line number	Diff line change
`@@ -104,14 +104,15 @@ export class SimpleLocator<I = PythonEnvInfo> extends Locator<I> {`
`104`	`104`	`constructor(`
`105`	`105`	`private envs: I[],`
`106`	`106`	`public callbacks: {`
`107`		`- resolve?: null \| ((env: PythonEnvInfo) => Promise<PythonEnvInfo \| undefined>);`
	`107`	`+ resolve?: null \| ((env: PythonEnvInfo \| string) => Promise<PythonEnvInfo \| undefined>);`
`108`	`108`	`before?(): Promise<void>;`
`109`	`109`	`after?(): Promise<void>;`
`110`	`110`	`onUpdated?: Event<PythonEnvUpdatedEvent<I> \| ProgressNotificationEvent>;`
`111`	`111`	`beforeEach?(e: I): Promise<void>;`
`112`	`112`	`afterEach?(e: I): Promise<void>;`
`113`	`113`	`onQuery?(query: PythonLocatorQuery \| undefined, envs: I[]): Promise<I[]>;`
`114`	`114`	`} = {},`
	`115`	`+ private options?: { resolveAsString?: boolean },`
`115`	`116`	`) {`
`116`	`117`	`super();`
`117`	`118`	`}`
`@@ -172,7 +173,7 @@ export class SimpleLocator<I = PythonEnvInfo> extends Locator<I> {`
`172`	`173`	`if (this.callbacks?.resolve === null) {`
`173`	`174`	`return undefined;`
`174`	`175`	`}`
`175`		`- return this.callbacks.resolve(envInfo);`
	`176`	`+ return this.callbacks.resolve(this.options?.resolveAsString ? env : envInfo);`
`176`	`177`	`}`
`177`	`178`	`}`
`178`	`179`