From d16568eefa51323f69d7e95bb27149de810bf872 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 16 Mar 2023 16:32:21 -0700 Subject: [PATCH] Ensure `resolveEnvironment` API resolves the latest details for conda envs without python (#20862) Closes https://github.com/microsoft/vscode-python/issues/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. --- src/client/interpreter/interpreterService.ts | 31 +++++- .../locators/composite/envsCollectionCache.ts | 28 +++++- src/test/pythonEnvironments/base/common.ts | 5 +- .../envsCollectionService.unit.test.ts | 95 ++++++++++++++++--- .../base/locators/envTestUtils.ts | 18 +++- .../condaLackingPython/bin/dummy | 0 6 files changed, 149 insertions(+), 28 deletions(-) create mode 100644 src/test/pythonEnvironments/common/envlayouts/envsWithoutPython/condaLackingPython/bin/dummy diff --git a/src/client/interpreter/interpreterService.ts b/src/client/interpreter/interpreterService.ts index 2fee9aaec22e..7bb6f9c2edce 100644 --- 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 { const interpreterDisplay = this.serviceContainer.get(IInterpreterDisplay); await interpreterDisplay.refresh(resource); - this.ensureEnvironmentContainsPython(this.configService.getSettings(resource).pythonPath).ignoreErrors(); + const workspaceFolder = this.serviceContainer + .get(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) + .getWorkspaceFolder(resource); reportActiveInterpreterChanged({ path: pySettings.pythonPath, - resource: this.serviceContainer.get(IWorkspaceService).getWorkspaceFolder(resource), + resource: workspaceFolder, }); const interpreterDisplay = this.serviceContainer.get(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); 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 index acf7626e6935..d3ece41f163d 100644 --- 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 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 { // `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 index 7276560d5e71..847d6e752273 100644 --- a/src/test/pythonEnvironments/base/common.ts +++ b/src/test/pythonEnvironments/base/common.ts @@ -104,7 +104,7 @@ export class SimpleLocator extends Locator { constructor( private envs: I[], public callbacks: { - resolve?: null | ((env: PythonEnvInfo) => Promise); + resolve?: null | ((env: PythonEnvInfo | string) => Promise); before?(): Promise; after?(): Promise; onUpdated?: Event | ProgressNotificationEvent>; @@ -112,6 +112,7 @@ export class SimpleLocator extends Locator { afterEach?(e: I): Promise; onQuery?(query: PythonLocatorQuery | undefined, envs: I[]): Promise; } = {}, + private options?: { resolveAsString?: boolean }, ) { super(); } @@ -172,7 +173,7 @@ export class SimpleLocator extends Locator { 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 index ac7157365f02..f48d91cf24ae 100644 --- 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 index 64f8f9558d3c..d1099ee4f840 100644 --- 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 { + await fsapi.writeFile(filename, text); + return filename; +} + +export async function deleteFile(filename: string): Promise { + 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 new file mode 100644 index 000000000000..e69de29bb2d1