Skip to content
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

Ensure IDs of Conda environments without python does not change after python is installed #20609

Merged
merged 7 commits into from
Feb 1, 2023
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
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