From 338bbb02ff719b77072f2dfc6b2334290aec9d86 Mon Sep 17 00:00:00 2001 From: Eduardo Villalpando Mello Date: Fri, 25 Sep 2026 00:22:51 -0700 Subject: [PATCH 01/11] feat: add project-scoped package managers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/CHANGELOG.md | 6 + api/package-lock.json | 4 +- api/package.json | 2 +- docs/README.md | 1 + src/extensionApi.ts | 8 +- src/features/envCommands.ts | 6 +- src/features/envManagers.ts | 105 +++++++++++++++--- src/managers/common/packageWatcher.ts | 4 +- src/managers/common/registeredManagers.ts | 27 ++++- src/managers/poetry/poetryPackageManager.ts | 69 +++++++----- src/test/extensionApi.unit.test.ts | 7 +- .../envManagers.packageEvents.unit.test.ts | 55 ++++++++- .../features/packageManager.api.unit.test.ts | 71 +++++++++++- ...ageManagerHeadlessConformance.unit.test.ts | 5 +- .../common/packageWatcher.unit.test.ts | 48 +++++++- .../poetry/poetryPackageManager.unit.test.ts | 57 +++++++--- src/types.ts | 10 ++ 17 files changed, 403 insertions(+), 82 deletions(-) diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index 35ad498f7..11bfb4b81 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to the `@vscode/python-environments` API package are documen The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.5.0] + +### Added + +- Added the optional `PackageManager.createForProject` factory for package managers whose operations depend on the calling Python project. + ## [1.4.0] ### Changed diff --git a/api/package-lock.json b/api/package-lock.json index 4c8908c6b..62fda10a8 100644 --- a/api/package-lock.json +++ b/api/package-lock.json @@ -1,12 +1,12 @@ { "name": "@vscode/python-environments", - "version": "1.4.0", + "version": "1.5.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@vscode/python-environments", - "version": "1.4.0", + "version": "1.5.0", "license": "MIT", "dependencies": { "@renovatebot/pep440": "^3.1.0" diff --git a/api/package.json b/api/package.json index 84bb1e735..ada2db299 100644 --- a/api/package.json +++ b/api/package.json @@ -1,7 +1,7 @@ { "name": "@vscode/python-environments", "description": "An API facade for the Python Environments extension in VS Code", - "version": "1.4.0", + "version": "1.5.0", "author": { "name": "Microsoft Corporation" }, diff --git a/docs/README.md b/docs/README.md index 0d4c1e862..1569431ca 100644 --- a/docs/README.md +++ b/docs/README.md @@ -1613,6 +1613,7 @@ Reports and changes the packages of an environment. | `refresh(environment)` | `(environment: PythonEnvironment) => Promise` | Yes | Re-reads the installed package list. | | `getPackages(environment, options?)` | `(environment: PythonEnvironment, options?: GetPackagesOptions) => Promise` | Yes | Returns installed packages, or `undefined` if they cannot be retrieved. | | `getPackageWatchTargets(environment)` | `(environment: PythonEnvironment) => RelativePattern[]` | No | Extra filesystem patterns to watch for install and uninstall changes, appended to the default site-packages locations. Implement for manager-specific locations such as `conda-meta`. | +| `createForProject(project)` | `(project: PythonProject) => PackageManager` | No | Creates a manager bound to a project for project-sensitive operations. | | `getDirectPackageNames(environment)` | `(environment: PythonEnvironment) => Promise \| undefined>` | No | Best-effort set of non-transitive package names. Most tools cannot record user intent - pip uses `pip list --not-required`, which reports leaf packages rather than explicitly installed ones. | | `clearCache()` | `() => Promise` | No | Drops cached package data. | | `getVersion(environment)` | `(environment: PythonEnvironment) => Promise` | No | Version of the underlying tool, such as pip, uv, or conda. | diff --git a/src/extensionApi.ts b/src/extensionApi.ts index 1643e40a6..337322ed1 100644 --- a/src/extensionApi.ts +++ b/src/extensionApi.ts @@ -86,6 +86,7 @@ export class PythonEnvironmentApiImpl implements PythonEnvironmentApi { this._onDidChangePythonProjects, this._onDidChangePackages, this._onDidChangeEnvironmentVariables, + this.envManagers.onDidChangePackageProviderPackages((e) => this._onDidChangePackages.fire(e)), this.envManagers.onDidChangeActiveEnvironment((e) => { this._onDidChangeEnvironment.fire(e); const location = e.uri?.fsPath ?? 'global'; @@ -295,12 +296,7 @@ export class PythonEnvironmentApiImpl implements PythonEnvironmentApi { } registerPackageManager(manager: PackageManager, options?: { extensionId?: string }): Disposable { - const disposables: Disposable[] = []; - disposables.push(this.envManagers.registerPackageManager(manager, options)); - if (manager.onDidChangePackages) { - disposables.push(manager.onDidChangePackages((e) => this._onDidChangePackages.fire(e))); - } - return new Disposable(() => disposables.forEach((d) => d.dispose())); + return this.envManagers.registerPackageManager(manager, options); } async managePackages(context: PythonEnvironment, options: PackageManagementOptions): Promise { await waitForEnvManagerId([context.envId.managerId]); diff --git a/src/features/envCommands.ts b/src/features/envCommands.ts index 7575ecf89..4d16cb7ee 100644 --- a/src/features/envCommands.ts +++ b/src/features/envCommands.ts @@ -343,7 +343,8 @@ export async function handlePackageUninstall(context: unknown, em: EnvironmentMa } const moduleName = context.pkg.name; const environment = context.parent.environment; - const packageManager = em.getPackageManager(environment); + const packageManager = + context instanceof ProjectPackage ? context.manager : em.getPackageManager(environment); await packageManager?.manage(environment, { uninstall: [moduleName], install: [] }); return; } @@ -358,7 +359,8 @@ export async function managePackageVersion(context: unknown, em: EnvironmentMana if (context instanceof PackageTreeItem || context instanceof ProjectPackage) { const pkg = context.pkg; const environment = context.parent.environment; - const packageManager = em.getPackageManager(environment); + const packageManager = + context instanceof ProjectPackage ? context.manager : em.getPackageManager(environment); if (!packageManager) { return; diff --git a/src/features/envManagers.ts b/src/features/envManagers.ts index 4127aa54b..b882e8021 100644 --- a/src/features/envManagers.ts +++ b/src/features/envManagers.ts @@ -111,6 +111,7 @@ export interface EnvironmentManagers extends Disposable { */ onDidChangeActiveEnvironment: Event; onDidChangePackages: Event; + onDidChangePackageProviderPackages: Event; onDidChangeEnvironmentManager: Event; onDidChangePackageManager: Event; @@ -171,6 +172,11 @@ function generateId(name: string, extensionId?: string): string { export class PythonEnvironmentManagers implements EnvironmentManagers { private _environmentManagers: Map = new Map(); private _packageManagers: Map = new Map(); + private _projectPackageManagers: Map = new Map(); + private readonly _packageManagerEventSubscriptions = new Map< + string, + Map, Disposable> + >(); private readonly subscriptions: Disposable[] = []; /** @@ -198,6 +204,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { /** Fires when the active (selected) environment for a scope actually changes. */ private _onDidChangeActiveEnvironment = new EventEmitter(); private _onDidChangePackages = new EventEmitter(); + private _onDidChangePackageProviderPackages = new EventEmitter(); public onDidChangeEnvironmentManager: Event = this._onDidChangeEnvironmentManager.event; @@ -208,6 +215,8 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { public onDidChangeManagerEnvironment: Event = this._onDidChangeManagerEnvironment.event; public onDidChangePackages: Event = this._onDidChangePackages.event; + public onDidChangePackageProviderPackages: Event = + this._onDidChangePackageProviderPackages.event; /** Fires only when the *selected* manager's environment for a scope actually changes. */ public onDidChangeActiveEnvironment: Event = @@ -301,20 +310,9 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { traceError(ex); throw ex; } - const disposables: Disposable[] = []; const mgr = new InternalPackageManager(managerId, manager); - disposables.push( - mgr.onDidChangePackages((e: DidChangePackagesEventArgs) => { - setImmediate(() => - this._onDidChangePackages.fire({ - environment: e.environment, - manager: mgr, - changes: e.changes, - }), - ); - }), - ); + this.subscribeToPackageManagerEvents(mgr, mgr); this._packageManagers.set(managerId, mgr); this._onDidChangePackageManager.fire({ kind: 'registered', manager: mgr }); @@ -327,7 +325,12 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { return new Disposable(() => { this._packageManagers.delete(managerId); - disposables.forEach((d) => d.dispose()); + for (const [key, scopedManager] of this._projectPackageManagers) { + if (scopedManager.id === managerId) { + this._projectPackageManagers.delete(key); + } + } + this.disposePackageManagerEventSubscriptions(managerId); setImmediate(() => this._onDidChangePackageManager.fire({ kind: 'unregistered', manager: mgr })); }); } @@ -335,6 +338,10 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { public dispose() { this._environmentManagers.clear(); this._packageManagers.clear(); + this._projectPackageManagers.clear(); + for (const managerId of this._packageManagerEventSubscriptions.keys()) { + this.disposePackageManagerEventSubscriptions(managerId); + } this._inlineRoutingOverrides.clear(); this.subscriptions.forEach((subscription) => subscription.dispose()); this._onDidChangeEnvironmentManager.dispose(); @@ -343,6 +350,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { this._onDidChangeManagerEnvironment.dispose(); this._onDidChangeActiveEnvironment.dispose(); this._onDidChangePackages.dispose(); + this._onDidChangePackageProviderPackages.dispose(); } /** @@ -407,17 +415,21 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { } if (context === undefined || context instanceof Uri) { + const project = context ? this.pm.get(context) : undefined; const defaultPkgManagerId = getDefaultPkgManagerSetting(this.pm, context); const defaultEnvManagerId = getDefaultEnvManagerSetting(this.pm, context); if (defaultPkgManagerId) { - return this._packageManagers.get(defaultPkgManagerId); + return this.getProjectPackageManager(this._packageManagers.get(defaultPkgManagerId), project); } if (defaultEnvManagerId) { const preferredPkgManagerId = this._environmentManagers.get(defaultEnvManagerId)?.preferredPackageManagerId; if (preferredPkgManagerId) { - return this._packageManagers.get(preferredPkgManagerId); + return this.getProjectPackageManager( + this._packageManagers.get(preferredPkgManagerId), + project, + ); } } return undefined; @@ -439,6 +451,69 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { return undefined; } + private getProjectPackageManager( + manager: InternalPackageManager | undefined, + project: PythonProject | undefined, + ): InternalPackageManager | undefined { + if (!manager || !project) { + return manager; + } + + const key = `${manager.id}:${normalizePath(project.uri.fsPath)}`; + let scopedManager = this._projectPackageManagers.get(key); + if (!scopedManager) { + scopedManager = manager.createProjectScopedManager(project); + if (scopedManager) { + this._projectPackageManagers.set(key, scopedManager); + this.subscribeToPackageManagerEvents(manager, scopedManager); + } + } + return scopedManager ?? manager; + } + + private subscribeToPackageManagerEvents( + provider: InternalPackageManager, + manager: InternalPackageManager, + ): void { + const event = manager.packageChangeEvent; + if (!event) { + return; + } + + let subscriptions = this._packageManagerEventSubscriptions.get(provider.id); + if (!subscriptions) { + subscriptions = new Map(); + this._packageManagerEventSubscriptions.set(provider.id, subscriptions); + } + if (subscriptions.has(event)) { + return; + } + + subscriptions.set( + event, + event((e) => { + this._onDidChangePackageProviderPackages.fire(e); + const eventManager = + Array.from(this._projectPackageManagers.values()).find( + (candidate) => candidate.id === provider.id && candidate.wraps(e.manager), + ) ?? provider; + setImmediate(() => + this._onDidChangePackages.fire({ + environment: e.environment, + manager: eventManager, + changes: e.changes, + }), + ); + }), + ); + } + + private disposePackageManagerEventSubscriptions(managerId: string): void { + const subscriptions = this._packageManagerEventSubscriptions.get(managerId); + subscriptions?.forEach((subscription) => subscription.dispose()); + this._packageManagerEventSubscriptions.delete(managerId); + } + public get managers(): InternalEnvironmentManager[] { return Array.from(this._environmentManagers.values()); } diff --git a/src/managers/common/packageWatcher.ts b/src/managers/common/packageWatcher.ts index f6a3e8778..a0b79e881 100644 --- a/src/managers/common/packageWatcher.ts +++ b/src/managers/common/packageWatcher.ts @@ -153,7 +153,9 @@ export function registerPackageWatchers( return; } - const watcherKey = `${environment.envId.managerId}:${environment.envId.id}:${selectedPackageManager.id}`; + const packageManagerKey = + `${selectedPackageManager.id}:${selectedPackageManager.project?.uri.toString() ?? ''}`; + const watcherKey = `${environment.envId.managerId}:${environment.envId.id}:${packageManagerKey}`; if (activeWatcherByConsumer.get(consumer) === watcherKey) { return; } diff --git a/src/managers/common/registeredManagers.ts b/src/managers/common/registeredManagers.ts index 5daaff64d..498563d07 100644 --- a/src/managers/common/registeredManagers.ts +++ b/src/managers/common/registeredManagers.ts @@ -2,7 +2,7 @@ // Licensed under the MIT License. import type { Pep440Version } from '@renovatebot/pep440'; -import { CancellationError, Disposable, LogOutputChannel, MarkdownString, RelativePattern } from 'vscode'; +import { CancellationError, Disposable, Event, LogOutputChannel, MarkdownString, RelativePattern } from 'vscode'; import { PackageVersionLookupNotSupportedError } from '../../publicErrors'; import { ISSUES_URL } from '../../common/constants'; import { CreateEnvironmentNotSupported, RemoveEnvironmentNotSupported } from '../../common/errors/NotSupportedError'; @@ -27,6 +27,7 @@ import type { PackageManagementOptions, PackageManager, PythonEnvironment, + PythonProject, QuickCreateConfig, RefreshEnvironmentsScope, RemoveEnvironmentOptions, @@ -210,10 +211,17 @@ function inferPackageManagementTrigger( } export class InternalPackageManager implements PackageManager { + private readonly relatedManagers: WeakSet; + public constructor( public readonly id: string, private readonly manager: PackageManager, - ) {} + public readonly project?: PythonProject, + relatedManagers?: WeakSet, + ) { + this.relatedManagers = relatedManagers ?? new WeakSet(); + this.relatedManagers.add(manager); + } public get name(): string { return this.manager.name; @@ -279,10 +287,23 @@ export class InternalPackageManager implements PackageManager { return this.manager.onDidChangePackages ? this.manager.onDidChangePackages(handler) : new Disposable(() => {}); } - equals(other: PackageManager): boolean { + get packageChangeEvent(): Event | undefined { + return this.manager.onDidChangePackages; + } + + wraps(other: PackageManager): boolean { return this.manager === other; } + equals(other: PackageManager): boolean { + return this.relatedManagers.has(other); + } + + createProjectScopedManager(project: PythonProject): InternalPackageManager | undefined { + const manager = this.manager.createForProject?.(project); + return manager ? new InternalPackageManager(this.id, manager, project, this.relatedManagers) : undefined; + } + getVersion(environment: PythonEnvironment): Promise { return this.manager.getVersion ? this.manager.getVersion(environment) : Promise.resolve(undefined); } diff --git a/src/managers/poetry/poetryPackageManager.ts b/src/managers/poetry/poetryPackageManager.ts index 430c8ac6e..7cc5c6f20 100644 --- a/src/managers/poetry/poetryPackageManager.ts +++ b/src/managers/poetry/poetryPackageManager.ts @@ -23,6 +23,7 @@ import { PackageVersionLookupNotSupportedError, PythonEnvironment, PythonEnvironmentApi, + PythonProject, } from '../../api'; import { showErrorMessage, showInputBox, withProgress } from '../../common/window.apis'; import { updatePackagesAndNotify } from '../common/packageChanges'; @@ -38,15 +39,16 @@ import { PoetryManager } from './poetryManager'; import { getPoetry } from './poetryUtils'; export class PoetryPackageManager implements PackageManager, Disposable { - private readonly _onDidChangePackages = new EventEmitter(); - onDidChangePackages: Event = this._onDidChangePackages.event; + private readonly packagesChangedEmitter = new EventEmitter(); + readonly onDidChangePackages: Event = this.packagesChangedEmitter.event; private packages: Map = new Map(); constructor( private readonly api: PythonEnvironmentApi, public readonly log: LogOutputChannel, - _poetry: PoetryManager, + private readonly poetryManager: PoetryManager, + private readonly project?: PythonProject, ) { this.name = 'poetry'; this.displayName = 'Poetry'; @@ -60,6 +62,21 @@ export class PoetryPackageManager implements PackageManager, Disposable { readonly tooltip?: string | MarkdownString; readonly iconPath?: IconPath; + /** + * Creates a Poetry package manager bound to a Python project. + * + * @param project The project whose working directory Poetry commands should use. + * @returns A Poetry package manager scoped to the project. + */ + createForProject(project: PythonProject): PoetryPackageManager { + return new PoetryPackageManager( + this.api, + this.log, + this.poetryManager, + project, + ); + } + async manage(environment: PythonEnvironment, options: PackageManagementOptions): Promise { let toInstall: string[] = [...(options.install ?? [])]; let toUninstall: string[] = [...(options.uninstall ?? [])]; @@ -87,15 +104,16 @@ export class PoetryPackageManager implements PackageManager, Disposable { } } + const cwd = await this.getProjectCwd(); const execute = async (token?: CancellationToken): Promise => { try { - await this.runPoetryManage({ install: toInstall, uninstall: toUninstall }, token); + await this.runPoetryManage({ install: toInstall, uninstall: toUninstall }, cwd, token); await updatePackagesAndNotify( this, environment, this.packages.get(environment.envId.id), (changes) => { - this._onDidChangePackages.fire({ environment, manager: this, changes }); + this.packagesChangedEmitter.fire({ environment, manager: this, changes }); }, ); } catch (e) { @@ -131,6 +149,7 @@ export class PoetryPackageManager implements PackageManager, Disposable { } async refresh(environment: PythonEnvironment): Promise { + await this.getProjectCwd(); await withProgress( { location: ProgressLocation.Window, @@ -143,7 +162,7 @@ export class PoetryPackageManager implements PackageManager, Disposable { environment, this.packages.get(environment.envId.id), (changes) => { - this._onDidChangePackages.fire({ environment, manager: this, changes }); + this.packagesChangedEmitter.fire({ environment, manager: this, changes }); }, ); this.packages.set(environment.envId.id, packages ?? []); @@ -162,6 +181,9 @@ export class PoetryPackageManager implements PackageManager, Disposable { } async getPackages(environment: PythonEnvironment, options?: GetPackagesOptions): Promise { + if (!this.project) { + return undefined; + } if (options?.skipCache || !this.packages.has(environment.envId.id)) { const packages = await this.fetchPackagesFromTool(environment); this.packages.set(environment.envId.id, packages); @@ -197,12 +219,13 @@ export class PoetryPackageManager implements PackageManager, Disposable { } dispose(): void { - this._onDidChangePackages.dispose(); + this.packagesChangedEmitter.dispose(); this.packages.clear(); } private async runPoetryManage( options: { install?: string[]; uninstall?: string[] }, + cwd: string, token?: CancellationToken, ): Promise { const poetry = await getPoetry(); @@ -217,6 +240,7 @@ export class PoetryPackageManager implements PackageManager, Disposable { if (options.uninstall && options.uninstall.length > 0) { const removeCmd = new PoetryRemoveCommand({ pythonExecutable: poetry, + cwd, log: this.log, }); const packages = parsePackageSpecs(options.uninstall); @@ -227,6 +251,7 @@ export class PoetryPackageManager implements PackageManager, Disposable { if (options.install && options.install.length > 0) { const addCmd = new PoetryAddCommand({ pythonExecutable: poetry, + cwd, log: this.log, }); const packages = parsePackageSpecs(options.install); @@ -244,7 +269,7 @@ export class PoetryPackageManager implements PackageManager, Disposable { ); } - const cwd = await this.getPoetryCwd(environment); + const cwd = await this.getProjectCwd(); const showCmd = new PoetryShowCommand({ pythonExecutable: poetry, cwd, @@ -260,6 +285,9 @@ export class PoetryPackageManager implements PackageManager, Disposable { } async getDirectPackageNames(_environment: PythonEnvironment): Promise | undefined> { + if (!this.project) { + return undefined; + } try { const poetry = await getPoetry(); if (!poetry) { @@ -267,6 +295,7 @@ export class PoetryPackageManager implements PackageManager, Disposable { } const showTopLevelCmd = new PoetryShowTopLevelCommand({ pythonExecutable: poetry, + cwd: await this.getProjectCwd(), log: this.log, }); return await showTopLevelCmd.execute(); @@ -276,12 +305,10 @@ export class PoetryPackageManager implements PackageManager, Disposable { } } - private async getPoetryCwd(environment: PythonEnvironment): Promise { - const projects = this.api.getPythonProjects(); - if (projects.length === 0) { - return undefined; + private async getProjectCwd(): Promise { + if (!this.project) { + throw new Error(l10n.t('Poetry package operations require a Python project.')); } - const toDirectory = async (fsPath: string): Promise => { try { const stat = await fsapi.stat(fsPath); @@ -291,20 +318,6 @@ export class PoetryPackageManager implements PackageManager, Disposable { } }; - if (projects.length === 1) { - return toDirectory(projects[0].uri.fsPath); - } - - const matchingDirectories = new Set(); - await Promise.all( - projects.map(async (project) => { - const projectEnvironment = await this.api.getEnvironment(project.uri); - if (projectEnvironment?.envId.id === environment.envId.id) { - matchingDirectories.add(await toDirectory(project.uri.fsPath)); - } - }), - ); - - return Array.from(matchingDirectories).sort((a, b) => b.length - a.length)[0]; + return toDirectory(this.project.uri.fsPath); } } diff --git a/src/test/extensionApi.unit.test.ts b/src/test/extensionApi.unit.test.ts index d49bc79c3..b34a79e6f 100644 --- a/src/test/extensionApi.unit.test.ts +++ b/src/test/extensionApi.unit.test.ts @@ -16,7 +16,10 @@ suite('PythonEnvironmentApiImpl - onDidChangePythonProjects', () => { } as unknown as PythonProjectManager; type ApiArgs = ConstructorParameters; - const mockEnvManagers = { onDidChangeActiveEnvironment: new EventEmitter().event } as unknown as ApiArgs[0]; + const mockEnvManagers = { + onDidChangeActiveEnvironment: new EventEmitter().event, + onDidChangePackageProviderPackages: new EventEmitter().event, + } as unknown as ApiArgs[0]; const mockProjectCreators = {} as unknown as ApiArgs[2]; const mockTerminalManager = {} as unknown as ApiArgs[3]; const mockEnvVarManager = { onDidChangeEnvironmentVariables: new EventEmitter().event } as unknown as ApiArgs[4]; @@ -91,6 +94,7 @@ suite('PythonEnvironmentApiImpl - getEnvironment timeout fallback', () => { type ApiArgs = ConstructorParameters; const mockEnvManagers = { onDidChangeActiveEnvironment: new EventEmitter().event, + onDidChangePackageProviderPackages: new EventEmitter().event, getEnvironment: sinon.stub().returns( new Promise((resolve) => { resolveEnvironment = resolve; @@ -134,6 +138,7 @@ suite('PythonEnvironmentApiImpl - getEnvironment timeout fallback', () => { type ApiArgs = ConstructorParameters; const mockEnvManagers = { onDidChangeActiveEnvironment: new EventEmitter().event, + onDidChangePackageProviderPackages: new EventEmitter().event, getEnvironment: sinon.stub().returns( new Promise((resolve) => { resolveEnvironment = resolve; diff --git a/src/test/features/envManagers.packageEvents.unit.test.ts b/src/test/features/envManagers.packageEvents.unit.test.ts index 7694a6da2..78ab7eaa3 100644 --- a/src/test/features/envManagers.packageEvents.unit.test.ts +++ b/src/test/features/envManagers.packageEvents.unit.test.ts @@ -4,7 +4,7 @@ import assert from 'assert'; import * as path from 'path'; import * as sinon from 'sinon'; -import { Disposable, EventEmitter } from 'vscode'; +import { Disposable, EventEmitter, Uri, WorkspaceConfiguration } from 'vscode'; import { DidChangeEnvironmentVariablesEventArgs, DidChangePackagesEventArgs, @@ -15,6 +15,7 @@ import { import { InlineScriptRoutingRegistry } from '../../common/inlineScript/routingRegistry'; import * as telemetry from '../../common/telemetry/sender'; import * as frameUtils from '../../common/utils/frameUtils'; +import * as workspaceApis from '../../common/workspace.apis'; import { PythonEnvironmentManagers } from '../../features/envManagers'; import { PythonEnvironmentApiImpl } from '../../extensionApi'; import type { InternalDidChangePackagesEventArgs } from '../../features/envManagers'; @@ -31,7 +32,10 @@ for (const inlineEnabled of [false, true]) { let managers: PythonEnvironmentManagers; let api: PythonEnvironmentApiImpl; let provider: PackageManager; + let scopedProvider: PackageManager | undefined; let emitter: EventEmitter; + let scopedEmitter: EventEmitter; + let project: PythonProject; let routing: InlineScriptRoutingRegistry | undefined; let disposables: Disposable[]; let internalEvents: InternalDidChangePackagesEventArgs[]; @@ -45,10 +49,12 @@ for (const inlineEnabled of [false, true]) { disposables = []; internalEvents = []; publicEvents = []; + project = { name: 'project', uri: Uri.file(path.join(process.cwd(), 'project')) }; const projectEvents = new EventEmitter(); const variableEvents = new EventEmitter(); const projectManager: Partial = { getProjects: () => [], + get: () => project, onDidChangeProjects: projectEvents.event, }; routing = inlineEnabled ? new InlineScriptRoutingRegistry() : undefined; @@ -60,15 +66,30 @@ for (const inlineEnabled of [false, true]) { {} as ApiArgs[2], {} as ApiArgs[3], variables as ApiArgs[4], disposables, ); emitter = new EventEmitter(); + scopedEmitter = new EventEmitter(); provider = { name: 'custom', manage: async () => undefined, refresh: async () => undefined, getPackages: async () => [], onDidChangePackages: emitter.event, + createForProject: () => { + scopedProvider = { + name: 'custom', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + onDidChangePackages: scopedEmitter.event, + }; + return scopedProvider; + }, }; + sinon.stub(workspaceApis, 'getConfiguration').returns({ + get: (section: string, defaultValue?: unknown) => + section === 'defaultPackageManager' ? 'test.packages:custom' : defaultValue, + } as WorkspaceConfiguration); disposables.push( - projectEvents, variableEvents, emitter, + projectEvents, variableEvents, emitter, scopedEmitter, api.registerPackageManager(provider, { extensionId: 'test.packages' }), managers.onDidChangePackages((event) => internalEvents.push(event)), api.onDidChangePackages((event) => publicEvents.push(event)), @@ -124,5 +145,35 @@ for (const inlineEnabled of [false, true]) { verifyForwarding(event); }); + + test('forwards events from a scoped manager with an independent event emitter', () => { + const scopedManager = managers.getPackageManager(project.uri); + assert.ok(scopedManager); + assert.ok(scopedProvider); + + const scopedChanges: DidChangePackagesEventArgs['changes'] = [{ + kind: PackageChangeKind.add, + pkg: api.createPackageItem( + { name: 'scoped', displayName: 'scoped', version: '1.0' }, + environment, + scopedProvider, + ), + }]; + const event: DidChangePackagesEventArgs = { + environment, + manager: scopedProvider, + changes: scopedChanges, + }; + + scopedEmitter.fire(event); + clock.runAll(); + + assert.strictEqual(publicEvents.length, 1); + assert.strictEqual(publicEvents[0], event); + assert.strictEqual(internalEvents.length, 1); + assert.strictEqual(internalEvents[0].manager, scopedManager); + assert.strictEqual(internalEvents[0].changes, scopedChanges); + assert.strictEqual(scopedChanges[0].pkg.pkgId.managerId, scopedManager.id); + }); }); } diff --git a/src/test/features/packageManager.api.unit.test.ts b/src/test/features/packageManager.api.unit.test.ts index 40902c350..e4f549d75 100644 --- a/src/test/features/packageManager.api.unit.test.ts +++ b/src/test/features/packageManager.api.unit.test.ts @@ -14,9 +14,10 @@ import { Extension } from 'vscode'; import * as assert from 'assert'; +import * as path from 'path'; import * as sinon from 'sinon'; import * as typeMoq from 'typemoq'; -import { Disposable, EventEmitter, Uri } from 'vscode'; +import { Disposable, EventEmitter, Uri, WorkspaceConfiguration } from 'vscode'; import { DidChangeEnvironmentEventArgs, DidChangeEnvironmentsEventArgs, @@ -26,8 +27,10 @@ import { PackageManagementOptions, PackageManager, PythonEnvironment, + PythonProject, } from '../../api'; import * as extensionApis from '../../common/extension.apis'; +import * as workspaceApis from '../../common/workspace.apis'; import { PythonEnvironmentManagers } from '../../features/envManagers'; import type { PythonProjectManager } from '../../features/projectManager'; import { setupNonThenable } from '../mocks/helper'; @@ -737,5 +740,71 @@ suite('PythonPackageManagerApi Tests', () => { // Assert assert.strictEqual(manager, undefined, 'Should return undefined for non-existent ID'); }); + + test('Should cache project-bound package managers by project', () => { + disposable.dispose(); + const firstProject = { + name: 'first', + uri: Uri.file(path.join(process.cwd(), 'first-project')), + } as PythonProject; + const secondProject = { + name: 'second', + uri: Uri.file(path.join(process.cwd(), 'second-project')), + } as PythonProject; + projectManager.setup((pm) => pm.get(firstProject.uri)).returns(() => firstProject); + projectManager.setup((pm) => pm.get(secondProject.uri)).returns(() => secondProject); + + const scopedManagers: PackageManager[] = []; + const scopedProvider: PackageManager = { + name: 'project-pkg-mgr', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + createForProject: () => { + const scopedManager: PackageManager = { + name: 'project-pkg-mgr', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + }; + scopedManagers.push(scopedManager); + return scopedManager; + }, + }; + disposable = envManagers.registerPackageManager(scopedProvider); + const registeredManager = envManagers.packageManagers[0]; + sinon.stub(workspaceApis, 'getConfiguration').returns({ + get: (section: string, defaultValue?: unknown) => + section === 'defaultPackageManager' ? registeredManager.id : defaultValue, + } as WorkspaceConfiguration); + + const first = envManagers.getPackageManager(firstProject.uri); + const repeatedFirst = envManagers.getPackageManager(firstProject.uri); + const second = envManagers.getPackageManager(secondProject.uri); + + assert.strictEqual(first, repeatedFirst); + assert.notStrictEqual(first, second); + assert.strictEqual(scopedManagers.length, 2); + assert.strictEqual(first?.project, firstProject); + assert.strictEqual(second?.project, secondProject); + assert.ok(registeredManager.equals(scopedManagers[0])); + assert.ok(registeredManager.equals(scopedManagers[1])); + assert.strictEqual(envManagers.packageManagers.length, 1); + }); + + test('Should share a project-independent package manager across projects', () => { + const project = { + name: 'project', + uri: Uri.file(path.join(process.cwd(), 'project')), + } as PythonProject; + projectManager.setup((pm) => pm.get(project.uri)).returns(() => project); + const registeredManager = envManagers.packageManagers[0]; + sinon.stub(workspaceApis, 'getConfiguration').returns({ + get: (section: string, defaultValue?: unknown) => + section === 'defaultPackageManager' ? registeredManager.id : defaultValue, + } as WorkspaceConfiguration); + + assert.strictEqual(envManagers.getPackageManager(project.uri), registeredManager); + }); }); }); diff --git a/src/test/managers/common/packageManagerHeadlessConformance.unit.test.ts b/src/test/managers/common/packageManagerHeadlessConformance.unit.test.ts index 9e65ae415..f41a0eb69 100644 --- a/src/test/managers/common/packageManagerHeadlessConformance.unit.test.ts +++ b/src/test/managers/common/packageManagerHeadlessConformance.unit.test.ts @@ -202,7 +202,10 @@ suite('Package manager headless conformance', () => { getProjectsByEnvironment: sinon.stub().returns([]), } as unknown as VenvManager); const conda = new CondaPackageManager(api, log); - const poetry = new PoetryPackageManager(api, log, {} as PoetryManager); + const poetry = new PoetryPackageManager(api, log, {} as PoetryManager).createForProject({ + name: 'project', + uri: Uri.file(process.cwd()), + }); return { pip, conda, poetry, all: [pip, conda, poetry] }; } diff --git a/src/test/managers/common/packageWatcher.unit.test.ts b/src/test/managers/common/packageWatcher.unit.test.ts index c8f09f7f6..490b1bce9 100644 --- a/src/test/managers/common/packageWatcher.unit.test.ts +++ b/src/test/managers/common/packageWatcher.unit.test.ts @@ -4,7 +4,13 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import { Disposable, EventEmitter, LogOutputChannel, RelativePattern, Terminal, Uri } from 'vscode'; -import { DidChangeEnvironmentEventArgs, PackageManager, PythonEnvironment, PythonEnvironmentId } from '../../../api'; +import { + DidChangeEnvironmentEventArgs, + PackageManager, + PythonEnvironment, + PythonEnvironmentId, + PythonProject, +} from '../../../api'; import * as windowApis from '../../../common/window.apis'; import * as workspaceApis from '../../../common/workspace.apis'; import type { EnvironmentManagers } from '../../../features/envManagers'; @@ -406,6 +412,36 @@ suite('Package Watcher', () => { assert.ok((mockWatcher.dispose as sinon.SinonStub).called, 'Should dispose watcher after the final scope'); }); + test('should use separate watchers for project-bound package managers', () => { + createFileSystemWatcherStub.returns(createMockWatcher()); + const environmentChanges = new EventEmitter(); + const firstScope = Uri.file('workspace-one'); + const secondScope = Uri.file('workspace-two'); + const firstPackageManager = new InternalPackageManager( + 'poetry', + createMockPackageManager() as PackageManager, + { name: 'first', uri: firstScope } as PythonProject, + ); + const secondPackageManager = new InternalPackageManager( + 'poetry', + createMockPackageManager() as PackageManager, + { name: 'second', uri: secondScope } as PythonProject, + ); + const envManagers = { + onDidChangeActiveEnvironment: environmentChanges.event, + getPackageManager: sandbox + .stub() + .callsFake((scope) => (scope === firstScope ? firstPackageManager : secondPackageManager)), + } as unknown as EnvironmentManagers; + const env = createMockEnvironment(); + + registerPackageWatchers(envManagers, mockTerminalActivation, mockLogOutputChannel as LogOutputChannel); + environmentChanges.fire({ uri: firstScope, new: env, old: undefined }); + environmentChanges.fire({ uri: secondScope, new: env, old: undefined }); + + assert.strictEqual(createFileSystemWatcherStub.callCount, 2); + }); + test('should stop watching an environment when the active environment changes', () => { const firstWatcher = createMockWatcher(); const secondWatcher = createMockWatcher(); @@ -573,10 +609,16 @@ suite('Package Watcher', () => { configurationChanges.event(listener), ); const environmentChanges = new EventEmitter(); - const firstPackageManager = new InternalPackageManager('pip', createMockPackageManager() as PackageManager); + const project = { name: 'project', uri: Uri.file('workspace') } as PythonProject; + const firstPackageManager = new InternalPackageManager( + 'first', + createMockPackageManager() as PackageManager, + project, + ); const secondPackageManager = new InternalPackageManager( - 'conda', + 'second', createMockPackageManager() as PackageManager, + project, ); let selectedPackageManager = firstPackageManager; const envManagers = { diff --git a/src/test/managers/poetry/poetryPackageManager.unit.test.ts b/src/test/managers/poetry/poetryPackageManager.unit.test.ts index e96cf746a..28ee9b64c 100644 --- a/src/test/managers/poetry/poetryPackageManager.unit.test.ts +++ b/src/test/managers/poetry/poetryPackageManager.unit.test.ts @@ -21,17 +21,12 @@ suite('PoetryPackageManager', () => { }); let logError: sinon.SinonStub; let manager: PoetryPackageManager; + let projectManager: PoetryPackageManager; let runPoetryStub: sinon.SinonStub; + const projectUri = Uri.file(path.join(process.cwd(), 'project', 'pyproject.toml')); setup(() => { - const api = { - getPythonProjects: () => [ - { - name: 'project', - uri: Uri.file(path.join(process.cwd(), 'project', 'pyproject.toml')), - }, - ], - } as unknown as PythonEnvironmentApi; + const api = {} as PythonEnvironmentApi; logError = sinon.stub(); const log = { append: sinon.stub(), @@ -45,6 +40,7 @@ suite('PoetryPackageManager', () => { sinon.stub(packageChanges, 'updatePackagesAndNotify').resolves([]); runPoetryStub = sinon.stub(runPoetryModule, 'runPoetry').resolves(''); manager = new PoetryPackageManager(api, log, {} as PoetryManager); + projectManager = manager.createForProject({ name: 'project', uri: projectUri }); }); teardown(() => { @@ -52,28 +48,57 @@ suite('PoetryPackageManager', () => { sinon.restore(); }); - test('package management inherits the process working directory', async () => { - await manager.manage(environment, { install: ['requests'], uninstall: ['flask'] }); + test('package management uses the project working directory', async () => { + await projectManager.manage(environment, { install: ['requests'], uninstall: ['flask'] }); assert.strictEqual(runPoetryStub.callCount, 2); - assert.strictEqual(runPoetryStub.firstCall.args[1], undefined); - assert.strictEqual(runPoetryStub.secondCall.args[1], undefined); + assert.strictEqual(runPoetryStub.firstCall.args[1], path.dirname(projectUri.fsPath)); + assert.strictEqual(runPoetryStub.secondCall.args[1], path.dirname(projectUri.fsPath)); }); - test('direct package listing inherits the process working directory', async () => { - await manager.getDirectPackageNames(environment); + test('direct package listing uses the project working directory', async () => { + await projectManager.getDirectPackageNames(environment); assert.strictEqual(runPoetryStub.callCount, 1); - assert.strictEqual(runPoetryStub.firstCall.args[1], undefined); + assert.strictEqual(runPoetryStub.firstCall.args[1], path.dirname(projectUri.fsPath)); + }); + + test('directory project URIs are used directly as the working directory', async () => { + const directoryUri = Uri.file(process.cwd()); + const directoryManager = manager.createForProject({ name: 'directory-project', uri: directoryUri }); + + await directoryManager.getDirectPackageNames(environment); + + assert.strictEqual(runPoetryStub.callCount, 1); + assert.strictEqual(runPoetryStub.firstCall.args[1], directoryUri.fsPath); }); test('package loading returns an empty list when poetry show fails', async () => { const showError = new Error('poetry show failed'); runPoetryStub.rejects(showError); - const packages = await manager.getPackages(environment, { skipCache: true }); + const packages = await projectManager.getPackages(environment, { skipCache: true }); assert.deepStrictEqual(packages, []); assert.ok(logError.calledOnceWithExactly(`Error refreshing packages with Poetry: ${showError}`)); }); + + test('project-sensitive reads are unavailable without a project', async () => { + assert.strictEqual(await manager.getPackages(environment), undefined); + assert.strictEqual(await manager.getDirectPackageNames(environment), undefined); + assert.strictEqual(runPoetryStub.callCount, 0); + }); + + test('package management rejects operations without a project', async () => { + await assert.rejects( + manager.manage(environment, { install: ['requests'] }), + /require a Python project/, + ); + assert.strictEqual(runPoetryStub.callCount, 0); + }); + + test('refresh rejects operations without a project', async () => { + await assert.rejects(manager.refresh(environment), /require a Python project/); + assert.strictEqual(runPoetryStub.callCount, 0); + }); }); diff --git a/src/types.ts b/src/types.ts index 12c93df99..b314cb91f 100644 --- a/src/types.ts +++ b/src/types.ts @@ -718,6 +718,16 @@ export interface PackageManager { */ onDidChangePackages?: Event; + /** + * Creates a package manager bound to a Python project. + * + * Project-independent package managers can omit this method. + * + * @param project - The project to bind to the package manager. + * @returns A package manager that uses the project for project-sensitive operations. + */ + createForProject?(project: PythonProject): PackageManager; + /** * Fetches the names of direct (non-transitive) packages for the specified Python environment. * From f61652b70075cce7647e0725e3da46af60debfba Mon Sep 17 00:00:00 2001 From: Eduardo Villalpando Mello Date: Fri, 25 Sep 2026 00:45:24 -0700 Subject: [PATCH 02/11] fix: address scoped package manager lifecycle feedback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/features/envManagers.ts | 84 +++++++++++++++---- src/managers/common/packageWatcher.ts | 17 +++- src/managers/common/registeredManagers.ts | 4 + src/managers/poetry/poetryPackageManager.ts | 2 +- .../features/packageManager.api.unit.test.ts | 65 ++++++++++++++ .../common/packageWatcher.unit.test.ts | 55 ++++++++++++ .../poetry/poetryPackageManager.unit.test.ts | 13 +++ 7 files changed, 221 insertions(+), 19 deletions(-) diff --git a/src/features/envManagers.ts b/src/features/envManagers.ts index b882e8021..18ff47d81 100644 --- a/src/features/envManagers.ts +++ b/src/features/envManagers.ts @@ -175,7 +175,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { private _projectPackageManagers: Map = new Map(); private readonly _packageManagerEventSubscriptions = new Map< string, - Map, Disposable> + Map, { disposable: Disposable; references: number }> >(); private readonly subscriptions: Disposable[] = []; @@ -235,6 +235,15 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { private readonly pm: PythonProjectManager, private readonly inlineScriptRouting?: InlineScriptRoutingRegistry, ) { + const projectChanges = this.pm.onDidChangeProjects; + if (projectChanges) { + const subscription = projectChanges((projects) => + this.evictRemovedProjectPackageManagers(projects ?? this.pm.getProjects()), + ); + if (subscription) { + this.subscriptions.push(subscription); + } + } if (this.inlineScriptRouting) { this.subscriptions.push( this.inlineScriptRouting.onDidChangeRouteability((e) => { @@ -328,6 +337,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { for (const [key, scopedManager] of this._projectPackageManagers) { if (scopedManager.id === managerId) { this._projectPackageManagers.delete(key); + this.unsubscribeFromPackageManagerEvents(scopedManager); } } this.disposePackageManagerEventSubscriptions(managerId); @@ -485,32 +495,72 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { subscriptions = new Map(); this._packageManagerEventSubscriptions.set(provider.id, subscriptions); } - if (subscriptions.has(event)) { + const existing = subscriptions.get(event); + if (existing) { + existing.references += 1; return; } subscriptions.set( event, - event((e) => { - this._onDidChangePackageProviderPackages.fire(e); - const eventManager = - Array.from(this._projectPackageManagers.values()).find( - (candidate) => candidate.id === provider.id && candidate.wraps(e.manager), - ) ?? provider; - setImmediate(() => - this._onDidChangePackages.fire({ - environment: e.environment, - manager: eventManager, - changes: e.changes, - }), - ); - }), + { + references: 1, + disposable: event((e) => { + this._onDidChangePackageProviderPackages.fire(e); + const eventManager = + Array.from(this._projectPackageManagers.values()).find( + (candidate) => candidate.id === provider.id && candidate.wraps(e.manager), + ) ?? provider; + setImmediate(() => + this._onDidChangePackages.fire({ + environment: e.environment, + manager: eventManager, + changes: e.changes, + }), + ); + }), + }, ); } + private unsubscribeFromPackageManagerEvents(manager: InternalPackageManager): void { + const event = manager.packageChangeEvent; + if (!event) { + return; + } + + const subscriptions = this._packageManagerEventSubscriptions.get(manager.id); + const subscription = subscriptions?.get(event); + if (!subscription) { + return; + } + + subscription.references -= 1; + if (subscription.references === 0) { + subscription.disposable.dispose(); + subscriptions?.delete(event); + if (subscriptions?.size === 0) { + this._packageManagerEventSubscriptions.delete(manager.id); + } + } + } + + private evictRemovedProjectPackageManagers(projects: readonly PythonProject[]): void { + const projectsByPath = new Map( + projects.map((project) => [normalizePath(project.uri.fsPath), project] as const), + ); + for (const [key, manager] of this._projectPackageManagers) { + const projectPath = manager.project && normalizePath(manager.project.uri.fsPath); + if (!projectPath || projectsByPath.get(projectPath) !== manager.project) { + this._projectPackageManagers.delete(key); + this.unsubscribeFromPackageManagerEvents(manager); + } + } + } + private disposePackageManagerEventSubscriptions(managerId: string): void { const subscriptions = this._packageManagerEventSubscriptions.get(managerId); - subscriptions?.forEach((subscription) => subscription.dispose()); + subscriptions?.forEach(({ disposable }) => disposable.dispose()); this._packageManagerEventSubscriptions.delete(managerId); } diff --git a/src/managers/common/packageWatcher.ts b/src/managers/common/packageWatcher.ts index a0b79e881..c3a5059a5 100644 --- a/src/managers/common/packageWatcher.ts +++ b/src/managers/common/packageWatcher.ts @@ -188,7 +188,22 @@ export function registerPackageWatchers( const terminalActivationDisposable = terminalActivation.onDidChangeTerminalActivationState((changes) => { if (changes.activated) { if (!closedTerminals.has(changes.terminal)) { - watchEnvironment(changes.terminal, changes.environment, changes.environment); + const projectScope = Array.from(activeEnvironmentByScope.values()).find( + ({ scope, environment }) => + scope && + environment.envId.id === changes.environment.envId.id && + environment.envId.managerId === changes.environment.envId.managerId, + )?.scope; + const managerContext = projectScope ?? changes.environment; + const packageManager = envManagers.getPackageManager(managerContext); + if (!projectScope && packageManager?.supportsProjectBinding) { + releaseConsumer(changes.terminal); + log.debug( + `Skipping unscoped package watcher for project-aware manager ${packageManager.id}`, + ); + return; + } + watchEnvironment(changes.terminal, managerContext, changes.environment); } } else { releaseConsumer(changes.terminal); diff --git a/src/managers/common/registeredManagers.ts b/src/managers/common/registeredManagers.ts index 498563d07..0f618ae49 100644 --- a/src/managers/common/registeredManagers.ts +++ b/src/managers/common/registeredManagers.ts @@ -291,6 +291,10 @@ export class InternalPackageManager implements PackageManager { return this.manager.onDidChangePackages; } + get supportsProjectBinding(): boolean { + return this.manager.createForProject !== undefined; + } + wraps(other: PackageManager): boolean { return this.manager === other; } diff --git a/src/managers/poetry/poetryPackageManager.ts b/src/managers/poetry/poetryPackageManager.ts index 7cc5c6f20..25c156d64 100644 --- a/src/managers/poetry/poetryPackageManager.ts +++ b/src/managers/poetry/poetryPackageManager.ts @@ -78,6 +78,7 @@ export class PoetryPackageManager implements PackageManager, Disposable { } async manage(environment: PythonEnvironment, options: PackageManagementOptions): Promise { + const cwd = await this.getProjectCwd(); let toInstall: string[] = [...(options.install ?? [])]; let toUninstall: string[] = [...(options.uninstall ?? [])]; @@ -104,7 +105,6 @@ export class PoetryPackageManager implements PackageManager, Disposable { } } - const cwd = await this.getProjectCwd(); const execute = async (token?: CancellationToken): Promise => { try { await this.runPoetryManage({ install: toInstall, uninstall: toUninstall }, cwd, token); diff --git a/src/test/features/packageManager.api.unit.test.ts b/src/test/features/packageManager.api.unit.test.ts index e4f549d75..329243073 100644 --- a/src/test/features/packageManager.api.unit.test.ts +++ b/src/test/features/packageManager.api.unit.test.ts @@ -48,6 +48,7 @@ suite('PythonPackageManagerApi Tests', () => { let environment: typeMoq.IMock; let packageManager: typeMoq.IMock; let onDidChangePackagesEmitter: EventEmitter; + let projectChangesEmitter: EventEmitter; let getExtensionStub: sinon.SinonStub; setup(() => { @@ -71,6 +72,9 @@ suite('PythonPackageManagerApi Tests', () => { // Mock project manager projectManager = typeMoq.Mock.ofType(); + projectChangesEmitter = new EventEmitter(); + projectManager.setup((pm) => pm.getProjects()).returns(() => []); + projectManager.setup((pm) => pm.onDidChangeProjects).returns(() => projectChangesEmitter.event); setupNonThenable(projectManager); // Create environment managers instance @@ -96,6 +100,7 @@ suite('PythonPackageManagerApi Tests', () => { sinon.restore(); envManagers.dispose(); onDidChangePackagesEmitter.dispose(); + projectChangesEmitter.dispose(); }); /** @@ -806,5 +811,65 @@ suite('PythonPackageManagerApi Tests', () => { assert.strictEqual(envManagers.getPackageManager(project.uri), registeredManager); }); + + test('Should evict scoped package managers when their project is removed', async () => { + disposable.dispose(); + const projectUri = Uri.file(path.join(process.cwd(), 'removed-project')); + let currentProject = { name: 'original', uri: projectUri } as PythonProject; + projectManager.setup((pm) => pm.get(projectUri)).returns(() => currentProject); + const scopedManagers: PackageManager[] = []; + const scopedEmitters: EventEmitter[] = []; + const provider: PackageManager = { + name: 'project-pkg-mgr', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + createForProject: () => { + const emitter = new EventEmitter(); + const scopedManager: PackageManager = { + name: 'project-pkg-mgr', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + onDidChangePackages: emitter.event, + }; + scopedEmitters.push(emitter); + scopedManagers.push(scopedManager); + return scopedManager; + }, + }; + disposable = envManagers.registerPackageManager(provider); + const registeredManager = envManagers.packageManagers[0]; + sinon.stub(workspaceApis, 'getConfiguration').returns({ + get: (section: string, defaultValue?: unknown) => + section === 'defaultPackageManager' ? registeredManager.id : defaultValue, + } as WorkspaceConfiguration); + const events: unknown[] = []; + const eventDisposable = envManagers.onDidChangePackages((event) => events.push(event)); + + const original = envManagers.getPackageManager(projectUri); + currentProject = { name: 'replacement', uri: projectUri } as PythonProject; + projectChangesEmitter.fire([currentProject]); + const replacement = envManagers.getPackageManager(projectUri); + + assert.notStrictEqual(original, replacement); + assert.strictEqual(scopedManagers.length, 2); + assert.strictEqual(replacement?.project, currentProject); + + const packageChange = { + environment: environment.object, + changes: [], + }; + scopedEmitters[0].fire({ ...packageChange, manager: scopedManagers[0] }); + await new Promise((resolve) => setImmediate(resolve)); + assert.strictEqual(events.length, 0); + + scopedEmitters[1].fire({ ...packageChange, manager: scopedManagers[1] }); + await new Promise((resolve) => setImmediate(resolve)); + assert.strictEqual(events.length, 1); + + eventDisposable.dispose(); + scopedEmitters.forEach((emitter) => emitter.dispose()); + }); }); }); diff --git a/src/test/managers/common/packageWatcher.unit.test.ts b/src/test/managers/common/packageWatcher.unit.test.ts index 490b1bce9..0f1b1bd5e 100644 --- a/src/test/managers/common/packageWatcher.unit.test.ts +++ b/src/test/managers/common/packageWatcher.unit.test.ts @@ -538,6 +538,61 @@ suite('Package Watcher', () => { assert.strictEqual(createFileSystemWatcherStub.callCount, 1); }); + test('should reuse a project-scoped watcher for terminal activation', () => { + createFileSystemWatcherStub.returns(createMockWatcher()); + const environmentChanges = new EventEmitter(); + const scope = Uri.file('workspace'); + const project = { name: 'project', uri: scope } as PythonProject; + const rootManager = new InternalPackageManager('poetry', { + ...createMockPackageManager(), + createForProject: () => createMockPackageManager() as PackageManager, + } as PackageManager); + const scopedManager = new InternalPackageManager( + 'poetry', + createMockPackageManager() as PackageManager, + project, + ); + const env = createMockEnvironment(); + const terminal = { name: 'terminal' } as Terminal; + const envManagers = { + onDidChangeActiveEnvironment: environmentChanges.event, + getPackageManager: sandbox + .stub() + .callsFake((context) => (context instanceof Uri ? scopedManager : rootManager)), + } as unknown as EnvironmentManagers; + + registerPackageWatchers(envManagers, mockTerminalActivation, mockLogOutputChannel as LogOutputChannel); + environmentChanges.fire({ uri: scope, new: env, old: undefined }); + terminalActivationChanges.fire({ terminal, environment: env, activated: true }); + + assert.strictEqual(createFileSystemWatcherStub.callCount, 1); + assert.ok((envManagers.getPackageManager as sinon.SinonStub).calledWith(scope)); + }); + + test('should skip terminal-only watchers for project-aware package managers', () => { + const environmentChanges = new EventEmitter(); + const packageManager = new InternalPackageManager('poetry', { + ...createMockPackageManager(), + createForProject: () => createMockPackageManager() as PackageManager, + } as PackageManager); + const env = createMockEnvironment(); + const terminal = { name: 'terminal' } as Terminal; + const envManagers = { + onDidChangeActiveEnvironment: environmentChanges.event, + getPackageManager: sandbox.stub().returns(packageManager), + } as unknown as EnvironmentManagers; + + registerPackageWatchers(envManagers, mockTerminalActivation, mockLogOutputChannel as LogOutputChannel); + terminalActivationChanges.fire({ terminal, environment: env, activated: true }); + + assert.strictEqual(createFileSystemWatcherStub.callCount, 0); + assert.ok( + (mockLogOutputChannel.debug as sinon.SinonStub).calledWith( + 'Skipping unscoped package watcher for project-aware manager poetry', + ), + ); + }); + test('should release a terminal environment watcher when the terminal closes', () => { const mockWatcher = createMockWatcher(); createFileSystemWatcherStub.returns(mockWatcher); diff --git a/src/test/managers/poetry/poetryPackageManager.unit.test.ts b/src/test/managers/poetry/poetryPackageManager.unit.test.ts index 28ee9b64c..855eebac9 100644 --- a/src/test/managers/poetry/poetryPackageManager.unit.test.ts +++ b/src/test/managers/poetry/poetryPackageManager.unit.test.ts @@ -97,6 +97,19 @@ suite('PoetryPackageManager', () => { assert.strictEqual(runPoetryStub.callCount, 0); }); + test('unbound package management rejects before no-op exits or prompts', async () => { + const showInputBox = sinon.stub(windowApis, 'showInputBox'); + + await assert.rejects( + manager.manage(environment, { install: [], runHeadless: true }), + /require a Python project/, + ); + await assert.rejects(manager.manage(environment, { install: [] }), /require a Python project/); + + assert.ok(showInputBox.notCalled); + assert.strictEqual(runPoetryStub.callCount, 0); + }); + test('refresh rejects operations without a project', async () => { await assert.rejects(manager.refresh(environment), /require a Python project/); assert.strictEqual(runPoetryStub.callCount, 0); From 1a888a1d8c1598ecc9cdfc5693a27147948dc4f1 Mon Sep 17 00:00:00 2001 From: Eduardo Villalpando Mello Date: Fri, 25 Sep 2026 01:19:58 -0700 Subject: [PATCH 03/11] fix: keep scoped package refreshes current Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/managers/common/packageWatcher.ts | 27 ++++++++++- src/managers/poetry/poetryPackageManager.ts | 20 ++++----- ...ageManagerHeadlessConformance.unit.test.ts | 12 ++++- .../common/packageWatcher.unit.test.ts | 45 +++++++++++++++++++ .../poetry/poetryPackageManager.unit.test.ts | 20 ++++++++- 5 files changed, 110 insertions(+), 14 deletions(-) diff --git a/src/managers/common/packageWatcher.ts b/src/managers/common/packageWatcher.ts index c3a5059a5..adccc94fc 100644 --- a/src/managers/common/packageWatcher.ts +++ b/src/managers/common/packageWatcher.ts @@ -45,12 +45,14 @@ function getDefaultPackageWatchTargets(env: PythonEnvironment): RelativePattern[ * @param env - The Python environment to watch. * @param packageManager - The package manager to call refresh on when changes occur. * @param log - Logger for diagnostic messages. + * @param resolvePackageManager - Resolves the current package manager before each refresh. * @returns A disposable that removes the watcher when disposed. */ export function watchPackageChangesForEnvironment( env: PythonEnvironment, packageManager: PackageManager, log: LogOutputChannel, + resolvePackageManager: () => PackageManager | undefined = () => packageManager, ): Disposable { const watchTargets = [ ...getDefaultPackageWatchTargets(env), @@ -63,7 +65,12 @@ export function watchPackageChangesForEnvironment( const debouncedRefresh = createSimpleDebounce(500, () => { log.debug(`Package change detected for environment ${env.envId.id}, refreshing packages.`); - void packageManager.refresh(env).catch((ex) => { + const currentPackageManager = resolvePackageManager(); + if (!currentPackageManager) { + log.debug(`No current package manager found for environment ${env.envId.id}`); + return; + } + void currentPackageManager.refresh(env).catch((ex) => { log.error( `Failed to refresh packages for environment ${env.envId.id}: ${ex instanceof Error ? ex.message : String(ex)}`, ); @@ -166,8 +173,24 @@ export function registerPackageWatchers( if (sharedWatcher) { sharedWatcher.references += 1; } else { + const resolvePackageManager = () => { + const currentPackageManager = + envManagers.getPackageManager(packageManagerContext) ?? envManagers.getPackageManager(environment); + if ( + selectedPackageManager.project && + currentPackageManager?.project?.uri.toString() !== selectedPackageManager.project.uri.toString() + ) { + return undefined; + } + return currentPackageManager; + }; sharedWatchers.set(watcherKey, { - disposable: watchPackageChangesForEnvironment(environment, selectedPackageManager, log), + disposable: watchPackageChangesForEnvironment( + environment, + selectedPackageManager, + log, + resolvePackageManager, + ), references: 1, }); } diff --git a/src/managers/poetry/poetryPackageManager.ts b/src/managers/poetry/poetryPackageManager.ts index 25c156d64..3324df5cf 100644 --- a/src/managers/poetry/poetryPackageManager.ts +++ b/src/managers/poetry/poetryPackageManager.ts @@ -1,11 +1,11 @@ import type { Pep440Version } from '@renovatebot/pep440'; -import * as fsapi from 'fs-extra'; import * as path from 'path'; import { CancellationError, CancellationToken, Event, EventEmitter, + FileType, l10n, LogOutputChannel, MarkdownString, @@ -26,6 +26,7 @@ import { PythonProject, } from '../../api'; import { showErrorMessage, showInputBox, withProgress } from '../../common/window.apis'; +import * as workspaceFs from '../../common/workspace.fs.apis'; import { updatePackagesAndNotify } from '../common/packageChanges'; import { parsePackageSpecs } from '../common/packageUtils'; import { @@ -309,15 +310,14 @@ export class PoetryPackageManager implements PackageManager, Disposable { if (!this.project) { throw new Error(l10n.t('Poetry package operations require a Python project.')); } - const toDirectory = async (fsPath: string): Promise => { - try { - const stat = await fsapi.stat(fsPath); - return stat.isDirectory() ? fsPath : path.dirname(fsPath); - } catch { - return path.dirname(fsPath); - } - }; - return toDirectory(this.project.uri.fsPath); + try { + const stat = await workspaceFs.stat(this.project.uri); + return stat.type === FileType.Directory ? this.project.uri.fsPath : path.dirname(this.project.uri.fsPath); + } catch (error) { + const message = l10n.t('Unable to access the Python project at "{0}".', this.project.uri.fsPath); + this.log.error(message, error); + throw new Error(message); + } } } diff --git a/src/test/managers/common/packageManagerHeadlessConformance.unit.test.ts b/src/test/managers/common/packageManagerHeadlessConformance.unit.test.ts index f41a0eb69..46eaf7f5c 100644 --- a/src/test/managers/common/packageManagerHeadlessConformance.unit.test.ts +++ b/src/test/managers/common/packageManagerHeadlessConformance.unit.test.ts @@ -3,7 +3,7 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; -import { LogOutputChannel, Uri } from 'vscode'; +import { FileType, LogOutputChannel, Uri } from 'vscode'; import { PackageManager, PythonEnvironment, @@ -13,6 +13,7 @@ import { import * as childProcessApis from '../../../common/childProcess.apis'; import * as errorUtils from '../../../common/errors/utils'; import * as windowApis from '../../../common/window.apis'; +import * as workspaceFs from '../../../common/workspace.fs.apis'; import * as workspaceApis from '../../../common/workspace.apis'; import { InternalPackageManager } from '../../../managers/common/registeredManagers'; import { PipInstallCommand } from '../../../managers/builtin/commands/install'; @@ -40,6 +41,15 @@ suite('Package manager headless conformance', () => { version: '3.12.0', } as unknown as PythonEnvironment; + setup(() => { + sinon.stub(workspaceFs, 'stat').resolves({ + type: FileType.Directory, + ctime: 0, + mtime: 0, + size: 0, + }); + }); + teardown(() => { sinon.restore(); }); diff --git a/src/test/managers/common/packageWatcher.unit.test.ts b/src/test/managers/common/packageWatcher.unit.test.ts index 0f1b1bd5e..6612b2659 100644 --- a/src/test/managers/common/packageWatcher.unit.test.ts +++ b/src/test/managers/common/packageWatcher.unit.test.ts @@ -695,5 +695,50 @@ suite('Package Watcher', () => { assert.strictEqual(createFileSystemWatcherStub.callCount, 2); configurationChanges.dispose(); }); + + test('should resolve the current scoped manager when refreshing', async () => { + const clock = sandbox.useFakeTimers(); + const mockWatcher = createMockWatcher(); + createFileSystemWatcherStub.returns(mockWatcher); + const environmentChanges = new EventEmitter(); + const project = { name: 'project', uri: Uri.file('workspace') } as PythonProject; + const firstProvider = createMockPackageManager(); + const secondProvider = createMockPackageManager(); + const firstManager = new InternalPackageManager( + 'poetry', + firstProvider as PackageManager, + project, + ); + const secondManager = new InternalPackageManager( + 'poetry', + secondProvider as PackageManager, + { name: 'replacement', uri: project.uri } as PythonProject, + ); + const rootProvider = createMockPackageManager(); + const rootManager = new InternalPackageManager('poetry', rootProvider as PackageManager); + let selectedManager = firstManager; + const envManagers = { + onDidChangeActiveEnvironment: environmentChanges.event, + getPackageManager: sandbox.stub().callsFake(() => selectedManager), + } as unknown as EnvironmentManagers; + const env = createMockEnvironment(); + + registerPackageWatchers(envManagers, mockTerminalActivation, mockLogOutputChannel as LogOutputChannel); + environmentChanges.fire({ uri: project.uri, new: env, old: undefined }); + + selectedManager = secondManager; + mockWatcher._changeEmitter.fire(Uri.file('replacement.dist-info')); + await clock.tickAsync(600); + + assert.ok((firstProvider.refresh as sinon.SinonStub).notCalled); + assert.ok((secondProvider.refresh as sinon.SinonStub).calledOnceWithExactly(env)); + + selectedManager = rootManager; + mockWatcher._changeEmitter.fire(Uri.file('removed.dist-info')); + await clock.tickAsync(600); + + assert.ok((secondProvider.refresh as sinon.SinonStub).calledOnce); + assert.ok((rootProvider.refresh as sinon.SinonStub).notCalled); + }); }); }); diff --git a/src/test/managers/poetry/poetryPackageManager.unit.test.ts b/src/test/managers/poetry/poetryPackageManager.unit.test.ts index 855eebac9..a51937abd 100644 --- a/src/test/managers/poetry/poetryPackageManager.unit.test.ts +++ b/src/test/managers/poetry/poetryPackageManager.unit.test.ts @@ -4,9 +4,10 @@ import assert from 'assert'; import * as path from 'path'; import * as sinon from 'sinon'; -import { LogOutputChannel, Uri } from 'vscode'; +import { FileType, LogOutputChannel, Uri } from 'vscode'; import { PythonEnvironmentApi } from '../../../api'; import * as windowApis from '../../../common/window.apis'; +import * as workspaceFs from '../../../common/workspace.fs.apis'; import * as packageChanges from '../../../managers/common/packageChanges'; import * as runPoetryModule from '../../../managers/poetry/commands/runPoetry'; import { PoetryPackageManager } from '../../../managers/poetry/poetryPackageManager'; @@ -23,6 +24,7 @@ suite('PoetryPackageManager', () => { let manager: PoetryPackageManager; let projectManager: PoetryPackageManager; let runPoetryStub: sinon.SinonStub; + let statStub: sinon.SinonStub; const projectUri = Uri.file(path.join(process.cwd(), 'project', 'pyproject.toml')); setup(() => { @@ -39,6 +41,8 @@ suite('PoetryPackageManager', () => { sinon.stub(windowApis, 'withProgress').callsFake((_options, task) => task({} as never, {} as never)); sinon.stub(packageChanges, 'updatePackagesAndNotify').resolves([]); runPoetryStub = sinon.stub(runPoetryModule, 'runPoetry').resolves(''); + statStub = sinon.stub(workspaceFs, 'stat'); + statStub.resolves({ type: FileType.File, ctime: 0, mtime: 0, size: 0 }); manager = new PoetryPackageManager(api, log, {} as PoetryManager); projectManager = manager.createForProject({ name: 'project', uri: projectUri }); }); @@ -65,6 +69,7 @@ suite('PoetryPackageManager', () => { test('directory project URIs are used directly as the working directory', async () => { const directoryUri = Uri.file(process.cwd()); + statStub.withArgs(directoryUri).resolves({ type: FileType.Directory, ctime: 0, mtime: 0, size: 0 }); const directoryManager = manager.createForProject({ name: 'directory-project', uri: directoryUri }); await directoryManager.getDirectPackageNames(environment); @@ -73,6 +78,19 @@ suite('PoetryPackageManager', () => { assert.strictEqual(runPoetryStub.firstCall.args[1], directoryUri.fsPath); }); + test('inaccessible projects reject instead of falling back to the parent directory', async () => { + const inaccessibleUri = Uri.file(path.join(process.cwd(), 'missing-project')); + statStub.withArgs(inaccessibleUri).rejects(new Error('access denied')); + const inaccessibleManager = manager.createForProject({ name: 'missing', uri: inaccessibleUri }); + + await assert.rejects( + inaccessibleManager.manage(environment, { install: ['requests'] }), + /Unable to access the Python project/, + ); + + assert.strictEqual(runPoetryStub.callCount, 0); + }); + test('package loading returns an empty list when poetry show fails', async () => { const showError = new Error('poetry show failed'); runPoetryStub.rejects(showError); From 39b042f3fa2370e7655254319b6ca5f750a59423 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 25 Sep 2026 18:20:24 +0000 Subject: [PATCH 04/11] fix: resolve unbound Poetry manager UI regressions and symlink misclassification Co-authored-by: edvilme <5952839+edvilme@users.noreply.github.com> --- src/extension.ts | 7 ++- src/features/envCommands.ts | 27 ++++++++-- src/managers/common/errors.ts | 20 +++++++ src/managers/poetry/poetryPackageManager.ts | 11 ++-- src/test/features/envCommands.unit.test.ts | 54 ++++++++++++++++++- .../poetry/poetryPackageManager.unit.test.ts | 20 ++++++- 6 files changed, 126 insertions(+), 13 deletions(-) create mode 100644 src/managers/common/errors.ts diff --git a/src/extension.ts b/src/extension.ts index 5dfa3db4c..3d8f9c51c 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -115,6 +115,7 @@ import { NativePythonFinder, } from './managers/common/nativePythonFinder'; import { registerPackageWatchers } from './managers/common/packageWatcher'; +import { PackageManagerRequiresProjectError } from './managers/common/errors'; import { IDisposable } from './managers/common/types'; import { registerCondaFeatures } from './managers/conda/main'; import { registerPipenvFeatures } from './managers/pipenv/main'; @@ -367,8 +368,12 @@ export async function activate(context: ExtensionContext): Promise { - await this.getProjectCwd(); + if (!this.project) { + return; + } await withProgress( { location: ProgressLocation.Window, @@ -308,12 +311,14 @@ export class PoetryPackageManager implements PackageManager, Disposable { private async getProjectCwd(): Promise { if (!this.project) { - throw new Error(l10n.t('Poetry package operations require a Python project.')); + throw new PackageManagerRequiresProjectError(); } try { const stat = await workspaceFs.stat(this.project.uri); - return stat.type === FileType.Directory ? this.project.uri.fsPath : path.dirname(this.project.uri.fsPath); + return (stat.type & FileType.Directory) === FileType.Directory + ? this.project.uri.fsPath + : path.dirname(this.project.uri.fsPath); } catch (error) { const message = l10n.t('Unable to access the Python project at "{0}".', this.project.uri.fsPath); this.log.error(message, error); diff --git a/src/test/features/envCommands.unit.test.ts b/src/test/features/envCommands.unit.test.ts index 40618b996..fe538f991 100644 --- a/src/test/features/envCommands.unit.test.ts +++ b/src/test/features/envCommands.unit.test.ts @@ -14,6 +14,7 @@ import { clearEnvironmentCachesCommand, clearScriptEnvironmentCacheCommand, createAnyEnvironmentCommand, + handlePackageUninstall, removeEnvironmentCommand, removePythonProject, revealEnvInManagerView, @@ -26,10 +27,11 @@ import * as shellProviders from '../../features/terminal/shells/providers'; import { ShellStartupScriptProvider } from '../../features/terminal/shells/startupProvider'; import { TerminalManager } from '../../features/terminal/terminalManager'; import { EnvManagerView } from '../../features/views/envManagersView'; -import { ProjectEnvironment, ProjectItem } from '../../features/views/treeViewItems'; +import { EnvManagerTreeItem, PackageTreeItem, ProjectEnvironment, ProjectItem, PythonEnvTreeItem } from '../../features/views/treeViewItems'; import type { EnvironmentManagers } from '../../features/envManagers'; import type { PythonProjectManager } from '../../features/projectManager'; -import { InternalEnvironmentManager } from '../../managers/common/registeredManagers'; +import { InternalEnvironmentManager, InternalPackageManager } from '../../managers/common/registeredManagers'; +import { PackageManagerRequiresProjectError } from '../../managers/common/errors'; import { setupNonThenable } from '../mocks/helper'; import { createMockPythonEnvironment } from '../mocks/pythonEnvironment'; @@ -640,3 +642,51 @@ suite('Run In Terminal Command Tests', () => { sinon.assert.notCalled(runInTerminalStub); }); }); + +suite('handlePackageUninstall - unbound package manager', () => { + let showError: sinon.SinonStub; + + setup(() => { + showError = sinon.stub(windowApis, 'showErrorMessage').resolves(undefined); + }); + + teardown(() => sinon.restore()); + + test('shows a friendly message instead of throwing when the resolved manager requires a project', async () => { + const environment = createMockPythonEnvironment({ + envPath: path.join(process.cwd(), 'unbound-poetry-env'), + managerId: 'ms-python.python:poetry', + }); + const rawManager = { + name: 'poetry', + manage: sinon.stub().rejects(new PackageManagerRequiresProjectError()), + refresh: async () => undefined, + getPackages: async () => undefined, + }; + const packageManager = new InternalPackageManager('ms-python.python:poetry', rawManager as never); + const envManagerMock: Partial = { + getPackageManager: () => packageManager, + }; + const provider = { + name: 'poetry', + preferredPackageManagerId: 'ms-python.python:poetry', + get: async () => environment, + set: async () => undefined, + getEnvironments: async () => [environment], + refresh: async () => undefined, + resolve: async () => undefined, + }; + const parent = new EnvManagerTreeItem(new InternalEnvironmentManager('ms-python.python:poetry', provider)); + const envItem = new PythonEnvTreeItem(environment, parent); + const pkg = { + name: 'requests', + displayName: 'requests', + pkgId: { id: 'requests', managerId: 'ms-python.python:poetry', environmentId: environment.envId.id }, + }; + const context = new PackageTreeItem(pkg, envItem, packageManager); + + await handlePackageUninstall(context, envManagerMock as EnvironmentManagers); + + assert.ok(showError.calledOnceWithExactly(new PackageManagerRequiresProjectError().message)); + }); +}); diff --git a/src/test/managers/poetry/poetryPackageManager.unit.test.ts b/src/test/managers/poetry/poetryPackageManager.unit.test.ts index a51937abd..13e61e4be 100644 --- a/src/test/managers/poetry/poetryPackageManager.unit.test.ts +++ b/src/test/managers/poetry/poetryPackageManager.unit.test.ts @@ -78,6 +78,22 @@ suite('PoetryPackageManager', () => { assert.strictEqual(runPoetryStub.firstCall.args[1], directoryUri.fsPath); }); + test('symlinked directory project URIs are used directly as the working directory', async () => { + const symlinkedDirectoryUri = Uri.file(path.join(process.cwd(), 'symlinked-project')); + statStub + .withArgs(symlinkedDirectoryUri) + .resolves({ type: FileType.Directory | FileType.SymbolicLink, ctime: 0, mtime: 0, size: 0 }); + const symlinkedDirectoryManager = manager.createForProject({ + name: 'symlinked-directory-project', + uri: symlinkedDirectoryUri, + }); + + await symlinkedDirectoryManager.getDirectPackageNames(environment); + + assert.strictEqual(runPoetryStub.callCount, 1); + assert.strictEqual(runPoetryStub.firstCall.args[1], symlinkedDirectoryUri.fsPath); + }); + test('inaccessible projects reject instead of falling back to the parent directory', async () => { const inaccessibleUri = Uri.file(path.join(process.cwd(), 'missing-project')); statStub.withArgs(inaccessibleUri).rejects(new Error('access denied')); @@ -128,8 +144,8 @@ suite('PoetryPackageManager', () => { assert.strictEqual(runPoetryStub.callCount, 0); }); - test('refresh rejects operations without a project', async () => { - await assert.rejects(manager.refresh(environment), /require a Python project/); + test('refresh is a no-op without a project', async () => { + await manager.refresh(environment); assert.strictEqual(runPoetryStub.callCount, 0); }); }); From 960104ac406fc38a6a3f73e90fcd731554eb0f3e Mon Sep 17 00:00:00 2001 From: Eduardo Villalpando Mello Date: Fri, 25 Sep 2026 12:26:20 -0700 Subject: [PATCH 05/11] Allow empty projects --- src/extension.ts | 8 +- src/extensionApi.ts | 9 +- src/features/envCommands.ts | 27 +-- src/features/envManagers.ts | 62 +++++- src/features/views/envManagersView.ts | 15 +- src/features/views/projectView.ts | 5 +- src/managers/common/packageWatcher.ts | 2 +- src/managers/common/registeredManagers.ts | 25 ++- src/test/features/envCommands.unit.test.ts | 5 +- .../features/packageManager.api.unit.test.ts | 190 +++++++++++++++++- 10 files changed, 290 insertions(+), 58 deletions(-) diff --git a/src/extension.ts b/src/extension.ts index 3d8f9c51c..058843dbf 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -298,7 +298,9 @@ export async function activate(context: ExtensionContext): Promise { - const manager = envManagers.getPackageManager(environment); + const manager = + (await envManagers.resolvePackageManager(environment)) ?? + envManagers.getPackageManager(environment); const names = await manager?.getDirectPackageNames?.(environment); return names ? Array.from(names) : undefined; }, @@ -378,10 +380,10 @@ export async function activate(context: ExtensionContext): Promise { - await handlePackageUninstall(context, envManagers); + await handlePackageUninstall(context); }), commands.registerCommand('python-envs.managePackageVersion', async (context: unknown) => { - await managePackageVersion(context, envManagers); + await managePackageVersion(context); }), commands.registerCommand('python-envs.set', async (item) => { await setEnvironmentCommand(item, envManagers, projectManager); diff --git a/src/extensionApi.ts b/src/extensionApi.ts index 337322ed1..c782e8e86 100644 --- a/src/extensionApi.ts +++ b/src/extensionApi.ts @@ -300,7 +300,8 @@ export class PythonEnvironmentApiImpl implements PythonEnvironmentApi { } async managePackages(context: PythonEnvironment, options: PackageManagementOptions): Promise { await waitForEnvManagerId([context.envId.managerId]); - const manager = this.envManagers.getPackageManager(context); + const manager = + (await this.envManagers.resolvePackageManager(context)) ?? this.envManagers.getPackageManager(context); if (!manager) { return Promise.reject(new Error('No package manager found')); } @@ -308,7 +309,8 @@ export class PythonEnvironmentApiImpl implements PythonEnvironmentApi { } async refreshPackages(context: PythonEnvironment): Promise { await waitForEnvManagerId([context.envId.managerId]); - const manager = this.envManagers.getPackageManager(context); + const manager = + (await this.envManagers.resolvePackageManager(context)) ?? this.envManagers.getPackageManager(context); if (!manager) { return Promise.reject(new Error('No package manager found')); } @@ -316,7 +318,8 @@ export class PythonEnvironmentApiImpl implements PythonEnvironmentApi { } async getPackages(context: PythonEnvironment, options?: GetPackagesOptions): Promise { await waitForEnvManagerId([context.envId.managerId]); - const manager = this.envManagers.getPackageManager(context); + const manager = + (await this.envManagers.resolvePackageManager(context)) ?? this.envManagers.getPackageManager(context); if (!manager) { return Promise.resolve(undefined); } diff --git a/src/features/envCommands.ts b/src/features/envCommands.ts index 67f51de01..cdc3531a5 100644 --- a/src/features/envCommands.ts +++ b/src/features/envCommands.ts @@ -138,16 +138,16 @@ export async function refreshPackagesCommand(context: unknown, managers?: Enviro if (context instanceof ProjectEnvironment) { const view = context as ProjectEnvironment; if (managers) { - const pkgManager = managers.getPackageManager(view.parent.project.uri); + const pkgManager = await managers.resolvePackageManager(view.environment, view.parent.project); if (pkgManager) { await pkgManager.refresh(view.environment); } } } else if (context instanceof PythonEnvTreeItem) { const view = context as PythonEnvTreeItem; - const envManager = - view.parent.kind === EnvTreeItemKind.environmentGroup ? view.parent.parent.manager : view.parent.manager; - const pkgManager = managers?.getPackageManager(envManager.preferredPackageManagerId); + const pkgManager = + (await managers?.resolvePackageManager(view.environment)) ?? + managers?.getPackageManager(view.environment); if (pkgManager) { await pkgManager.refresh(view.environment); } @@ -326,7 +326,7 @@ export async function removeEnvironmentCommand(context: unknown, managers: Envir } } -export async function handlePackageUninstall(context: unknown, em: EnvironmentManagers) { +export async function handlePackageUninstall(context: unknown) { if (context instanceof PackageTreeItem || context instanceof ProjectPackage) { if (context.pkg.isTransitive) { const confirm = await showInformationMessage( @@ -344,10 +344,8 @@ export async function handlePackageUninstall(context: unknown, em: EnvironmentMa } const moduleName = context.pkg.name; const environment = context.parent.environment; - const packageManager = - context instanceof ProjectPackage ? context.manager : em.getPackageManager(environment); try { - await packageManager?.manage(environment, { uninstall: [moduleName], install: [] }); + await context.manager.manage(environment, { uninstall: [moduleName], install: [] }); } catch (error) { if (error instanceof PackageManagerRequiresProjectError) { await showErrorMessage(error.message); @@ -364,16 +362,11 @@ export async function handlePackageUninstall(context: unknown, em: EnvironmentMa * Manages package versions by allowing the user to select from available versions or enter a specific version. * If available versions can be fetched, a QuickPick is shown. Otherwise, an InputBox is used for free-text version entry. */ -export async function managePackageVersion(context: unknown, em: EnvironmentManagers) { +export async function managePackageVersion(context: unknown) { if (context instanceof PackageTreeItem || context instanceof ProjectPackage) { const pkg = context.pkg; const environment = context.parent.environment; - const packageManager = - context instanceof ProjectPackage ? context.manager : em.getPackageManager(environment); - - if (!packageManager) { - return; - } + const packageManager = context.manager; if (pkg.isTransitive) { const confirm = await showInformationMessage( @@ -802,7 +795,7 @@ async function resolvePackageCommandOptions( if (e instanceof ProjectEnvironment) { const environment = e.environment; - const packageManager = em.getPackageManager(e.parent.project.uri); + const packageManager = await em.resolvePackageManager(environment, e.parent.project); if (packageManager) { return { environment, packageManager }; } @@ -810,7 +803,7 @@ async function resolvePackageCommandOptions( if (e instanceof PythonEnvTreeItem) { const environment = e.environment; - const packageManager = em.getPackageManager(environment); + const packageManager = (await em.resolvePackageManager(environment)) ?? em.getPackageManager(environment); if (packageManager) { return { environment, packageManager }; } diff --git a/src/features/envManagers.ts b/src/features/envManagers.ts index 18ff47d81..2c73a4fc9 100644 --- a/src/features/envManagers.ts +++ b/src/features/envManagers.ts @@ -119,6 +119,22 @@ export interface EnvironmentManagers extends Disposable { getEnvironmentManager(scope: EnvironmentManagerScope): InternalEnvironmentManager | undefined; getPackageManager(scope: PackageManagerScope): InternalPackageManager | undefined; + /** + * Resolves the package manager for an environment and optional explicit project context. + * + * When a project is supplied, its configured project-scoped manager is returned directly. + * Without a project, project-independent providers are returned directly and project-aware + * providers are scoped only when exactly one tracked project currently uses the environment. + * + * @param environment The environment whose package manager should be resolved. + * @param project The project to bind to, when the caller has explicit project context. + * @returns The package manager, or undefined when project ownership is absent or ambiguous. + */ + resolvePackageManager( + environment: PythonEnvironment, + project?: PythonProject, + ): Promise; + managers: InternalEnvironmentManager[]; packageManagers: InternalPackageManager[]; @@ -429,14 +445,14 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { const defaultPkgManagerId = getDefaultPkgManagerSetting(this.pm, context); const defaultEnvManagerId = getDefaultEnvManagerSetting(this.pm, context); if (defaultPkgManagerId) { - return this.getProjectPackageManager(this._packageManagers.get(defaultPkgManagerId), project); + return this.getOrCreateProjectScopedManager(this._packageManagers.get(defaultPkgManagerId), project); } if (defaultEnvManagerId) { const preferredPkgManagerId = this._environmentManagers.get(defaultEnvManagerId)?.preferredPackageManagerId; if (preferredPkgManagerId) { - return this.getProjectPackageManager( + return this.getOrCreateProjectScopedManager( this._packageManagers.get(preferredPkgManagerId), project, ); @@ -461,7 +477,45 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { return undefined; } - private getProjectPackageManager( + public async resolvePackageManager( + environment: PythonEnvironment, + project?: PythonProject, + ): Promise { + if (project) { + return this.getPackageManager(project.uri); + } + + const manager = this.getPackageManager(environment); + if (!manager?.createForProject) { + return manager; + } + + const projects = this.pm.getProjects(); + const projectEnvironments = await Promise.all( + projects.map(async (project) => ({ + project, + environment: await this.getEnvironment(project.uri), + })), + ); + const matchingProjects = projectEnvironments.filter(({ environment: projectEnvironment }) => + this.isSameEnvironment(environment, projectEnvironment), + ); + if (matchingProjects.length !== 1) { + traceVerbose( + `Unable to resolve project-scoped package manager for environment ${environment.envId.id}: ` + + `found ${matchingProjects.length} matching projects`, + ); + return undefined; + } + + const resolvedManager = this.getPackageManager(matchingProjects[0].project.uri); + if (resolvedManager?.createForProject && !resolvedManager.project) { + return undefined; + } + return resolvedManager; + } + + private getOrCreateProjectScopedManager( manager: InternalPackageManager | undefined, project: PythonProject | undefined, ): InternalPackageManager | undefined { @@ -472,7 +526,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { const key = `${manager.id}:${normalizePath(project.uri.fsPath)}`; let scopedManager = this._projectPackageManagers.get(key); if (!scopedManager) { - scopedManager = manager.createProjectScopedManager(project); + scopedManager = manager.createForProject?.(project); if (scopedManager) { this._projectPackageManagers.set(key, scopedManager); this.subscribeToPackageManagerEvents(manager, scopedManager); diff --git a/src/features/views/envManagersView.ts b/src/features/views/envManagersView.ts index 6ce731013..66dcfffcf 100644 --- a/src/features/views/envManagersView.ts +++ b/src/features/views/envManagersView.ts @@ -10,10 +10,6 @@ import type { InternalDidChangeEnvironmentsEventArgs, InternalDidChangePackagesEventArgs, } from '../envManagers'; -import type { - InternalEnvironmentManager, - InternalPackageManager, -} from '../../managers/common/registeredManagers'; import { ITemporaryStateManager } from './temporaryStateManager'; import { EnvInfoTreeItem, @@ -244,12 +240,7 @@ export class EnvManagerView implements TreeDataProvider, Disposable if (element.kind === EnvTreeItemKind.environment) { const pythonEnvItem = element as PythonEnvTreeItem; const environment = pythonEnvItem.environment; - const envManager = - pythonEnvItem.parent.kind === EnvTreeItemKind.environmentGroup - ? pythonEnvItem.parent.parent.manager - : pythonEnvItem.parent.manager; - - const pkgManager = this.getSupportedPackageManager(envManager); + const pkgManager = await this.providers.resolvePackageManager(environment); const parent = element as PythonEnvTreeItem; const views: EnvTreeItem[] = []; @@ -321,10 +312,6 @@ export class EnvManagerView implements TreeDataProvider, Disposable } } - private getSupportedPackageManager(manager: InternalEnvironmentManager): InternalPackageManager | undefined { - return this.providers.getPackageManager(manager.preferredPackageManagerId); - } - private onDidChangeEnvironmentManager(_args: DidChangeEnvironmentManagerEventArgs) { this.fireDataChanged(undefined); } diff --git a/src/features/views/projectView.ts b/src/features/views/projectView.ts index 4a2ce9aaf..6daa59876 100644 --- a/src/features/views/projectView.ts +++ b/src/features/views/projectView.ts @@ -237,9 +237,10 @@ export class ProjectView implements TreeDataProvider { const environmentItem = element as ProjectEnvironment; const parent = environmentItem.parent; - const uri = parent.id === 'global' ? undefined : parent.project.uri; - const pkgManager = this.envManagers.getPackageManager(uri); + const project = parent.id === 'global' ? undefined : parent.project; + const uri = project?.uri; const environment = environmentItem.environment; + const pkgManager = await this.envManagers.resolvePackageManager(environment, project); if (!pkgManager) { return [new ProjectEnvironmentInfo(environmentItem, ProjectViews.noPackageManager)]; diff --git a/src/managers/common/packageWatcher.ts b/src/managers/common/packageWatcher.ts index adccc94fc..720a59cfd 100644 --- a/src/managers/common/packageWatcher.ts +++ b/src/managers/common/packageWatcher.ts @@ -219,7 +219,7 @@ export function registerPackageWatchers( )?.scope; const managerContext = projectScope ?? changes.environment; const packageManager = envManagers.getPackageManager(managerContext); - if (!projectScope && packageManager?.supportsProjectBinding) { + if (!projectScope && packageManager?.createForProject) { releaseConsumer(changes.terminal); log.debug( `Skipping unscoped package watcher for project-aware manager ${packageManager.id}`, diff --git a/src/managers/common/registeredManagers.ts b/src/managers/common/registeredManagers.ts index 0f618ae49..c026d532f 100644 --- a/src/managers/common/registeredManagers.ts +++ b/src/managers/common/registeredManagers.ts @@ -212,6 +212,7 @@ function inferPackageManagementTrigger( export class InternalPackageManager implements PackageManager { private readonly relatedManagers: WeakSet; + public readonly createForProject?: (project: PythonProject) => InternalPackageManager; public constructor( public readonly id: string, @@ -221,6 +222,21 @@ export class InternalPackageManager implements PackageManager { ) { this.relatedManagers = relatedManagers ?? new WeakSet(); this.relatedManagers.add(manager); + const createForProject = manager.createForProject?.bind(manager); + if (createForProject) { + this.createForProject = (scopedProject) => { + const scopedManager = createForProject(scopedProject); + if (!scopedManager) { + throw new Error(`Package manager ${this.id} did not create a manager for the requested project`); + } + return new InternalPackageManager( + this.id, + scopedManager, + scopedProject, + this.relatedManagers, + ); + }; + } } public get name(): string { @@ -291,10 +307,6 @@ export class InternalPackageManager implements PackageManager { return this.manager.onDidChangePackages; } - get supportsProjectBinding(): boolean { - return this.manager.createForProject !== undefined; - } - wraps(other: PackageManager): boolean { return this.manager === other; } @@ -303,11 +315,6 @@ export class InternalPackageManager implements PackageManager { return this.relatedManagers.has(other); } - createProjectScopedManager(project: PythonProject): InternalPackageManager | undefined { - const manager = this.manager.createForProject?.(project); - return manager ? new InternalPackageManager(this.id, manager, project, this.relatedManagers) : undefined; - } - getVersion(environment: PythonEnvironment): Promise { return this.manager.getVersion ? this.manager.getVersion(environment) : Promise.resolve(undefined); } diff --git a/src/test/features/envCommands.unit.test.ts b/src/test/features/envCommands.unit.test.ts index fe538f991..63b96979f 100644 --- a/src/test/features/envCommands.unit.test.ts +++ b/src/test/features/envCommands.unit.test.ts @@ -664,9 +664,6 @@ suite('handlePackageUninstall - unbound package manager', () => { getPackages: async () => undefined, }; const packageManager = new InternalPackageManager('ms-python.python:poetry', rawManager as never); - const envManagerMock: Partial = { - getPackageManager: () => packageManager, - }; const provider = { name: 'poetry', preferredPackageManagerId: 'ms-python.python:poetry', @@ -685,7 +682,7 @@ suite('handlePackageUninstall - unbound package manager', () => { }; const context = new PackageTreeItem(pkg, envItem, packageManager); - await handlePackageUninstall(context, envManagerMock as EnvironmentManagers); + await handlePackageUninstall(context); assert.ok(showError.calledOnceWithExactly(new PackageManagerRequiresProjectError().message)); }); diff --git a/src/test/features/packageManager.api.unit.test.ts b/src/test/features/packageManager.api.unit.test.ts index 329243073..8652fdbee 100644 --- a/src/test/features/packageManager.api.unit.test.ts +++ b/src/test/features/packageManager.api.unit.test.ts @@ -33,6 +33,7 @@ import * as extensionApis from '../../common/extension.apis'; import * as workspaceApis from '../../common/workspace.apis'; import { PythonEnvironmentManagers } from '../../features/envManagers'; import type { PythonProjectManager } from '../../features/projectManager'; +import { InternalPackageManager } from '../../managers/common/registeredManagers'; import { setupNonThenable } from '../mocks/helper'; /** @@ -50,6 +51,7 @@ suite('PythonPackageManagerApi Tests', () => { let onDidChangePackagesEmitter: EventEmitter; let projectChangesEmitter: EventEmitter; let getExtensionStub: sinon.SinonStub; + let projects: PythonProject[]; setup(() => { // Mock extension APIs to avoid registration errors @@ -73,7 +75,8 @@ suite('PythonPackageManagerApi Tests', () => { // Mock project manager projectManager = typeMoq.Mock.ofType(); projectChangesEmitter = new EventEmitter(); - projectManager.setup((pm) => pm.getProjects()).returns(() => []); + projects = []; + projectManager.setup((pm) => pm.getProjects()).returns(() => projects); projectManager.setup((pm) => pm.onDidChangeProjects).returns(() => projectChangesEmitter.event); setupNonThenable(projectManager); @@ -653,6 +656,86 @@ suite('PythonPackageManagerApi Tests', () => { disposable.dispose(); }); + function registerEnvironmentProvider( + preferredPackageManagerId: string, + usesEnvironment: boolean, + ): { + environment: PythonEnvironment; + getEnvironment: sinon.SinonStub; + managerId: string; + disposable: Disposable; + } { + const onDidChangeEnvironmentsEmitter = new EventEmitter(); + const onDidChangeEnvironmentEmitter = new EventEmitter(); + let selectedEnvironment: PythonEnvironment | undefined; + const getEnvironment = sinon.stub().callsFake(async () => selectedEnvironment); + const registration = envManagers.registerEnvironmentManager( + { + name: 'resolver-env-mgr', + preferredPackageManagerId, + onDidChangeEnvironments: onDidChangeEnvironmentsEmitter.event, + onDidChangeEnvironment: onDidChangeEnvironmentEmitter.event, + refresh: async () => undefined, + getEnvironments: async () => [], + set: async () => undefined, + get: getEnvironment, + resolve: async () => undefined, + }, + { extensionId: 'test-ext' }, + ); + const managerId = envManagers.managers.find((manager) => manager.name === 'resolver-env-mgr')!.id; + const resolvedEnvironment: PythonEnvironment = { + ...environment.object, + envId: { id: environment.object.envId.id, managerId }, + }; + selectedEnvironment = usesEnvironment ? resolvedEnvironment : undefined; + return { + environment: resolvedEnvironment, + getEnvironment, + managerId, + disposable: Disposable.from( + registration, + onDidChangeEnvironmentsEmitter, + onDidChangeEnvironmentEmitter, + ), + }; + } + + function registerProjectAwarePackageManager(): { + manager: InternalPackageManager; + createForProject: sinon.SinonStub; + } { + disposable.dispose(); + const createForProject = sinon.stub().callsFake(() => ({ + name: 'project-pkg-mgr', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + })); + disposable = envManagers.registerPackageManager({ + name: 'project-pkg-mgr', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + createForProject, + }); + return { manager: envManagers.packageManagers[0], createForProject }; + } + + function configureDefaultManagers(packageManagerId: string, environmentManagerId: string): void { + sinon.stub(workspaceApis, 'getConfiguration').returns({ + get: (section: string, defaultValue?: unknown) => { + if (section === 'defaultPackageManager') { + return packageManagerId; + } + if (section === 'defaultEnvManager') { + return environmentManagerId; + } + return defaultValue; + }, + } as WorkspaceConfiguration); + } + test('Should retrieve package manager by ID string', () => { // Mock - Get registered package manager ID const managerId = envManagers.packageManagers[0].id; @@ -798,6 +881,13 @@ suite('PythonPackageManagerApi Tests', () => { }); test('Should share a project-independent package manager across projects', () => { + disposable.dispose(); + disposable = envManagers.registerPackageManager({ + name: 'project-independent-pkg-mgr', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + }); const project = { name: 'project', uri: Uri.file(path.join(process.cwd(), 'project')), @@ -812,6 +902,104 @@ suite('PythonPackageManagerApi Tests', () => { assert.strictEqual(envManagers.getPackageManager(project.uri), registeredManager); }); + test('Should return a project-independent package manager without resolving projects', async () => { + disposable.dispose(); + disposable = envManagers.registerPackageManager({ + name: 'project-independent-pkg-mgr', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + }); + const project = { + name: 'project', + uri: Uri.file(path.join(process.cwd(), 'project-independent')), + } as PythonProject; + projects = [project]; + const registeredManager = envManagers.packageManagers[0]; + const provider = registerEnvironmentProvider(registeredManager.id, true); + + const manager = await envManagers.resolvePackageManager(provider.environment); + + assert.strictEqual(manager, registeredManager); + assert.ok(provider.getEnvironment.notCalled); + + provider.disposable.dispose(); + }); + + test('Should bind the provided project without inferring it from the environment', async () => { + const project = { + name: 'project', + uri: Uri.file(path.join(process.cwd(), 'explicit-project')), + } as PythonProject; + projectManager.setup((pm) => pm.get(typeMoq.It.isAny())).returns(() => project); + const packageProvider = registerProjectAwarePackageManager(); + const environmentProvider = registerEnvironmentProvider(packageProvider.manager.id, false); + configureDefaultManagers(packageProvider.manager.id, environmentProvider.managerId); + + const manager = await envManagers.resolvePackageManager(environmentProvider.environment, project); + + assert.strictEqual(manager?.project, project); + assert.ok(packageProvider.createForProject.calledOnceWithExactly(project)); + assert.ok(environmentProvider.getEnvironment.notCalled); + + environmentProvider.disposable.dispose(); + }); + + test('Should resolve a project-aware package manager for the unique project using an environment', async () => { + const project = { + name: 'project', + uri: Uri.file(path.join(process.cwd(), 'unique-project')), + } as PythonProject; + projects = [project]; + projectManager.setup((pm) => pm.get(typeMoq.It.isAny())).returns(() => project); + const packageProvider = registerProjectAwarePackageManager(); + const environmentProvider = registerEnvironmentProvider(packageProvider.manager.id, true); + configureDefaultManagers(packageProvider.manager.id, environmentProvider.managerId); + + const manager = await envManagers.resolvePackageManager(environmentProvider.environment); + + assert.ok(packageProvider.createForProject.calledOnceWithExactly(project)); + assert.strictEqual(manager?.project, project); + + environmentProvider.disposable.dispose(); + }); + + test('Should not resolve a project-aware package manager without a matching project', async () => { + const packageProvider = registerProjectAwarePackageManager(); + const environmentProvider = registerEnvironmentProvider(packageProvider.manager.id, false); + + const manager = await envManagers.resolvePackageManager(environmentProvider.environment); + + assert.strictEqual(manager, undefined); + assert.ok(packageProvider.createForProject.notCalled); + + environmentProvider.disposable.dispose(); + }); + + test('Should not choose a project-aware package manager when multiple projects use an environment', async () => { + disposable.dispose(); + const firstProject = { + name: 'first', + uri: Uri.file(path.join(process.cwd(), 'ambiguous-first-project')), + } as PythonProject; + const secondProject = { + name: 'second', + uri: Uri.file(path.join(process.cwd(), 'ambiguous-second-project')), + } as PythonProject; + projects = [firstProject, secondProject]; + + const packageProvider = registerProjectAwarePackageManager(); + const environmentProvider = registerEnvironmentProvider(packageProvider.manager.id, true); + configureDefaultManagers(packageProvider.manager.id, environmentProvider.managerId); + + const manager = await envManagers.resolvePackageManager(environmentProvider.environment); + + assert.strictEqual(manager, undefined); + assert.ok(packageProvider.createForProject.notCalled); + + environmentProvider.disposable.dispose(); + }); + test('Should evict scoped package managers when their project is removed', async () => { disposable.dispose(); const projectUri = Uri.file(path.join(process.cwd(), 'removed-project')); From c9ae4c794edd0f4d6cdbe15030d2ebe4684faee3 Mon Sep 17 00:00:00 2001 From: Eduardo Villalpando Mello Date: Fri, 25 Sep 2026 12:34:25 -0700 Subject: [PATCH 06/11] Update docs --- api/CHANGELOG.md | 2 +- docs/README.md | 38 ++++++++++++++++++++++++++++++++++++++ src/types.ts | 9 ++++++++- 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index 11bfb4b81..9a18702b2 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Added the optional `PackageManager.createForProject` factory for package managers whose operations depend on the calling Python project. +- Added the optional `PackageManager.createForProject` factory for package managers whose operations depend on the calling Python project. Explicit project contexts are used directly; environment-only operations use a scoped manager only when exactly one tracked project matches. ## [1.4.0] diff --git a/docs/README.md b/docs/README.md index 1569431ca..cb4e3bfb6 100644 --- a/docs/README.md +++ b/docs/README.md @@ -1621,6 +1621,44 @@ Reports and changes the packages of an environment. | `formatInstallSpec(packageName, version)` | `(packageName: string, version: string) => string` | No | Formats a pinned specifier for this tool, for example `requests==2.31.0` for pip or `requests=2.31.0` for conda. Callers default to `name==version` when absent. | | `onDidChangePackages` | `Event` | No | Fire when packages change. | +##### Project-scoped package managers + +Implement `createForProject` when package operations depend on project files or +the process working directory, as they do for tools such as Poetry. Callers that +already have a `PythonProject` use that project directly. For environment-only +operations, the extension selects a project-scoped manager only when exactly one +tracked project uses the environment; it does not choose arbitrarily when no +project or multiple projects match. + +Environment-only command and API paths may still use the originally registered +manager as an unbound fallback, while package views can suppress operations when +there is no unique project. Project-sensitive methods on the root manager must +therefore fail clearly or report that data is unavailable rather than running +from the extension host's working directory. Keep project-specific caches and +mutable state on the manager returned by `createForProject`. + +```typescript +class ProjectPackageManager implements PackageManager { + readonly name = 'project-pm'; + + constructor(private readonly project?: PythonProject) {} + + createForProject(project: PythonProject): PackageManager { + return new ProjectPackageManager(project); + } + + async manage( + environment: PythonEnvironment, + options: PackageManagementOptions, + ): Promise { + if (!this.project) { + throw new Error('Package management requires a Python project.'); + } + await runPackageCommand(options, { cwd: this.project.uri.fsPath }); + } +} +``` + ```typescript class MyPackageManager implements PackageManager { readonly name = 'my-pm'; diff --git a/src/types.ts b/src/types.ts index b314cb91f..2631dcebb 100644 --- a/src/types.ts +++ b/src/types.ts @@ -721,7 +721,14 @@ export interface PackageManager { /** * Creates a package manager bound to a Python project. * - * Project-independent package managers can omit this method. + * The extension uses the explicit project supplied by project-based callers. When a caller + * provides only an environment, the extension uses a project-bound manager only if exactly + * one tracked project uses that environment. The registered root manager may still receive + * environment-only operations when no project can be selected safely, so project-sensitive + * operations must handle an unbound manager without running in an arbitrary working directory. + * + * Project-independent package managers can omit this method. Implementations should keep + * project-specific caches and mutable state on the returned manager rather than the root. * * @param project - The project to bind to the package manager. * @returns A package manager that uses the project for project-sensitive operations. From 3accdd75adec1dfe4468c8874c51d7ef7ca3efbd Mon Sep 17 00:00:00 2001 From: Eduardo Villalpando Mello Date: Fri, 25 Sep 2026 12:48:31 -0700 Subject: [PATCH 07/11] fix: finalize scoped package manager lifecycle Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/CHANGELOG.md | 1 + docs/README.md | 12 ++- src/features/envCommands.ts | 4 +- src/features/envManagers.ts | 99 +++++++------------ src/managers/common/errors.ts | 5 +- src/managers/common/registeredManagers.ts | 22 ++++- src/managers/poetry/poetryPackageManager.ts | 2 +- .../envManagers.packageEvents.unit.test.ts | 27 ++++- .../features/packageManager.api.unit.test.ts | 72 ++++++++++++++ .../poetry/poetryPackageManager.unit.test.ts | 5 +- src/types.ts | 11 +++ 11 files changed, 181 insertions(+), 79 deletions(-) diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index 9a18702b2..0ba6d37fb 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Added the optional `PackageManager.createForProject` factory for package managers whose operations depend on the calling Python project. Explicit project contexts are used directly; environment-only operations use a scoped manager only when exactly one tracked project matches. +- Added optional `PackageManager.dispose` support for releasing resources owned by project-scoped package managers. ## [1.4.0] diff --git a/docs/README.md b/docs/README.md index cb4e3bfb6..3a3518d48 100644 --- a/docs/README.md +++ b/docs/README.md @@ -1614,6 +1614,7 @@ Reports and changes the packages of an environment. | `getPackages(environment, options?)` | `(environment: PythonEnvironment, options?: GetPackagesOptions) => Promise` | Yes | Returns installed packages, or `undefined` if they cannot be retrieved. | | `getPackageWatchTargets(environment)` | `(environment: PythonEnvironment) => RelativePattern[]` | No | Extra filesystem patterns to watch for install and uninstall changes, appended to the default site-packages locations. Implement for manager-specific locations such as `conda-meta`. | | `createForProject(project)` | `(project: PythonProject) => PackageManager` | No | Creates a manager bound to a project for project-sensitive operations. | +| `dispose()` | `() => void` | No | Releases resources owned by the manager. The extension disposes project-scoped managers when their project is removed or replaced, their provider is unregistered, or the extension shuts down. | | `getDirectPackageNames(environment)` | `(environment: PythonEnvironment) => Promise \| undefined>` | No | Best-effort set of non-transitive package names. Most tools cannot record user intent - pip uses `pip list --not-required`, which reports leaf packages rather than explicitly installed ones. | | `clearCache()` | `() => Promise` | No | Drops cached package data. | | `getVersion(environment)` | `(environment: PythonEnvironment) => Promise` | No | Version of the underlying tool, such as pip, uv, or conda. | @@ -1635,7 +1636,12 @@ manager as an unbound fallback, while package views can suppress operations when there is no unique project. Project-sensitive methods on the root manager must therefore fail clearly or report that data is unavailable rather than running from the extension host's working directory. Keep project-specific caches and -mutable state on the manager returned by `createForProject`. +mutable state on the manager returned by `createForProject`, and implement +`dispose` when that manager owns resources. + +When a scoped manager fires `onDidChangePackages`, the event's `manager` must be +the exact scoped instance returned by `createForProject`. This requirement also +applies when root and scoped managers share an event emitter. ```typescript class ProjectPackageManager implements PackageManager { @@ -1647,6 +1653,10 @@ class ProjectPackageManager implements PackageManager { return new ProjectPackageManager(project); } + dispose(): void { + // Release project-specific watchers or processes. + } + async manage( environment: PythonEnvironment, options: PackageManagementOptions, diff --git a/src/features/envCommands.ts b/src/features/envCommands.ts index cdc3531a5..bfa5f4850 100644 --- a/src/features/envCommands.ts +++ b/src/features/envCommands.ts @@ -145,9 +145,7 @@ export async function refreshPackagesCommand(context: unknown, managers?: Enviro } } else if (context instanceof PythonEnvTreeItem) { const view = context as PythonEnvTreeItem; - const pkgManager = - (await managers?.resolvePackageManager(view.environment)) ?? - managers?.getPackageManager(view.environment); + const pkgManager = await managers?.resolvePackageManager(view.environment); if (pkgManager) { await pkgManager.refresh(view.environment); } diff --git a/src/features/envManagers.ts b/src/features/envManagers.ts index 2c73a4fc9..4faed374b 100644 --- a/src/features/envManagers.ts +++ b/src/features/envManagers.ts @@ -189,10 +189,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { private _environmentManagers: Map = new Map(); private _packageManagers: Map = new Map(); private _projectPackageManagers: Map = new Map(); - private readonly _packageManagerEventSubscriptions = new Map< - string, - Map, { disposable: Disposable; references: number }> - >(); + private readonly _packageManagerEventSubscriptions = new Map(); private readonly subscriptions: Disposable[] = []; /** @@ -337,7 +334,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { } const mgr = new InternalPackageManager(managerId, manager); - this.subscribeToPackageManagerEvents(mgr, mgr); + this.subscribeToPackageManagerEvents(mgr); this._packageManagers.set(managerId, mgr); this._onDidChangePackageManager.fire({ kind: 'registered', manager: mgr }); @@ -354,6 +351,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { if (scopedManager.id === managerId) { this._projectPackageManagers.delete(key); this.unsubscribeFromPackageManagerEvents(scopedManager); + scopedManager.dispose(); } } this.disposePackageManagerEventSubscriptions(managerId); @@ -364,10 +362,15 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { public dispose() { this._environmentManagers.clear(); this._packageManagers.clear(); + for (const manager of this._projectPackageManagers.values()) { + this.unsubscribeFromPackageManagerEvents(manager); + manager.dispose(); + } this._projectPackageManagers.clear(); - for (const managerId of this._packageManagerEventSubscriptions.keys()) { - this.disposePackageManagerEventSubscriptions(managerId); + for (const subscription of this._packageManagerEventSubscriptions.values()) { + subscription.dispose(); } + this._packageManagerEventSubscriptions.clear(); this._inlineRoutingOverrides.clear(); this.subscriptions.forEach((subscription) => subscription.dispose()); this._onDidChangeEnvironmentManager.dispose(); @@ -529,74 +532,36 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { scopedManager = manager.createForProject?.(project); if (scopedManager) { this._projectPackageManagers.set(key, scopedManager); - this.subscribeToPackageManagerEvents(manager, scopedManager); + this.subscribeToPackageManagerEvents(scopedManager); } } return scopedManager ?? manager; } - private subscribeToPackageManagerEvents( - provider: InternalPackageManager, - manager: InternalPackageManager, - ): void { + private subscribeToPackageManagerEvents(manager: InternalPackageManager): void { const event = manager.packageChangeEvent; - if (!event) { - return; - } - - let subscriptions = this._packageManagerEventSubscriptions.get(provider.id); - if (!subscriptions) { - subscriptions = new Map(); - this._packageManagerEventSubscriptions.set(provider.id, subscriptions); - } - const existing = subscriptions.get(event); - if (existing) { - existing.references += 1; + if (!event || this._packageManagerEventSubscriptions.has(manager)) { return; } - subscriptions.set( - event, - { - references: 1, - disposable: event((e) => { - this._onDidChangePackageProviderPackages.fire(e); - const eventManager = - Array.from(this._projectPackageManagers.values()).find( - (candidate) => candidate.id === provider.id && candidate.wraps(e.manager), - ) ?? provider; - setImmediate(() => - this._onDidChangePackages.fire({ - environment: e.environment, - manager: eventManager, - changes: e.changes, - }), - ); - }), - }, + this._packageManagerEventSubscriptions.set( + manager, + event((e) => { + this._onDidChangePackageProviderPackages.fire(e); + setImmediate(() => + this._onDidChangePackages.fire({ + environment: e.environment, + manager, + changes: e.changes, + }), + ); + }), ); } private unsubscribeFromPackageManagerEvents(manager: InternalPackageManager): void { - const event = manager.packageChangeEvent; - if (!event) { - return; - } - - const subscriptions = this._packageManagerEventSubscriptions.get(manager.id); - const subscription = subscriptions?.get(event); - if (!subscription) { - return; - } - - subscription.references -= 1; - if (subscription.references === 0) { - subscription.disposable.dispose(); - subscriptions?.delete(event); - if (subscriptions?.size === 0) { - this._packageManagerEventSubscriptions.delete(manager.id); - } - } + this._packageManagerEventSubscriptions.get(manager)?.dispose(); + this._packageManagerEventSubscriptions.delete(manager); } private evictRemovedProjectPackageManagers(projects: readonly PythonProject[]): void { @@ -608,14 +573,18 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { if (!projectPath || projectsByPath.get(projectPath) !== manager.project) { this._projectPackageManagers.delete(key); this.unsubscribeFromPackageManagerEvents(manager); + manager.dispose(); } } } private disposePackageManagerEventSubscriptions(managerId: string): void { - const subscriptions = this._packageManagerEventSubscriptions.get(managerId); - subscriptions?.forEach(({ disposable }) => disposable.dispose()); - this._packageManagerEventSubscriptions.delete(managerId); + for (const [manager, subscription] of this._packageManagerEventSubscriptions) { + if (manager.id === managerId) { + subscription.dispose(); + this._packageManagerEventSubscriptions.delete(manager); + } + } } public get managers(): InternalEnvironmentManager[] { diff --git a/src/managers/common/errors.ts b/src/managers/common/errors.ts index 647792e27..1ff0d24ea 100644 --- a/src/managers/common/errors.ts +++ b/src/managers/common/errors.ts @@ -5,8 +5,7 @@ import { l10n } from 'vscode'; /** * Raised when a package-management operation is invoked on a package manager that is not - * bound to a Python project (for example, Poetry, which needs a project to determine the - * working directory for its commands). + * bound to a Python project. * * Callers that resolve package managers without a specific project (e.g. the environment * manager view) should catch this error and surface a friendly message rather than letting @@ -14,7 +13,7 @@ import { l10n } from 'vscode'; */ export class PackageManagerRequiresProjectError extends Error { constructor() { - super(l10n.t('Poetry package operations require a Python project.')); + super(l10n.t('Package operations require a Python project.')); this.name = 'PackageManagerRequiresProjectError'; } } diff --git a/src/managers/common/registeredManagers.ts b/src/managers/common/registeredManagers.ts index c026d532f..72cd84c77 100644 --- a/src/managers/common/registeredManagers.ts +++ b/src/managers/common/registeredManagers.ts @@ -212,6 +212,8 @@ function inferPackageManagementTrigger( export class InternalPackageManager implements PackageManager { private readonly relatedManagers: WeakSet; + private readonly packageChangeEventValue: Event | undefined; + private isDisposed = false; public readonly createForProject?: (project: PythonProject) => InternalPackageManager; public constructor( @@ -222,6 +224,15 @@ export class InternalPackageManager implements PackageManager { ) { this.relatedManagers = relatedManagers ?? new WeakSet(); this.relatedManagers.add(manager); + const packageChangeEvent = manager.onDidChangePackages; + if (packageChangeEvent) { + this.packageChangeEventValue = (listener) => + packageChangeEvent((event) => { + if (event.manager === manager) { + listener(event); + } + }); + } const createForProject = manager.createForProject?.bind(manager); if (createForProject) { this.createForProject = (scopedProject) => { @@ -300,11 +311,11 @@ export class InternalPackageManager implements PackageManager { } onDidChangePackages(handler: (e: DidChangePackagesEventArgs) => void): Disposable { - return this.manager.onDidChangePackages ? this.manager.onDidChangePackages(handler) : new Disposable(() => {}); + return this.packageChangeEventValue ? this.packageChangeEventValue(handler) : new Disposable(() => {}); } get packageChangeEvent(): Event | undefined { - return this.manager.onDidChangePackages; + return this.packageChangeEventValue; } wraps(other: PackageManager): boolean { @@ -315,6 +326,13 @@ export class InternalPackageManager implements PackageManager { return this.relatedManagers.has(other); } + dispose(): void { + if (!this.isDisposed) { + this.isDisposed = true; + this.manager.dispose?.(); + } + } + getVersion(environment: PythonEnvironment): Promise { return this.manager.getVersion ? this.manager.getVersion(environment) : Promise.resolve(undefined); } diff --git a/src/managers/poetry/poetryPackageManager.ts b/src/managers/poetry/poetryPackageManager.ts index 0091eb393..a62b30c92 100644 --- a/src/managers/poetry/poetryPackageManager.ts +++ b/src/managers/poetry/poetryPackageManager.ts @@ -152,7 +152,7 @@ export class PoetryPackageManager implements PackageManager, Disposable { async refresh(environment: PythonEnvironment): Promise { if (!this.project) { - return; + throw new PackageManagerRequiresProjectError(); } await withProgress( { diff --git a/src/test/features/envManagers.packageEvents.unit.test.ts b/src/test/features/envManagers.packageEvents.unit.test.ts index 78ab7eaa3..073a60ebf 100644 --- a/src/test/features/envManagers.packageEvents.unit.test.ts +++ b/src/test/features/envManagers.packageEvents.unit.test.ts @@ -4,7 +4,7 @@ import assert from 'assert'; import * as path from 'path'; import * as sinon from 'sinon'; -import { Disposable, EventEmitter, Uri, WorkspaceConfiguration } from 'vscode'; +import { Disposable, Event, EventEmitter, Uri, WorkspaceConfiguration } from 'vscode'; import { DidChangeEnvironmentVariablesEventArgs, DidChangePackagesEventArgs, @@ -35,6 +35,7 @@ for (const inlineEnabled of [false, true]) { let scopedProvider: PackageManager | undefined; let emitter: EventEmitter; let scopedEmitter: EventEmitter; + let scopedEvent: Event; let project: PythonProject; let routing: InlineScriptRoutingRegistry | undefined; let disposables: Disposable[]; @@ -67,6 +68,7 @@ for (const inlineEnabled of [false, true]) { ); emitter = new EventEmitter(); scopedEmitter = new EventEmitter(); + scopedEvent = scopedEmitter.event; provider = { name: 'custom', manage: async () => undefined, @@ -79,7 +81,7 @@ for (const inlineEnabled of [false, true]) { manage: async () => undefined, refresh: async () => undefined, getPackages: async () => [], - onDidChangePackages: scopedEmitter.event, + onDidChangePackages: scopedEvent, }; return scopedProvider; }, @@ -175,5 +177,26 @@ for (const inlineEnabled of [false, true]) { assert.strictEqual(internalEvents[0].changes, scopedChanges); assert.strictEqual(scopedChanges[0].pkg.pkgId.managerId, scopedManager.id); }); + + test('routes a shared emitter event by the exact scoped manager instance', () => { + scopedEvent = emitter.event; + const scopedManager = managers.getPackageManager(project.uri); + assert.ok(scopedManager); + assert.ok(scopedProvider); + + const event: DidChangePackagesEventArgs = { + environment, + manager: scopedProvider, + changes, + }; + + emitter.fire(event); + clock.runAll(); + + assert.strictEqual(publicEvents.length, 1); + assert.strictEqual(publicEvents[0], event); + assert.strictEqual(internalEvents.length, 1); + assert.strictEqual(internalEvents[0].manager, scopedManager); + }); }); } diff --git a/src/test/features/packageManager.api.unit.test.ts b/src/test/features/packageManager.api.unit.test.ts index 8652fdbee..5c3de8569 100644 --- a/src/test/features/packageManager.api.unit.test.ts +++ b/src/test/features/packageManager.api.unit.test.ts @@ -1007,6 +1007,7 @@ suite('PythonPackageManagerApi Tests', () => { projectManager.setup((pm) => pm.get(projectUri)).returns(() => currentProject); const scopedManagers: PackageManager[] = []; const scopedEmitters: EventEmitter[] = []; + const scopedDisposers: sinon.SinonStub[] = []; const provider: PackageManager = { name: 'project-pkg-mgr', manage: async () => undefined, @@ -1014,14 +1015,17 @@ suite('PythonPackageManagerApi Tests', () => { getPackages: async () => [], createForProject: () => { const emitter = new EventEmitter(); + const dispose = sinon.stub(); const scopedManager: PackageManager = { name: 'project-pkg-mgr', manage: async () => undefined, refresh: async () => undefined, getPackages: async () => [], onDidChangePackages: emitter.event, + dispose, }; scopedEmitters.push(emitter); + scopedDisposers.push(dispose); scopedManagers.push(scopedManager); return scopedManager; }, @@ -1043,6 +1047,8 @@ suite('PythonPackageManagerApi Tests', () => { assert.notStrictEqual(original, replacement); assert.strictEqual(scopedManagers.length, 2); assert.strictEqual(replacement?.project, currentProject); + assert.ok(scopedDisposers[0].calledOnce); + assert.ok(scopedDisposers[1].notCalled); const packageChange = { environment: environment.object, @@ -1059,5 +1065,71 @@ suite('PythonPackageManagerApi Tests', () => { eventDisposable.dispose(); scopedEmitters.forEach((emitter) => emitter.dispose()); }); + + test('Should dispose scoped package managers when their provider is unregistered', () => { + disposable.dispose(); + const project = { + name: 'project', + uri: Uri.file(path.join(process.cwd(), 'unregistered-provider-project')), + } as PythonProject; + projectManager.setup((pm) => pm.get(typeMoq.It.isAny())).returns(() => project); + const scopedDispose = sinon.stub(); + disposable = envManagers.registerPackageManager({ + name: 'project-pkg-mgr', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + createForProject: () => ({ + name: 'project-pkg-mgr', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + dispose: scopedDispose, + }), + }); + const registeredManager = envManagers.packageManagers[0]; + sinon.stub(workspaceApis, 'getConfiguration').returns({ + get: (section: string, defaultValue?: unknown) => + section === 'defaultPackageManager' ? registeredManager.id : defaultValue, + } as WorkspaceConfiguration); + assert.ok(envManagers.getPackageManager(project.uri)?.project); + + disposable.dispose(); + + assert.ok(scopedDispose.calledOnce); + }); + + test('Should dispose scoped package managers during environment-manager shutdown', () => { + disposable.dispose(); + const project = { + name: 'project', + uri: Uri.file(path.join(process.cwd(), 'shutdown-project')), + } as PythonProject; + projectManager.setup((pm) => pm.get(typeMoq.It.isAny())).returns(() => project); + const scopedDispose = sinon.stub(); + disposable = envManagers.registerPackageManager({ + name: 'project-pkg-mgr', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + createForProject: () => ({ + name: 'project-pkg-mgr', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + dispose: scopedDispose, + }), + }); + const registeredManager = envManagers.packageManagers[0]; + sinon.stub(workspaceApis, 'getConfiguration').returns({ + get: (section: string, defaultValue?: unknown) => + section === 'defaultPackageManager' ? registeredManager.id : defaultValue, + } as WorkspaceConfiguration); + assert.ok(envManagers.getPackageManager(project.uri)?.project); + + envManagers.dispose(); + + assert.ok(scopedDispose.calledOnce); + }); }); }); diff --git a/src/test/managers/poetry/poetryPackageManager.unit.test.ts b/src/test/managers/poetry/poetryPackageManager.unit.test.ts index 13e61e4be..a22062cfb 100644 --- a/src/test/managers/poetry/poetryPackageManager.unit.test.ts +++ b/src/test/managers/poetry/poetryPackageManager.unit.test.ts @@ -8,6 +8,7 @@ import { FileType, LogOutputChannel, Uri } from 'vscode'; import { PythonEnvironmentApi } from '../../../api'; import * as windowApis from '../../../common/window.apis'; import * as workspaceFs from '../../../common/workspace.fs.apis'; +import { PackageManagerRequiresProjectError } from '../../../managers/common/errors'; import * as packageChanges from '../../../managers/common/packageChanges'; import * as runPoetryModule from '../../../managers/poetry/commands/runPoetry'; import { PoetryPackageManager } from '../../../managers/poetry/poetryPackageManager'; @@ -144,8 +145,8 @@ suite('PoetryPackageManager', () => { assert.strictEqual(runPoetryStub.callCount, 0); }); - test('refresh is a no-op without a project', async () => { - await manager.refresh(environment); + test('refresh rejects without a project', async () => { + await assert.rejects(manager.refresh(environment), PackageManagerRequiresProjectError); assert.strictEqual(runPoetryStub.callCount, 0); }); }); diff --git a/src/types.ts b/src/types.ts index 2631dcebb..d6f8ff624 100644 --- a/src/types.ts +++ b/src/types.ts @@ -715,6 +715,9 @@ export interface PackageManager { /** * Event that is fired when packages change. + * + * A manager returned by createForProject must report that exact manager instance in + * DidChangePackagesEventArgs.manager, even when root and scoped managers share an emitter. */ onDidChangePackages?: Event; @@ -735,6 +738,14 @@ export interface PackageManager { */ createForProject?(project: PythonProject): PackageManager; + /** + * Releases resources owned by this package manager. + * + * The extension invokes this method for managers returned by createForProject when their + * project is removed or replaced, their provider is unregistered, or the extension shuts down. + */ + dispose?(): void; + /** * Fetches the names of direct (non-transitive) packages for the specified Python environment. * From a9d499decea8c96f19963ef9a07c139e48385d2f Mon Sep 17 00:00:00 2001 From: Eduardo Villalpando Mello Date: Fri, 25 Sep 2026 15:55:50 -0700 Subject: [PATCH 08/11] Simplify --- .../testing-workflow.instructions.md | 1 + api/CHANGELOG.md | 5 + docs/README.md | 50 +++-- src/extension.ts | 11 +- src/extensionApi.ts | 34 +-- src/features/envCommands.ts | 17 +- src/features/envManagers.ts | 196 +++++++----------- src/features/views/envManagersView.ts | 5 +- src/features/views/projectView.ts | 7 +- src/managers/common/errors.ts | 12 +- .../projectScopedPackageManagerCache.ts | 123 +++++++++++ src/managers/common/registeredManagers.ts | 15 ++ src/publicErrors.ts | 42 ++++ src/test/extensionApi.unit.test.ts | 82 +++++++- .../features/packageManager.api.unit.test.ts | 59 ++++-- .../views/envManagersView.unit.test.ts | 24 +++ ...jectScopedPackageManagerCache.unit.test.ts | 183 ++++++++++++++++ src/types.ts | 13 +- 18 files changed, 687 insertions(+), 192 deletions(-) create mode 100644 src/managers/common/projectScopedPackageManagerCache.ts create mode 100644 src/test/managers/common/projectScopedPackageManagerCache.unit.test.ts diff --git a/.github/instructions/testing-workflow.instructions.md b/.github/instructions/testing-workflow.instructions.md index 1164921d3..6eeac8aa3 100644 --- a/.github/instructions/testing-workflow.instructions.md +++ b/.github/instructions/testing-workflow.instructions.md @@ -19,6 +19,7 @@ This guide covers the full testing lifecycle: ## Learnings - Pip commands that return JSON must pass `--disable-pip-version-check`; the process helper combines stderr with stdout, so update notices can otherwise make valid JSON unparseable (1). +- When a view subscribes to a newly added provider event, TypeMoq-based view tests must return a real `EventEmitter.event`; an unstubbed event yields an undefined disposable and fails during teardown (1). ### When to Use This Guide diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index 0ba6d37fb..52f4112e3 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added the optional `PackageManager.createForProject` factory for package managers whose operations depend on the calling Python project. Explicit project contexts are used directly; environment-only operations use a scoped manager only when exactly one tracked project matches. - Added optional `PackageManager.dispose` support for releasing resources owned by project-scoped package managers. +- Added `PackageManagerRequiresProjectError` and `isPackageManagerRequiresProjectError` for environment-only package mutations and refreshes that cannot identify a unique project. + +### Changed + +- Environment-only package operations no longer fall back to an unbound project-aware package manager. Package reads return `undefined`; mutations and refreshes reject with `PackageManagerRequiresProjectError`. ## [1.4.0] diff --git a/docs/README.md b/docs/README.md index 3a3518d48..1e528b271 100644 --- a/docs/README.md +++ b/docs/README.md @@ -793,8 +793,8 @@ getPackages( **Returns** `Promise`. `undefined` means the manager could not produce a list - for example no package manager is associated with -the environment - which is different from an empty array meaning "nothing -installed". +the environment or a project-aware manager cannot identify one unique project - +which is different from an empty array meaning "nothing installed". ```typescript const packages = await api.getPackages(env); @@ -818,7 +818,9 @@ refreshPackages(environment: PythonEnvironment): Promise; | `environment` | [`PythonEnvironment`](#pythonenvironment) | Yes | The environment whose package list should be refreshed. | **Returns** `Promise`. Changes surface through -[`onDidChangePackages`](#ondidchangepackages). +[`onDidChangePackages`](#ondidchangepackages). Rejects with +`PackageManagerRequiresProjectError` when a project-aware manager cannot identify +one unique project for the environment. ```typescript // Packages were installed outside the extension - re-read the list. @@ -842,8 +844,9 @@ managePackages( | `environment` | [`PythonEnvironment`](#pythonenvironment) | Yes | The environment to modify. | | `options` | [`PackageManagementOptions`](#packagemanagementoptions) | Yes | Must specify `install`, `uninstall`, or both. Also carries `upgrade`, `showSkipOption`, and `runHeadless`. | -**Returns** `Promise`, resolving when the operation finishes. Rejects if -the underlying tool fails. +**Returns** `Promise`, resolving when the operation finishes. Rejects with +`PackageManagerRequiresProjectError` when a project-aware manager cannot identify +one unique project for the environment, or if the underlying tool fails. ```typescript await api.managePackages(env, { @@ -973,6 +976,30 @@ context.subscriptions.push( ### Package errors +#### `PackageManagerRequiresProjectError` + +Thrown by environment-only package mutations and refreshes when the selected +package manager is project-aware but the environment does not identify exactly +one tracked project. Its `code` is the stable +`'PackageManagerRequiresProject'` discriminator. + +Use `isPackageManagerRequiresProjectError(error)` instead of `instanceof` when +the error may cross extension bundle boundaries: + +```typescript +import { isPackageManagerRequiresProjectError } from '@vscode/python-environments'; + +try { + await api.refreshPackages(env); +} catch (error) { + if (isPackageManagerRequiresProjectError(error)) { + // Ask the user to open or select the intended Python project. + } else { + throw error; + } +} +``` + #### `PackageVersionLookupNotSupportedError` Thrown when a package manager cannot list available versions at all. It @@ -1631,13 +1658,12 @@ operations, the extension selects a project-scoped manager only when exactly one tracked project uses the environment; it does not choose arbitrarily when no project or multiple projects match. -Environment-only command and API paths may still use the originally registered -manager as an unbound fallback, while package views can suppress operations when -there is no unique project. Project-sensitive methods on the root manager must -therefore fail clearly or report that data is unavailable rather than running -from the extension host's working directory. Keep project-specific caches and -mutable state on the manager returned by `createForProject`, and implement -`dispose` when that manager owns resources. +Environment-only paths never use a project-aware provider as an unbound +fallback. When no unique project can be inferred, package reads return +`undefined`, package views suppress those operations, and package mutations or +refreshes reject with `PackageManagerRequiresProjectError`. Keep +project-specific caches and mutable state on the manager returned by +`createForProject`, and implement `dispose` when that manager owns resources. When a scoped manager fires `onDidChangePackages`, the event's `manager` must be the exact scoped instance returned by `createForProject`. This requirement also diff --git a/src/extension.ts b/src/extension.ts index 058843dbf..d585e2b82 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -298,9 +298,7 @@ export async function activate(context: ExtensionContext): Promise { - const manager = - (await envManagers.resolvePackageManager(environment)) ?? - envManagers.getPackageManager(environment); + const { manager } = envManagers.resolvePackageManagerForEnvironment(environment); const names = await manager?.getDirectPackageNames?.(environment); return names ? Array.from(names) : undefined; }, @@ -361,11 +359,14 @@ export async function activate(context: ExtensionContext): Promise { await waitForEnvManagerId([context.envId.managerId]); - const manager = - (await this.envManagers.resolvePackageManager(context)) ?? this.envManagers.getPackageManager(context); - if (!manager) { - return Promise.reject(new Error('No package manager found')); - } + const manager = this.requirePackageManagerForEnvironment(context); return manager.manage(context, options); } async refreshPackages(context: PythonEnvironment): Promise { await waitForEnvManagerId([context.envId.managerId]); - const manager = - (await this.envManagers.resolvePackageManager(context)) ?? this.envManagers.getPackageManager(context); - if (!manager) { - return Promise.reject(new Error('No package manager found')); - } + const manager = this.requirePackageManagerForEnvironment(context); return manager.refresh(context); } async getPackages(context: PythonEnvironment, options?: GetPackagesOptions): Promise { await waitForEnvManagerId([context.envId.managerId]); - const manager = - (await this.envManagers.resolvePackageManager(context)) ?? this.envManagers.getPackageManager(context); - if (!manager) { - return Promise.resolve(undefined); + const { manager } = this.envManagers.resolvePackageManagerForEnvironment(context); + return manager?.getPackages(context, options); + } + + private requirePackageManagerForEnvironment(context: PythonEnvironment): InternalPackageManager { + const resolution = this.envManagers.resolvePackageManagerForEnvironment(context); + switch (resolution.kind) { + case 'resolved': + return resolution.manager; + case 'projectRequired': + throw new PackageManagerRequiresProjectError(); + case 'notFound': + throw new Error('No package manager found'); } - return manager.getPackages(context, options); } + getPackageAvailableVersions( context: PythonEnvironment, packageName: string, diff --git a/src/features/envCommands.ts b/src/features/envCommands.ts index bfa5f4850..583770f90 100644 --- a/src/features/envCommands.ts +++ b/src/features/envCommands.ts @@ -138,14 +138,14 @@ export async function refreshPackagesCommand(context: unknown, managers?: Enviro if (context instanceof ProjectEnvironment) { const view = context as ProjectEnvironment; if (managers) { - const pkgManager = await managers.resolvePackageManager(view.environment, view.parent.project); + const pkgManager = managers.getPackageManagerForProject(view.parent.project); if (pkgManager) { await pkgManager.refresh(view.environment); } } } else if (context instanceof PythonEnvTreeItem) { const view = context as PythonEnvTreeItem; - const pkgManager = await managers?.resolvePackageManager(view.environment); + const pkgManager = managers?.resolvePackageManagerForEnvironment(view.environment).manager; if (pkgManager) { await pkgManager.refresh(view.environment); } @@ -793,7 +793,7 @@ async function resolvePackageCommandOptions( if (e instanceof ProjectEnvironment) { const environment = e.environment; - const packageManager = await em.resolvePackageManager(environment, e.parent.project); + const packageManager = em.getPackageManagerForProject(e.parent.project); if (packageManager) { return { environment, packageManager }; } @@ -801,9 +801,14 @@ async function resolvePackageCommandOptions( if (e instanceof PythonEnvTreeItem) { const environment = e.environment; - const packageManager = (await em.resolvePackageManager(environment)) ?? em.getPackageManager(environment); - if (packageManager) { - return { environment, packageManager }; + const resolution = em.resolvePackageManagerForEnvironment(environment); + switch (resolution.kind) { + case 'resolved': + return { environment, packageManager: resolution.manager }; + case 'projectRequired': + throw new PackageManagerRequiresProjectError(); + case 'notFound': + break; } } diff --git a/src/features/envManagers.ts b/src/features/envManagers.ts index 4faed374b..5cb9dd926 100644 --- a/src/features/envManagers.ts +++ b/src/features/envManagers.ts @@ -30,6 +30,7 @@ import { sendTelemetryEvent } from '../common/telemetry/sender'; import { getCallingExtension } from '../common/utils/frameUtils'; import { normalizePath } from '../common/utils/pathUtils'; import { InternalEnvironmentManager, InternalPackageManager } from '../managers/common/registeredManagers'; +import { ProjectScopedPackageManagerCache } from '../managers/common/projectScopedPackageManagerCache'; import type { PythonProjectManager, PythonProjectSettings } from './projectManager'; import { EditAllManagerSettings, @@ -86,6 +87,11 @@ export interface InternalDidChangeEnvironmentsEventArgs { changes: DidChangeEnvironmentsEventArgs; } +export type PackageManagerResolution = + | { kind: 'resolved'; manager: InternalPackageManager } + | { kind: 'projectRequired'; manager?: never } + | { kind: 'notFound'; manager?: never }; + export interface EnvironmentManagers extends Disposable { registerEnvironmentManager(manager: EnvironmentManager, options?: { extensionId?: string }): Disposable; registerPackageManager(manager: PackageManager, options?: { extensionId?: string }): Disposable; @@ -115,25 +121,27 @@ export interface EnvironmentManagers extends Disposable { onDidChangeEnvironmentManager: Event; onDidChangePackageManager: Event; + /** Fires when cached project-scoped managers are replaced or removed. */ + onDidChangeProjectPackageManager: Event; getEnvironmentManager(scope: EnvironmentManagerScope): InternalEnvironmentManager | undefined; getPackageManager(scope: PackageManagerScope): InternalPackageManager | undefined; /** - * Resolves the package manager for an environment and optional explicit project context. + * Returns the configured package manager for an explicit tracked project. * - * When a project is supplied, its configured project-scoped manager is returned directly. - * Without a project, project-independent providers are returned directly and project-aware - * providers are scoped only when exactly one tracked project currently uses the environment. + * @param project The project whose package manager should be returned. + * @returns The shared or project-scoped package manager. + */ + getPackageManagerForProject(project: PythonProject): InternalPackageManager | undefined; + + /** + * Resolves a package manager for an environment using last-known project selections. * * @param environment The environment whose package manager should be resolved. - * @param project The project to bind to, when the caller has explicit project context. - * @returns The package manager, or undefined when project ownership is absent or ambiguous. + * @returns A resolved manager or the reason resolution was not possible. */ - resolvePackageManager( - environment: PythonEnvironment, - project?: PythonProject, - ): Promise; + resolvePackageManagerForEnvironment(environment: PythonEnvironment): PackageManagerResolution; managers: InternalEnvironmentManager[]; packageManagers: InternalPackageManager[]; @@ -188,8 +196,8 @@ function generateId(name: string, extensionId?: string): string { export class PythonEnvironmentManagers implements EnvironmentManagers { private _environmentManagers: Map = new Map(); private _packageManagers: Map = new Map(); - private _projectPackageManagers: Map = new Map(); - private readonly _packageManagerEventSubscriptions = new Map(); + private readonly packageManagerEventSubscriptions = new Map(); + private readonly projectPackageManagers: ProjectScopedPackageManagerCache; private readonly subscriptions: Disposable[] = []; /** @@ -209,6 +217,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { private _onDidChangeEnvironmentManager = new EventEmitter(); private _onDidChangePackageManager = new EventEmitter(); + private _onDidChangeProjectPackageManager = new EventEmitter(); private _onDidChangeEnvironments = new EventEmitter(); /** Fires when ANY manager reports a selection change, regardless of whether that manager is selected. */ @@ -222,6 +231,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { public onDidChangeEnvironmentManager: Event = this._onDidChangeEnvironmentManager.event; public onDidChangePackageManager: Event = this._onDidChangePackageManager.event; + public onDidChangeProjectPackageManager: Event = this._onDidChangeProjectPackageManager.event; public onDidChangeEnvironments: Event = this._onDidChangeEnvironments.event; /** Fires when any registered manager reports a change — even if that manager is not the selected one. */ @@ -248,10 +258,14 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { private readonly pm: PythonProjectManager, private readonly inlineScriptRouting?: InlineScriptRoutingRegistry, ) { + this.projectPackageManagers = new ProjectScopedPackageManagerCache( + (manager) => this.subscribeToPackageManagerEvents(manager), + () => this._onDidChangeProjectPackageManager.fire(), + ); const projectChanges = this.pm.onDidChangeProjects; if (projectChanges) { const subscription = projectChanges((projects) => - this.evictRemovedProjectPackageManagers(projects ?? this.pm.getProjects()), + this.projectPackageManagers.reconcileProjects(projects ?? this.pm.getProjects()), ); if (subscription) { this.subscriptions.push(subscription); @@ -334,7 +348,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { } const mgr = new InternalPackageManager(managerId, manager); - this.subscribeToPackageManagerEvents(mgr); + this.packageManagerEventSubscriptions.set(mgr, this.subscribeToPackageManagerEvents(mgr)); this._packageManagers.set(managerId, mgr); this._onDidChangePackageManager.fire({ kind: 'registered', manager: mgr }); @@ -347,14 +361,9 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { return new Disposable(() => { this._packageManagers.delete(managerId); - for (const [key, scopedManager] of this._projectPackageManagers) { - if (scopedManager.id === managerId) { - this._projectPackageManagers.delete(key); - this.unsubscribeFromPackageManagerEvents(scopedManager); - scopedManager.dispose(); - } - } - this.disposePackageManagerEventSubscriptions(managerId); + this.projectPackageManagers.removeProvider(mgr); + this.packageManagerEventSubscriptions.get(mgr)?.dispose(); + this.packageManagerEventSubscriptions.delete(mgr); setImmediate(() => this._onDidChangePackageManager.fire({ kind: 'unregistered', manager: mgr })); }); } @@ -362,19 +371,16 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { public dispose() { this._environmentManagers.clear(); this._packageManagers.clear(); - for (const manager of this._projectPackageManagers.values()) { - this.unsubscribeFromPackageManagerEvents(manager); - manager.dispose(); - } - this._projectPackageManagers.clear(); - for (const subscription of this._packageManagerEventSubscriptions.values()) { + this.projectPackageManagers.dispose(); + for (const subscription of this.packageManagerEventSubscriptions.values()) { subscription.dispose(); } - this._packageManagerEventSubscriptions.clear(); + this.packageManagerEventSubscriptions.clear(); this._inlineRoutingOverrides.clear(); this.subscriptions.forEach((subscription) => subscription.dispose()); this._onDidChangeEnvironmentManager.dispose(); this._onDidChangePackageManager.dispose(); + this._onDidChangeProjectPackageManager.dispose(); this._onDidChangeEnvironments.dispose(); this._onDidChangeManagerEnvironment.dispose(); this._onDidChangeActiveEnvironment.dispose(); @@ -448,17 +454,17 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { const defaultPkgManagerId = getDefaultPkgManagerSetting(this.pm, context); const defaultEnvManagerId = getDefaultEnvManagerSetting(this.pm, context); if (defaultPkgManagerId) { - return this.getOrCreateProjectScopedManager(this._packageManagers.get(defaultPkgManagerId), project); + return project + ? this.projectPackageManagers.getOrCreate(this._packageManagers.get(defaultPkgManagerId), project) + : this._packageManagers.get(defaultPkgManagerId); } if (defaultEnvManagerId) { const preferredPkgManagerId = this._environmentManagers.get(defaultEnvManagerId)?.preferredPackageManagerId; if (preferredPkgManagerId) { - return this.getOrCreateProjectScopedManager( - this._packageManagers.get(preferredPkgManagerId), - project, - ); + const manager = this._packageManagers.get(preferredPkgManagerId); + return project ? this.projectPackageManagers.getOrCreate(manager, project) : manager; } } return undefined; @@ -480,111 +486,59 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { return undefined; } - public async resolvePackageManager( - environment: PythonEnvironment, - project?: PythonProject, - ): Promise { - if (project) { - return this.getPackageManager(project.uri); + public getPackageManagerForProject(project: PythonProject): InternalPackageManager | undefined { + const canonicalProject = this.pm.get(project.uri); + if (canonicalProject !== project) { + traceVerbose(`Unable to resolve package manager for untracked project ${project.uri.fsPath}`); + return undefined; } + return this.getPackageManager(canonicalProject.uri); + } + public resolvePackageManagerForEnvironment( + environment: PythonEnvironment, + ): PackageManagerResolution { const manager = this.getPackageManager(environment); - if (!manager?.createForProject) { - return manager; + if (!manager) { + return { kind: 'notFound' }; + } + if (!manager.createForProject) { + return { kind: 'resolved', manager }; } - const projects = this.pm.getProjects(); - const projectEnvironments = await Promise.all( - projects.map(async (project) => ({ - project, - environment: await this.getEnvironment(project.uri), - })), - ); - const matchingProjects = projectEnvironments.filter(({ environment: projectEnvironment }) => - this.isSameEnvironment(environment, projectEnvironment), + const matchingProjects = this.pm.getProjects().filter((project) => + this.isSameEnvironment(environment, this.getLastKnownEnvironment(project.uri)), ); if (matchingProjects.length !== 1) { traceVerbose( `Unable to resolve project-scoped package manager for environment ${environment.envId.id}: ` + `found ${matchingProjects.length} matching projects`, ); - return undefined; - } - - const resolvedManager = this.getPackageManager(matchingProjects[0].project.uri); - if (resolvedManager?.createForProject && !resolvedManager.project) { - return undefined; + return { kind: 'projectRequired' }; } - return resolvedManager; - } - private getOrCreateProjectScopedManager( - manager: InternalPackageManager | undefined, - project: PythonProject | undefined, - ): InternalPackageManager | undefined { - if (!manager || !project) { - return manager; - } - - const key = `${manager.id}:${normalizePath(project.uri.fsPath)}`; - let scopedManager = this._projectPackageManagers.get(key); - if (!scopedManager) { - scopedManager = manager.createForProject?.(project); - if (scopedManager) { - this._projectPackageManagers.set(key, scopedManager); - this.subscribeToPackageManagerEvents(scopedManager); - } - } - return scopedManager ?? manager; + const scopedManager = this.getPackageManagerForProject(matchingProjects[0]); + return scopedManager + ? { kind: 'resolved', manager: scopedManager } + : { kind: 'notFound' }; } - private subscribeToPackageManagerEvents(manager: InternalPackageManager): void { + private subscribeToPackageManagerEvents(manager: InternalPackageManager): Disposable { const event = manager.packageChangeEvent; - if (!event || this._packageManagerEventSubscriptions.has(manager)) { - return; - } - - this._packageManagerEventSubscriptions.set( - manager, - event((e) => { - this._onDidChangePackageProviderPackages.fire(e); - setImmediate(() => - this._onDidChangePackages.fire({ - environment: e.environment, - manager, - changes: e.changes, - }), - ); - }), - ); - } - - private unsubscribeFromPackageManagerEvents(manager: InternalPackageManager): void { - this._packageManagerEventSubscriptions.get(manager)?.dispose(); - this._packageManagerEventSubscriptions.delete(manager); - } - - private evictRemovedProjectPackageManagers(projects: readonly PythonProject[]): void { - const projectsByPath = new Map( - projects.map((project) => [normalizePath(project.uri.fsPath), project] as const), - ); - for (const [key, manager] of this._projectPackageManagers) { - const projectPath = manager.project && normalizePath(manager.project.uri.fsPath); - if (!projectPath || projectsByPath.get(projectPath) !== manager.project) { - this._projectPackageManagers.delete(key); - this.unsubscribeFromPackageManagerEvents(manager); - manager.dispose(); - } + if (!event) { + return new Disposable(() => {}); } - } - private disposePackageManagerEventSubscriptions(managerId: string): void { - for (const [manager, subscription] of this._packageManagerEventSubscriptions) { - if (manager.id === managerId) { - subscription.dispose(); - this._packageManagerEventSubscriptions.delete(manager); - } - } + return event((e) => { + this._onDidChangePackageProviderPackages.fire(e); + setImmediate(() => + this._onDidChangePackages.fire({ + environment: e.environment, + manager, + changes: e.changes, + }), + ); + }); } public get managers(): InternalEnvironmentManager[] { diff --git a/src/features/views/envManagersView.ts b/src/features/views/envManagersView.ts index 66dcfffcf..3c3207c2d 100644 --- a/src/features/views/envManagersView.ts +++ b/src/features/views/envManagersView.ts @@ -115,6 +115,9 @@ export class EnvManagerView implements TreeDataProvider, Disposable this.providers.onDidChangePackageManager((p: DidChangePackageManagerEventArgs) => { this.onDidChangePackageManager(p); }), + this.providers.onDidChangeProjectPackageManager(() => { + this.fireDataChanged(undefined); + }), ); this.disposables.push( @@ -240,7 +243,7 @@ export class EnvManagerView implements TreeDataProvider, Disposable if (element.kind === EnvTreeItemKind.environment) { const pythonEnvItem = element as PythonEnvTreeItem; const environment = pythonEnvItem.environment; - const pkgManager = await this.providers.resolvePackageManager(environment); + const { manager: pkgManager } = this.providers.resolvePackageManagerForEnvironment(environment); const parent = element as PythonEnvTreeItem; const views: EnvTreeItem[] = []; diff --git a/src/features/views/projectView.ts b/src/features/views/projectView.ts index 6daa59876..78c0e35b2 100644 --- a/src/features/views/projectView.ts +++ b/src/features/views/projectView.ts @@ -66,6 +66,9 @@ export class ProjectView implements TreeDataProvider { this.envManagers.onDidChangeEnvironments(() => { this.debouncedUpdateProject.trigger(); }), + this.envManagers.onDidChangeProjectPackageManager(() => { + this.debouncedUpdateProject.trigger(); + }), this.envManagers.onDidChangePackages((e) => { this.updatePackagesForEnvironment(e.environment); }), @@ -240,7 +243,9 @@ export class ProjectView implements TreeDataProvider { const project = parent.id === 'global' ? undefined : parent.project; const uri = project?.uri; const environment = environmentItem.environment; - const pkgManager = await this.envManagers.resolvePackageManager(environment, project); + const pkgManager = project + ? this.envManagers.getPackageManagerForProject(project) + : this.envManagers.resolvePackageManagerForEnvironment(environment).manager; if (!pkgManager) { return [new ProjectEnvironmentInfo(environmentItem, ProjectViews.noPackageManager)]; diff --git a/src/managers/common/errors.ts b/src/managers/common/errors.ts index 1ff0d24ea..eacb7e31c 100644 --- a/src/managers/common/errors.ts +++ b/src/managers/common/errors.ts @@ -2,18 +2,10 @@ // Licensed under the MIT License. import { l10n } from 'vscode'; +import { PackageManagerRequiresProjectError as PublicPackageManagerRequiresProjectError } from '../../publicErrors'; -/** - * Raised when a package-management operation is invoked on a package manager that is not - * bound to a Python project. - * - * Callers that resolve package managers without a specific project (e.g. the environment - * manager view) should catch this error and surface a friendly message rather than letting - * it propagate as an unhandled failure. - */ -export class PackageManagerRequiresProjectError extends Error { +export class PackageManagerRequiresProjectError extends PublicPackageManagerRequiresProjectError { constructor() { super(l10n.t('Package operations require a Python project.')); - this.name = 'PackageManagerRequiresProjectError'; } } diff --git a/src/managers/common/projectScopedPackageManagerCache.ts b/src/managers/common/projectScopedPackageManagerCache.ts new file mode 100644 index 000000000..43d180ae0 --- /dev/null +++ b/src/managers/common/projectScopedPackageManagerCache.ts @@ -0,0 +1,123 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { Disposable } from 'vscode'; +import { PythonProject } from '../../api'; +import { InternalPackageManager } from './registeredManagers'; + +interface ProjectScopedPackageManagerEntry { + provider: InternalPackageManager; + manager: InternalPackageManager; + subscription: Disposable; +} + +/** + * Owns the package manager scoped to each tracked Python project. + * + * A project has at most one active scoped manager. Changing its configured provider replaces and + * disposes the previous manager. Project removal, provider removal, and cache disposal also release + * the scoped manager and its event subscription. + */ +export class ProjectScopedPackageManagerCache implements Disposable { + private readonly entries = new Map(); + + constructor( + private readonly subscribe: (manager: InternalPackageManager) => Disposable, + private readonly onDidInvalidate: () => void, + ) {} + + /** + * Returns the manager for a project, creating and caching a scoped manager when supported. + * + * @param provider The project's currently configured root package manager. + * @param project The canonical tracked project. + * @returns The scoped manager, the shared provider, or undefined when no provider is configured. + */ + getOrCreate( + provider: InternalPackageManager | undefined, + project: PythonProject, + ): InternalPackageManager | undefined { + const existing = this.entries.get(project); + if (!provider?.createForProject) { + if (existing) { + this.entries.delete(project); + this.disposeEntry(existing); + this.onDidInvalidate(); + } + return provider; + } + if (existing?.provider === provider) { + return existing.manager; + } + + const manager = provider.createForProject(project); + let subscription: Disposable; + try { + subscription = this.subscribe(manager); + } catch (error) { + manager.dispose(); + throw error; + } + + if (existing) { + this.disposeEntry(existing); + } + this.entries.set(project, { provider, manager, subscription }); + if (existing) { + this.onDidInvalidate(); + } + return manager; + } + + /** + * Disposes entries whose canonical projects are no longer tracked. + * + * @param projects The complete current set of tracked projects. + */ + reconcileProjects(projects: readonly PythonProject[]): void { + const activeProjects = new Set(projects); + let invalidated = false; + for (const [project, entry] of this.entries) { + if (!activeProjects.has(project)) { + this.entries.delete(project); + this.disposeEntry(entry); + invalidated = true; + } + } + if (invalidated) { + this.onDidInvalidate(); + } + } + + /** + * Disposes all scoped managers created from a provider. + * + * @param provider The provider being unregistered. + */ + removeProvider(provider: InternalPackageManager): void { + let invalidated = false; + for (const [project, entry] of this.entries) { + if (entry.provider === provider) { + this.entries.delete(project); + this.disposeEntry(entry); + invalidated = true; + } + } + if (invalidated) { + this.onDidInvalidate(); + } + } + + /** Disposes every cached scoped manager and its event subscription. */ + dispose(): void { + for (const entry of this.entries.values()) { + this.disposeEntry(entry); + } + this.entries.clear(); + } + + private disposeEntry(entry: ProjectScopedPackageManagerEntry): void { + entry.subscription.dispose(); + entry.manager.dispose(); + } +} diff --git a/src/managers/common/registeredManagers.ts b/src/managers/common/registeredManagers.ts index 72cd84c77..4c929eaa2 100644 --- a/src/managers/common/registeredManagers.ts +++ b/src/managers/common/registeredManagers.ts @@ -236,6 +236,7 @@ export class InternalPackageManager implements PackageManager { const createForProject = manager.createForProject?.bind(manager); if (createForProject) { this.createForProject = (scopedProject) => { + this.throwIfDisposed(); const scopedManager = createForProject(scopedProject); if (!scopedManager) { throw new Error(`Package manager ${this.id} did not create a manager for the requested project`); @@ -270,6 +271,7 @@ export class InternalPackageManager implements PackageManager { } async manage(environment: PythonEnvironment, options: PackageManagementOptions): Promise { + this.throwIfDisposed(); const stopWatch = new StopWatch(); const triggerSource = inferPackageManagementTrigger(options); try { @@ -299,14 +301,17 @@ export class InternalPackageManager implements PackageManager { } refresh(environment: PythonEnvironment): Promise { + this.throwIfDisposed(); return this.manager.refresh(environment); } getPackages(environment: PythonEnvironment, options?: GetPackagesOptions): Promise { + this.throwIfDisposed(); return this.manager.getPackages(environment, options); } getPackageWatchTargets(environment: PythonEnvironment): RelativePattern[] { + this.throwIfDisposed(); return this.manager.getPackageWatchTargets?.(environment) ?? []; } @@ -334,6 +339,7 @@ export class InternalPackageManager implements PackageManager { } getVersion(environment: PythonEnvironment): Promise { + this.throwIfDisposed(); return this.manager.getVersion ? this.manager.getVersion(environment) : Promise.resolve(undefined); } @@ -356,6 +362,7 @@ export class InternalPackageManager implements PackageManager { packageName: string, options?: GetPackageAvailableVersionsOptions, ): Promise { + this.throwIfDisposed(); const shouldThrow = options?.errorMode === 'throw'; try { if (!this.manager.getPackageAvailableVersions) { @@ -379,14 +386,22 @@ export class InternalPackageManager implements PackageManager { } getDirectPackageNames(environment: PythonEnvironment): Promise | undefined> { + this.throwIfDisposed(); return this.manager.getDirectPackageNames ? this.manager.getDirectPackageNames(environment) : Promise.resolve(undefined); } formatInstallSpec(packageName: string, version: string): string { + this.throwIfDisposed(); return this.manager.formatInstallSpec ? this.manager.formatInstallSpec(packageName, version) : `${packageName}==${version}`; } + + private throwIfDisposed(): void { + if (this.isDisposed) { + throw new Error(`Package manager ${this.id} has been disposed`); + } + } } diff --git a/src/publicErrors.ts b/src/publicErrors.ts index f4799d8ce..efc68519f 100644 --- a/src/publicErrors.ts +++ b/src/publicErrors.ts @@ -7,6 +7,48 @@ * that facade small. */ +/** + * Error thrown when a project-aware package manager cannot determine which Python project to use. + * + * The {@link code} property is a stable discriminator that can be checked across extension bundle + * boundaries with {@link isPackageManagerRequiresProjectError}. + */ +export class PackageManagerRequiresProjectError extends Error { + /** + * Stable discriminator identifying this error type across bundle boundaries. + */ + public readonly code = 'PackageManagerRequiresProject'; + + /** + * Creates a project-required package error. + * + * @param message Optional caller-facing explanation. + */ + constructor(message?: string) { + super(message ?? 'Package operations require a Python project.'); + this.name = 'PackageManagerRequiresProjectError'; + Object.setPrototypeOf(this, new.target.prototype); + } +} + +/** + * Reports whether an error means that a package operation requires an unambiguous Python project. + * + * @param error The value to test. + * @returns `true` when the error carries the project-required discriminator. + */ +export function isPackageManagerRequiresProjectError( + error: unknown, +): error is PackageManagerRequiresProjectError { + return ( + error instanceof PackageManagerRequiresProjectError || + (typeof error === 'object' && + error !== null && + 'code' in error && + (error as { code?: unknown }).code === 'PackageManagerRequiresProject') + ); +} + /** * Error thrown when a package manager cannot list available package versions. * diff --git a/src/test/extensionApi.unit.test.ts b/src/test/extensionApi.unit.test.ts index b34a79e6f..12aa2802f 100644 --- a/src/test/extensionApi.unit.test.ts +++ b/src/test/extensionApi.unit.test.ts @@ -1,10 +1,16 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import { EventEmitter, Uri } from 'vscode'; -import { PythonEnvironment, PythonProject } from '../api'; +import { + isPackageManagerRequiresProjectError, + PythonEnvironment, + PythonProject, +} from '../api'; import * as managerReady from '../features/common/managerReady'; import { PythonEnvironmentApiImpl } from '../extensionApi'; +import type { PackageManagerResolution } from '../features/envManagers'; import type { PythonProjectManager } from '../features/projectManager'; +import type { InternalPackageManager } from '../managers/common/registeredManagers'; suite('PythonEnvironmentApiImpl - onDidChangePythonProjects', () => { test('fires event with correct added and removed projects', () => { @@ -165,3 +171,77 @@ suite('PythonEnvironmentApiImpl - getEnvironment timeout fallback', () => { assert.strictEqual(await pending, undefined); }); }); + +suite('PythonEnvironmentApiImpl - project-aware package resolution', () => { + setup(() => { + sinon.stub(managerReady, 'waitForEnvManagerId').resolves(); + }); + + teardown(() => { + sinon.restore(); + }); + + function createApi(resolution: PackageManagerResolution): { + api: PythonEnvironmentApiImpl; + resolvePackageManager: sinon.SinonStub; + } { + type ApiArgs = ConstructorParameters; + const resolvePackageManager = sinon.stub().returns(resolution); + const envManagers = { + onDidChangeActiveEnvironment: new EventEmitter().event, + onDidChangePackageProviderPackages: new EventEmitter().event, + resolvePackageManagerForEnvironment: resolvePackageManager, + } as unknown as ApiArgs[0]; + const projectManager = { + getProjects: () => [], + onDidChangeProjects: new EventEmitter().event, + } as unknown as ApiArgs[1]; + return { + api: new PythonEnvironmentApiImpl( + envManagers, + projectManager, + {} as ApiArgs[2], + {} as ApiArgs[3], + { onDidChangeEnvironmentVariables: new EventEmitter().event } as unknown as ApiArgs[4], + ), + resolvePackageManager, + }; + } + + const environment = { + envId: { id: 'environment', managerId: 'environment-manager' }, + } as PythonEnvironment; + + test('rejects mutations and refreshes when a project-aware manager is unresolved', async () => { + const { api } = createApi({ kind: 'projectRequired' }); + + await assert.rejects( + api.managePackages(environment, { install: ['example'] }), + isPackageManagerRequiresProjectError, + ); + await assert.rejects(api.refreshPackages(environment), isPackageManagerRequiresProjectError); + }); + + test('returns undefined for reads when a project-aware manager is unresolved', async () => { + const { api } = createApi({ kind: 'projectRequired' }); + + assert.strictEqual(await api.getPackages(environment), undefined); + }); + + test('uses the resolved manager without a second provider lookup', async () => { + const manage = sinon.stub().resolves(); + const manager = { manage } as unknown as InternalPackageManager; + const { api, resolvePackageManager } = createApi({ kind: 'resolved', manager }); + + await api.managePackages(environment, { install: ['example'] }); + + assert.ok(resolvePackageManager.calledOnceWithExactly(environment)); + assert.ok(manage.calledOnceWithExactly(environment, { install: ['example'] })); + }); + + test('preserves the no-package-manager failure', async () => { + const { api } = createApi({ kind: 'notFound' }); + + await assert.rejects(api.refreshPackages(environment), /No package manager found/); + }); +}); diff --git a/src/test/features/packageManager.api.unit.test.ts b/src/test/features/packageManager.api.unit.test.ts index 5c3de8569..32e753bb9 100644 --- a/src/test/features/packageManager.api.unit.test.ts +++ b/src/test/features/packageManager.api.unit.test.ts @@ -662,6 +662,7 @@ suite('PythonPackageManagerApi Tests', () => { ): { environment: PythonEnvironment; getEnvironment: sinon.SinonStub; + getLastKnownEnvironment: sinon.SinonStub; managerId: string; disposable: Disposable; } { @@ -689,9 +690,13 @@ suite('PythonPackageManagerApi Tests', () => { envId: { id: environment.object.envId.id, managerId }, }; selectedEnvironment = usesEnvironment ? resolvedEnvironment : undefined; + const getLastKnownEnvironment = sinon + .stub(envManagers, 'getLastKnownEnvironment') + .callsFake(() => selectedEnvironment); return { environment: resolvedEnvironment, getEnvironment, + getLastKnownEnvironment, managerId, disposable: Disposable.from( registration, @@ -829,6 +834,15 @@ suite('PythonPackageManagerApi Tests', () => { assert.strictEqual(manager, undefined, 'Should return undefined for non-existent ID'); }); + test('Should report when no package manager can be resolved for an environment', () => { + disposable.dispose(); + + const resolution = envManagers.resolvePackageManagerForEnvironment(environment.object); + + assert.strictEqual(resolution.kind, 'notFound'); + assert.strictEqual(resolution.manager, undefined); + }); + test('Should cache project-bound package managers by project', () => { disposable.dispose(); const firstProject = { @@ -902,7 +916,7 @@ suite('PythonPackageManagerApi Tests', () => { assert.strictEqual(envManagers.getPackageManager(project.uri), registeredManager); }); - test('Should return a project-independent package manager without resolving projects', async () => { + test('Should return a project-independent package manager without resolving projects', () => { disposable.dispose(); disposable = envManagers.registerPackageManager({ name: 'project-independent-pkg-mgr', @@ -918,15 +932,17 @@ suite('PythonPackageManagerApi Tests', () => { const registeredManager = envManagers.packageManagers[0]; const provider = registerEnvironmentProvider(registeredManager.id, true); - const manager = await envManagers.resolvePackageManager(provider.environment); + const resolution = envManagers.resolvePackageManagerForEnvironment(provider.environment); - assert.strictEqual(manager, registeredManager); + assert.strictEqual(resolution.kind, 'resolved'); + assert.strictEqual(resolution.manager, registeredManager); assert.ok(provider.getEnvironment.notCalled); + assert.ok(provider.getLastKnownEnvironment.notCalled); provider.disposable.dispose(); }); - test('Should bind the provided project without inferring it from the environment', async () => { + test('Should bind the provided project without inferring it from the environment', () => { const project = { name: 'project', uri: Uri.file(path.join(process.cwd(), 'explicit-project')), @@ -936,16 +952,17 @@ suite('PythonPackageManagerApi Tests', () => { const environmentProvider = registerEnvironmentProvider(packageProvider.manager.id, false); configureDefaultManagers(packageProvider.manager.id, environmentProvider.managerId); - const manager = await envManagers.resolvePackageManager(environmentProvider.environment, project); + const manager = envManagers.getPackageManagerForProject(project); assert.strictEqual(manager?.project, project); assert.ok(packageProvider.createForProject.calledOnceWithExactly(project)); assert.ok(environmentProvider.getEnvironment.notCalled); + assert.ok(environmentProvider.getLastKnownEnvironment.notCalled); environmentProvider.disposable.dispose(); }); - test('Should resolve a project-aware package manager for the unique project using an environment', async () => { + test('Should resolve a project-aware package manager for the unique project using an environment', () => { const project = { name: 'project', uri: Uri.file(path.join(process.cwd(), 'unique-project')), @@ -956,27 +973,32 @@ suite('PythonPackageManagerApi Tests', () => { const environmentProvider = registerEnvironmentProvider(packageProvider.manager.id, true); configureDefaultManagers(packageProvider.manager.id, environmentProvider.managerId); - const manager = await envManagers.resolvePackageManager(environmentProvider.environment); + const resolution = envManagers.resolvePackageManagerForEnvironment(environmentProvider.environment); assert.ok(packageProvider.createForProject.calledOnceWithExactly(project)); - assert.strictEqual(manager?.project, project); + assert.strictEqual(resolution.kind, 'resolved'); + assert.strictEqual(resolution.manager?.project, project); + assert.strictEqual(environmentProvider.getLastKnownEnvironment.callCount, 1); + assert.ok(environmentProvider.getEnvironment.notCalled); environmentProvider.disposable.dispose(); }); - test('Should not resolve a project-aware package manager without a matching project', async () => { + test('Should not resolve a project-aware package manager without a matching project', () => { const packageProvider = registerProjectAwarePackageManager(); const environmentProvider = registerEnvironmentProvider(packageProvider.manager.id, false); - const manager = await envManagers.resolvePackageManager(environmentProvider.environment); + const resolution = envManagers.resolvePackageManagerForEnvironment(environmentProvider.environment); - assert.strictEqual(manager, undefined); + assert.strictEqual(resolution.kind, 'projectRequired'); + assert.strictEqual(resolution.manager, undefined); assert.ok(packageProvider.createForProject.notCalled); + assert.ok(environmentProvider.getEnvironment.notCalled); environmentProvider.disposable.dispose(); }); - test('Should not choose a project-aware package manager when multiple projects use an environment', async () => { + test('Should not choose a project-aware package manager when multiple projects use an environment', () => { disposable.dispose(); const firstProject = { name: 'first', @@ -992,10 +1014,13 @@ suite('PythonPackageManagerApi Tests', () => { const environmentProvider = registerEnvironmentProvider(packageProvider.manager.id, true); configureDefaultManagers(packageProvider.manager.id, environmentProvider.managerId); - const manager = await envManagers.resolvePackageManager(environmentProvider.environment); + const resolution = envManagers.resolvePackageManagerForEnvironment(environmentProvider.environment); - assert.strictEqual(manager, undefined); + assert.strictEqual(resolution.kind, 'projectRequired'); + assert.strictEqual(resolution.manager, undefined); assert.ok(packageProvider.createForProject.notCalled); + assert.strictEqual(environmentProvider.getLastKnownEnvironment.callCount, 2); + assert.ok(environmentProvider.getEnvironment.notCalled); environmentProvider.disposable.dispose(); }); @@ -1040,8 +1065,10 @@ suite('PythonPackageManagerApi Tests', () => { const eventDisposable = envManagers.onDidChangePackages((event) => events.push(event)); const original = envManagers.getPackageManager(projectUri); + const originalProject = currentProject; currentProject = { name: 'replacement', uri: projectUri } as PythonProject; projectChangesEmitter.fire([currentProject]); + assert.strictEqual(envManagers.getPackageManagerForProject(originalProject), undefined); const replacement = envManagers.getPackageManager(projectUri); assert.notStrictEqual(original, replacement); @@ -1049,6 +1076,10 @@ suite('PythonPackageManagerApi Tests', () => { assert.strictEqual(replacement?.project, currentProject); assert.ok(scopedDisposers[0].calledOnce); assert.ok(scopedDisposers[1].notCalled); + await assert.rejects( + () => original!.manage(environment.object, { install: ['example'] }), + /Package manager .* has been disposed/, + ); const packageChange = { environment: environment.object, diff --git a/src/test/features/views/envManagersView.unit.test.ts b/src/test/features/views/envManagersView.unit.test.ts index 96a604205..79ad6d2ed 100644 --- a/src/test/features/views/envManagersView.unit.test.ts +++ b/src/test/features/views/envManagersView.unit.test.ts @@ -1,3 +1,4 @@ +import * as assert from 'assert'; import * as sinon from 'sinon'; import * as typeMoq from 'typemoq'; import { EventEmitter, TreeView, Uri } from 'vscode'; @@ -28,6 +29,7 @@ suite('EnvManagerView.reveal Tests', () => { let onDidChangeEnvironmentManagerEmitter: EventEmitter; let onDidChangePackagesEmitter: EventEmitter; let onDidChangePackageManagerEmitter: EventEmitter; + let onDidChangeProjectPackageManagerEmitter: EventEmitter; let onDidChangeStateEmitter: EventEmitter<{ itemId: string; stateKey: string }>; setup(() => { @@ -36,6 +38,7 @@ suite('EnvManagerView.reveal Tests', () => { onDidChangeEnvironmentManagerEmitter = new EventEmitter(); onDidChangePackagesEmitter = new EventEmitter(); onDidChangePackageManagerEmitter = new EventEmitter(); + onDidChangeProjectPackageManagerEmitter = new EventEmitter(); onDidChangeStateEmitter = new EventEmitter(); // Mock manager @@ -53,6 +56,12 @@ suite('EnvManagerView.reveal Tests', () => { .returns(() => onDidChangeEnvironmentManagerEmitter.event); envManagers.setup((e) => e.onDidChangePackages).returns(() => onDidChangePackagesEmitter.event); envManagers.setup((e) => e.onDidChangePackageManager).returns(() => onDidChangePackageManagerEmitter.event); + envManagers + .setup((e) => e.resolvePackageManagerForEnvironment(typeMoq.It.isAny())) + .returns(() => ({ kind: 'notFound' })); + envManagers + .setup((e) => e.onDidChangeProjectPackageManager) + .returns(() => onDidChangeProjectPackageManagerEmitter.event); setupNonThenable(envManagers); // Mock state manager @@ -75,6 +84,7 @@ suite('EnvManagerView.reveal Tests', () => { onDidChangeEnvironmentManagerEmitter.dispose(); onDidChangePackagesEmitter.dispose(); onDidChangePackageManagerEmitter.dispose(); + onDidChangeProjectPackageManagerEmitter.dispose(); onDidChangeStateEmitter.dispose(); }); @@ -218,4 +228,18 @@ suite('EnvManagerView.reveal Tests', () => { view.dispose(); }); + + test('Refreshes the tree when a project-scoped package manager is invalidated', () => { + const clock = sinon.useFakeTimers(); + const view = new EnvManagerView(envManagers.object, stateManager.object); + const changed = sinon.stub(); + const listener = view.onDidChangeTreeData(changed); + + onDidChangeProjectPackageManagerEmitter.fire(); + clock.tick(500); + + assert.ok(changed.calledOnce); + listener.dispose(); + view.dispose(); + }); }); diff --git a/src/test/managers/common/projectScopedPackageManagerCache.unit.test.ts b/src/test/managers/common/projectScopedPackageManagerCache.unit.test.ts new file mode 100644 index 000000000..5e59dcd97 --- /dev/null +++ b/src/test/managers/common/projectScopedPackageManagerCache.unit.test.ts @@ -0,0 +1,183 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as assert from 'assert'; +import * as path from 'path'; +import * as sinon from 'sinon'; +import { Disposable, Uri } from 'vscode'; +import { PackageManager, PythonProject } from '../../../api'; +import { ProjectScopedPackageManagerCache } from '../../../managers/common/projectScopedPackageManagerCache'; +import { InternalPackageManager } from '../../../managers/common/registeredManagers'; + +suite('ProjectScopedPackageManagerCache', () => { + let invalidate: sinon.SinonStub; + let subscribe: sinon.SinonStub; + let subscriptionDisposers: sinon.SinonStub[]; + let cache: ProjectScopedPackageManagerCache; + + setup(() => { + invalidate = sinon.stub(); + subscriptionDisposers = []; + subscribe = sinon.stub().callsFake(() => { + const dispose = sinon.stub(); + subscriptionDisposers.push(dispose); + return new Disposable(dispose); + }); + cache = new ProjectScopedPackageManagerCache(subscribe, invalidate); + }); + + teardown(() => { + cache.dispose(); + sinon.restore(); + }); + + function createProject(name: string): PythonProject { + return { + name, + uri: Uri.file(path.join(process.cwd(), name)), + }; + } + + function createProvider(name: string): { + provider: InternalPackageManager; + createForProject: sinon.SinonStub; + scopedDisposers: sinon.SinonStub[]; + } { + const scopedDisposers: sinon.SinonStub[] = []; + const createForProject = sinon.stub().callsFake(() => { + const dispose = sinon.stub(); + scopedDisposers.push(dispose); + return { + name, + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + dispose, + } satisfies PackageManager; + }); + const provider = new InternalPackageManager(name, { + name, + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + createForProject, + }); + return { provider, createForProject, scopedDisposers }; + } + + test('memoizes one scoped manager per canonical project', () => { + const { provider, createForProject } = createProvider('project-aware'); + const firstProject = createProject('first-project'); + const secondProject = createProject('second-project'); + + const first = cache.getOrCreate(provider, firstProject); + const repeatedFirst = cache.getOrCreate(provider, firstProject); + const second = cache.getOrCreate(provider, secondProject); + + assert.strictEqual(first, repeatedFirst); + assert.notStrictEqual(first, second); + assert.strictEqual(first?.project, firstProject); + assert.strictEqual(second?.project, secondProject); + assert.strictEqual(createForProject.callCount, 2); + assert.strictEqual(subscribe.callCount, 2); + assert.ok(invalidate.notCalled); + }); + + test('replaces and disposes a scoped manager when the provider changes', () => { + const firstProvider = createProvider('first-provider'); + const secondProvider = createProvider('second-provider'); + const project = createProject('provider-change-project'); + const first = cache.getOrCreate(firstProvider.provider, project); + + const second = cache.getOrCreate(secondProvider.provider, project); + + assert.notStrictEqual(first, second); + assert.ok(firstProvider.scopedDisposers[0].calledOnce); + assert.ok(subscriptionDisposers[0].calledOnce); + assert.ok(secondProvider.scopedDisposers[0].notCalled); + assert.ok(subscriptionDisposers[1].notCalled); + assert.ok(invalidate.calledOnce); + }); + + test('reconciles replaced and removed canonical projects', () => { + const { provider, scopedDisposers } = createProvider('project-aware'); + const original = createProject('reconciled-project'); + const replacement = createProject('reconciled-project'); + cache.getOrCreate(provider, original); + + cache.reconcileProjects([replacement]); + + assert.ok(scopedDisposers[0].calledOnce); + assert.ok(subscriptionDisposers[0].calledOnce); + assert.ok(invalidate.calledOnce); + + cache.getOrCreate(provider, replacement); + cache.reconcileProjects([]); + + assert.ok(scopedDisposers[1].calledOnce); + assert.ok(subscriptionDisposers[1].calledOnce); + assert.ok(invalidate.calledTwice); + }); + + test('removes only scoped managers created by the requested provider', () => { + const firstProvider = createProvider('first-provider'); + const secondProvider = createProvider('second-provider'); + cache.getOrCreate(firstProvider.provider, createProject('first-provider-project')); + cache.getOrCreate(secondProvider.provider, createProject('second-provider-project')); + + cache.removeProvider(firstProvider.provider); + + assert.ok(firstProvider.scopedDisposers[0].calledOnce); + assert.ok(subscriptionDisposers[0].calledOnce); + assert.ok(secondProvider.scopedDisposers[0].notCalled); + assert.ok(subscriptionDisposers[1].notCalled); + assert.ok(invalidate.calledOnce); + }); + + test('disposes all scoped managers and subscriptions', () => { + const { provider, scopedDisposers } = createProvider('project-aware'); + cache.getOrCreate(provider, createProject('first-project')); + cache.getOrCreate(provider, createProject('second-project')); + + cache.dispose(); + + assert.ok(scopedDisposers[0].calledOnce); + assert.ok(scopedDisposers[1].calledOnce); + assert.ok(subscriptionDisposers[0].calledOnce); + assert.ok(subscriptionDisposers[1].calledOnce); + }); + + test('returns project-independent providers without caching or subscribing', () => { + const provider = new InternalPackageManager('shared', { + name: 'shared', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + }); + + const resolved = cache.getOrCreate(provider, createProject('shared-project')); + + assert.strictEqual(resolved, provider); + assert.ok(subscribe.notCalled); + assert.ok(invalidate.notCalled); + }); + + test('disposes a scoped manager when its project switches to a project-independent provider', () => { + const projectAwareProvider = createProvider('project-aware'); + const sharedProvider = new InternalPackageManager('shared', { + name: 'shared', + manage: async () => undefined, + refresh: async () => undefined, + getPackages: async () => [], + }); + const project = createProject('provider-kind-change-project'); + cache.getOrCreate(projectAwareProvider.provider, project); + + const resolved = cache.getOrCreate(sharedProvider, project); + + assert.strictEqual(resolved, sharedProvider); + assert.ok(projectAwareProvider.scopedDisposers[0].calledOnce); + assert.ok(subscriptionDisposers[0].calledOnce); + assert.ok(invalidate.calledOnce); + }); +}); diff --git a/src/types.ts b/src/types.ts index d6f8ff624..8699ce618 100644 --- a/src/types.ts +++ b/src/types.ts @@ -726,9 +726,8 @@ export interface PackageManager { * * The extension uses the explicit project supplied by project-based callers. When a caller * provides only an environment, the extension uses a project-bound manager only if exactly - * one tracked project uses that environment. The registered root manager may still receive - * environment-only operations when no project can be selected safely, so project-sensitive - * operations must handle an unbound manager without running in an arbitrary working directory. + * one tracked project uses that environment. It does not invoke project-sensitive operations + * on the registered root manager when no project can be selected safely. * * Project-independent package managers can omit this method. Implementations should keep * project-specific caches and mutable state on the returned manager rather than the root. @@ -1180,6 +1179,8 @@ export interface PythonPackageGetterApi { * * @param environment The Python Environment for which the list of packages is to be refreshed. * @returns A promise that resolves when the list of packages has been refreshed. + * @throws {@link PackageManagerRequiresProjectError} if a project-aware manager cannot identify + * a unique project for the environment. */ refreshPackages(environment: PythonEnvironment): Promise; @@ -1188,7 +1189,8 @@ export interface PythonPackageGetterApi { * * @param environment The Python Environment for which the list of packages is required. * @param options Optional settings for package retrieval. - * @returns The list of packages in the Python Environment. + * @returns The list of packages in the Python Environment, or `undefined` if no package manager + * can be resolved for the environment. */ getPackages(environment: PythonEnvironment, options?: GetPackagesOptions): Promise; @@ -1241,8 +1243,9 @@ export interface PythonPackageManagementApi { * Install/Uninstall packages into a Python Environment. * * @param environment The Python Environment into which packages are to be installed. - * @param packages The packages to install. * @param options Options for installing packages. + * @throws {@link PackageManagerRequiresProjectError} if a project-aware manager cannot identify + * a unique project for the environment. */ managePackages(environment: PythonEnvironment, options: PackageManagementOptions): Promise; } From 99125d47cfbbc1916cee9a847c7274b1953fd7ac Mon Sep 17 00:00:00 2001 From: Eduardo Villalpando Mello Date: Mon, 28 Sep 2026 09:06:23 -0700 Subject: [PATCH 09/11] Simplify --- src/features/envCommands.ts | 42 ++++------- src/features/envManagers.ts | 43 +++--------- src/features/views/projectView.ts | 7 +- src/test/features/envCommands.unit.test.ts | 69 +++++++++---------- .../features/packageManager.api.unit.test.ts | 4 +- 5 files changed, 55 insertions(+), 110 deletions(-) diff --git a/src/features/envCommands.ts b/src/features/envCommands.ts index 583770f90..dde018304 100644 --- a/src/features/envCommands.ts +++ b/src/features/envCommands.ts @@ -138,7 +138,7 @@ export async function refreshPackagesCommand(context: unknown, managers?: Enviro if (context instanceof ProjectEnvironment) { const view = context as ProjectEnvironment; if (managers) { - const pkgManager = managers.getPackageManagerForProject(view.parent.project); + const pkgManager = managers.getPackageManager(view.parent.project.uri); if (pkgManager) { await pkgManager.refresh(view.environment); } @@ -342,15 +342,7 @@ export async function handlePackageUninstall(context: unknown) { } const moduleName = context.pkg.name; const environment = context.parent.environment; - try { - await context.manager.manage(environment, { uninstall: [moduleName], install: [] }); - } catch (error) { - if (error instanceof PackageManagerRequiresProjectError) { - await showErrorMessage(error.message); - return; - } - throw error; - } + await context.manager.manage(environment, { uninstall: [moduleName], install: [] }); return; } traceError(`Invalid context for uninstall command: ${typeof context}`); @@ -434,18 +426,10 @@ export async function managePackageVersion(context: unknown) { return; } - try { - await packageManager.manage(environment, { - install: [packageManager.formatInstallSpec(pkg.name, version)], - uninstall: [], - }); - } catch (error) { - if (error instanceof PackageManagerRequiresProjectError) { - await showErrorMessage(error.message); - return; - } - throw error; - } + await packageManager.manage(environment, { + install: [packageManager.formatInstallSpec(pkg.name, version)], + uninstall: [], + }); } else { traceError(`Invalid context for manage package version command: ${typeof context}`); } @@ -793,7 +777,7 @@ async function resolvePackageCommandOptions( if (e instanceof ProjectEnvironment) { const environment = e.environment; - const packageManager = em.getPackageManagerForProject(e.parent.project); + const packageManager = em.getPackageManager(e.parent.project.uri); if (packageManager) { return { environment, packageManager }; } @@ -802,13 +786,11 @@ async function resolvePackageCommandOptions( if (e instanceof PythonEnvTreeItem) { const environment = e.environment; const resolution = em.resolvePackageManagerForEnvironment(environment); - switch (resolution.kind) { - case 'resolved': - return { environment, packageManager: resolution.manager }; - case 'projectRequired': - throw new PackageManagerRequiresProjectError(); - case 'notFound': - break; + if (resolution.manager) { + return { environment, packageManager: resolution.manager }; + } + if (resolution.kind === 'projectRequired') { + throw new PackageManagerRequiresProjectError(); } } diff --git a/src/features/envManagers.ts b/src/features/envManagers.ts index 5cb9dd926..c38827d36 100644 --- a/src/features/envManagers.ts +++ b/src/features/envManagers.ts @@ -127,14 +127,6 @@ export interface EnvironmentManagers extends Disposable { getEnvironmentManager(scope: EnvironmentManagerScope): InternalEnvironmentManager | undefined; getPackageManager(scope: PackageManagerScope): InternalPackageManager | undefined; - /** - * Returns the configured package manager for an explicit tracked project. - * - * @param project The project whose package manager should be returned. - * @returns The shared or project-scoped package manager. - */ - getPackageManagerForProject(project: PythonProject): InternalPackageManager | undefined; - /** * Resolves a package manager for an environment using last-known project selections. * @@ -451,23 +443,13 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { if (context === undefined || context instanceof Uri) { const project = context ? this.pm.get(context) : undefined; - const defaultPkgManagerId = getDefaultPkgManagerSetting(this.pm, context); - const defaultEnvManagerId = getDefaultEnvManagerSetting(this.pm, context); - if (defaultPkgManagerId) { - return project - ? this.projectPackageManagers.getOrCreate(this._packageManagers.get(defaultPkgManagerId), project) - : this._packageManagers.get(defaultPkgManagerId); - } - - if (defaultEnvManagerId) { - const preferredPkgManagerId = - this._environmentManagers.get(defaultEnvManagerId)?.preferredPackageManagerId; - if (preferredPkgManagerId) { - const manager = this._packageManagers.get(preferredPkgManagerId); - return project ? this.projectPackageManagers.getOrCreate(manager, project) : manager; - } - } - return undefined; + const defaultPackageManagerId = getDefaultPkgManagerSetting(this.pm, context); + const defaultEnvironmentManagerId = getDefaultEnvManagerSetting(this.pm, context); + const packageManagerId = + defaultPackageManagerId || + this._environmentManagers.get(defaultEnvironmentManagerId)?.preferredPackageManagerId; + const manager = packageManagerId ? this._packageManagers.get(packageManagerId) : undefined; + return project ? this.projectPackageManagers.getOrCreate(manager, project) : manager; } if (typeof context === 'string') { @@ -486,15 +468,6 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { return undefined; } - public getPackageManagerForProject(project: PythonProject): InternalPackageManager | undefined { - const canonicalProject = this.pm.get(project.uri); - if (canonicalProject !== project) { - traceVerbose(`Unable to resolve package manager for untracked project ${project.uri.fsPath}`); - return undefined; - } - return this.getPackageManager(canonicalProject.uri); - } - public resolvePackageManagerForEnvironment( environment: PythonEnvironment, ): PackageManagerResolution { @@ -517,7 +490,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { return { kind: 'projectRequired' }; } - const scopedManager = this.getPackageManagerForProject(matchingProjects[0]); + const scopedManager = this.getPackageManager(matchingProjects[0].uri); return scopedManager ? { kind: 'resolved', manager: scopedManager } : { kind: 'notFound' }; diff --git a/src/features/views/projectView.ts b/src/features/views/projectView.ts index 78c0e35b2..feb28dd38 100644 --- a/src/features/views/projectView.ts +++ b/src/features/views/projectView.ts @@ -240,11 +240,10 @@ export class ProjectView implements TreeDataProvider { const environmentItem = element as ProjectEnvironment; const parent = environmentItem.parent; - const project = parent.id === 'global' ? undefined : parent.project; - const uri = project?.uri; + const uri = parent.id === 'global' ? undefined : parent.project.uri; const environment = environmentItem.environment; - const pkgManager = project - ? this.envManagers.getPackageManagerForProject(project) + const pkgManager = uri + ? this.envManagers.getPackageManager(uri) : this.envManagers.resolvePackageManagerForEnvironment(environment).manager; if (!pkgManager) { diff --git a/src/test/features/envCommands.unit.test.ts b/src/test/features/envCommands.unit.test.ts index 63b96979f..64808d382 100644 --- a/src/test/features/envCommands.unit.test.ts +++ b/src/test/features/envCommands.unit.test.ts @@ -27,11 +27,15 @@ import * as shellProviders from '../../features/terminal/shells/providers'; import { ShellStartupScriptProvider } from '../../features/terminal/shells/startupProvider'; import { TerminalManager } from '../../features/terminal/terminalManager'; import { EnvManagerView } from '../../features/views/envManagersView'; -import { EnvManagerTreeItem, PackageTreeItem, ProjectEnvironment, ProjectItem, PythonEnvTreeItem } from '../../features/views/treeViewItems'; +import { + PackageTreeItem, + ProjectEnvironment, + ProjectItem, + type PythonEnvTreeItem, +} from '../../features/views/treeViewItems'; import type { EnvironmentManagers } from '../../features/envManagers'; import type { PythonProjectManager } from '../../features/projectManager'; import { InternalEnvironmentManager, InternalPackageManager } from '../../managers/common/registeredManagers'; -import { PackageManagerRequiresProjectError } from '../../managers/common/errors'; import { setupNonThenable } from '../mocks/helper'; import { createMockPythonEnvironment } from '../mocks/pythonEnvironment'; @@ -643,47 +647,36 @@ suite('Run In Terminal Command Tests', () => { }); }); -suite('handlePackageUninstall - unbound package manager', () => { - let showError: sinon.SinonStub; - - setup(() => { - showError = sinon.stub(windowApis, 'showErrorMessage').resolves(undefined); - }); - - teardown(() => sinon.restore()); - - test('shows a friendly message instead of throwing when the resolved manager requires a project', async () => { +suite('Package command manager ownership', () => { + test('uninstalls with the manager attached to the package item', async () => { const environment = createMockPythonEnvironment({ - envPath: path.join(process.cwd(), 'unbound-poetry-env'), - managerId: 'ms-python.python:poetry', + envPath: path.join(process.cwd(), 'package-environment'), + managerId: 'test:environment-manager', }); - const rawManager = { - name: 'poetry', - manage: sinon.stub().rejects(new PackageManagerRequiresProjectError()), + const manage = sinon.stub().resolves(); + const packageManager = new InternalPackageManager('test:package-manager', { + name: 'package-manager', + manage, refresh: async () => undefined, getPackages: async () => undefined, - }; - const packageManager = new InternalPackageManager('ms-python.python:poetry', rawManager as never); - const provider = { - name: 'poetry', - preferredPackageManagerId: 'ms-python.python:poetry', - get: async () => environment, - set: async () => undefined, - getEnvironments: async () => [environment], - refresh: async () => undefined, - resolve: async () => undefined, - }; - const parent = new EnvManagerTreeItem(new InternalEnvironmentManager('ms-python.python:poetry', provider)); - const envItem = new PythonEnvTreeItem(environment, parent); - const pkg = { - name: 'requests', - displayName: 'requests', - pkgId: { id: 'requests', managerId: 'ms-python.python:poetry', environmentId: environment.envId.id }, - }; - const context = new PackageTreeItem(pkg, envItem, packageManager); + }); + const environmentItem = { environment } as PythonEnvTreeItem; + const packageItem = new PackageTreeItem( + { + name: 'requests', + displayName: 'requests', + pkgId: { + id: 'requests', + managerId: packageManager.id, + environmentId: environment.envId.id, + }, + }, + environmentItem, + packageManager, + ); - await handlePackageUninstall(context); + await handlePackageUninstall(packageItem); - assert.ok(showError.calledOnceWithExactly(new PackageManagerRequiresProjectError().message)); + assert.ok(manage.calledOnceWithExactly(environment, { uninstall: ['requests'], install: [] })); }); }); diff --git a/src/test/features/packageManager.api.unit.test.ts b/src/test/features/packageManager.api.unit.test.ts index 32e753bb9..dd71f0bf7 100644 --- a/src/test/features/packageManager.api.unit.test.ts +++ b/src/test/features/packageManager.api.unit.test.ts @@ -952,7 +952,7 @@ suite('PythonPackageManagerApi Tests', () => { const environmentProvider = registerEnvironmentProvider(packageProvider.manager.id, false); configureDefaultManagers(packageProvider.manager.id, environmentProvider.managerId); - const manager = envManagers.getPackageManagerForProject(project); + const manager = envManagers.getPackageManager(project.uri); assert.strictEqual(manager?.project, project); assert.ok(packageProvider.createForProject.calledOnceWithExactly(project)); @@ -1065,10 +1065,8 @@ suite('PythonPackageManagerApi Tests', () => { const eventDisposable = envManagers.onDidChangePackages((event) => events.push(event)); const original = envManagers.getPackageManager(projectUri); - const originalProject = currentProject; currentProject = { name: 'replacement', uri: projectUri } as PythonProject; projectChangesEmitter.fire([currentProject]); - assert.strictEqual(envManagers.getPackageManagerForProject(originalProject), undefined); const replacement = envManagers.getPackageManager(projectUri); assert.notStrictEqual(original, replacement); From 662c51c5194d11681246f16c26b9b0e78de627b6 Mon Sep 17 00:00:00 2001 From: Eduardo Villalpando Mello Date: Mon, 28 Sep 2026 09:48:44 -0700 Subject: [PATCH 10/11] Simplify --- api/CHANGELOG.md | 3 +- docs/README.md | 47 +++++---------- src/extension.ts | 14 +---- src/extensionApi.ts | 31 +++++----- src/features/envCommands.ts | 12 ++-- src/features/envManagers.ts | 57 ++++++------------- src/features/views/envManagersView.ts | 4 +- src/features/views/projectView.ts | 4 +- src/managers/common/errors.ts | 9 ++- src/publicErrors.ts | 42 -------------- src/test/extensionApi.unit.test.ts | 41 +++++-------- .../features/packageManager.api.unit.test.ts | 28 +++++---- .../views/envManagersView.unit.test.ts | 4 +- 13 files changed, 91 insertions(+), 205 deletions(-) diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index 52f4112e3..eb86291b4 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -11,11 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added the optional `PackageManager.createForProject` factory for package managers whose operations depend on the calling Python project. Explicit project contexts are used directly; environment-only operations use a scoped manager only when exactly one tracked project matches. - Added optional `PackageManager.dispose` support for releasing resources owned by project-scoped package managers. -- Added `PackageManagerRequiresProjectError` and `isPackageManagerRequiresProjectError` for environment-only package mutations and refreshes that cannot identify a unique project. ### Changed -- Environment-only package operations no longer fall back to an unbound project-aware package manager. Package reads return `undefined`; mutations and refreshes reject with `PackageManagerRequiresProjectError`. +- Environment-only package operations no longer fall back to an unbound project-aware package manager. When no unique project can be inferred, package reads return `undefined` and mutations and refreshes reject with `No package manager found`. ## [1.4.0] diff --git a/docs/README.md b/docs/README.md index 1e528b271..bbeb1b09a 100644 --- a/docs/README.md +++ b/docs/README.md @@ -793,8 +793,9 @@ getPackages( **Returns** `Promise`. `undefined` means the manager could not produce a list - for example no package manager is associated with -the environment or a project-aware manager cannot identify one unique project - -which is different from an empty array meaning "nothing installed". +the environment, or a project-aware manager cannot identify one unique project +for the environment - which is different from an empty array meaning "nothing +installed". ```typescript const packages = await api.getPackages(env); @@ -819,8 +820,9 @@ refreshPackages(environment: PythonEnvironment): Promise; **Returns** `Promise`. Changes surface through [`onDidChangePackages`](#ondidchangepackages). Rejects with -`PackageManagerRequiresProjectError` when a project-aware manager cannot identify -one unique project for the environment. +`No package manager found` when no package manager can be resolved for the +environment - including when a project-aware manager cannot identify one unique +project. ```typescript // Packages were installed outside the extension - re-read the list. @@ -845,8 +847,9 @@ managePackages( | `options` | [`PackageManagementOptions`](#packagemanagementoptions) | Yes | Must specify `install`, `uninstall`, or both. Also carries `upgrade`, `showSkipOption`, and `runHeadless`. | **Returns** `Promise`, resolving when the operation finishes. Rejects with -`PackageManagerRequiresProjectError` when a project-aware manager cannot identify -one unique project for the environment, or if the underlying tool fails. +`No package manager found` when no package manager can be resolved for the +environment - including when a project-aware manager cannot identify one unique +project - or if the underlying tool fails. ```typescript await api.managePackages(env, { @@ -976,30 +979,6 @@ context.subscriptions.push( ### Package errors -#### `PackageManagerRequiresProjectError` - -Thrown by environment-only package mutations and refreshes when the selected -package manager is project-aware but the environment does not identify exactly -one tracked project. Its `code` is the stable -`'PackageManagerRequiresProject'` discriminator. - -Use `isPackageManagerRequiresProjectError(error)` instead of `instanceof` when -the error may cross extension bundle boundaries: - -```typescript -import { isPackageManagerRequiresProjectError } from '@vscode/python-environments'; - -try { - await api.refreshPackages(env); -} catch (error) { - if (isPackageManagerRequiresProjectError(error)) { - // Ask the user to open or select the intended Python project. - } else { - throw error; - } -} -``` - #### `PackageVersionLookupNotSupportedError` Thrown when a package manager cannot list available versions at all. It @@ -1659,10 +1638,10 @@ tracked project uses the environment; it does not choose arbitrarily when no project or multiple projects match. Environment-only paths never use a project-aware provider as an unbound -fallback. When no unique project can be inferred, package reads return -`undefined`, package views suppress those operations, and package mutations or -refreshes reject with `PackageManagerRequiresProjectError`. Keep -project-specific caches and mutable state on the manager returned by +fallback. When no unique project can be inferred, `getPackageManager` returns no +manager, so package reads return `undefined`, package views suppress those +operations, and package mutations or refreshes reject with `No package manager +found`. Keep project-specific caches and mutable state on the manager returned by `createForProject`, and implement `dispose` when that manager owns resources. When a scoped manager fires `onDidChangePackages`, the event's `manager` must be diff --git a/src/extension.ts b/src/extension.ts index d585e2b82..b7fa8c5df 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -115,7 +115,6 @@ import { NativePythonFinder, } from './managers/common/nativePythonFinder'; import { registerPackageWatchers } from './managers/common/packageWatcher'; -import { PackageManagerRequiresProjectError } from './managers/common/errors'; import { IDisposable } from './managers/common/types'; import { registerCondaFeatures } from './managers/conda/main'; import { registerPipenvFeatures } from './managers/pipenv/main'; @@ -298,7 +297,7 @@ export async function activate(context: ExtensionContext): Promise { - const { manager } = envManagers.resolvePackageManagerForEnvironment(environment); + const manager = envManagers.getPackageManager(environment); const names = await manager?.getDirectPackageNames?.(environment); return names ? Array.from(names) : undefined; }, @@ -359,24 +358,17 @@ export async function activate(context: ExtensionContext): Promise { await waitForEnvManagerId([context.envId.managerId]); - const manager = this.requirePackageManagerForEnvironment(context); + const manager = this.envManagers.getPackageManager(context); + if (!manager) { + return Promise.reject(new Error('No package manager found')); + } return manager.manage(context, options); } async refreshPackages(context: PythonEnvironment): Promise { await waitForEnvManagerId([context.envId.managerId]); - const manager = this.requirePackageManagerForEnvironment(context); + const manager = this.envManagers.getPackageManager(context); + if (!manager) { + return Promise.reject(new Error('No package manager found')); + } return manager.refresh(context); } async getPackages(context: PythonEnvironment, options?: GetPackagesOptions): Promise { await waitForEnvManagerId([context.envId.managerId]); - const { manager } = this.envManagers.resolvePackageManagerForEnvironment(context); - return manager?.getPackages(context, options); - } - - private requirePackageManagerForEnvironment(context: PythonEnvironment): InternalPackageManager { - const resolution = this.envManagers.resolvePackageManagerForEnvironment(context); - switch (resolution.kind) { - case 'resolved': - return resolution.manager; - case 'projectRequired': - throw new PackageManagerRequiresProjectError(); - case 'notFound': - throw new Error('No package manager found'); + const manager = this.envManagers.getPackageManager(context); + if (!manager) { + return Promise.resolve(undefined); } + return manager.getPackages(context, options); } - getPackageAvailableVersions( context: PythonEnvironment, packageName: string, diff --git a/src/features/envCommands.ts b/src/features/envCommands.ts index dde018304..db71f93a3 100644 --- a/src/features/envCommands.ts +++ b/src/features/envCommands.ts @@ -31,7 +31,6 @@ import type { InternalEnvironmentManager, InternalPackageManager, } from '../managers/common/registeredManagers'; -import { PackageManagerRequiresProjectError } from '../managers/common/errors'; import { removePythonProjectSetting, setEnvironmentManager, @@ -145,7 +144,7 @@ export async function refreshPackagesCommand(context: unknown, managers?: Enviro } } else if (context instanceof PythonEnvTreeItem) { const view = context as PythonEnvTreeItem; - const pkgManager = managers?.resolvePackageManagerForEnvironment(view.environment).manager; + const pkgManager = managers?.getPackageManager(view.environment); if (pkgManager) { await pkgManager.refresh(view.environment); } @@ -785,12 +784,9 @@ async function resolvePackageCommandOptions( if (e instanceof PythonEnvTreeItem) { const environment = e.environment; - const resolution = em.resolvePackageManagerForEnvironment(environment); - if (resolution.manager) { - return { environment, packageManager: resolution.manager }; - } - if (resolution.kind === 'projectRequired') { - throw new PackageManagerRequiresProjectError(); + const packageManager = em.getPackageManager(environment); + if (packageManager) { + return { environment, packageManager }; } } diff --git a/src/features/envManagers.ts b/src/features/envManagers.ts index c38827d36..05124b68a 100644 --- a/src/features/envManagers.ts +++ b/src/features/envManagers.ts @@ -87,11 +87,6 @@ export interface InternalDidChangeEnvironmentsEventArgs { changes: DidChangeEnvironmentsEventArgs; } -export type PackageManagerResolution = - | { kind: 'resolved'; manager: InternalPackageManager } - | { kind: 'projectRequired'; manager?: never } - | { kind: 'notFound'; manager?: never }; - export interface EnvironmentManagers extends Disposable { registerEnvironmentManager(manager: EnvironmentManager, options?: { extensionId?: string }): Disposable; registerPackageManager(manager: PackageManager, options?: { extensionId?: string }): Disposable; @@ -127,14 +122,6 @@ export interface EnvironmentManagers extends Disposable { getEnvironmentManager(scope: EnvironmentManagerScope): InternalEnvironmentManager | undefined; getPackageManager(scope: PackageManagerScope): InternalPackageManager | undefined; - /** - * Resolves a package manager for an environment using last-known project selections. - * - * @param environment The environment whose package manager should be resolved. - * @returns A resolved manager or the reason resolution was not possible. - */ - resolvePackageManagerForEnvironment(environment: PythonEnvironment): PackageManagerResolution; - managers: InternalEnvironmentManager[]; packageManagers: InternalPackageManager[]; @@ -458,42 +445,32 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { if ('pkgId' in context) { return this._packageManagers.get(context.pkgId.managerId); - } else { - const id = this._environmentManagers.get(context.envId.managerId)?.preferredPackageManagerId; - if (id) { - return this._packageManagers.get(id); - } } - return undefined; - } - - public resolvePackageManagerForEnvironment( - environment: PythonEnvironment, - ): PackageManagerResolution { - const manager = this.getPackageManager(environment); - if (!manager) { - return { kind: 'notFound' }; - } - if (!manager.createForProject) { - return { kind: 'resolved', manager }; + const preferredId = this._environmentManagers.get(context.envId.managerId)?.preferredPackageManagerId; + const manager = preferredId ? this._packageManagers.get(preferredId) : undefined; + if (!manager?.createForProject) { + return manager; } + // The preferred manager is project-aware (e.g. Poetry). Bind it to the single tracked + // project that uses this environment; without a unique project there is no working + // directory to run in, so no manager is returned. + const project = this.findUniqueProjectForEnvironment(context); + return project ? this.getPackageManager(project.uri) : undefined; + } - const matchingProjects = this.pm.getProjects().filter((project) => - this.isSameEnvironment(environment, this.getLastKnownEnvironment(project.uri)), - ); + private findUniqueProjectForEnvironment(environment: PythonEnvironment): PythonProject | undefined { + const matchingProjects = this.pm + .getProjects() + .filter((project) => this.isSameEnvironment(environment, this.getLastKnownEnvironment(project.uri))); if (matchingProjects.length !== 1) { traceVerbose( - `Unable to resolve project-scoped package manager for environment ${environment.envId.id}: ` + + `Unable to infer a unique project for environment ${environment.envId.id}: ` + `found ${matchingProjects.length} matching projects`, ); - return { kind: 'projectRequired' }; + return undefined; } - - const scopedManager = this.getPackageManager(matchingProjects[0].uri); - return scopedManager - ? { kind: 'resolved', manager: scopedManager } - : { kind: 'notFound' }; + return matchingProjects[0]; } private subscribeToPackageManagerEvents(manager: InternalPackageManager): Disposable { diff --git a/src/features/views/envManagersView.ts b/src/features/views/envManagersView.ts index 3c3207c2d..c756e3c38 100644 --- a/src/features/views/envManagersView.ts +++ b/src/features/views/envManagersView.ts @@ -242,8 +242,8 @@ export class EnvManagerView implements TreeDataProvider, Disposable if (element.kind === EnvTreeItemKind.environment) { const pythonEnvItem = element as PythonEnvTreeItem; - const environment = pythonEnvItem.environment; - const { manager: pkgManager } = this.providers.resolvePackageManagerForEnvironment(environment); + const { environment } = pythonEnvItem; + const pkgManager = this.providers.getPackageManager(environment); const parent = element as PythonEnvTreeItem; const views: EnvTreeItem[] = []; diff --git a/src/features/views/projectView.ts b/src/features/views/projectView.ts index feb28dd38..6019a4bae 100644 --- a/src/features/views/projectView.ts +++ b/src/features/views/projectView.ts @@ -242,9 +242,7 @@ export class ProjectView implements TreeDataProvider { const parent = environmentItem.parent; const uri = parent.id === 'global' ? undefined : parent.project.uri; const environment = environmentItem.environment; - const pkgManager = uri - ? this.envManagers.getPackageManager(uri) - : this.envManagers.resolvePackageManagerForEnvironment(environment).manager; + const pkgManager = this.envManagers.getPackageManager(uri ?? environment); if (!pkgManager) { return [new ProjectEnvironmentInfo(environmentItem, ProjectViews.noPackageManager)]; diff --git a/src/managers/common/errors.ts b/src/managers/common/errors.ts index eacb7e31c..6cbbe53f6 100644 --- a/src/managers/common/errors.ts +++ b/src/managers/common/errors.ts @@ -2,10 +2,15 @@ // Licensed under the MIT License. import { l10n } from 'vscode'; -import { PackageManagerRequiresProjectError as PublicPackageManagerRequiresProjectError } from '../../publicErrors'; -export class PackageManagerRequiresProjectError extends PublicPackageManagerRequiresProjectError { +/** + * Raised when a project-aware package manager (such as Poetry) is asked to run an operation + * without a bound Python project. Package operations that depend on the working directory must + * fail clearly rather than run from an arbitrary location. + */ +export class PackageManagerRequiresProjectError extends Error { constructor() { super(l10n.t('Package operations require a Python project.')); + this.name = 'PackageManagerRequiresProjectError'; } } diff --git a/src/publicErrors.ts b/src/publicErrors.ts index efc68519f..f4799d8ce 100644 --- a/src/publicErrors.ts +++ b/src/publicErrors.ts @@ -7,48 +7,6 @@ * that facade small. */ -/** - * Error thrown when a project-aware package manager cannot determine which Python project to use. - * - * The {@link code} property is a stable discriminator that can be checked across extension bundle - * boundaries with {@link isPackageManagerRequiresProjectError}. - */ -export class PackageManagerRequiresProjectError extends Error { - /** - * Stable discriminator identifying this error type across bundle boundaries. - */ - public readonly code = 'PackageManagerRequiresProject'; - - /** - * Creates a project-required package error. - * - * @param message Optional caller-facing explanation. - */ - constructor(message?: string) { - super(message ?? 'Package operations require a Python project.'); - this.name = 'PackageManagerRequiresProjectError'; - Object.setPrototypeOf(this, new.target.prototype); - } -} - -/** - * Reports whether an error means that a package operation requires an unambiguous Python project. - * - * @param error The value to test. - * @returns `true` when the error carries the project-required discriminator. - */ -export function isPackageManagerRequiresProjectError( - error: unknown, -): error is PackageManagerRequiresProjectError { - return ( - error instanceof PackageManagerRequiresProjectError || - (typeof error === 'object' && - error !== null && - 'code' in error && - (error as { code?: unknown }).code === 'PackageManagerRequiresProject') - ); -} - /** * Error thrown when a package manager cannot list available package versions. * diff --git a/src/test/extensionApi.unit.test.ts b/src/test/extensionApi.unit.test.ts index 12aa2802f..04f702249 100644 --- a/src/test/extensionApi.unit.test.ts +++ b/src/test/extensionApi.unit.test.ts @@ -2,13 +2,11 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import { EventEmitter, Uri } from 'vscode'; import { - isPackageManagerRequiresProjectError, PythonEnvironment, PythonProject, } from '../api'; import * as managerReady from '../features/common/managerReady'; import { PythonEnvironmentApiImpl } from '../extensionApi'; -import type { PackageManagerResolution } from '../features/envManagers'; import type { PythonProjectManager } from '../features/projectManager'; import type { InternalPackageManager } from '../managers/common/registeredManagers'; @@ -172,7 +170,7 @@ suite('PythonEnvironmentApiImpl - getEnvironment timeout fallback', () => { }); }); -suite('PythonEnvironmentApiImpl - project-aware package resolution', () => { +suite('PythonEnvironmentApiImpl - package resolution', () => { setup(() => { sinon.stub(managerReady, 'waitForEnvManagerId').resolves(); }); @@ -181,16 +179,16 @@ suite('PythonEnvironmentApiImpl - project-aware package resolution', () => { sinon.restore(); }); - function createApi(resolution: PackageManagerResolution): { + function createApi(manager: InternalPackageManager | undefined): { api: PythonEnvironmentApiImpl; - resolvePackageManager: sinon.SinonStub; + getPackageManager: sinon.SinonStub; } { type ApiArgs = ConstructorParameters; - const resolvePackageManager = sinon.stub().returns(resolution); + const getPackageManager = sinon.stub().returns(manager); const envManagers = { onDidChangeActiveEnvironment: new EventEmitter().event, onDidChangePackageProviderPackages: new EventEmitter().event, - resolvePackageManagerForEnvironment: resolvePackageManager, + getPackageManager, } as unknown as ApiArgs[0]; const projectManager = { getProjects: () => [], @@ -204,7 +202,7 @@ suite('PythonEnvironmentApiImpl - project-aware package resolution', () => { {} as ApiArgs[3], { onDidChangeEnvironmentVariables: new EventEmitter().event } as unknown as ApiArgs[4], ), - resolvePackageManager, + getPackageManager, }; } @@ -212,36 +210,27 @@ suite('PythonEnvironmentApiImpl - project-aware package resolution', () => { envId: { id: 'environment', managerId: 'environment-manager' }, } as PythonEnvironment; - test('rejects mutations and refreshes when a project-aware manager is unresolved', async () => { - const { api } = createApi({ kind: 'projectRequired' }); + test('rejects mutations and refreshes when no package manager resolves', async () => { + const { api } = createApi(undefined); - await assert.rejects( - api.managePackages(environment, { install: ['example'] }), - isPackageManagerRequiresProjectError, - ); - await assert.rejects(api.refreshPackages(environment), isPackageManagerRequiresProjectError); + await assert.rejects(api.managePackages(environment, { install: ['example'] }), /No package manager found/); + await assert.rejects(api.refreshPackages(environment), /No package manager found/); }); - test('returns undefined for reads when a project-aware manager is unresolved', async () => { - const { api } = createApi({ kind: 'projectRequired' }); + test('returns undefined for reads when no package manager resolves', async () => { + const { api } = createApi(undefined); assert.strictEqual(await api.getPackages(environment), undefined); }); - test('uses the resolved manager without a second provider lookup', async () => { + test('delegates to the resolved manager', async () => { const manage = sinon.stub().resolves(); const manager = { manage } as unknown as InternalPackageManager; - const { api, resolvePackageManager } = createApi({ kind: 'resolved', manager }); + const { api, getPackageManager } = createApi(manager); await api.managePackages(environment, { install: ['example'] }); - assert.ok(resolvePackageManager.calledOnceWithExactly(environment)); + assert.ok(getPackageManager.calledWithExactly(environment)); assert.ok(manage.calledOnceWithExactly(environment, { install: ['example'] })); }); - - test('preserves the no-package-manager failure', async () => { - const { api } = createApi({ kind: 'notFound' }); - - await assert.rejects(api.refreshPackages(environment), /No package manager found/); - }); }); diff --git a/src/test/features/packageManager.api.unit.test.ts b/src/test/features/packageManager.api.unit.test.ts index dd71f0bf7..b7e56522c 100644 --- a/src/test/features/packageManager.api.unit.test.ts +++ b/src/test/features/packageManager.api.unit.test.ts @@ -93,6 +93,9 @@ suite('PythonPackageManagerApi Tests', () => { onDidChangePackagesEmitter = new EventEmitter(); packageManager = typeMoq.Mock.ofType(); packageManager.setup((pm) => pm.name).returns(() => 'test-pkg-mgr'); + // The default mock is a project-independent manager; typemoq otherwise fabricates a + // truthy createForProject, which would make it look project-aware. + packageManager.setup((pm) => pm.createForProject).returns(() => undefined); packageManager.setup((pm) => pm.displayName).returns(() => 'Test Package Manager'); packageManager.setup((pm) => pm.description).returns(() => 'Test package manager description'); packageManager.setup((pm) => pm.onDidChangePackages).returns(() => onDidChangePackagesEmitter.event); @@ -837,10 +840,9 @@ suite('PythonPackageManagerApi Tests', () => { test('Should report when no package manager can be resolved for an environment', () => { disposable.dispose(); - const resolution = envManagers.resolvePackageManagerForEnvironment(environment.object); + const manager = envManagers.getPackageManager(environment.object); - assert.strictEqual(resolution.kind, 'notFound'); - assert.strictEqual(resolution.manager, undefined); + assert.strictEqual(manager, undefined); }); test('Should cache project-bound package managers by project', () => { @@ -932,10 +934,9 @@ suite('PythonPackageManagerApi Tests', () => { const registeredManager = envManagers.packageManagers[0]; const provider = registerEnvironmentProvider(registeredManager.id, true); - const resolution = envManagers.resolvePackageManagerForEnvironment(provider.environment); + const manager = envManagers.getPackageManager(provider.environment); - assert.strictEqual(resolution.kind, 'resolved'); - assert.strictEqual(resolution.manager, registeredManager); + assert.strictEqual(manager, registeredManager); assert.ok(provider.getEnvironment.notCalled); assert.ok(provider.getLastKnownEnvironment.notCalled); @@ -973,11 +974,10 @@ suite('PythonPackageManagerApi Tests', () => { const environmentProvider = registerEnvironmentProvider(packageProvider.manager.id, true); configureDefaultManagers(packageProvider.manager.id, environmentProvider.managerId); - const resolution = envManagers.resolvePackageManagerForEnvironment(environmentProvider.environment); + const manager = envManagers.getPackageManager(environmentProvider.environment); assert.ok(packageProvider.createForProject.calledOnceWithExactly(project)); - assert.strictEqual(resolution.kind, 'resolved'); - assert.strictEqual(resolution.manager?.project, project); + assert.strictEqual(manager?.project, project); assert.strictEqual(environmentProvider.getLastKnownEnvironment.callCount, 1); assert.ok(environmentProvider.getEnvironment.notCalled); @@ -988,10 +988,9 @@ suite('PythonPackageManagerApi Tests', () => { const packageProvider = registerProjectAwarePackageManager(); const environmentProvider = registerEnvironmentProvider(packageProvider.manager.id, false); - const resolution = envManagers.resolvePackageManagerForEnvironment(environmentProvider.environment); + const manager = envManagers.getPackageManager(environmentProvider.environment); - assert.strictEqual(resolution.kind, 'projectRequired'); - assert.strictEqual(resolution.manager, undefined); + assert.strictEqual(manager, undefined); assert.ok(packageProvider.createForProject.notCalled); assert.ok(environmentProvider.getEnvironment.notCalled); @@ -1014,10 +1013,9 @@ suite('PythonPackageManagerApi Tests', () => { const environmentProvider = registerEnvironmentProvider(packageProvider.manager.id, true); configureDefaultManagers(packageProvider.manager.id, environmentProvider.managerId); - const resolution = envManagers.resolvePackageManagerForEnvironment(environmentProvider.environment); + const manager = envManagers.getPackageManager(environmentProvider.environment); - assert.strictEqual(resolution.kind, 'projectRequired'); - assert.strictEqual(resolution.manager, undefined); + assert.strictEqual(manager, undefined); assert.ok(packageProvider.createForProject.notCalled); assert.strictEqual(environmentProvider.getLastKnownEnvironment.callCount, 2); assert.ok(environmentProvider.getEnvironment.notCalled); diff --git a/src/test/features/views/envManagersView.unit.test.ts b/src/test/features/views/envManagersView.unit.test.ts index 79ad6d2ed..ad7015308 100644 --- a/src/test/features/views/envManagersView.unit.test.ts +++ b/src/test/features/views/envManagersView.unit.test.ts @@ -57,8 +57,8 @@ suite('EnvManagerView.reveal Tests', () => { envManagers.setup((e) => e.onDidChangePackages).returns(() => onDidChangePackagesEmitter.event); envManagers.setup((e) => e.onDidChangePackageManager).returns(() => onDidChangePackageManagerEmitter.event); envManagers - .setup((e) => e.resolvePackageManagerForEnvironment(typeMoq.It.isAny())) - .returns(() => ({ kind: 'notFound' })); + .setup((e) => e.getPackageManager(typeMoq.It.isAny())) + .returns(() => undefined); envManagers .setup((e) => e.onDidChangeProjectPackageManager) .returns(() => onDidChangeProjectPackageManagerEmitter.event); From 5e51aad3c2438dd8e104a1922e8c396268099577 Mon Sep 17 00:00:00 2001 From: Eduardo Villalpando Mello Date: Mon, 28 Sep 2026 10:22:13 -0700 Subject: [PATCH 11/11] Address feedback --- src/features/envManagers.ts | 35 +++++++++---------- src/test/extensionApi.unit.test.ts | 11 ++++-- .../features/packageManager.api.unit.test.ts | 7 +++- 3 files changed, 32 insertions(+), 21 deletions(-) diff --git a/src/features/envManagers.ts b/src/features/envManagers.ts index 05124b68a..ece0a6bc3 100644 --- a/src/features/envManagers.ts +++ b/src/features/envManagers.ts @@ -428,33 +428,32 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { return undefined; } - if (context === undefined || context instanceof Uri) { - const project = context ? this.pm.get(context) : undefined; - const defaultPackageManagerId = getDefaultPkgManagerSetting(this.pm, context); - const defaultEnvironmentManagerId = getDefaultEnvManagerSetting(this.pm, context); - const packageManagerId = - defaultPackageManagerId || - this._environmentManagers.get(defaultEnvironmentManagerId)?.preferredPackageManagerId; - const manager = packageManagerId ? this._packageManagers.get(packageManagerId) : undefined; - return project ? this.projectPackageManagers.getOrCreate(manager, project) : manager; - } - + // Direct lookups by manager id or package identity. if (typeof context === 'string') { return this._packageManagers.get(context); } - - if ('pkgId' in context) { + if (context !== undefined && 'pkgId' in context) { return this._packageManagers.get(context.pkgId.managerId); } - const preferredId = this._environmentManagers.get(context.envId.managerId)?.preferredPackageManagerId; - const manager = preferredId ? this._packageManagers.get(preferredId) : undefined; + // Project or global scope: resolve the configured manager and scope it to the project. + if (context === undefined || context instanceof Uri) { + const project = context ? this.pm.get(context) : undefined; + const managerId = + getDefaultPkgManagerSetting(this.pm, context) || + this._environmentManagers.get(getDefaultEnvManagerSetting(this.pm, context))?.preferredPackageManagerId; + const manager = managerId ? this._packageManagers.get(managerId) : undefined; + return project ? this.projectPackageManagers.getOrCreate(manager, project) : manager; + } + + // Environment scope: use the environment manager's preferred package manager. A + // project-aware manager (e.g. Poetry) is bound to the single tracked project that uses + // this environment; without a unique project there is no working directory to run in. + const managerId = this._environmentManagers.get(context.envId.managerId)?.preferredPackageManagerId; + const manager = managerId ? this._packageManagers.get(managerId) : undefined; if (!manager?.createForProject) { return manager; } - // The preferred manager is project-aware (e.g. Poetry). Bind it to the single tracked - // project that uses this environment; without a unique project there is no working - // directory to run in, so no manager is returned. const project = this.findUniqueProjectForEnvironment(context); return project ? this.getPackageManager(project.uri) : undefined; } diff --git a/src/test/extensionApi.unit.test.ts b/src/test/extensionApi.unit.test.ts index 04f702249..62459fbc3 100644 --- a/src/test/extensionApi.unit.test.ts +++ b/src/test/extensionApi.unit.test.ts @@ -210,11 +210,18 @@ suite('PythonEnvironmentApiImpl - package resolution', () => { envId: { id: 'environment', managerId: 'environment-manager' }, } as PythonEnvironment; + function expectNoPackageManagerError(error: unknown): true { + assert.ok(error instanceof Error, 'Expected an Error'); + assert.strictEqual(error.name, 'Error'); + assert.strictEqual(error.message, 'No package manager found'); + return true; + } + test('rejects mutations and refreshes when no package manager resolves', async () => { const { api } = createApi(undefined); - await assert.rejects(api.managePackages(environment, { install: ['example'] }), /No package manager found/); - await assert.rejects(api.refreshPackages(environment), /No package manager found/); + await assert.rejects(api.managePackages(environment, { install: ['example'] }), expectNoPackageManagerError); + await assert.rejects(api.refreshPackages(environment), expectNoPackageManagerError); }); test('returns undefined for reads when no package manager resolves', async () => { diff --git a/src/test/features/packageManager.api.unit.test.ts b/src/test/features/packageManager.api.unit.test.ts index b7e56522c..64f0ad4cd 100644 --- a/src/test/features/packageManager.api.unit.test.ts +++ b/src/test/features/packageManager.api.unit.test.ts @@ -1074,7 +1074,12 @@ suite('PythonPackageManagerApi Tests', () => { assert.ok(scopedDisposers[1].notCalled); await assert.rejects( () => original!.manage(environment.object, { install: ['example'] }), - /Package manager .* has been disposed/, + (error: unknown) => { + assert.ok(error instanceof Error, 'Expected an Error'); + assert.strictEqual(error.name, 'Error'); + assert.strictEqual(error.message, `Package manager ${original!.id} has been disposed`); + return true; + }, ); const packageChange = {