From db12c6ae45a5ace52516710ecfb6215334071804 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 2 Sep 2021 15:22:24 -0700 Subject: [PATCH 1/3] Do path comparisons appropriately in the discovery component (#17244) * Do path comparisons appropriately in the discovery component * News entry * Fix some tests * Some more fixes * Fix poetry unit tests --- news/2 Fixes/17244.md | 1 + .../pythonEnvironments/base/info/executable.ts | 2 ++ .../base/locators/composite/resolverUtils.ts | 4 ++-- .../pythonEnvironments/common/commonUtils.ts | 3 ++- .../common/environmentManagers/conda.ts | 4 ++-- .../common/environmentManagers/pipenv.ts | 6 +++--- .../common/environmentManagers/pyenv.ts | 17 ++++------------- .../environmentManagers/simplevirtualenvs.ts | 11 ++--------- .../common/externalDependencies.ts | 3 +++ .../environmentManagers/poetry.unit.test.ts | 13 +++++++++++-- 10 files changed, 32 insertions(+), 32 deletions(-) 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. 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/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/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 { - 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 pathToCheck.startsWith(pyenvDir); + return isParentPath(pathToCheck, pyenvDir); } export interface IPyenvVersionStrings { diff --git a/src/client/pythonEnvironments/common/environmentManagers/simplevirtualenvs.ts b/src/client/pythonEnvironments/common/environmentManagers/simplevirtualenvs.ts index 824c540689ed..915bc8950a01 100644 --- a/src/client/pythonEnvironments/common/environmentManagers/simplevirtualenvs.ts +++ b/src/client/pythonEnvironments/common/environmentManagers/simplevirtualenvs.ts @@ -8,7 +8,7 @@ import { getEnvironmentVariable, getOSType, getUserHomeDir, OSType } from '../.. import { PythonVersion, UNKNOWN_PYTHON_VERSION } from '../../base/info'; import { comparePythonVersionSpecificity } from '../../base/info/env'; import { parseBasicVersion, parseRelease, parseVersion } from '../../base/info/pythonVersion'; -import { pathExists, readFile } from '../externalDependencies'; +import { isParentPath, pathExists, readFile } from '../externalDependencies'; function getPyvenvConfigPathsFrom(interpreterPath: string): string[] { const pyvenvConfigFile = 'pyvenv.cfg'; @@ -99,20 +99,13 @@ function getWorkOnHome(): 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/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)); } 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; From 749a50d96ee83b4092564c4bf364e5fec8f69755 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 7 Sep 2021 11:46:52 -0700 Subject: [PATCH 2/3] Ensure we trigger discovery for the first time as part of extension activation (#17304) * Ensure we trigger discovery for the first time as part of extension activation * Oops --- news/2 Fixes/17303.md | 1 + .../base/locators/composite/envsCollectionService.ts | 3 +++ src/client/pythonEnvironments/index.ts | 7 ++----- 3 files changed, 6 insertions(+), 5 deletions(-) create mode 100644 news/2 Fixes/17303.md diff --git a/news/2 Fixes/17303.md b/news/2 Fixes/17303.md new file mode 100644 index 000000000000..b995c190008f --- /dev/null +++ b/news/2 Fixes/17303.md @@ -0,0 +1 @@ +Ensure we trigger discovery for the first time as part of extension activation. diff --git a/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts b/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts index 49d95f55bd97..61eb61a451d1 100644 --- a/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts +++ b/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts @@ -55,6 +55,9 @@ export class EnvsCollectionService extends PythonEnvsWatcher { }; } - addItemsToRunAfterActivation(() => { - // Force an initial background refresh of the environments. - api.triggerRefresh().ignoreErrors(); - }); + // Force an initial background refresh of the environments. + api.triggerRefresh().ignoreErrors(); return { fullyReady: Promise.resolve(), From b4726bf84926b773711954dc86e11ac9cd3807aa Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Tue, 7 Sep 2021 12:25:28 -0700 Subject: [PATCH 3/3] Fix parent path check (#17274) * Fix parent path check * only buiild vsix * More verbose logging * Revert "only buiild vsix" This reverts commit b601e59f17293bd5c6b1518ff58ab52b6cb8dd7e. --- .../base/locators/lowLevel/fsWatchingLocator.ts | 1 + src/client/pythonEnvironments/common/externalDependencies.ts | 3 +++ src/client/pythonEnvironments/common/pythonBinariesWatcher.ts | 2 +- src/client/pythonEnvironments/legacyIOC.ts | 3 ++- 4 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/client/pythonEnvironments/base/locators/lowLevel/fsWatchingLocator.ts b/src/client/pythonEnvironments/base/locators/lowLevel/fsWatchingLocator.ts index fab78d42de05..7181ec975eea 100644 --- a/src/client/pythonEnvironments/base/locators/lowLevel/fsWatchingLocator.ts +++ b/src/client/pythonEnvironments/base/locators/lowLevel/fsWatchingLocator.ts @@ -143,6 +143,7 @@ export abstract class FSWatchingLocator extends LazyResourceB const searchLocation = Uri.file( this.opts.searchLocation ?? path.dirname(getEnvironmentDirFromPath(executable)), ); + traceVerbose('Fired event ', JSON.stringify({ type, kind, searchLocation }), 'from locator'); this.emitter.fire({ type, kind, searchLocation }); }; diff --git a/src/client/pythonEnvironments/common/externalDependencies.ts b/src/client/pythonEnvironments/common/externalDependencies.ts index 27f5cea2ee4d..058f0fb59d15 100644 --- a/src/client/pythonEnvironments/common/externalDependencies.ts +++ b/src/client/pythonEnvironments/common/externalDependencies.ts @@ -85,6 +85,9 @@ export function isParentPath(filePath: string, parentPath: string): boolean { if (!parentPath.endsWith(path.sep)) { parentPath += path.sep; } + if (!filePath.endsWith(path.sep)) { + filePath += path.sep; + } return normCasePath(filePath).startsWith(normCasePath(parentPath)); } diff --git a/src/client/pythonEnvironments/common/pythonBinariesWatcher.ts b/src/client/pythonEnvironments/common/pythonBinariesWatcher.ts index 364157b02fb0..3cdf9860814b 100644 --- a/src/client/pythonEnvironments/common/pythonBinariesWatcher.ts +++ b/src/client/pythonEnvironments/common/pythonBinariesWatcher.ts @@ -27,7 +27,7 @@ export function watchLocationForPythonBinaries( const resolvedGlob = path.posix.normalize(executableGlob); const [baseGlob] = resolvedGlob.split('/').slice(-1); function callbackClosure(type: FileChangeType, e: string) { - traceVerbose('Received event', JSON.stringify(e), 'for baseglob', baseGlob); + traceVerbose('Received event', type, JSON.stringify(e), 'for baseglob', baseGlob); const isMatch = minimatch(path.basename(e), baseGlob, { nocase: getOSType() === OSType.Windows }); if (!isMatch) { // When deleting the file for some reason path to all directories leading up to python are reported diff --git a/src/client/pythonEnvironments/legacyIOC.ts b/src/client/pythonEnvironments/legacyIOC.ts index 25ea840d8976..0b18fb2c11e3 100644 --- a/src/client/pythonEnvironments/legacyIOC.ts +++ b/src/client/pythonEnvironments/legacyIOC.ts @@ -5,7 +5,7 @@ import { injectable } from 'inversify'; import { intersection } from 'lodash'; import * as vscode from 'vscode'; import { DiscoveryVariants } from '../common/experiments/groups'; -import { traceError } from '../common/logger'; +import { traceError, traceVerbose } from '../common/logger'; import { FileChangeType } from '../common/platform/fileSystemWatcher'; import { Resource } from '../common/types'; import { @@ -177,6 +177,7 @@ class ComponentAdapter implements IComponentAdapter { if (!workspaceFolder || !e.searchLocation) { return; } + traceVerbose(`Recieved event ${JSON.stringify(e)} file change event`); if ( e.type === FileChangeType.Created && isParentPath(e.searchLocation.fsPath, workspaceFolder.uri.fsPath)