Skip to content

Commit

Permalink
Ensure IDs of Conda environments without python does not change after…
Browse files Browse the repository at this point in the history
… python is installed (#20609)

Fixes #20176

As we're changing IDs we've to ensure the same environment in cache with
older ID is migrated to use the new one, this is already ensured here:
https://github.com/microsoft/vscode-python/blob/32f55109c976e66bf39e8da6aae0c9b6f5115df2/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts#L109
  • Loading branch information
Kartik Raj authored Feb 1, 2023
1 parent 92d2d85 commit 401418a
Show file tree
Hide file tree
Showing 6 changed files with 37 additions and 13 deletions.
5 changes: 3 additions & 2 deletions src/client/common/installer/condaInstaller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import { inject, injectable } from 'inversify';
import { ICondaService, IComponentAdapter } from '../../interpreter/contracts';
import { IServiceContainer } from '../../ioc/types';
import { getEnvPath } from '../../pythonEnvironments/base/info/env';
import { ModuleInstallerType } from '../../pythonEnvironments/info';
import { ExecutionInfo, IConfigurationService, Product } from '../types';
import { isResource } from '../utils/misc';
Expand Down Expand Up @@ -79,7 +80,7 @@ export class CondaInstaller extends ModuleInstaller {

const pythonPath = isResource(resource)
? this.serviceContainer.get<IConfigurationService>(IConfigurationService).getSettings(resource).pythonPath
: resource.id ?? '';
: getEnvPath(resource.path, resource.envPath).path ?? '';
const condaLocatorService = this.serviceContainer.get<IComponentAdapter>(IComponentAdapter);
const info = await condaLocatorService.getCondaEnvironment(pythonPath);
const args = [flags & ModuleInstallFlags.upgrade ? 'update' : 'install'];
Expand Down Expand Up @@ -132,7 +133,7 @@ export class CondaInstaller extends ModuleInstaller {
const condaService = this.serviceContainer.get<IComponentAdapter>(IComponentAdapter);
const pythonPath = isResource(resource)
? this.serviceContainer.get<IConfigurationService>(IConfigurationService).getSettings(resource).pythonPath
: resource.id ?? '';
: getEnvPath(resource.path, resource.envPath).path ?? '';
return condaService.isCondaEnvironment(pythonPath);
}
}
2 changes: 1 addition & 1 deletion src/client/proposedApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ export function convertCompleteEnvInfo(env: PythonEnvInfo): ResolvedEnvironment
const { path } = getEnvPath(env.executable.filename, env.location);
const resolvedEnv: ResolvedEnvironment = {
path,
id: getEnvID(path),
id: env.id!,
executable: {
uri: env.executable.filename === 'python' ? undefined : Uri.file(env.executable.filename),
bitness: convertBitness(env.arch),
Expand Down
12 changes: 11 additions & 1 deletion src/client/pythonEnvironments/base/info/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ function getMinimalPartialInfo(env: string | PythonEnvInfo | BasicEnvInfo): Part
return undefined;
}
return {
id: '',
executable: {
filename: env,
sysPrefix: '',
Expand All @@ -208,6 +209,7 @@ function getMinimalPartialInfo(env: string | PythonEnvInfo | BasicEnvInfo): Part
}
if ('executablePath' in env) {
return {
id: '',
executable: {
filename: env.executablePath,
sysPrefix: '',
Expand Down Expand Up @@ -235,7 +237,7 @@ export function getEnvPath(interpreterPath: string, envFolderPath?: string): Env
}

/**
* Gets unique identifier for an environment.
* Gets general unique identifier for most environments.
*/
export function getEnvID(interpreterPath: string, envFolderPath?: string): string {
return normCasePath(getEnvPath(interpreterPath, envFolderPath).path);
Expand Down Expand Up @@ -266,7 +268,15 @@ export function areSameEnv(
const leftFilename = leftInfo.executable!.filename;
const rightFilename = rightInfo.executable!.filename;

if (leftInfo.id && leftInfo.id === rightInfo.id) {
// In case IDs are available, use it.
return true;
}

if (getEnvID(leftFilename, leftInfo.location) === getEnvID(rightFilename, rightInfo.location)) {
// Otherwise use ID function to get the ID. Note ID returned by function may itself change if executable of
// an environment changes, for eg. when conda installs python into the env. So only use it as a fallback if
// ID is not available.
return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@ import {
import { buildEnvInfo, comparePythonVersionSpecificity, setEnvDisplayString, getEnvID } from '../../info/env';
import { getEnvironmentDirFromPath, getPythonVersionFromPath } from '../../../common/commonUtils';
import { arePathsSame, getFileInfo, isParentPath } from '../../../common/externalDependencies';
import { AnacondaCompanyName, Conda, isCondaEnvironment } from '../../../common/environmentManagers/conda';
import {
AnacondaCompanyName,
Conda,
getCondaInterpreterPath,
isCondaEnvironment,
} from '../../../common/environmentManagers/conda';
import { getPyenvVersionsDir, parsePyenvVersion } from '../../../common/environmentManagers/pyenv';
import { Architecture, getOSType, OSType } from '../../../../common/utils/platform';
import { getPythonVersionFromPath as parsePythonVersionFromPath, parseVersion } from '../../info/pythonVersion';
Expand Down Expand Up @@ -57,7 +62,6 @@ export async function resolveBasicEnv(env: BasicEnvInfo): Promise<PythonEnvInfo>
await updateEnvUsingRegistry(resolvedEnv);
}
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;
Expand Down Expand Up @@ -189,6 +193,13 @@ async function resolveCondaEnv(env: BasicEnvInfo): Promise<PythonEnvInfo> {
if (name) {
info.name = name;
}
if (env.envPath && path.basename(executable) === executable) {
// For environments without python, set ID using the predicted executable path after python is installed.
// Another alternative could've been to set ID of all conda environments to the environment path, as that
// remains constant even after python installation.
const predictedExecutable = getCondaInterpreterPath(env.envPath);
info.id = getEnvID(predictedExecutable, env.envPath);
}
return info;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,14 +226,11 @@ export async function getPythonVersionFromConda(interpreterPath: string): Promis
/**
* Return the interpreter's filename for the given environment.
*/
async function getInterpreterPath(condaEnvironmentPath: string): Promise<string | undefined> {
export function getCondaInterpreterPath(condaEnvironmentPath: string): string {
// where to find the Python binary within a conda env.
const relativePath = getOSType() === OSType.Windows ? 'python.exe' : path.join('bin', 'python');
const filePath = path.join(condaEnvironmentPath, relativePath);
if (await pathExists(filePath)) {
return filePath;
}
return undefined;
return filePath;
}

// Minimum version number of conda required to be able to use 'conda run' with '--no-capture-output' flag.
Expand Down Expand Up @@ -494,8 +491,8 @@ export class Conda {
*/
// eslint-disable-next-line class-methods-use-this
public async getInterpreterPathForEnvironment(condaEnv: CondaEnvInfo | { prefix: string }): Promise<string> {
const executablePath = await getInterpreterPath(condaEnv.prefix);
if (executablePath) {
const executablePath = getCondaInterpreterPath(condaEnv.prefix);
if (await pathExists(executablePath)) {
traceVerbose('Found executable within conda env', JSON.stringify(condaEnv));
return executablePath;
}
Expand Down
5 changes: 5 additions & 0 deletions src/test/proposedApi.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ suite('Proposed Extension API', () => {
test('environments: python found', async () => {
const expectedEnvs = [
{
id: normCasePath('this/is/a/test/python/path1'),
executable: {
filename: 'this/is/a/test/python/path1',
ctime: 1,
Expand All @@ -273,6 +274,7 @@ suite('Proposed Extension API', () => {
},
},
{
id: normCasePath('this/is/a/test/python/path2'),
executable: {
filename: 'this/is/a/test/python/path2',
ctime: 1,
Expand All @@ -297,6 +299,7 @@ suite('Proposed Extension API', () => {
const envs = [
...expectedEnvs,
{
id: normCasePath('this/is/a/test/python/path3'),
executable: {
filename: 'this/is/a/test/python/path3',
ctime: 1,
Expand Down Expand Up @@ -343,6 +346,7 @@ suite('Proposed Extension API', () => {
searchLocation: Uri.file(workspacePath),
}),
{
id: normCasePath('this/is/a/test/python/path1'),
executable: {
filename: 'this/is/a/test/python/path1',
ctime: 1,
Expand All @@ -364,6 +368,7 @@ suite('Proposed Extension API', () => {
},
},
{
id: normCasePath('this/is/a/test/python/path2'),
executable: {
filename: 'this/is/a/test/python/path2',
ctime: 1,
Expand Down

0 comments on commit 401418a

Please sign in to comment.