Wait for refresh to complete before deciding whether an environment has updated

src/client/common/utils/version.ts

@@ -70,11 +70,6 @@ export const EMPTY_VERSION: RawBasicVersionInfo = {
     major: -1,
     minor: -1,
     micro: -1,
-    unnormalized: {
-        major: undefined,
-        minor: undefined,
-        micro: undefined,
-    },
 };
 Object.freeze(EMPTY_VERSION);
 

src/client/pythonEnvironments/base/info/env.ts

@@ -1,7 +1,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
-import { cloneDeep } from 'lodash';
+import { cloneDeep, isEqual } from 'lodash';
 import * as path from 'path';
 import { Uri } from 'vscode';
 import { getArchitectureDisplayName } from '../../../common/platform/registry';
@@ -77,6 +77,19 @@ export function buildEnvInfo(init?: {
     return env;
 }
 
+export function areEnvsDeepEqual(env1: PythonEnvInfo, env2: PythonEnvInfo): boolean {
+    const env1Clone = cloneDeep(env1);
+    const env2Clone = cloneDeep(env2);
+    // Cannot compare searchLocation as they are Uri objects.
+    delete env1Clone.searchLocation;
+    delete env2Clone.searchLocation;
+    env1Clone.source = env1Clone.source.sort();
+    env2Clone.source = env2Clone.source.sort();
+    const searchLocation1 = env1.searchLocation?.fsPath ?? '';
+    const searchLocation2 = env2.searchLocation?.fsPath ?? '';
+    return isEqual(env1Clone, env2Clone) && arePathsSame(searchLocation1, searchLocation2);
+}
+
 /**
  * Return a deep copy of the given env info.
  *

src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts

@@ -6,7 +6,7 @@ import { isTestExecution } from '../../../../common/constants';
 import { traceInfo } from '../../../../logging';
 import { arePathsSame, getFileInfo, pathExists } from '../../../common/externalDependencies';
 import { PythonEnvInfo } from '../../info';
-import { areSameEnv, getEnvPath } from '../../info/env';
+import { areEnvsDeepEqual, areSameEnv, getEnvPath } from '../../info/env';
 import {
     BasicPythonEnvCollectionChangedEvent,
     PythonEnvCollectionChangedEvent,
@@ -52,15 +52,14 @@ export interface IEnvsCollectionCache {
     /**
      * Removes invalid envs from cache. Note this does not check for outdated info when
      * validating cache.
-     * @param latestListOfEnvs Carries list of latest envs for this workspace session if known.
+     * @param envs Carries list of envs for the latest refresh.
+     * @param isCompleteList Carries whether the list of envs is complete or not.
      */
-    validateCache(latestListOfEnvs?: PythonEnvInfo[]): Promise<void>;
+    validateCache(envs?: PythonEnvInfo[], isCompleteList?: boolean): Promise<void>;
 }
 
-export type PythonEnvLatestInfo = { hasLatestInfo?: boolean } & PythonEnvInfo;
-
 interface IPersistentStorage {
-    load(): Promise<PythonEnvInfo[]>;
+    get(): PythonEnvInfo[];
     store(envs: PythonEnvInfo[]): Promise<void>;
 }
 
@@ -69,13 +68,24 @@ interface IPersistentStorage {
  */
 export class PythonEnvInfoCache extends PythonEnvsWatcher<PythonEnvCollectionChangedEvent>
     implements IEnvsCollectionCache {
-    private envs: PythonEnvLatestInfo[] = [];
+    private envs: PythonEnvInfo[] = [];
+
+    /**
+     * Carries the list of envs which have been validated to have latest info.
+     */
+    private validatedEnvs = new Set<string>();
+
+    /**
+     * Carries the list of envs which have been flushed to persistent storage.
+     * It signifies that the env info is likely up-to-date.
+     */
+    private flushedEnvs = new Set<string>();
 
     constructor(private readonly persistentStorage: IPersistentStorage) {
         super();
     }
 
-    public async validateCache(latestListOfEnvs?: PythonEnvInfo[]): Promise<void> {
+    public async validateCache(envs?: PythonEnvInfo[], isCompleteList?: boolean): Promise<void> {
         /**
          * We do check if an env has updated as we already run discovery in background
          * which means env cache will have up-to-date envs eventually. This also means
@@ -86,7 +96,7 @@ export class PythonEnvInfoCache extends PythonEnvsWatcher<PythonEnvCollectionCha
             this.envs.map(async (cachedEnv) => {
                 const { path } = getEnvPath(cachedEnv.executable.filename, cachedEnv.location);
                 if (await pathExists(path)) {
-                    if (latestListOfEnvs) {
+                    if (envs && isCompleteList) {
                         /**
                          * Only consider a cached env to be valid if it's relevant. That means:
                          * * It is either reported in the latest complete refresh for this session.
@@ -95,7 +105,7 @@ export class PythonEnvInfoCache extends PythonEnvsWatcher<PythonEnvCollectionCha
                         if (cachedEnv.searchLocation) {
                             return true;
                         }
-                        if (latestListOfEnvs.some((env) => cachedEnv.id === env.id)) {
+                        if (envs.some((env) => cachedEnv.id === env.id)) {
                             return true;
                         }
                     } else {
@@ -113,25 +123,39 @@ export class PythonEnvInfoCache extends PythonEnvsWatcher<PythonEnvCollectionCha
             const env = this.envs.splice(index, 1)[0];
             this.fire({ old: env, new: undefined });
         });
+        if (envs) {
+            // See if any env has updated after the last refresh and fire events.
+            envs.forEach((env) => {
+                const cachedEnv = this.envs.find((e) => e.id === env.id);
+                if (cachedEnv && !areEnvsDeepEqual(cachedEnv, env)) {
+                    this.updateEnv(cachedEnv, env, true);
+                }
+            });
+        }
     }
 
     public getAllEnvs(): PythonEnvInfo[] {
         return this.envs;
     }
 
-    public addEnv(env: PythonEnvLatestInfo, hasLatestInfo?: boolean): void {
+    public addEnv(env: PythonEnvInfo, hasLatestInfo?: boolean): void {
         const found = this.envs.find((e) => areSameEnv(e, env));
         if (hasLatestInfo) {
-            env.hasLatestInfo = true;
-            this.flush(false).ignoreErrors();
+            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): void {
+    public updateEnv(oldValue: PythonEnvInfo, newValue: PythonEnvInfo | undefined, forceUpdate = false): void {
+        if (this.flushedEnvs.has(oldValue.id!) && !forceUpdate) {
+            // We have already flushed this env to persistent storage, so it likely has upto date info.
+            // If we have latest info, then we do not need to update the cache.
+            return;
+        }
         const index = this.envs.findIndex((e) => areSameEnv(e, oldValue));
         if (index !== -1) {
             if (newValue === undefined) {
@@ -146,40 +170,42 @@ 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?.hasLatestInfo) {
-            return env;
-        }
-        if (env && (env?.hasLatestInfo || (await validateInfo(env)))) {
-            return env;
+        if (env) {
+            if (this.validatedEnvs.has(env.id!)) {
+                return env;
+            }
+            if (await validateInfo(env)) {
+                this.validatedEnvs.add(env.id!);
+                return env;
+            }
         }
         return undefined;
     }
 
-    public async clearAndReloadFromStorage(): Promise<void> {
-        this.envs = await this.persistentStorage.load();
-        this.envs.forEach((e) => {
-            delete e.hasLatestInfo;
-        });
+    public clearAndReloadFromStorage(): void {
+        this.envs = this.persistentStorage.get();
+        this.markAllEnvsAsFlushed();
     }
 
-    public async flush(allEnvsHaveLatestInfo = true): Promise<void> {
-        if (this.envs.length) {
-            traceInfo('Environments added to cache', JSON.stringify(this.envs));
-            if (allEnvsHaveLatestInfo) {
-                this.envs.forEach((e) => {
-                    e.hasLatestInfo = true;
-                });
-            }
-            await this.persistentStorage.store(this.envs);
+    public async flush(env?: PythonEnvInfo): Promise<void> {
+        if (env) {
+            // Flush only the given env.
+            const envs = this.persistentStorage.get();
+            const index = envs.findIndex((e) => e.id === env.id);
+            envs[index] = env;
+            this.flushedEnvs.add(env.id!);
+            await this.persistentStorage.store(envs);
+            return;
         }
+        traceInfo('Environments added to cache', JSON.stringify(this.envs));
+        this.markAllEnvsAsFlushed();
+        await this.persistentStorage.store(this.envs);
     }
 
-    public clearCache(): Promise<void> {
+    private markAllEnvsAsFlushed(): void {
         this.envs.forEach((e) => {
-            this.fire({ old: e, new: undefined });
+            this.flushedEnvs.add(e.id!);
         });
-        this.envs = [];
-        return Promise.resolve();
     }
 }
 
@@ -198,7 +224,7 @@ async function validateInfo(env: PythonEnvInfo) {
  */
 export async function createCollectionCache(storage: IPersistentStorage): Promise<PythonEnvInfoCache> {
     const cache = new PythonEnvInfoCache(storage);
-    await cache.clearAndReloadFromStorage();
+    cache.clearAndReloadFromStorage();
     await validateCache(cache);
     return cache;
 }

src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts

@@ -177,7 +177,7 @@ export class EnvsCollectionService extends PythonEnvsWatcher<PythonEnvCollection
         }
         await updatesDone.promise;
         // If query for all envs is done, `seen` should contain the list of all envs.
-        await this.cache.validateCache(query === undefined ? seen : undefined);
+        await this.cache.validateCache(seen, query === undefined);
         this.cache.flush().ignoreErrors();
     }
 

src/client/pythonEnvironments/index.ts

@@ -189,7 +189,7 @@ function createWorkspaceLocator(ext: ExtensionState): WorkspaceLocators<BasicEnv
 async function createCollectionCache(ext: ExtensionState): Promise<IEnvsCollectionCache> {
     const storage = getGlobalStorage<PythonEnvInfo[]>(ext.context, 'PYTHON_ENV_INFO_CACHE', []);
     const cache = await createCache({
-        load: async () => storage.get(),
+        get: () => storage.get(),
         store: async (e) => storage.set(e),
     });
     return cache;

src/client/terminals/codeExecution/terminalCodeExecution.ts

@@ -32,9 +32,9 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
 
     public async executeFile(file: Uri) {
         await this.setCwdForFileExecution(file);
-        const x = file.fsPath;
-        const hello = x.fileToCommandArgumentForPythonExt();
-        const { command, args } = await this.getExecuteFileArgs(file, [hello]);
+        const { command, args } = await this.getExecuteFileArgs(file, [
+            file.fsPath.fileToCommandArgumentForPythonExt(),
+        ]);
 
         await this.getTerminalService(file).sendCommand(command, args);
     }

src/test/pythonEnvironments/base/common.ts

@@ -3,7 +3,7 @@
 
 import { expect } from 'chai';
 import * as path from 'path';
-import { Event } from 'vscode';
+import { Event, Uri } from 'vscode';
 import { createDeferred, flattenIterator, iterable, mapToIterator } from '../../../client/common/utils/async';
 import { getArchitecture } from '../../../client/common/utils/platform';
 import { getVersionString } from '../../../client/common/utils/version';
@@ -35,6 +35,7 @@ export function createLocatedEnv(
     kind = PythonEnvKind.Unknown,
     exec: string | PythonExecutableInfo = 'python',
     distro: PythonDistroInfo = { org: '' },
+    searchLocation?: Uri,
 ): PythonEnvInfo {
     const location =
         locationStr === ''
@@ -57,6 +58,7 @@ export function createLocatedEnv(
         executable,
         location,
         version,
+        searchLocation,
     });
     env.arch = getArchitecture();
     env.distro = distro;

src/test/pythonEnvironments/base/info/env.unit.test.ts

@@ -2,15 +2,17 @@
 // Licensed under the MIT License.
 
 import * as assert from 'assert';
+import { Uri } from 'vscode';
 import { Architecture } from '../../../../client/common/utils/platform';
 import { parseVersionInfo } from '../../../../client/common/utils/version';
 import { PythonEnvInfo, PythonDistroInfo, PythonEnvKind } from '../../../../client/pythonEnvironments/base/info';
-import { setEnvDisplayString } from '../../../../client/pythonEnvironments/base/info/env';
+import { areEnvsDeepEqual, setEnvDisplayString } from '../../../../client/pythonEnvironments/base/info/env';
 import { createLocatedEnv } from '../common';
 
-suite('pyenvs info - getEnvDisplayString()', () => {
+suite('Environment helpers', () => {
     const name = 'my-env';
     const location = 'x/y/z/spam/';
+    const searchLocation = 'x/y/z';
     const arch = Architecture.x64;
     const version = '3.8.1';
     const kind = PythonEnvKind.Venv;
@@ -28,40 +30,63 @@ suite('pyenvs info - getEnvDisplayString()', () => {
         distro?: PythonDistroInfo;
         display?: string;
         location?: string;
+        searchLocation?: string;
     }): PythonEnvInfo {
         const env = createLocatedEnv(
             info.location || '',
             info.version || '',
             info.kind || PythonEnvKind.Unknown,
             'python', // exec
             info.distro,
+            info.searchLocation ? Uri.file(info.searchLocation) : undefined,
         );
         env.name = info.name || '';
         env.arch = info.arch || Architecture.Unknown;
         env.display = info.display;
         return env;
     }
-    const tests: [PythonEnvInfo, string, string][] = [
-        [getEnv({}), 'Python', 'Python'],
-        [getEnv({ version, arch, name, kind, distro }), "Python 3.8.1 ('my-env')", "Python 3.8.1 ('my-env': venv)"],
-        // without "suffix" info
-        [getEnv({ version }), 'Python 3.8.1', 'Python 3.8.1'],
-        [getEnv({ arch }), 'Python 64-bit', 'Python 64-bit'],
-        [getEnv({ version, arch }), 'Python 3.8.1 64-bit', 'Python 3.8.1 64-bit'],
-        // with "suffix" info
-        [getEnv({ name }), "Python ('my-env')", "Python ('my-env')"],
-        [getEnv({ kind }), 'Python', 'Python (venv)'],
-        [getEnv({ name, kind }), "Python ('my-env')", "Python ('my-env': venv)"],
-        // env.location is ignored.
-        [getEnv({ location }), 'Python', 'Python'],
-        [getEnv({ name, location }), "Python ('my-env')", "Python ('my-env')"],
-    ];
-    tests.forEach(([env, expectedDisplay, expectedDetailedDisplay]) => {
+    function testGenerator() {
+        const tests: [PythonEnvInfo, string, string][] = [
+            [getEnv({}), 'Python', 'Python'],
+            [getEnv({ version, arch, name, kind, distro }), "Python 3.8.1 ('my-env')", "Python 3.8.1 ('my-env': venv)"],
+            // without "suffix" info
+            [getEnv({ version }), 'Python 3.8.1', 'Python 3.8.1'],
+            [getEnv({ arch }), 'Python 64-bit', 'Python 64-bit'],
+            [getEnv({ version, arch }), 'Python 3.8.1 64-bit', 'Python 3.8.1 64-bit'],
+            // with "suffix" info
+            [getEnv({ name }), "Python ('my-env')", "Python ('my-env')"],
+            [getEnv({ kind }), 'Python', 'Python (venv)'],
+            [getEnv({ name, kind }), "Python ('my-env')", "Python ('my-env': venv)"],
+            // env.location is ignored.
+            [getEnv({ location }), 'Python', 'Python'],
+            [getEnv({ name, location }), "Python ('my-env')", "Python ('my-env')"],
+            [
+                getEnv({ name, location, searchLocation, version, arch }),
+                "Python 3.8.1 64-bit ('my-env')",
+                "Python 3.8.1 64-bit ('my-env')",
+            ],
+        ];
+        return tests;
+    }
+    testGenerator().forEach(([env, expectedDisplay, expectedDetailedDisplay]) => {
         test(`"${expectedDisplay}"`, () => {
             setEnvDisplayString(env);
 
             assert.equal(env.display, expectedDisplay);
             assert.equal(env.detailedDisplayName, expectedDetailedDisplay);
         });
     });
+    testGenerator().forEach(([env1, _d1, display1], index1) => {
+        testGenerator().forEach(([env2, _d2, display2], index2) => {
+            if (index1 === index2) {
+                test(`"${display1}" === "${display2}"`, () => {
+                    assert.strictEqual(areEnvsDeepEqual(env1, env2), true);
+                });
+            } else {
+                test(`"${display1}" !== "${display2}"`, () => {
+                    assert.strictEqual(areEnvsDeepEqual(env1, env2), false);
+                });
+            }
+        });
+    });
 });