Skip to content

Remove sys.prefix workaround which is no longer needed but still validate envs #19164

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import { Event } from 'vscode';
import { traceInfo } from '../../../../logging';
import { reportInterpretersChanged } from '../../../../proposedApi';
import { arePathsSame, pathExists } from '../../../common/externalDependencies';
import { arePathsSame, getFileInfo, pathExists } from '../../../common/externalDependencies';
import { PythonEnvInfo } from '../../info';
import { areSameEnv, getEnvPath } from '../../info/env';
import {
Expand Down Expand Up @@ -33,18 +33,19 @@ export interface IEnvsCollectionCache {
/**
* Adds environment to cache.
*/
addEnv(env: PythonEnvInfo, hasCompleteInfo?: boolean): void;
addEnv(env: PythonEnvInfo, hasLatestInfo?: boolean): void;

/**
* Return cached environment information for a given path if it exists and
* has complete info, otherwise return `undefined`.
* is up to date, otherwise return `undefined`.
*
* @param path - Python executable path or path to environment
*/
getCompleteInfo(path: string): PythonEnvInfo | undefined;
getLatestInfo(path: string): Promise<PythonEnvInfo | undefined>;

/**
* Writes the content of the in-memory cache to persistent storage.
* Writes the content of the in-memory cache to persistent storage. It is assumed
* all envs have upto date info when this is called.
*/
flush(): Promise<void>;

Expand All @@ -60,7 +61,7 @@ export interface IEnvsCollectionCache {
clearCache(): Promise<void>;
}

export type PythonEnvCompleteInfo = { hasCompleteInfo?: boolean } & PythonEnvInfo;
export type PythonEnvLatestInfo = { hasLatestInfo?: boolean } & PythonEnvInfo;

interface IPersistentStorage {
load(): Promise<PythonEnvInfo[]>;
Expand All @@ -72,7 +73,7 @@ interface IPersistentStorage {
*/
export class PythonEnvInfoCache extends PythonEnvsWatcher<PythonEnvCollectionChangedEvent>
implements IEnvsCollectionCache {
private envs: PythonEnvCompleteInfo[] = [];
private envs: PythonEnvLatestInfo[] = [];

constructor(private readonly persistentStorage: IPersistentStorage) {
super();
Expand Down Expand Up @@ -103,10 +104,11 @@ export class PythonEnvInfoCache extends PythonEnvsWatcher<PythonEnvCollectionCha
return this.envs;
}

public addEnv(env: PythonEnvCompleteInfo, hasCompleteInfo?: boolean): void {
public addEnv(env: PythonEnvLatestInfo, hasLatestInfo?: boolean): void {
const found = this.envs.find((e) => areSameEnv(e, env));
if (hasCompleteInfo) {
env.hasCompleteInfo = true;
if (hasLatestInfo) {
env.hasLatestInfo = true;
this.flush(false).ignoreErrors();
}
if (!found) {
this.envs.push(env);
Expand All @@ -133,26 +135,33 @@ export class PythonEnvInfoCache extends PythonEnvsWatcher<PythonEnvCollectionCha
}
}

public getCompleteInfo(path: string): PythonEnvInfo | undefined {
public async getLatestInfo(path: string): Promise<PythonEnvInfo | undefined> {
// `path` can either be path to environment or executable path
let env = this.envs.find((e) => arePathsSame(e.location, path));
if (env?.hasCompleteInfo) {
const env = this.envs.find((e) => arePathsSame(e.location, path)) ?? this.envs.find((e) => areSameEnv(e, path));
if (env?.hasLatestInfo) {
return env;
}
env = this.envs.find((e) => areSameEnv(e, path));
return env?.hasCompleteInfo ? env : undefined;
if (env && (env?.hasLatestInfo || (await validateInfo(env)))) {
return env;
}
return undefined;
}

public async clearAndReloadFromStorage(): Promise<void> {
this.envs = await this.persistentStorage.load();
this.envs.forEach((e) => {
delete e.hasLatestInfo;
});
}

public async flush(): Promise<void> {
public async flush(allEnvsHaveLatestInfo = true): Promise<void> {
if (this.envs.length) {
traceInfo('Environments added to cache', JSON.stringify(this.envs));
this.envs.forEach((e) => {
e.hasCompleteInfo = true;
});
if (allEnvsHaveLatestInfo) {
this.envs.forEach((e) => {
e.hasLatestInfo = true;
});
}
await this.persistentStorage.store(this.envs);
}
}
Expand All @@ -167,6 +176,16 @@ export class PythonEnvInfoCache extends PythonEnvsWatcher<PythonEnvCollectionCha
}
}

async function validateInfo(env: PythonEnvInfo) {
const { ctime, mtime } = await getFileInfo(env.executable.filename);
if (ctime === env.executable.ctime && mtime === env.executable.mtime) {
return true;
}
env.executable.ctime = ctime;
env.executable.mtime = mtime;
return false;
}

/**
* Build a cache of PythonEnvInfo that is ready to use.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export class EnvsCollectionService extends PythonEnvsWatcher<PythonEnvCollection
// Note cache may have incomplete info when a refresh is happening.
// This API is supposed to return complete info by definition, so
// only use cache if it has complete info on an environment.
const cachedEnv = this.cache.getCompleteInfo(path);
const cachedEnv = await this.cache.getLatestInfo(path);
if (cachedEnv) {
return cachedEnv;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
getInterpreterPathFromDir,
getPythonVersionFromPath,
} from '../../../common/commonUtils';
import { arePathsSame, getWorkspaceFolders, isParentPath } from '../../../common/externalDependencies';
import { arePathsSame, getFileInfo, getWorkspaceFolders, isParentPath } from '../../../common/externalDependencies';
import { AnacondaCompanyName, Conda, isCondaEnvironment } from '../../../common/environmentManagers/conda';
import { getPyenvVersionsDir, parsePyenvVersion } from '../../../common/environmentManagers/pyenv';
import { Architecture, getOSType, OSType } from '../../../../common/utils/platform';
Expand Down Expand Up @@ -59,6 +59,9 @@ export async function resolveBasicEnv(env: BasicEnvInfo, useCache = false): Prom
}
setEnvDisplayString(resolvedEnv);
resolvedEnv.id = getEnvID(resolvedEnv.executable.filename, resolvedEnv.location);
const { ctime, mtime } = await getFileInfo(resolvedEnv.executable.filename);
resolvedEnv.executable.ctime = ctime;
resolvedEnv.executable.mtime = mtime;
return resolvedEnv;
}

Expand Down
14 changes: 14 additions & 0 deletions src/client/pythonEnvironments/common/externalDependencies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,20 @@ export async function resolveSymbolicLink(absPath: string, stats?: fsapi.Stats):
return absPath;
}

export async function getFileInfo(filePath: string): Promise<{ ctime: number; mtime: number }> {
try {
const data = await fsapi.lstat(filePath);
return {
ctime: data.ctime.valueOf(),
mtime: data.mtime.valueOf(),
};
} catch (ex) {
// This can fail on some cases, such as, `reparse points` on windows. So, return the
// time as -1. Which we treat as not set in the extension.
return { ctime: -1, mtime: -1 };
}
}

export async function isFile(filePath: string): Promise<boolean> {
const stats = await fsapi.lstat(filePath);
if (stats.isSymbolicLink()) {
Expand Down
9 changes: 0 additions & 9 deletions src/client/pythonEnvironments/legacyIOC.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import { isParentPath } from './common/externalDependencies';
import { EnvironmentType, PythonEnvironment } from './info';
import { toSemverLikeVersion } from './base/info/pythonVersion';
import { PythonVersion } from './info/pythonVersion';
import { EnvironmentInfoServiceQueuePriority, getEnvironmentInfoService } from './base/info/environmentInfoService';
import { createDeferred } from '../common/utils/async';
import { PythonEnvCollectionChangedEvent } from './base/watcher';
import { asyncFilter } from '../common/utils/arrayUtils';
Expand Down Expand Up @@ -170,14 +169,6 @@ class ComponentAdapter implements IComponentAdapter {
if (!env) {
return undefined;
}
if (env?.executable.sysPrefix) {
const execInfoService = getEnvironmentInfoService();
const info = await execInfoService.getEnvironmentInfo(env, EnvironmentInfoServiceQueuePriority.High);
if (info) {
env.executable.sysPrefix = info.executable.sysPrefix;
env.version = info.version;
}
}
return convertEnvInfo(env);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@ import {
} from '../../../../../client/pythonEnvironments/base/locator';
import {
createCollectionCache,
PythonEnvCompleteInfo,
PythonEnvLatestInfo,
} from '../../../../../client/pythonEnvironments/base/locators/composite/envsCollectionCache';
import { EnvsCollectionService } from '../../../../../client/pythonEnvironments/base/locators/composite/envsCollectionService';
import { PythonEnvCollectionChangedEvent } from '../../../../../client/pythonEnvironments/base/watcher';
import * as externalDependencies from '../../../../../client/pythonEnvironments/common/externalDependencies';
import { noop } from '../../../../core';
import { TEST_LAYOUT_ROOT } from '../../../common/commonTestConstants';
import { SimpleLocator } from '../../common';
Expand Down Expand Up @@ -100,13 +101,13 @@ suite('Python envs locator - Environments Collection', async () => {
updatedName,
);
if (doNotIncludeCached) {
return [env1, env2, env3].map((e: PythonEnvCompleteInfo) => {
e.hasCompleteInfo = true;
return [env1, env2, env3].map((e: PythonEnvLatestInfo) => {
e.hasLatestInfo = true;
return e;
});
}
return [envCached1, env1, env2, env3].map((e: PythonEnvCompleteInfo) => {
e.hasCompleteInfo = true;
return [envCached1, env1, env2, env3].map((e: PythonEnvLatestInfo) => {
e.hasLatestInfo = true;
return e;
});
}
Expand Down Expand Up @@ -542,11 +543,14 @@ suite('Python envs locator - Environments Collection', async () => {
sinon.assert.callCount(reportInterpretersChangedStub, eventData.length);
});

test('resolveEnv() uses cache if complete info is available', async () => {
test('resolveEnv() uses cache if complete and up to date info is available', async () => {
const resolvedViaLocator = buildEnvInfo({ executable: 'Resolved via locator' });
const cachedEnvs = getCachedEnvs();
const env: PythonEnvCompleteInfo = cachedEnvs[0];
env.hasCompleteInfo = true; // Has complete info
const env: PythonEnvLatestInfo = cachedEnvs[0];
env.executable.ctime = 100;
env.executable.mtime = 100;
sinon.stub(externalDependencies, 'getFileInfo').resolves({ ctime: 100, mtime: 100 });
env.hasLatestInfo = true; // Has complete info
const parentLocator = new SimpleLocator([], {
resolve: async (e: PythonEnvInfo) => {
if (env.executable.filename === e.executable.filename) {
Expand All @@ -565,11 +569,40 @@ suite('Python envs locator - Environments Collection', async () => {
sinon.assert.calledOnce(reportInterpretersChangedStub);
});

test('resolveEnv() uses underlying locator if cache does not have up to date info for env', async () => {
const cachedEnvs = getCachedEnvs();
const env: PythonEnvLatestInfo = cachedEnvs[0];
const resolvedViaLocator = buildEnvInfo({
executable: env.executable.filename,
sysPrefix: 'Resolved via locator',
});
env.executable.ctime = 101;
env.executable.mtime = 90;
sinon.stub(externalDependencies, 'getFileInfo').resolves({ ctime: 100, mtime: 100 });
env.hasLatestInfo = true; // Has complete info
const parentLocator = new SimpleLocator([], {
resolve: async (e: PythonEnvInfo) => {
if (env.executable.filename === e.executable.filename) {
return resolvedViaLocator;
}
return undefined;
},
});
const cache = await createCollectionCache({
load: async () => cachedEnvs,
store: async () => noop(),
});
collectionService = new EnvsCollectionService(cache, parentLocator);
const resolved = await collectionService.resolveEnv(env.executable.filename);
assertEnvEqual(resolved, resolvedViaLocator);
sinon.assert.calledOnce(reportInterpretersChangedStub);
});

test('resolveEnv() uses underlying locator if cache does not have complete info for env', async () => {
const resolvedViaLocator = buildEnvInfo({ executable: 'Resolved via locator' });
const cachedEnvs = getCachedEnvs();
const env: PythonEnvCompleteInfo = cachedEnvs[0];
env.hasCompleteInfo = false; // Does not have complete info
const env: PythonEnvLatestInfo = cachedEnvs[0];
env.hasLatestInfo = false; // Does not have complete info
const parentLocator = new SimpleLocator([], {
resolve: async (e: PythonEnvInfo) => {
if (env.executable.filename === e.executable.filename) {
Expand Down Expand Up @@ -618,11 +651,11 @@ suite('Python envs locator - Environments Collection', async () => {
store: async () => noop(),
});
collectionService = new EnvsCollectionService(cache, parentLocator);
const resolved: PythonEnvCompleteInfo | undefined = await collectionService.resolveEnv(
const resolved: PythonEnvLatestInfo | undefined = await collectionService.resolveEnv(
resolvedViaLocator.executable.filename,
);
const envs = collectionService.getEnvs();
expect(resolved?.hasCompleteInfo).to.equal(true);
expect(resolved?.hasLatestInfo).to.equal(true);
assertEnvsEqual(envs, [resolved]);
sinon.assert.calledOnceWithExactly(reportInterpretersChangedStub, [
{ path: resolved?.executable.filename, type: 'add' },
Expand Down