From 095d5c5a9f7c751087a6f103d8bf56820357c6fa Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 1 Sep 2021 15:57:20 -0700 Subject: [PATCH 1/5] Do path comparisons appropriately in the discovery component --- src/client/pythonEnvironments/base/info/executable.ts | 2 ++ .../base/locators/composite/resolverUtils.ts | 4 ++-- .../common/environmentManagers/conda.ts | 4 ++-- .../common/environmentManagers/pipenv.ts | 6 +++--- .../common/environmentManagers/pyenv.ts | 4 ++-- .../common/environmentManagers/simplevirtualenvs.ts | 11 ++--------- src/client/pythonEnvironments/common/windowsUtils.ts | 3 ++- 7 files changed, 15 insertions(+), 19 deletions(-) diff --git a/src/client/pythonEnvironments/base/info/executable.ts b/src/client/pythonEnvironments/base/info/executable.ts index 9d1beef317f2..ab5a67d79315 100644 --- a/src/client/pythonEnvironments/base/info/executable.ts +++ b/src/client/pythonEnvironments/base/info/executable.ts @@ -6,6 +6,7 @@ import { getOSType, OSType } from '../../../common/utils/platform'; import { getEmptyVersion, parseVersion } from './pythonVersion'; import { PythonVersion } from '.'; +import { normCasePath } from '../../common/externalDependencies'; /** * Determine a best-effort Python version based on the given filename. @@ -21,6 +22,7 @@ export function parseVersionFromExecutable(filename: string): PythonVersion { } function parseBasename(basename: string): PythonVersion { + basename = normCasePath(basename); if (getOSType() === OSType.Windows) { if (basename === 'python.exe') { // On Windows we can't assume it is 2.7. diff --git a/src/client/pythonEnvironments/base/locators/composite/resolverUtils.ts b/src/client/pythonEnvironments/base/locators/composite/resolverUtils.ts index 2e5e00e1891d..9251404f4e54 100644 --- a/src/client/pythonEnvironments/base/locators/composite/resolverUtils.ts +++ b/src/client/pythonEnvironments/base/locators/composite/resolverUtils.ts @@ -12,7 +12,7 @@ import { getInterpreterPathFromDir, getPythonVersionFromPath, } from '../../../common/commonUtils'; -import { getWorkspaceFolders, isParentPath } from '../../../common/externalDependencies'; +import { arePathsSame, getWorkspaceFolders, isParentPath } from '../../../common/externalDependencies'; import { AnacondaCompanyName, Conda } from '../../../common/environmentManagers/conda'; import { parsePyenvVersion } from '../../../common/environmentManagers/pyenv'; import { Architecture, getOSType, OSType } from '../../../../common/utils/platform'; @@ -82,7 +82,7 @@ async function updateEnvUsingRegistry(env: PythonEnvInfo): Promise { traceError('Expected registry interpreter cache to be initialized already'); interpreters = await getRegistryInterpreters(); } - const data = interpreters.find((i) => i.interpreterPath.toUpperCase() === env.executable.filename.toUpperCase()); + const data = interpreters.find((i) => arePathsSame(i.interpreterPath, env.executable.filename)); if (data) { const versionStr = data.versionStr ?? data.sysVersionStr ?? data.interpreterPath; let version; diff --git a/src/client/pythonEnvironments/common/environmentManagers/conda.ts b/src/client/pythonEnvironments/common/environmentManagers/conda.ts index 30359a06fda7..540325eae5c9 100644 --- a/src/client/pythonEnvironments/common/environmentManagers/conda.ts +++ b/src/client/pythonEnvironments/common/environmentManagers/conda.ts @@ -2,7 +2,7 @@ import * as fsapi from 'fs-extra'; import * as path from 'path'; import { traceVerbose } from '../../../common/logger'; import { getEnvironmentVariable, getOSType, getUserHomeDir, OSType } from '../../../common/utils/platform'; -import { exec, getPythonSetting, pathExists, readFile } from '../externalDependencies'; +import { arePathsSame, exec, getPythonSetting, pathExists, readFile } from '../externalDependencies'; import { PythonVersion, UNKNOWN_PYTHON_VERSION } from '../../base/info'; import { parseVersion } from '../../base/info/pythonVersion'; @@ -373,7 +373,7 @@ export class Conda { } function getName(prefix: string) { - if (prefix === info.root_prefix) { + if (info.root_prefix && arePathsSame(prefix, info.root_prefix)) { return 'base'; } diff --git a/src/client/pythonEnvironments/common/environmentManagers/pipenv.ts b/src/client/pythonEnvironments/common/environmentManagers/pipenv.ts index b17f99d6efa0..9bfbbcea4d08 100644 --- a/src/client/pythonEnvironments/common/environmentManagers/pipenv.ts +++ b/src/client/pythonEnvironments/common/environmentManagers/pipenv.ts @@ -4,7 +4,7 @@ import * as path from 'path'; import { traceError } from '../../../common/logger'; import { getEnvironmentVariable } from '../../../common/utils/platform'; -import { arePathsSame, pathExists, readFile } from '../externalDependencies'; +import { arePathsSame, normCasePath, pathExists, readFile } from '../externalDependencies'; function getSearchHeight() { // PIPENV_MAX_DEPTH tells pipenv the maximum number of directories to recursively search for @@ -108,8 +108,8 @@ async function getPipfileIfGlobal(interpreterPath: string): Promise { */ export async function isVirtualenvwrapperEnvironment(interpreterPath: string): Promise { const workOnHomeDir = await getWorkOnHome(); - let pathToCheck = interpreterPath; - let workOnRoot = workOnHomeDir; - - if (getOSType() === OSType.Windows) { - workOnRoot = workOnHomeDir.toUpperCase(); - pathToCheck = interpreterPath.toUpperCase(); - } // For environment to be a virtualenvwrapper based it has to follow these two rules: // 1. It should be in a sub-directory under the WORKON_HOME // 2. It should be a valid virtualenv environment return ( (await pathExists(workOnHomeDir)) && - pathToCheck.startsWith(`${workOnRoot}${path.sep}`) && + isParentPath(interpreterPath, workOnHomeDir) && isVirtualenvEnvironment(interpreterPath) ); } diff --git a/src/client/pythonEnvironments/common/windowsUtils.ts b/src/client/pythonEnvironments/common/windowsUtils.ts index ab5834ac83a2..ab653ca15d58 100644 --- a/src/client/pythonEnvironments/common/windowsUtils.ts +++ b/src/client/pythonEnvironments/common/windowsUtils.ts @@ -5,6 +5,7 @@ import { uniqBy } from 'lodash'; import * as path from 'path'; import { isTestExecution } from '../../common/constants'; import { traceError, traceVerbose } from '../../common/logger'; +import { normCasePath } from './externalDependencies'; import { HKCU, HKLM, @@ -21,7 +22,7 @@ import { * Determine if the given filename looks like the simplest Python executable. */ export function matchBasicPythonBinFilename(filename: string): boolean { - return path.basename(filename).toLowerCase() === 'python.exe'; + return path.basename(normCasePath(filename)) === 'python.exe'; } /** From 28282651974cdd88a6fe401a5711806022f7b8dd Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 1 Sep 2021 16:02:58 -0700 Subject: [PATCH 2/5] News entry --- news/2 Fixes/17244.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/2 Fixes/17244.md diff --git a/news/2 Fixes/17244.md b/news/2 Fixes/17244.md new file mode 100644 index 000000000000..bbdacb744060 --- /dev/null +++ b/news/2 Fixes/17244.md @@ -0,0 +1 @@ +Do path comparisons appropriately in the new discovery component. From ee50938622280e0ab905a71864db035b67d9fb05 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 2 Sep 2021 12:30:20 -0700 Subject: [PATCH 3/5] Fix some tests --- src/client/pythonEnvironments/common/windowsUtils.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/client/pythonEnvironments/common/windowsUtils.ts b/src/client/pythonEnvironments/common/windowsUtils.ts index ab653ca15d58..ab5834ac83a2 100644 --- a/src/client/pythonEnvironments/common/windowsUtils.ts +++ b/src/client/pythonEnvironments/common/windowsUtils.ts @@ -5,7 +5,6 @@ import { uniqBy } from 'lodash'; import * as path from 'path'; import { isTestExecution } from '../../common/constants'; import { traceError, traceVerbose } from '../../common/logger'; -import { normCasePath } from './externalDependencies'; import { HKCU, HKLM, @@ -22,7 +21,7 @@ import { * Determine if the given filename looks like the simplest Python executable. */ export function matchBasicPythonBinFilename(filename: string): boolean { - return path.basename(normCasePath(filename)) === 'python.exe'; + return path.basename(filename).toLowerCase() === 'python.exe'; } /** From 66a611881f500eda48a15057db37731b62f6a23c Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 2 Sep 2021 13:10:01 -0700 Subject: [PATCH 4/5] Some more fixes --- src/client/pythonEnvironments/common/commonUtils.ts | 3 ++- .../common/environmentManagers/pyenv.ts | 13 ++----------- .../common/externalDependencies.ts | 3 +++ 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/client/pythonEnvironments/common/commonUtils.ts b/src/client/pythonEnvironments/common/commonUtils.ts index 196d21442f99..531d20c537f2 100644 --- a/src/client/pythonEnvironments/common/commonUtils.ts +++ b/src/client/pythonEnvironments/common/commonUtils.ts @@ -11,6 +11,7 @@ import { comparePythonVersionSpecificity } from '../base/info/env'; import { parseVersion } from '../base/info/pythonVersion'; import { getPythonVersionFromConda } from './environmentManagers/conda'; import { getPythonVersionFromPyvenvCfg } from './environmentManagers/simplevirtualenvs'; +import { normCasePath } from './externalDependencies'; import * as posix from './posixUtils'; import * as windows from './windowsUtils'; @@ -362,7 +363,7 @@ export function getEnvironmentDirFromPath(interpreterPath: string): string { // env <--- Return this directory if it is not 'bin' or 'scripts' // |__ python <--- interpreterPath const dir = path.basename(path.dirname(interpreterPath)); - if (!skipDirs.includes(dir.toLowerCase())) { + if (!skipDirs.map((e) => normCasePath(e)).includes(normCasePath(dir))) { return path.dirname(interpreterPath); } diff --git a/src/client/pythonEnvironments/common/environmentManagers/pyenv.ts b/src/client/pythonEnvironments/common/environmentManagers/pyenv.ts index 2aa6957d5db1..c33f4f5e0e98 100644 --- a/src/client/pythonEnvironments/common/environmentManagers/pyenv.ts +++ b/src/client/pythonEnvironments/common/environmentManagers/pyenv.ts @@ -37,22 +37,13 @@ export function isPyenvShimDir(dirPath: string): boolean { */ export async function isPyenvEnvironment(interpreterPath: string): Promise { - let pathToCheck = interpreterPath; - let pyenvDir = getPyenvDir(); + const pathToCheck = interpreterPath; + const pyenvDir = getPyenvDir(); if (!(await pathExists(pyenvDir))) { return false; } - if (!pyenvDir.endsWith(path.sep)) { - pyenvDir += path.sep; - } - - if (getOSType() === OSType.Windows) { - pyenvDir = pyenvDir.toUpperCase(); - pathToCheck = pathToCheck.toUpperCase(); - } - return isParentPath(pathToCheck, pyenvDir); } diff --git a/src/client/pythonEnvironments/common/externalDependencies.ts b/src/client/pythonEnvironments/common/externalDependencies.ts index bae4b7a0b774..27f5cea2ee4d 100644 --- a/src/client/pythonEnvironments/common/externalDependencies.ts +++ b/src/client/pythonEnvironments/common/externalDependencies.ts @@ -82,6 +82,9 @@ export const untildify: (value: string) => string = require('untildify'); * @param parentPath The potential parent path to check for */ export function isParentPath(filePath: string, parentPath: string): boolean { + if (!parentPath.endsWith(path.sep)) { + parentPath += path.sep; + } return normCasePath(filePath).startsWith(normCasePath(parentPath)); } From 2896a9ddad46cbee2b1a5b7a1a43d4d0d907fce7 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 2 Sep 2021 14:56:49 -0700 Subject: [PATCH 5/5] Fix poetry unit tests --- .../common/environmentManagers/poetry.unit.test.ts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/test/pythonEnvironments/common/environmentManagers/poetry.unit.test.ts b/src/test/pythonEnvironments/common/environmentManagers/poetry.unit.test.ts index 2355936b8f49..355a1251d118 100644 --- a/src/test/pythonEnvironments/common/environmentManagers/poetry.unit.test.ts +++ b/src/test/pythonEnvironments/common/environmentManagers/poetry.unit.test.ts @@ -5,7 +5,7 @@ import { assert, expect } from 'chai'; import * as path from 'path'; import * as sinon from 'sinon'; import { ExecutionResult, ShellOptions } from '../../../../client/common/process/types'; -import { getUserHomeDir } from '../../../../client/common/utils/platform'; +import * as platformApis from '../../../../client/common/utils/platform'; import * as externalDependencies from '../../../../client/pythonEnvironments/common/externalDependencies'; import { isPoetryEnvironment, Poetry } from '../../../../client/pythonEnvironments/common/environmentManagers/poetry'; import { TEST_LAYOUT_ROOT } from '../commonTestConstants'; @@ -20,6 +20,12 @@ suite('isPoetryEnvironment Tests', () => { let getPythonSetting: sinon.SinonStub; suite('Global poetry environment', async () => { + setup(() => { + sinon.stub(platformApis, 'getOSType').callsFake(() => platformApis.OSType.Windows); + }); + teardown(() => { + sinon.restore(); + }); test('Return true if environment folder name matches global env pattern and environment is of virtual env type', async () => { const result = await isPoetryEnvironment( path.join(testPoetryDir, 'poetry-tutorial-project-6hnqYwvD-py3.8', 'Scripts', 'python.exe'), @@ -60,16 +66,19 @@ suite('isPoetryEnvironment Tests', () => { }); test('Return true if environment folder name matches criteria for local envs', async () => { + sinon.stub(platformApis, 'getOSType').callsFake(() => platformApis.OSType.Windows); const result = await isPoetryEnvironment(path.join(project1, '.venv', 'Scripts', 'python.exe')); expect(result).to.equal(true); }); test(`Return false if environment folder name is not named '.venv' for local envs`, async () => { + sinon.stub(platformApis, 'getOSType').callsFake(() => platformApis.OSType.Windows); const result = await isPoetryEnvironment(path.join(project1, '.venv2', 'Scripts', 'python.exe')); expect(result).to.equal(false); }); test(`Return false if running poetry for project dir as cwd fails (pyproject.toml file is invalid)`, async () => { + sinon.stub(platformApis, 'getOSType').callsFake(() => platformApis.OSType.Linux); const result = await isPoetryEnvironment(path.join(project4, '.venv', 'bin', 'python')); expect(result).to.equal(false); }); @@ -148,7 +157,7 @@ suite('Poetry binary is located correctly', async () => { }); test('When poetry is not available on PATH, try using the default poetry location if valid', async () => { - const home = getUserHomeDir(); + const home = platformApis.getUserHomeDir(); if (!home) { assert(true); return;