From 2d36ce4c6c627347e9fd0b4245ba90134e025bd3 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Thu, 16 Jul 2026 15:02:23 -0700 Subject: [PATCH] Fix duplicate installs with git worktrees (#1627) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f42031a-bd90-4448-bffe-b5c0ab522b6d --- src/managers/builtin/pipUtils.ts | 158 ++++++-- src/managers/builtin/venvStepBasedFlow.ts | 5 +- src/managers/builtin/venvUtils.ts | 5 +- .../managers/builtin/pipUtils.unit.test.ts | 364 ++++++++++++++++++ 4 files changed, 507 insertions(+), 25 deletions(-) diff --git a/src/managers/builtin/pipUtils.ts b/src/managers/builtin/pipUtils.ts index a2bc4a0f0..055ddb965 100644 --- a/src/managers/builtin/pipUtils.ts +++ b/src/managers/builtin/pipUtils.ts @@ -7,12 +7,13 @@ import { PackageManagementOptions, PythonEnvironment, PythonEnvironmentApi, Pyth import { EXTENSION_ROOT_DIR } from '../../common/constants'; import { PackageManagement, Pickers, VenvManagerStrings } from '../../common/localize'; import { traceInfo } from '../../common/logging'; +import { normalizePath } from '../../common/utils/pathUtils'; import { showQuickPickWithButtons, withProgress } from '../../common/window.apis'; import { findFiles } from '../../common/workspace.apis'; import { selectFromCommonPackagesToInstall, selectFromInstallableToInstall } from '../common/pickers'; import { Installable } from '../common/types'; import { mergePackages } from '../common/utils'; -import { refreshPipPackages } from './utils'; +import { normalizePackageName, refreshPipPackages } from './utils'; export interface PyprojectToml { project?: { @@ -80,6 +81,17 @@ function isPipInstallableToml(toml: tomljs.JsonMap): boolean { return toml['build-system'] !== undefined && toml.project !== undefined; } +/** + * Extracts the declared package name from a parsed `pyproject.toml` (the + * `[project].name` field, PEP 621). Returns `undefined` when the file does not + * declare a name. + */ +function getTomlProjectName(toml: tomljs.JsonMap): string | undefined { + const project = toml.project as tomljs.JsonMap | undefined; + const name = project?.name; + return typeof name === 'string' && name.length > 0 ? name : undefined; +} + function getTomlInstallable(toml: tomljs.JsonMap, tomlPath: Uri): Installable[] { const extras: Installable[] = []; const projectDir = path.dirname(tomlPath.fsPath); @@ -239,6 +251,19 @@ export interface ValidationError { fileUri: Uri; } +export interface ProjectInstallableOptions { + /** + * Prevent automatic installation from passing multiple editable sources for + * the same Python package to pip. + */ + deduplicateProjectPackages?: boolean; + + /** + * Creation root used to prefer the pyproject.toml that owns the environment. + */ + preferredRoot?: Uri; +} + export async function getWorkspacePackagesToInstall( api: PythonEnvironmentApi, options: PackageManagementOptions, @@ -259,6 +284,7 @@ export async function getWorkspacePackagesToInstall( export async function getProjectInstallable( api: PythonEnvironmentApi, projects?: PythonProject[], + options?: ProjectInstallableOptions, ): Promise { if (!projects) { return { installables: [] }; @@ -312,38 +338,124 @@ export async function getProjectInstallable( if (depthA !== depthB) { return depthA - depthB; } - return a.fsPath.localeCompare(b.fsPath); + const pathA = normalizePath(a.fsPath); + const pathB = normalizePath(b.fsPath); + return pathA < pathB ? -1 : pathA > pathB ? 1 : 0; }); + // Parse all TOML files first (in parallel), keyed by fsPath, so the + // subsequent duplicate-detection pass can run sequentially and + // deterministically over the depth-sorted `filtered` list. + const parsedTomls = new Map(); await Promise.all( filtered.map(async (uri) => { if (uri.fsPath.endsWith('.toml')) { const toml = await tomlParse(uri.fsPath); + parsedTomls.set(uri.fsPath, toml); + } + }), + ); - // Validate pyproject.toml - if (!validationError) { - const error = validatePyprojectToml(toml); - if (error) { - validationError = { - message: error, - fileUri: uri, - }; - } - } + const selectedTomls = new Map(); + const ambiguousProjectNames = new Set(); + if (options?.deduplicateProjectPackages) { + const tomlsByProjectName = new Map(); + for (const uri of filtered) { + if (!uri.fsPath.endsWith('.toml')) { + continue; + } + const toml = parsedTomls.get(uri.fsPath) ?? {}; + const projectName = getTomlProjectName(toml); + if (!projectName || getTomlInstallable(toml, uri).length === 0) { + continue; + } + const normalizedName = normalizePackageName(projectName); + const candidates = tomlsByProjectName.get(normalizedName) ?? []; + candidates.push(uri); + tomlsByProjectName.set(normalizedName, candidates); + } - installable.push(...getTomlInstallable(toml, uri)); + tomlsByProjectName.forEach((candidates, projectName) => { + if (candidates.length === 1) { + selectedTomls.set(projectName, candidates[0]); + return; + } + + const preferredRoot = options.preferredRoot?.fsPath; + const preferredCandidates = preferredRoot + ? candidates + .filter((candidate) => { + const projectDir = path.dirname(candidate.fsPath); + const relative = path.relative(projectDir, preferredRoot); + return ( + relative === '' || + (relative !== '..' && + !relative.startsWith(`..${path.sep}`) && + !path.isAbsolute(relative)) + ); + }) + .sort( + (a, b) => + path.dirname(b.fsPath).length - path.dirname(a.fsPath).length, + ) + : []; + + if (preferredCandidates.length > 0) { + selectedTomls.set(projectName, preferredCandidates[0]); } else { - const name = path.basename(uri.fsPath); - installable.push({ - name, - uri, - displayName: name, - group: 'Requirements', - args: ['-r', uri.fsPath], - }); + ambiguousProjectNames.add(projectName); + traceInfo( + `Skipping ambiguous editable installs for package "${projectName}" because no ` + + 'candidate contains the environment creation root.', + ); } - }), - ); + }); + } + + for (const uri of filtered) { + if (uri.fsPath.endsWith('.toml')) { + const toml = parsedTomls.get(uri.fsPath) ?? {}; + const tomlInstallables = getTomlInstallable(toml, uri); + + const projectName = getTomlProjectName(toml); + if (options?.deduplicateProjectPackages && projectName && tomlInstallables.length > 0) { + const normalizedName = normalizePackageName(projectName); + const selected = selectedTomls.get(normalizedName); + if (ambiguousProjectNames.has(normalizedName)) { + continue; + } + if (selected && selected.fsPath !== uri.fsPath) { + traceInfo( + `Skipping duplicate project package "${projectName}" from ${uri.fsPath}; it is ` + + `already provided by ${selected.fsPath}.`, + ); + continue; + } + } + + // Validate pyproject.toml (report only the first error found). + if (!validationError) { + const error = validatePyprojectToml(toml); + if (error) { + validationError = { + message: error, + fileUri: uri, + }; + } + } + + installable.push(...tomlInstallables); + } else { + const name = path.basename(uri.fsPath); + installable.push({ + name, + uri, + displayName: name, + group: 'Requirements', + args: ['-r', uri.fsPath], + }); + } + } }, ); diff --git a/src/managers/builtin/venvStepBasedFlow.ts b/src/managers/builtin/venvStepBasedFlow.ts index 2e8e7dedb..b5ca2b4be 100644 --- a/src/managers/builtin/venvStepBasedFlow.ts +++ b/src/managers/builtin/venvStepBasedFlow.ts @@ -340,7 +340,10 @@ export async function createStepBasedVenvFlow( // Get workspace dependencies to install const project = api.getPythonProject(venvRoot); - const result = await getProjectInstallable(api, project ? [project] : undefined); + const result = await getProjectInstallable(api, project ? [project] : undefined, { + deduplicateProjectPackages: true, + preferredRoot: venvRoot, + }); const installables = result.installables; const allPackages = []; allPackages.push(...(installables?.flatMap((i) => i.args ?? []) ?? [])); diff --git a/src/managers/builtin/venvUtils.ts b/src/managers/builtin/venvUtils.ts index 4f06b3c3a..4efab305e 100644 --- a/src/managers/builtin/venvUtils.ts +++ b/src/managers/builtin/venvUtils.ts @@ -445,7 +445,10 @@ export async function quickCreateVenv( const project = api.getPythonProject(venvRoot); sendTelemetryEvent(EventNames.VENV_CREATION, undefined, { creationType: 'quick' }); - const result = await getProjectInstallable(api, project ? [project] : undefined); + const result = await getProjectInstallable(api, project ? [project] : undefined, { + deduplicateProjectPackages: true, + preferredRoot: venvRoot, + }); const installables = result.installables; const allPackages = []; allPackages.push(...(installables?.flatMap((i) => i.args ?? []) ?? [])); diff --git a/src/test/managers/builtin/pipUtils.unit.test.ts b/src/test/managers/builtin/pipUtils.unit.test.ts index f817f6c78..1cbeeec12 100644 --- a/src/test/managers/builtin/pipUtils.unit.test.ts +++ b/src/test/managers/builtin/pipUtils.unit.test.ts @@ -2,6 +2,8 @@ import assert from 'assert'; import * as path from 'path'; import * as sinon from 'sinon'; import { CancellationToken, Progress, ProgressOptions, Uri } from 'vscode'; +import * as fse from 'fs-extra'; +import * as os from 'os'; import { PythonEnvironmentApi, PythonProject } from '../../../api'; import * as winapi from '../../../common/window.apis'; import * as wapi from '../../../common/workspace.apis'; @@ -235,3 +237,365 @@ suite('Pip Utils - getProjectInstallable', () => { ); }); }); + +suite('Pip Utils - getProjectInstallable duplicate pyproject.toml handling', () => { + let findFilesStub: sinon.SinonStub; + let withProgressStub: sinon.SinonStub; + let mockApi: { getPythonProject: (uri: Uri) => PythonProject | undefined }; + + let tmpRoot: string; + let projectRoot: string; + + // Builds a valid, pip-installable pyproject.toml with the given package name. + function tomlFor(name: string, version = '0.1.0'): string { + return [ + '[project]', + `name = "${name}"`, + `version = "${version}"`, + '', + '[build-system]', + 'requires = ["setuptools"]', + 'build-backend = "setuptools.build_meta"', + '', + ].join('\n'); + } + + // Writes a pyproject.toml under / and returns its path. + async function writeToml(relDir: string, name: string): Promise { + return writeTomlContent(relDir, tomlFor(name)); + } + + async function writeTomlContent(relDir: string, content: string): Promise { + const dir = path.join(projectRoot, relDir); + await fse.mkdirp(dir); + const file = path.join(dir, 'pyproject.toml'); + await fse.writeFile(file, content); + return file; + } + + function automaticOptions(preferredRoot: string = projectRoot) { + return { + deduplicateProjectPackages: true, + preferredRoot: Uri.file(preferredRoot), + }; + } + + setup(async () => { + findFilesStub = sinon.stub(wapi, 'findFiles').resolves([]); + withProgressStub = sinon.stub(winapi, 'withProgress'); + withProgressStub.callsFake( + async ( + _options: ProgressOptions, + callback: ( + progress: Progress<{ message?: string; increment?: number }>, + token: CancellationToken, + ) => Thenable, + ) => { + return await callback( + {} as Progress<{ message?: string; increment?: number }>, + { isCancellationRequested: false } as CancellationToken, + ); + }, + ); + + // Use real temp files because fs-extra's readFile cannot be stubbed + // (non-configurable), and getProjectInstallable parses the toml contents. + tmpRoot = await fse.mkdtemp(path.join(os.tmpdir(), 'piputils-')); + // Normalize through Uri.file so drive-letter casing matches the paths that + // getProjectInstallable derives from the discovered Uris on Windows. + projectRoot = Uri.file(path.join(tmpRoot, 'myapp')).fsPath; + await fse.mkdirp(projectRoot); + + mockApi = { + getPythonProject: (uri: Uri) => { + // Every file under the project root belongs to the same project. + if (uri.fsPath.startsWith(projectRoot)) { + return { name: 'myapp', uri: Uri.file(projectRoot) }; + } + return undefined; + }, + }; + }); + + teardown(async () => { + sinon.restore(); + if (tmpRoot) { + await fse.remove(tmpRoot); + } + }); + + test('omits ambiguous editable installs from sibling git worktrees', async () => { + // Two git worktrees checked out as sibling folders under the project root, + // both declaring the same package name (issue #1627). + const mainToml = await writeToml('main', 'myapp'); + const copilotToml = await writeToml('copilot', 'myapp'); + + findFilesStub.callsFake((pattern: string) => { + if (pattern === '**/pyproject.toml') { + return Promise.resolve([Uri.file(mainToml), Uri.file(copilotToml)]); + } + return Promise.resolve([]); + }); + + const projects = [{ name: 'myapp', uri: Uri.file(projectRoot) }]; + const result = ( + await getProjectInstallable(mockApi as PythonEnvironmentApi, projects, automaticOptions()) + ).installables; + + const editable = result.filter((r) => r.args?.[0] === '-e'); + assert.strictEqual(editable.length, 0, 'Quick Create should not choose an arbitrary worktree'); + }); + + test('duplicate detection is case- and separator-insensitive (PEP 503)', async () => { + const firstToml = await writeToml('main', 'My_App'); + const secondToml = await writeToml('worktree', 'my-app'); + + findFilesStub.callsFake((pattern: string) => { + if (pattern === '**/pyproject.toml') { + return Promise.resolve([Uri.file(firstToml), Uri.file(secondToml)]); + } + return Promise.resolve([]); + }); + + const projects = [{ name: 'myapp', uri: Uri.file(projectRoot) }]; + const result = ( + await getProjectInstallable(mockApi as PythonEnvironmentApi, projects, automaticOptions()) + ).installables; + + const editable = result.filter((r) => r.args?.[0] === '-e'); + assert.strictEqual( + editable.length, + 0, + '"My_App" and "my-app" normalize to the same ambiguous package name', + ); + }); + + test('uses the candidate containing the creation root', async () => { + const invalidToml = await writeTomlContent( + 'a-invalid', + tomlFor('myapp').replace('version = "0.1.0"', 'version = "not a version"'), + ); + const validToml = await writeToml('z-valid', 'myapp'); + + findFilesStub.callsFake((pattern: string) => { + if (pattern === '**/pyproject.toml') { + return Promise.resolve([Uri.file(invalidToml), Uri.file(validToml)]); + } + return Promise.resolve([]); + }); + + const projects = [{ name: 'myapp', uri: Uri.file(projectRoot) }]; + const result = await getProjectInstallable( + mockApi as PythonEnvironmentApi, + projects, + automaticOptions(path.join(projectRoot, 'z-valid')), + ); + + const editable = result.installables.filter((item) => item.args?.[0] === '-e'); + assert.strictEqual(editable.length, 1); + assert.strictEqual(editable[0].args?.[1], path.join(projectRoot, 'z-valid')); + assert.strictEqual(result.validationError, undefined, 'ignored duplicates should not surface validation errors'); + }); + + test('reports validation from the preferred candidate instead of substituting another worktree', async () => { + const invalidToml = await writeTomlContent( + 'main', + tomlFor('myapp').replace('version = "0.1.0"', 'version = "not a version"'), + ); + const validToml = await writeToml('copilot', 'myapp'); + + findFilesStub.callsFake((pattern: string) => { + if (pattern === '**/pyproject.toml') { + return Promise.resolve([Uri.file(validToml), Uri.file(invalidToml)]); + } + return Promise.resolve([]); + }); + + const projects = [{ name: 'myapp', uri: Uri.file(projectRoot) }]; + const result = await getProjectInstallable( + mockApi as PythonEnvironmentApi, + projects, + automaticOptions(path.join(projectRoot, 'main')), + ); + + const editable = result.installables.filter((item) => item.args?.[0] === '-e'); + assert.strictEqual(editable.length, 1); + assert.strictEqual(editable[0].args?.[1], path.join(projectRoot, 'main')); + assert.strictEqual(result.validationError?.fileUri.fsPath, invalidToml); + }); + + test('does not let a metadata-only TOML with optional dependencies suppress an installable duplicate', async () => { + const metadataToml = await writeTomlContent( + 'a-metadata', + [ + '[project]', + 'name = "myapp"', + 'version = "0.1.0"', + '', + '[project.optional-dependencies]', + 'dev = ["pytest"]', + '', + ].join('\n'), + ); + const installableToml = await writeToml('z-installable', 'myapp'); + + findFilesStub.callsFake((pattern: string) => { + if (pattern === '**/pyproject.toml') { + return Promise.resolve([Uri.file(metadataToml), Uri.file(installableToml)]); + } + return Promise.resolve([]); + }); + + const projects = [{ name: 'myapp', uri: Uri.file(projectRoot) }]; + const result = ( + await getProjectInstallable( + mockApi as PythonEnvironmentApi, + projects, + automaticOptions(path.join(projectRoot, 'z-installable')), + ) + ).installables; + + const editable = result.filter((item) => item.args?.[0] === '-e'); + assert.strictEqual(editable.length, 1); + assert.strictEqual(editable[0].args?.[1], path.join(projectRoot, 'z-installable')); + }); + + test('prefers metadata that emits optional dependencies over an empty peer', async () => { + const emptyToml = await writeTomlContent( + 'a-empty', + ['[project]', 'name = "myapp"', 'version = "0.1.0"', ''].join('\n'), + ); + const optionalToml = await writeTomlContent( + 'z-optional', + [ + '[project]', + 'name = "myapp"', + 'version = "0.1.0"', + '', + '[project.optional-dependencies]', + 'dev = ["pytest"]', + '', + ].join('\n'), + ); + + findFilesStub.callsFake((pattern: string) => { + if (pattern === '**/pyproject.toml') { + return Promise.resolve([Uri.file(emptyToml), Uri.file(optionalToml)]); + } + return Promise.resolve([]); + }); + + const projects = [{ name: 'myapp', uri: Uri.file(projectRoot) }]; + const result = ( + await getProjectInstallable(mockApi as PythonEnvironmentApi, projects, automaticOptions()) + ).installables; + + const editable = result.filter((item) => item.args?.[0] === '-e'); + assert.strictEqual(editable.length, 1); + assert.strictEqual(editable[0].args?.[1], `${path.join(projectRoot, 'z-optional')}[dev]`); + }); + + test('prefers the containing worktree regardless of discovery order or path characters', async () => { + const zToml = await writeToml('z-worktree', 'myapp'); + const accentedToml = await writeToml('รค-worktree', 'myapp'); + + findFilesStub.callsFake((pattern: string) => { + if (pattern === '**/pyproject.toml') { + return Promise.resolve([Uri.file(accentedToml), Uri.file(zToml)]); + } + return Promise.resolve([]); + }); + + const projects = [{ name: 'myapp', uri: Uri.file(projectRoot) }]; + const result = ( + await getProjectInstallable( + mockApi as PythonEnvironmentApi, + projects, + automaticOptions(path.join(projectRoot, 'z-worktree')), + ) + ).installables; + + const editable = result.filter((item) => item.args?.[0] === '-e'); + assert.strictEqual(editable.length, 1); + assert.strictEqual(editable[0].args?.[1], path.join(projectRoot, 'z-worktree')); + }); + + test('keeps distinct packages in a monorepo (different names are not de-duplicated)', async () => { + const pkgAToml = await writeToml(path.join('packages', 'a'), 'pkg-a'); + const pkgBToml = await writeToml(path.join('packages', 'b'), 'pkg-b'); + + findFilesStub.callsFake((pattern: string) => { + if (pattern === '**/pyproject.toml') { + return Promise.resolve([Uri.file(pkgAToml), Uri.file(pkgBToml)]); + } + return Promise.resolve([]); + }); + + const projects = [{ name: 'myapp', uri: Uri.file(projectRoot) }]; + const result = ( + await getProjectInstallable(mockApi as PythonEnvironmentApi, projects, automaticOptions()) + ).installables; + + const editableDirs = result.filter((r) => r.args?.[0] === '-e').map((r) => r.args?.[1]); + assert.strictEqual(editableDirs.length, 2, 'Distinct package names should both be kept'); + assert.deepStrictEqual( + editableDirs.sort(), + [path.join(projectRoot, 'packages', 'a'), path.join(projectRoot, 'packages', 'b')].sort(), + ); + }); + + test('does not de-duplicate requirements.txt files by name', async () => { + // requirements files are not project packages; identical basenames in + // different folders must all be preserved. + const rootReq = path.join(projectRoot, 'requirements.txt'); + const subReq = path.join(projectRoot, 'subdir', 'requirements.txt'); + + findFilesStub.callsFake((pattern: string) => { + if (pattern === '**/*requirements*.txt') { + return Promise.resolve([Uri.file(subReq)]); + } + if (pattern === '*requirements*.txt') { + return Promise.resolve([Uri.file(rootReq)]); + } + return Promise.resolve([]); + }); + + const projects = [{ name: 'myapp', uri: Uri.file(projectRoot) }]; + const result = ( + await getProjectInstallable(mockApi as PythonEnvironmentApi, projects, automaticOptions()) + ).installables; + + const reqs = result.filter((r) => r.args?.[0] === '-r'); + assert.strictEqual(reqs.length, 2, 'Both requirements.txt files should be preserved'); + }); + + test('keeps same-named packages from separate projects in the manual picker', async () => { + const firstProjectRoot = path.join(projectRoot, 'first'); + const secondProjectRoot = path.join(projectRoot, 'second'); + const firstToml = await writeToml('first', 'shared-name'); + const secondToml = await writeToml('second', 'shared-name'); + mockApi.getPythonProject = (uri: Uri) => { + if (uri.fsPath.startsWith(firstProjectRoot)) { + return { name: 'first', uri: Uri.file(firstProjectRoot) }; + } + if (uri.fsPath.startsWith(secondProjectRoot)) { + return { name: 'second', uri: Uri.file(secondProjectRoot) }; + } + return undefined; + }; + findFilesStub.callsFake((pattern: string) => { + if (pattern === '**/pyproject.toml') { + return Promise.resolve([Uri.file(firstToml), Uri.file(secondToml)]); + } + return Promise.resolve([]); + }); + + const projects = [ + { name: 'first', uri: Uri.file(firstProjectRoot) }, + { name: 'second', uri: Uri.file(secondProjectRoot) }, + ]; + const result = (await getProjectInstallable(mockApi as PythonEnvironmentApi, projects)).installables; + + assert.strictEqual(result.filter((item) => item.args?.[0] === '-e').length, 2); + }); +});