From 727b54ee83aafdc7c0414c9a7c82f2921f18565b Mon Sep 17 00:00:00 2001 From: Rob Webb Date: Tue, 18 Aug 2026 23:23:06 +0100 Subject: [PATCH 1/8] refactor(providers): decouple the pipeline from Repository and pass ports explicitly - install_service.py and upgrade_service.py merge into pipeline.py, with 'PIPELINE_TIMEOUT' - build_orchestrator(), run_install(), and run_upgrade() now take 'catalog'/'cache_mgr'/'formula' as explicit keyword ports instead of a whole 'repo' object - 'RepositoryCatalogAdapter' becomes 'CatalogAdapter', taking the catalog and cache manager directly - New 'CatalogSource', 'InstalledSource', and 'FallbackBackend' protocols name only the four catalog and installed-state reads, and the two required verbs - Keeps the boundary checkable without dragging the full 'Catalog' surface across it - 'PackageBackend' splits into 'InstallBackend'/'UninstallBackend'/'UpgradeBackend', with 'PackageBackend' composing all three - _make_backend()'s 'SimpleNamespace' closure becomes a real 'BrewBackend' class with a static assertion against 'PackageBackend' - _remove_formula() moves from uninstall_service.py to cellar.remove_rack(), where the rest of the keg-removal primitives live - run_uninstall() now takes an 'UninstallBackend' rather than a repo - 'FormulaRowP' re-declared as read-only properties, as a protocol's mutable attributes are invariant and need a writable target, which the frozen 'FormulaRow' isn't - 'bottle_url'/ 'bottle_sha256' are now correctly typed as optional, with the narrowed sha bound to a local before use - pin_service.py renamed to pinning.py, and its test file to match - test_install_service.py/test_upgrade_service.py merge into test_pipeline.py - '_stubs.MockRepo' replaced by a 'RecordingBackend' - _remove_formula() cellar tests move alongside remove_rack() --- src/brewery/core/repo.py | 29 +- src/brewery/providers/base.py | 16 +- src/brewery/providers/brew.py | 45 +-- src/brewery/providers/cellar.py | 23 ++ src/brewery/providers/install_adapters.py | 67 ++++- src/brewery/providers/install_service.py | 99 ------- src/brewery/providers/orchestrator.py | 77 ++++- .../providers/{pin_service.py => pinning.py} | 0 src/brewery/providers/pipeline.py | 164 ++++++++++ src/brewery/providers/uninstall_service.py | 44 +-- src/brewery/providers/upgrade_service.py | 48 --- tests/integration/test_repo.py | 6 +- tests/unit/_stubs.py | 22 +- tests/unit/test_cellar.py | 52 +++- tests/unit/test_install_adapters.py | 77 +++-- tests/unit/test_install_service.py | 163 ---------- .../{test_pin_service.py => test_pinning.py} | 2 +- tests/unit/test_pipeline.py | 280 ++++++++++++++++++ tests/unit/test_uninstall_service.py | 112 ++----- tests/unit/test_upgrade_service.py | 123 -------- 20 files changed, 800 insertions(+), 649 deletions(-) delete mode 100644 src/brewery/providers/install_service.py rename src/brewery/providers/{pin_service.py => pinning.py} (100%) create mode 100644 src/brewery/providers/pipeline.py delete mode 100644 src/brewery/providers/upgrade_service.py delete mode 100644 tests/unit/test_install_service.py rename tests/unit/{test_pin_service.py => test_pinning.py} (97%) create mode 100644 tests/unit/test_pipeline.py delete mode 100644 tests/unit/test_upgrade_service.py diff --git a/src/brewery/core/repo.py b/src/brewery/core/repo.py index 39bfcc6..e0e1447 100644 --- a/src/brewery/core/repo.py +++ b/src/brewery/core/repo.py @@ -153,9 +153,16 @@ async def install_packages( await self.cask.install(names=names) else: - from brewery.providers.install_service import run_install - - await run_install(self, names, run_brew=run_brew, progress=progress) + from brewery.providers.pipeline import run_install + + await run_install( + names, + catalog=self.catalog, + cache_mgr=self.cache_mgr, + formula=self.formula, + run_brew=run_brew, + progress=progress, + ) self.cache_mgr.invalidate() installed_by_name: dict[str, Package] = { @@ -238,7 +245,7 @@ async def uninstall_packages( if formula_names: from brewery.providers.uninstall_service import run_uninstall - await run_uninstall(self, formula_names) + await run_uninstall(formula_names, formula=self.formula) if cask_names: await self.cask.uninstall(names=cask_names) @@ -426,7 +433,7 @@ async def upgrade_packages( } if formula_names: - from brewery.providers.upgrade_service import run_upgrade + from brewery.providers.pipeline import run_upgrade old_kegs = { p.name: Path(p.path) @@ -434,7 +441,13 @@ async def upgrade_packages( if p.kind == PackageKind.FORMULA and p.path } await run_upgrade( - self, formula_names, old_kegs, run_brew=run_brew, progress=progress + formula_names, + old_kegs, + catalog=self.catalog, + cache_mgr=self.cache_mgr, + formula=self.formula, + run_brew=run_brew, + progress=progress, ) if cask_names: @@ -620,7 +633,7 @@ def pin_packages( Returns: Tuple of (pinned names, (name, reason) advisories, (name, reason) failures). """ - from brewery.providers.pin_service import pin + from brewery.providers.pinning import pin env: BreweryENV = self.cache_mgr.env or get_brewery_env() pkgs, failures = self._resolve_installed_formulae(names) @@ -654,7 +667,7 @@ def unpin_packages( Returns: Tuple of (unpinned names, (name, reason) advisories, (name, reason) failures). """ - from brewery.providers.pin_service import unpin + from brewery.providers.pinning import unpin env: BreweryENV = self.cache_mgr.env or get_brewery_env() pkgs, failures = self._resolve_installed_formulae(names) diff --git a/src/brewery/providers/base.py b/src/brewery/providers/base.py index 85a5355..726c893 100644 --- a/src/brewery/providers/base.py +++ b/src/brewery/providers/base.py @@ -5,17 +5,29 @@ from typing import Protocol -class PackageBackend(Protocol): - """Protocol for package backends.""" +class InstallBackend(Protocol): + """Protocol for backends that can install packages.""" async def install(self, names: list[str]) -> list[str]: """Install package(s) by name.""" ... + +class UninstallBackend(Protocol): + """Protocol for backends that can uninstall packages.""" + async def uninstall(self, names: list[str]) -> list[str]: """Uninstall package(s) by name.""" ... + +class UpgradeBackend(Protocol): + """Protocol for backends that can upgrade packages.""" + async def upgrade(self, names: list[str]) -> list[str]: """Upgrade package(s) by name.""" ... + + +class PackageBackend(InstallBackend, UninstallBackend, UpgradeBackend, Protocol): + """Protocol for package backends.""" diff --git a/src/brewery/providers/brew.py b/src/brewery/providers/brew.py index d816424..67640be 100644 --- a/src/brewery/providers/brew.py +++ b/src/brewery/providers/brew.py @@ -1,15 +1,7 @@ -"""Homebrew package backends (formula + cask). - -Replaces the near-identical brew_formula.py and brew_cask.py: a single factory -builds both backends, differing only by the kind flag. The install/upgrade -"already installed" / "pinned" interpretation lives here -- it's package -semantics, not something the shell primitive should know. -""" +"""Homebrew fallback package backends (formula + cask).""" from __future__ import annotations -from types import SimpleNamespace - from brewery.core.errors import ( AlreadyInstalledWarning, BrewCommandError, @@ -17,6 +9,7 @@ ) from brewery.core.logging import BreweryLogger, get_logger from brewery.core.shell import BrewOutput, BrewResult, run_brew +from brewery.providers.base import PackageBackend log: BreweryLogger = get_logger(name=__name__) @@ -69,11 +62,22 @@ async def _run(subcommand: str, names: list[str], flags: list[str]) -> list[str] return names -def _make_backend(kind_flag: str) -> SimpleNamespace: - """Build a backend for a package kind. install/uninstall carry the kind flag; - upgrade takes none (brew infers it), matching the original providers.""" +class BrewBackend: # Implements base.PackageBackend + """A brew-backed package backend for one package kind. + + install/uninstall carry the kind flag; upgrade takes none (brew infers it), + matching the original providers. + """ + + def __init__(self, kind_flag: str) -> None: + """Initialise the backend. + + Args: + kind_flag: The brew flag selecting the kind, e.g. `"--formula"`. + """ + self._kind_flag = kind_flag - async def install(names: list[str]) -> list[str]: + async def install(self, names: list[str]) -> list[str]: """Install packages by name. Args: @@ -82,9 +86,9 @@ async def install(names: list[str]) -> list[str]: Returns: The same list of names on success. """ - return await _run("install", names, [kind_flag]) + return await _run("install", names, [self._kind_flag]) - async def uninstall(names: list[str]) -> list[str]: + async def uninstall(self, names: list[str]) -> list[str]: """Uninstall packages by name. Args: @@ -93,9 +97,9 @@ async def uninstall(names: list[str]) -> list[str]: Returns: The same list of names on success. """ - return await _run("uninstall", names, [kind_flag]) + return await _run("uninstall", names, [self._kind_flag]) - async def upgrade(names: list[str]) -> list[str]: + async def upgrade(self, names: list[str]) -> list[str]: """Upgrade packages by name. Args: @@ -106,8 +110,9 @@ async def upgrade(names: list[str]) -> list[str]: """ return await _run("upgrade", names, []) - return SimpleNamespace(install=install, uninstall=uninstall, upgrade=upgrade) +# Static assertion that the backend satisfies the protocol +_package_backend: type[PackageBackend] = BrewBackend -formula_backend = _make_backend("--formula") -cask_backend = _make_backend("--cask") +formula_backend = BrewBackend("--formula") +cask_backend = BrewBackend("--cask") diff --git a/src/brewery/providers/cellar.py b/src/brewery/providers/cellar.py index 9814ffd..13f747a 100644 --- a/src/brewery/providers/cellar.py +++ b/src/brewery/providers/cellar.py @@ -13,6 +13,8 @@ from pathlib import Path from brewery.core.errors import CellarError +from brewery.core.locks import formula_lock +from brewery.providers.linker import unlink_keg _IS_DARWIN = sys.platform == "darwin" @@ -97,6 +99,27 @@ def onerror(func, p, exc) -> None: shutil.rmtree(path, onerror=onerror) +def remove_rack(cellar_dir: Path, prefix: Path, name: str) -> None: + """Unlink every installed keg of a formula, then delete its cellar dir. + + Args: + cellar_dir: /Cellar/. + prefix: The Homebrew prefix. + name: The formula name. + + Raises: + OperationInProgressError: Another process holds the formula's rack lock. + """ + if not cellar_dir.exists(): + return + + with formula_lock(name, prefix=prefix): + for keg in sorted(p for p in cellar_dir.iterdir() if p.is_dir()): + unlink_keg(keg, prefix=prefix, name=name) # realpath no-ops old kegs + + shutil.rmtree(cellar_dir) + + def _link_opt(prefix: Path, name: str, version: str) -> Path: """Create/refresh /opt/ -> ../Cellar// (relative). diff --git a/src/brewery/providers/install_adapters.py b/src/brewery/providers/install_adapters.py index e5a50d2..924a02a 100644 --- a/src/brewery/providers/install_adapters.py +++ b/src/brewery/providers/install_adapters.py @@ -3,17 +3,57 @@ from __future__ import annotations from pathlib import Path +from typing import Protocol from brewery.core.errors import AlreadyInstalledWarning, BrewCommandError -from brewery.core.models import PackageKind +from brewery.core.models import Package, PackageKind +from brewery.providers.base import InstallBackend, UpgradeBackend from brewery.providers.orchestrator import BrewPort, CatalogPort, FormulaRowP -class RepositoryCatalogAdapter: # Implements orchestrator.CatalogPort - """Binds CatalogPort to the existing Repository. +class CatalogSource(Protocol): + """The catalog reads the adapter forwards (satisfied by `core.catalog.Catalog`). - Catalog lookups delegate to repo.catalog; installed-state (is_satisfied) - delegates to repo.cache_mgr, since the catalog has no view of what's + Narrower than `Catalog` on purpose: naming only these four keeps the adapter + checkable without dragging the whole catalog surface across the boundary. + """ + + def get_formula(self, name: str) -> FormulaRowP | None: + """Get a formula row by canonical name.""" + ... + + def resolve_alias(self, name: str) -> str: + """Resolve an alias to its canonical formula name.""" + ... + + def runtime_deps(self, name: str) -> list[str]: + """Direct runtime dependency names for a formula.""" + ... + + def aliases_of(self, name: str) -> list[str]: + """Aliases that resolve to a formula.""" + ... + + +class InstalledSource(Protocol): + """The installed-state read the adapter needs (satisfied by `CacheManager`).""" + + def find_installed( + self, name: str, kind: PackageKind | None = None + ) -> Package | None: + """Return one installed package by name, or None.""" + ... + + +class FallbackBackend(InstallBackend, UpgradeBackend, Protocol): + """The two verbs `BrewAdapter` delegates to the formula backend.""" + + +class CatalogAdapter: # Implements orchestrator.CatalogPort + """Binds CatalogPort to the catalog and the installed-state cache. + + Catalog lookups delegate to the catalog; installed-state (is_satisfied) + delegates to the cache manager, since the catalog has no view of what's installed. Every method here must be called on the thread that opened the catalog, i.e. @@ -21,14 +61,15 @@ class RepositoryCatalogAdapter: # Implements orchestrator.CatalogPort `asyncio.to_thread`, never from inside one. See `Catalog`'s docstring. """ - def __init__(self, repo) -> None: + def __init__(self, catalog: CatalogSource, cache_mgr: InstalledSource) -> None: """Initialise the adapter. Args: - repo: The repository instance providing catalog and cache access. + catalog: The catalog backing formula/alias/dependency lookups. + cache_mgr: The installed-state cache, for `is_satisfied`. """ - self._repo = repo - self._catalog = repo.catalog + self._catalog = catalog + self._cache_mgr = cache_mgr def get_formula(self, name: str) -> FormulaRowP | None: """Get a formula by name. @@ -86,7 +127,7 @@ def is_satisfied(self, name: str) -> bool: Returns: True if a complete installed keg is found in the cache, False otherwise. """ - pkg = self._repo.cache_mgr.find_installed(name, PackageKind.FORMULA) + pkg = self._cache_mgr.find_installed(name, PackageKind.FORMULA) if pkg is None or not pkg.path: return False @@ -94,7 +135,7 @@ def is_satisfied(self, name: str) -> bool: # Static assertion that the adapter satisfies the port -_catalog_port: type[CatalogPort] = RepositoryCatalogAdapter +_catalog_port: type[CatalogPort] = CatalogAdapter class BrewAdapter: @@ -107,11 +148,11 @@ class BrewAdapter: crossing the port boundary. """ - def __init__(self, formula_backend, run_brew) -> None: + def __init__(self, formula_backend: FallbackBackend, run_brew) -> None: """Initialise the adapter. Args: - formula_backend: e.g. brew_formula.backend (has async install()). + formula_backend: e.g. brew.formula_backend (has async install()). run_brew: async callable invoking `brew ` (your passthrough runner), raising BrewCommandError on a non-zero exit. """ diff --git a/src/brewery/providers/install_service.py b/src/brewery/providers/install_service.py deleted file mode 100644 index e2879de..0000000 --- a/src/brewery/providers/install_service.py +++ /dev/null @@ -1,99 +0,0 @@ -"""Assemble and run the native install pipeline for a set of formulae.""" - -from __future__ import annotations - -import functools -from collections.abc import Awaitable, Callable - -import httpx - -from brewery.core.config import BreweryENV, get_brewery_env -from brewery.providers.downloader import Downloader -from brewery.providers.install_adapters import BrewAdapter, RepositoryCatalogAdapter -from brewery.providers.manifest import fetch_bottle_tab -from brewery.providers.orchestrator import ( - InstallConfig, - InstallReport, - Orchestrator, - ProgressPort, -) - -RunBrew = Callable[[list[str]], Awaitable[object]] - -# httpx's 5s default is too tight for streaming a bottle body; read/write -# timeouts are per-socket-operation, not whole-request, so 30s bounds a stall -# without capping how long a large bottle may take -PIPELINE_TIMEOUT = httpx.Timeout(30.0, connect=10.0) - - -def build_orchestrator( - repo, - *, - client: httpx.AsyncClient, - env: BreweryENV, - run_brew: RunBrew, - progress: ProgressPort | None = None, -) -> Orchestrator: - """Assemble an Orchestrator bound to an open client and the repo's ports. - - Shared by the install and upgrade services. - - Args: - repo: The Repository providing catalog/cache/formula-backend ports. - client: An open httpx.AsyncClient. - env: Brewery environment (paths). - run_brew: Async `brew ` runner for link/postinstall fallback. - progress: Optional progress sink forwarded to the Orchestrator. - - Returns: - A configured Orchestrator. - """ - config = InstallConfig( - prefix=env.prefix, - repository=env.repository, - api_path=str(env.api_path), # /api/formula.jws.json - staging_root=env.prefix / "var" / "homebrew" / ".staging", - ) - - return Orchestrator( - catalog=RepositoryCatalogAdapter(repo), - downloader=Downloader(cache_dir=env.bottle_cache, client=client), - tab_fetcher=functools.partial(fetch_bottle_tab, client), - brew=BrewAdapter(repo.formula, run_brew), - config=config, - progress=progress, - ) - - -async def run_install( - repo, - names: list[str], - *, - run_brew: RunBrew, - env: BreweryENV | None = None, - progress: ProgressPort | None = None, -) -> InstallReport: - """Install `names` via the native pipeline, brew-falling-back per formula. - - Args: - repo: The Repository. - names: Formula names to install (deps resolved from the catalog). - run_brew: Async `brew ` runner for link/postinstall fallback. - env: Brewery environment, resolved if omitted. - progress: Optional progress sink forwarded to the Orchestrator. - - Returns: - The InstallReport (per-formula outcomes). - """ - env = env or get_brewery_env() - - async with httpx.AsyncClient(timeout=PIPELINE_TIMEOUT) as client: - orchestrator = build_orchestrator( - repo, - client=client, - env=env, - run_brew=run_brew, - progress=progress, - ) - - return await orchestrator.install(names) diff --git a/src/brewery/providers/orchestrator.py b/src/brewery/providers/orchestrator.py index ace8b6e..1621f6d 100644 --- a/src/brewery/providers/orchestrator.py +++ b/src/brewery/providers/orchestrator.py @@ -49,19 +49,67 @@ class FormulaRowP(Protocol): - """Protocol for interacting with formula rows.""" + """Protocol for interacting with formula rows. - name: str - tap: str | None - version: str - revision: int - version_scheme: int - keg_only: bool - post_install: bool - bottle_url: str - bottle_sha256: str - bottle_cellar: str | None - bottle_rebuild: int + Declared as read-only properties rather than plain attributes: a protocol's + mutable attributes are invariant and require a writable target, which the + frozen `catalog.store.FormulaRow` is not. Nothing here ever writes to a row. + """ + + @property + def name(self) -> str: + """The canonical formula name.""" + ... + + @property + def tap(self) -> str | None: + """The tap the formula came from, if recorded.""" + ... + + @property + def version(self) -> str: + """The upstream version string.""" + ... + + @property + def revision(self) -> int: + """Homebrew's packaging revision.""" + ... + + @property + def version_scheme(self) -> int: + """The formula's version-comparison scheme.""" + ... + + @property + def keg_only(self) -> bool: + """Whether the formula is keg-only (never linked into the prefix).""" + ... + + @property + def post_install(self) -> bool: + """Whether the formula defines a post-install step.""" + ... + + @property + def bottle_url(self) -> str | None: + """The bottle download URL, or None when no bottle exists here.""" + ... + + @property + def bottle_sha256(self) -> str | None: + """The bottle's sha256, or None when no bottle exists here.""" + ... + + @property + def bottle_cellar(self) -> str | None: + """The cellar path the bottle was built for, if recorded.""" + ... + + @property + def bottle_rebuild(self) -> int: + """The bottle's rebuild counter.""" + ... class CatalogPort(Protocol): @@ -640,7 +688,8 @@ async def _fetch( if fr is None or fr.bottle_url is None or fr.bottle_sha256 is None: return None, None, "no bottle in catalog", "no bottle in catalog" - ref = BottleRef(name, fr.bottle_url, fr.bottle_sha256) + sha256: str = fr.bottle_sha256 + ref = BottleRef(name, fr.bottle_url, sha256) tab_error: str | None = None bottle_error: str | None = None @@ -656,7 +705,7 @@ async def _tab() -> BottleTabInfo | None: return await self.tab_fetcher( name=name, version=fr.version, - bottle_sha256=fr.bottle_sha256, + bottle_sha256=sha256, revision=fr.revision, rebuild=fr.bottle_rebuild, ) diff --git a/src/brewery/providers/pin_service.py b/src/brewery/providers/pinning.py similarity index 100% rename from src/brewery/providers/pin_service.py rename to src/brewery/providers/pinning.py diff --git a/src/brewery/providers/pipeline.py b/src/brewery/providers/pipeline.py new file mode 100644 index 0000000..811973a --- /dev/null +++ b/src/brewery/providers/pipeline.py @@ -0,0 +1,164 @@ +"""Assemble and run the native bottle pipeline for a set of formulae. + +Install and upgrade share one Orchestrator assembly; only the terminal call +differs. Nothing here knows about `Repository` -- the ports it needs are passed +in explicitly, so the command policy that chooses the formulae lives a layer up +in `brewery.services`. +""" + +from __future__ import annotations + +import functools +from collections.abc import Awaitable, Callable +from pathlib import Path + +import httpx + +from brewery.core.config import BreweryENV, get_brewery_env +from brewery.providers.base import PackageBackend +from brewery.providers.downloader import Downloader +from brewery.providers.install_adapters import ( + BrewAdapter, + CatalogAdapter, + CatalogSource, + InstalledSource, +) +from brewery.providers.manifest import fetch_bottle_tab +from brewery.providers.orchestrator import ( + InstallConfig, + InstallReport, + Orchestrator, + ProgressPort, +) + +RunBrew = Callable[[list[str]], Awaitable[object]] + +# httpx's 5s default is too tight for streaming a bottle body; read/write +# timeouts are per-socket-operation, not whole-request, so 30s bounds a stall +# without capping how long a large bottle may take +PIPELINE_TIMEOUT = httpx.Timeout(30.0, connect=10.0) + + +def build_orchestrator( + *, + catalog: CatalogSource, + cache_mgr: InstalledSource, + formula: PackageBackend, + client: httpx.AsyncClient, + env: BreweryENV, + run_brew: RunBrew, + progress: ProgressPort | None = None, +) -> Orchestrator: + """Assemble an Orchestrator bound to an open client and the given ports. + + Shared by `run_install` and `run_upgrade`. + + Args: + catalog: The catalog backing formula/alias/dependency lookups. + cache_mgr: Installed-state cache, used to answer `is_satisfied`. + formula: Formula backend for the per-formula brew fallback. + client: An open httpx.AsyncClient. + env: Brewery environment (paths). + run_brew: Async `brew ` runner for link/postinstall fallback. + progress: Optional progress sink forwarded to the Orchestrator. + + Returns: + A configured Orchestrator. + """ + config = InstallConfig( + prefix=env.prefix, + repository=env.repository, + api_path=str(env.api_path), # /api/formula.jws.json + staging_root=env.prefix / "var" / "homebrew" / ".staging", + ) + + return Orchestrator( + catalog=CatalogAdapter(catalog=catalog, cache_mgr=cache_mgr), + downloader=Downloader(cache_dir=env.bottle_cache, client=client), + tab_fetcher=functools.partial(fetch_bottle_tab, client), + brew=BrewAdapter(formula, run_brew), + config=config, + progress=progress, + ) + + +async def run_install( + names: list[str], + *, + catalog: CatalogSource, + cache_mgr: InstalledSource, + formula: PackageBackend, + run_brew: RunBrew, + env: BreweryENV | None = None, + progress: ProgressPort | None = None, +) -> InstallReport: + """Install `names` via the native pipeline, brew-falling-back per formula. + + Args: + names: Formula names to install (deps resolved from the catalog). + catalog: The catalog backing formula/alias/dependency lookups. + cache_mgr: Installed-state cache, used to answer `is_satisfied`. + formula: Formula backend for the per-formula brew fallback. + run_brew: Async `brew ` runner for link/postinstall fallback. + env: Brewery environment, resolved if omitted. + progress: Optional progress sink forwarded to the Orchestrator. + + Returns: + The InstallReport (per-formula outcomes). + """ + env = env or get_brewery_env() + + async with httpx.AsyncClient(timeout=PIPELINE_TIMEOUT) as client: + orchestrator = build_orchestrator( + catalog=catalog, + cache_mgr=cache_mgr, + formula=formula, + client=client, + env=env, + run_brew=run_brew, + progress=progress, + ) + + return await orchestrator.install(names) + + +async def run_upgrade( + names: list[str], + old_kegs: dict[str, Path], + *, + catalog: CatalogSource, + cache_mgr: InstalledSource, + formula: PackageBackend, + run_brew: RunBrew, + env: BreweryENV | None = None, + progress: ProgressPort | None = None, +) -> InstallReport: + """Upgrade `names` via the native pipeline, brew-falling-back per formula. + + Args: + names: Formula names to upgrade (already resolved to outdated targets). + old_kegs: Each target's current active keg, to unlink and stamp as replaced. + catalog: The catalog backing formula/alias/dependency lookups. + cache_mgr: Installed-state cache, used to answer `is_satisfied`. + formula: Formula backend for the per-formula brew fallback. + run_brew: Async `brew ` runner for link/postinstall fallback. + env: Brewery environment, resolved if omitted. + progress: Optional progress sink forwarded to the Orchestrator. + + Returns: + The InstallReport (per-formula outcomes). + """ + env = env or get_brewery_env() + + async with httpx.AsyncClient(timeout=PIPELINE_TIMEOUT) as client: + orch = build_orchestrator( + catalog=catalog, + cache_mgr=cache_mgr, + formula=formula, + client=client, + env=env, + run_brew=run_brew, + progress=progress, + ) + + return await orch.upgrade(names, old_kegs) diff --git a/src/brewery/providers/uninstall_service.py b/src/brewery/providers/uninstall_service.py index 96e4cf8..4d86295 100644 --- a/src/brewery/providers/uninstall_service.py +++ b/src/brewery/providers/uninstall_service.py @@ -3,64 +3,42 @@ from __future__ import annotations import asyncio -import shutil from brewery.core.config import BreweryENV, get_brewery_env from brewery.core.errors import BrewCommandError, OperationInProgressError -from brewery.core.locks import formula_lock from brewery.core.logging import BreweryLogger, get_logger -from brewery.providers.linker import unlink_keg +from brewery.providers.base import UninstallBackend +from brewery.providers.cellar import remove_rack log: BreweryLogger = get_logger(name=__name__) async def run_uninstall( - repo, names: list[str], *, env: BreweryENV | None = None + names: list[str], + *, + formula: UninstallBackend, + env: BreweryENV | None = None, ) -> None: """Unlink + remove each formula's kegs, brew-falling-back per formula. Args: - repo: The Repository (for prefix/cellar paths). names: Canonical formula names to uninstall. - run_brew: Async `brew ` runner for the fallback path. + formula: Formula backend for the per-formula brew fallback. env: Brewery environment (paths), resolved if omitted. """ env = env or get_brewery_env() for name in names: try: - await asyncio.to_thread( - _remove_formula, env.cellar / name, env.prefix, name - ) + await asyncio.to_thread(remove_rack, env.cellar / name, env.prefix, name) except OperationInProgressError as exc: # brew locks the same rack, so falling back to it would fail too; - # repo._verify_removed reports the survivor as a failure + # the caller's removal verification reports the survivor as a failure log.warning(event="uninstall_rack_locked", formula=name, error=str(exc)) except OSError: try: - await repo.formula.uninstall(names=[name]) + await formula.uninstall(names=[name]) except BrewCommandError: - pass # repo._verify_removed reports the survivor as a failure - - -def _remove_formula(cellar_dir, prefix, name: str) -> None: - """Unlink every installed keg of a formula, then delete its cellar dir. - - Args: - cellar_dir: /Cellar/. - prefix: The Homebrew prefix. - name: The formula name. - - Raises: - OperationInProgressError: Another process holds the formula's rack lock. - """ - if not cellar_dir.exists(): - return - - with formula_lock(name, prefix=prefix): - for keg in sorted(p for p in cellar_dir.iterdir() if p.is_dir()): - unlink_keg(keg, prefix=prefix, name=name) # realpath no-ops old kegs - - shutil.rmtree(cellar_dir) + pass # verification reports the survivor as a failure diff --git a/src/brewery/providers/upgrade_service.py b/src/brewery/providers/upgrade_service.py deleted file mode 100644 index fa406e0..0000000 --- a/src/brewery/providers/upgrade_service.py +++ /dev/null @@ -1,48 +0,0 @@ -"""Assemble and run the native upgrade pipeline for a set of formulae.""" - -from __future__ import annotations - -from collections.abc import Awaitable, Callable -from pathlib import Path - -import httpx - -from brewery.core.config import BreweryENV, get_brewery_env -from brewery.providers.install_service import PIPELINE_TIMEOUT, build_orchestrator -from brewery.providers.orchestrator import InstallReport, ProgressPort - -RunBrew = Callable[[list[str]], Awaitable[object]] - - -async def run_upgrade( - repo, - names: list[str], - old_kegs: dict[str, Path], - *, - run_brew: RunBrew, - env: BreweryENV | None = None, - progress: ProgressPort | None = None, -) -> InstallReport: - """Upgrade `names` via the native pipeline, brew-falling-back per formula. - - Args: - repo: The Repository. - names: Formula names to upgrade (already resolved to outdated targets). - old_kegs: Each target's current active keg, to unlink and stamp as replaced. - run_brew: Async `brew ` runner for link/postinstall fallback. - env: Brewery environment, resolved if omitted. - progress: Optional progress sink forwarded to the Orchestrator. - - Returns: - The InstallReport (per-formula outcomes). - """ - env = env or get_brewery_env() - async with httpx.AsyncClient(timeout=PIPELINE_TIMEOUT) as client: - orch = build_orchestrator( - repo, - client=client, - env=env, - run_brew=run_brew, - progress=progress, - ) - return await orch.upgrade(names, old_kegs) diff --git a/tests/integration/test_repo.py b/tests/integration/test_repo.py index 763bde1..c193599 100644 --- a/tests/integration/test_repo.py +++ b/tests/integration/test_repo.py @@ -386,7 +386,7 @@ def _boom(*a, **k) -> None: """ raise OSError("native failed") - monkeypatch.setattr(svc, "_remove_formula", _boom) + monkeypatch.setattr(svc, "remove_rack", _boom) # mock_brew logs but does not delete the keg, so _verify_removed sees it removed, failures = await repo.uninstall_packages( @@ -522,7 +522,7 @@ def _boom(*a, **k) -> None: """ raise OSError("native failed") - monkeypatch.setattr(svc, "_remove_formula", _boom) + monkeypatch.setattr(svc, "remove_rack", _boom) await repo.uninstall_packages(["yazi"], kind=PackageKind.FORMULA) assert _provider_calls(mock_brew, "uninstall") @@ -723,8 +723,8 @@ async def test_native_upgrade_bumps_version_and_retains_old( import orjson - import brewery.providers.install_service as install_svc import brewery.providers.orchestrator as orch_mod + import brewery.providers.pipeline as install_svc from brewery.core import config from brewery.core.repo import Repository from brewery.core.shell import BrewResult diff --git a/tests/unit/_stubs.py b/tests/unit/_stubs.py index 5396c30..6022cc7 100644 --- a/tests/unit/_stubs.py +++ b/tests/unit/_stubs.py @@ -2,7 +2,7 @@ from __future__ import annotations -from typing import Self +from typing import Any, Self import httpx @@ -36,8 +36,8 @@ async def __aexit__(self, *exc) -> bool: def patch_httpx(monkeypatch) -> MockClient: """Patch httpx.AsyncClient with a stub that records its constructor kwargs. - Both service modules do a plain `import httpx`, so patching the attribute on - the shared module object covers whichever service is under test. + The pipeline module does a plain `import httpx`, so patching the attribute on + the shared module object covers whichever entry point is under test. Args: monkeypatch: The pytest monkeypatch fixture. @@ -65,14 +65,18 @@ def _client(**kwargs) -> MockClient: return client -class MockRepo: - """Minimal repo stub exposing catalog, cache_mgr, and formula attributes.""" +class MockPorts: + """The three ports the pipeline needs, as opaque sentinels. + + The pipeline only forwards them, so identity is all a test needs to assert; + they are `Any` because no method on them is ever called. + """ def __init__(self) -> None: - """Initialise with mock catalog, cache_mgr, and formula objects.""" - self.catalog = object() - self.cache_mgr = object() - self.formula = object() + """Initialise with distinct catalog, cache_mgr, and formula sentinels.""" + self.catalog: Any = object() + self.cache_mgr: Any = object() + self.formula: Any = object() async def _run_brew(args) -> None: diff --git a/tests/unit/test_cellar.py b/tests/unit/test_cellar.py index 84c64d0..cc8760d 100644 --- a/tests/unit/test_cellar.py +++ b/tests/unit/test_cellar.py @@ -10,7 +10,12 @@ import pytest import brewery.providers.cellar as _cellar -from brewery.providers.cellar import CellarError, clone_tree, install_to_cellar +from brewery.providers.cellar import ( + CellarError, + clone_tree, + install_to_cellar, + remove_rack, +) pytestmark = pytest.mark.unit @@ -220,3 +225,48 @@ def snapshot(root: Path) -> dict: return out assert snapshot(via_clone) == snapshot(via_copy) + + +class TestRemoveRack: + """Tests for remove_rack, the native removal of one formula's kegs.""" + + def test_refuses_a_locked_rack(self, tmp_path) -> None: + """The rack lock is really taken: a peer's hold keeps the kegs in place.""" + import fcntl + + from brewery.core.errors import OperationInProgressError + from brewery.core.locks import lock_path + + cellar = tmp_path / "Cellar" / "tool" + (cellar / "1.0" / "bin").mkdir(parents=True) + prefix = tmp_path / "prefix" + + path = lock_path(prefix, "tool") + path.parent.mkdir(parents=True, exist_ok=True) + fd = os.open(path, os.O_RDWR | os.O_CREAT, 0o644) + fcntl.flock(fd, fcntl.LOCK_EX) + try: + with pytest.raises(OperationInProgressError): + remove_rack(cellar, prefix, "tool") + + finally: + os.close(fd) + + assert cellar.exists() + + def test_missing_dir_is_noop(self, tmp_path) -> None: + """A missing cellar dir is a clean no-op (already-removed success path).""" + remove_rack(tmp_path / "Cellar" / "ghost", tmp_path / "prefix", "ghost") + + def test_unlinks_all_versions_then_removes(self, tmp_path, monkeypatch) -> None: + """Every version keg is unlinked before the formula's cellar dir is removed.""" + cellar = tmp_path / "Cellar" / "tool" + (cellar / "1.0" / "bin").mkdir(parents=True) + (cellar / "2.0" / "bin").mkdir(parents=True) + seen: list[str] = [] + monkeypatch.setattr( + _cellar, "unlink_keg", lambda keg, *, prefix, name: seen.append(keg.name) + ) + remove_rack(cellar, tmp_path / "prefix", "tool") + assert sorted(seen) == ["1.0", "2.0"] + assert not cellar.exists() diff --git a/tests/unit/test_install_adapters.py b/tests/unit/test_install_adapters.py index b5fd360..af57258 100644 --- a/tests/unit/test_install_adapters.py +++ b/tests/unit/test_install_adapters.py @@ -4,6 +4,7 @@ import json from pathlib import Path +from typing import Any import pytest @@ -13,7 +14,7 @@ PinnedPackageWarning, ) from brewery.core.models import Package, PackageKind -from brewery.providers.install_adapters import BrewAdapter, RepositoryCatalogAdapter +from brewery.providers.install_adapters import BrewAdapter, CatalogAdapter pytestmark = pytest.mark.asyncio @@ -25,9 +26,11 @@ def __init__(self) -> None: """Initialise the mock catalog with an empty call log.""" self.calls = [] - def get_formula(self, name: str) -> str: + def get_formula(self, name: str) -> Any: """Record the call and return a row sentinel string. + The adapter forwards the row untouched, so a marker stands in for one. + Args: name: The formula name to look up. @@ -90,7 +93,9 @@ def __init__(self, installed: dict[str, str | None] | None = None) -> None: self._installed: dict[str, str | None] = installed or {} self.calls: list = [] - def find_installed(self, name: str, kind: PackageKind) -> Package | None: + def find_installed( + self, name: str, kind: PackageKind | None = None + ) -> Package | None: """Return a Package if *name* is in the installed set, else None. Args: @@ -104,33 +109,36 @@ def find_installed(self, name: str, kind: PackageKind) -> Package | None: if name not in self._installed: return None - return Package(name, kind, path=self._installed[name]) + return Package(name, kind or PackageKind.FORMULA, path=self._installed[name]) -class MockRepo: - """Minimal repo stub wiring together a MockCatalog and MockCacheMgr.""" +def _adapter( + installed: dict[str, str | None] | None = None, +) -> tuple[CatalogAdapter, MockCatalog, MockCacheMgr]: + """Build a CatalogAdapter over fresh catalog and cache-manager stubs. - def __init__(self, installed: dict[str, str | None] | None = None) -> None: - """Initialise the mock repo. + Args: + installed: Mapping of package name -> keg path passed to MockCacheMgr. - Args: - installed: Mapping of package name -> keg path passed to MockCacheMgr. - """ - self.catalog = MockCatalog() - self.cache_mgr = MockCacheMgr(installed) - self.formula = None + Returns: + The adapter under test and the two stubs behind it, so a test can read + their call logs without reaching through the adapter's private fields. + """ + catalog = MockCatalog() + cache_mgr = MockCacheMgr(installed) + return CatalogAdapter(catalog, cache_mgr), catalog, cache_mgr -async def test_catalog_methods_delegate_to_repo_catalog() -> None: - """Test that each CatalogPort method delegates to repo.catalog.""" - repo = MockRepo(installed={}) - adapter = RepositoryCatalogAdapter(repo) + +async def test_catalog_methods_delegate_to_the_catalog() -> None: + """Test that each CatalogPort method delegates to the injected catalog.""" + adapter, catalog, _ = _adapter(installed={}) assert adapter.get_formula("wget") == "row:wget" assert adapter.resolve_alias("py") == "canon:py" assert adapter.runtime_deps("wget") == ["wget-dep"] assert adapter.aliases_of("openssl@3") == ["openssl@3-alias"] - assert ("get_formula", "wget") in repo.catalog.calls - assert ("aliases_of", "openssl@3") in repo.catalog.calls + assert ("get_formula", "wget") in catalog.calls + assert ("aliases_of", "openssl@3") in catalog.calls async def test_is_satisfied_true_when_installed_with_receipt( @@ -141,16 +149,14 @@ async def test_is_satisfied_true_when_installed_with_receipt( keg.mkdir(parents=True) (keg / "INSTALL_RECEIPT.json").write_text(json.dumps({})) - repo = MockRepo(installed={"wget": str(keg)}) - adapter = RepositoryCatalogAdapter(repo) + adapter, _, cache_mgr = _adapter(installed={"wget": str(keg)}) assert adapter.is_satisfied("wget") is True - assert repo.cache_mgr.calls == [("wget", PackageKind.FORMULA)] + assert cache_mgr.calls == [("wget", PackageKind.FORMULA)] async def test_is_satisfied_false_when_absent() -> None: """Test that is_satisfied returns False when the package is absent from the cache.""" - repo = MockRepo(installed={}) - adapter = RepositoryCatalogAdapter(repo) + adapter, _, _ = _adapter(installed={}) assert adapter.is_satisfied("wget") is False @@ -163,15 +169,13 @@ async def test_is_satisfied_false_when_keg_has_no_receipt(tmp_path: Path) -> Non keg = tmp_path / "wget" / "1.21.4" keg.mkdir(parents=True) - repo = MockRepo(installed={"wget": str(keg)}) - adapter = RepositoryCatalogAdapter(repo) + adapter, _, _ = _adapter(installed={"wget": str(keg)}) assert adapter.is_satisfied("wget") is False async def test_is_satisfied_false_when_pkg_has_no_path() -> None: """Test that is_satisfied returns False when find_installed returns a Package with no path.""" - repo = MockRepo(installed={"wget": None}) - adapter = RepositoryCatalogAdapter(repo) + adapter, _, _ = _adapter(installed={"wget": None}) assert adapter.is_satisfied("wget") is False @@ -202,6 +206,21 @@ async def install(self, names) -> list[str]: return names + async def upgrade(self, names) -> list[str]: + """Record the call and optionally raise the configured exception. + + Args: + names: The list of package names to upgrade. + + Returns: + The names list on success. + """ + self.calls.append(names) + if self.exc is not None: + raise self.exc + + return names + class MockRunBrew: """Minimal brew runner stub that fails on a configured set of subcommands.""" diff --git a/tests/unit/test_install_service.py b/tests/unit/test_install_service.py deleted file mode 100644 index f6fd1d2..0000000 --- a/tests/unit/test_install_service.py +++ /dev/null @@ -1,163 +0,0 @@ -"""Unit tests for the install assembly function.""" - -from __future__ import annotations - -import functools - -import pytest -from _stubs import MockClient, MockRepo, _run_brew, patch_httpx - -import brewery.providers.install_service as svc -from brewery.providers.install_adapters import BrewAdapter, RepositoryCatalogAdapter -from brewery.providers.orchestrator import InstallConfig, InstallReport, Outcome - -pytestmark = pytest.mark.asyncio - - -class MockDownloader: - """Downloader stub that records the most-recently constructed instance.""" - - last = None - - def __init__(self, cache_dir, client) -> None: - """Initialise the mock downloader and record this instance. - - Args: - cache_dir: The cache directory passed by the service. - client: The HTTP client passed by the service. - """ - MockDownloader.last = self - self.cache_dir = cache_dir - self.client = client - - -class MockOrchestrator: - """Orchestrator stub that records construction kwargs and install calls.""" - - last: MockOrchestrator | None = None - - def __init__(self, **kwargs) -> None: - """Initialise the mock orchestrator and record this instance. - - Args: - **kwargs: The keyword arguments passed by the service layer. - """ - MockOrchestrator.last = self - self.kwargs = kwargs - self.installed_with: list[str] | None = None - self.report: InstallReport | None = None - - async def install(self, names) -> InstallReport: - """Record the names and return a sentinel report. - - Args: - names: The list of formula names to install. - - Returns: - A report marking every name natively installed. - """ - self.installed_with = names - self.report = InstallReport(outcomes={n: Outcome.NATIVE for n in names}) - - return self.report - - -@pytest.fixture -def patched(monkeypatch) -> MockClient: - """Patch httpx.AsyncClient, Downloader, and Orchestrator with stubs. - - Args: - monkeypatch: The pytest monkeypatch fixture. - - Returns: - The MockClient instance that the patched AsyncClient constructor returns. - """ - client = patch_httpx(monkeypatch) - - monkeypatch.setattr(svc, "Downloader", MockDownloader) - monkeypatch.setattr(svc, "Orchestrator", MockOrchestrator) - - return client - - -async def test_returns_orchestrator_report(patched, mock_env) -> None: - """Test that run_install returns the report produced by the orchestrator.""" - repo = MockRepo() - report = await svc.run_install( - repo, ["wget", "curl"], run_brew=_run_brew, env=mock_env - ) - assert MockOrchestrator.last is not None - assert MockOrchestrator.last.installed_with == ["wget", "curl"] - assert report is MockOrchestrator.last.report # Passed through untouched - - -async def test_client_is_closed(patched, mock_env) -> None: - """Test that the HTTP client is closed after run_install completes.""" - repo = MockRepo() - await svc.run_install(repo, ["wget"], run_brew=_run_brew, env=mock_env) - assert patched.closed is True - - -async def test_client_gets_the_streaming_timeout(patched, mock_env) -> None: - """Test that the client is built with the pipeline timeout, not httpx's 5s default.""" - await svc.run_install(MockRepo(), ["wget"], run_brew=_run_brew, env=mock_env) - assert patched.kwargs["timeout"] is svc.PIPELINE_TIMEOUT - - # Pinned: a bottle body is streamed, so the read budget is a stall budget - assert svc.PIPELINE_TIMEOUT.read == 30.0 - assert svc.PIPELINE_TIMEOUT.connect == 10.0 - - -async def test_downloader_built_with_env_cache_and_client(patched, mock_env) -> None: - """Test that the Downloader is constructed with the env bottle_cache and the live client.""" - repo = MockRepo() - await svc.run_install(repo, ["wget"], run_brew=_run_brew, env=mock_env) - assert MockDownloader.last is not None - dl = MockDownloader.last - assert dl.cache_dir == mock_env.bottle_cache - assert dl.client is patched - - -async def test_orchestrator_wired_with_adapters_and_config(patched, mock_env) -> None: - """Test that the Orchestrator receives correctly wired adapters and InstallConfig.""" - repo = MockRepo() - await svc.run_install(repo, ["wget"], run_brew=_run_brew, env=mock_env) - assert MockOrchestrator.last is not None - kw = MockOrchestrator.last.kwargs - - # Catalog port is the repo-backed adapter - assert isinstance(kw["catalog"], RepositoryCatalogAdapter) - assert kw["catalog"]._repo is repo - - # Brew port wraps the formula backend + the injected runner - assert isinstance(kw["brew"], BrewAdapter) - assert kw["brew"]._backend is repo.formula - assert kw["brew"]._run_brew is _run_brew - - # Tab fetcher is fetch_bottle_tab bound to the live client - tf = kw["tab_fetcher"] - assert isinstance(tf, functools.partial) - assert tf.func is svc.fetch_bottle_tab - assert tf.args == (patched,) - - # Downloader forwarded; concurrency left to the Orchestrator's default - assert kw["downloader"] is MockDownloader.last - assert "install_concurrency" not in kw - - # Config derived from env - cfg = kw["config"] - assert isinstance(cfg, InstallConfig) - assert cfg.prefix == mock_env.prefix - assert cfg.repository == mock_env.repository - assert cfg.api_path == str(mock_env.api_path) - assert cfg.staging_root == mock_env.prefix / "var" / "homebrew" / ".staging" - - -async def test_env_resolved_when_omitted(patched, mock_env, monkeypatch) -> None: - """Test that omitting env= falls back to get_brewery_env() automatically.""" - monkeypatch.setattr(svc, "get_brewery_env", lambda: mock_env) - repo = MockRepo() - await svc.run_install(repo, ["wget"], run_brew=_run_brew) # no env= - assert MockOrchestrator.last is not None - cfg = MockOrchestrator.last.kwargs["config"] - assert cfg.prefix == mock_env.prefix diff --git a/tests/unit/test_pin_service.py b/tests/unit/test_pinning.py similarity index 97% rename from tests/unit/test_pin_service.py rename to tests/unit/test_pinning.py index 03ab56b..0e644c6 100644 --- a/tests/unit/test_pin_service.py +++ b/tests/unit/test_pinning.py @@ -7,7 +7,7 @@ import pytest -from brewery.providers.pin_service import is_pinned, pin, pin_path, unpin +from brewery.providers.pinning import is_pinned, pin, pin_path, unpin pytestmark = pytest.mark.unit diff --git a/tests/unit/test_pipeline.py b/tests/unit/test_pipeline.py new file mode 100644 index 0000000..7e125d1 --- /dev/null +++ b/tests/unit/test_pipeline.py @@ -0,0 +1,280 @@ +"""Unit tests for the install/upgrade pipeline assembly functions.""" + +from __future__ import annotations + +import functools +from pathlib import Path + +import pytest +from _stubs import MockClient, MockPorts, _run_brew, patch_httpx + +import brewery.providers.pipeline as svc +from brewery.providers.install_adapters import BrewAdapter, CatalogAdapter +from brewery.providers.orchestrator import InstallConfig, InstallReport, Outcome + +pytestmark = pytest.mark.asyncio + + +class MockDownloader: + """Downloader stub that records the most-recently constructed instance.""" + + last = None + + def __init__(self, cache_dir, client) -> None: + """Initialise the mock downloader and record this instance. + + Args: + cache_dir: The cache directory passed by the pipeline. + client: The HTTP client passed by the pipeline. + """ + MockDownloader.last = self + self.cache_dir = cache_dir + self.client = client + + +class MockOrchestrator: + """Orchestrator stub that records construction kwargs and install/upgrade calls.""" + + last: MockOrchestrator | None = None + + def __init__(self, **kwargs) -> None: + """Initialise the mock orchestrator and record this instance. + + Args: + **kwargs: The keyword arguments passed by the pipeline. + """ + MockOrchestrator.last = self + self.kwargs = kwargs + self.installed_with: list[str] | None = None + self.upgraded_with: tuple | None = None + self.report: InstallReport | None = None + + async def install(self, names) -> InstallReport: + """Record the names and return a sentinel report. + + Args: + names: The list of formula names to install. + + Returns: + A report marking every name natively installed. + """ + self.installed_with = names + self.report = InstallReport(outcomes={n: Outcome.NATIVE for n in names}) + + return self.report + + async def upgrade(self, names, old_kegs) -> InstallReport: + """Record the upgrade call and return a sentinel report. + + Args: + names: The names of the formulae to upgrade. + old_kegs: The old kegs to upgrade from. + + Returns: + A report marking every name natively upgraded. + """ + self.upgraded_with = (names, old_kegs) + self.report = InstallReport(outcomes={n: Outcome.NATIVE for n in names}) + + return self.report + + +@pytest.fixture +def patched(monkeypatch) -> MockClient: + """Patch httpx.AsyncClient, Downloader, and Orchestrator with stubs. + + Args: + monkeypatch: The pytest monkeypatch fixture. + + Returns: + The MockClient instance that the patched AsyncClient constructor returns. + """ + client = patch_httpx(monkeypatch) + + monkeypatch.setattr(svc, "Downloader", MockDownloader) + monkeypatch.setattr(svc, "Orchestrator", MockOrchestrator) + + return client + + +@pytest.fixture +def ports() -> MockPorts: + """The catalog/cache_mgr/formula sentinels every pipeline call needs. + + Returns: + A fresh MockPorts instance. + """ + return MockPorts() + + +def _install(names, ports, **kwargs): + """Call run_install with the sentinel ports spread out. + + Args: + names: Formula names to install. + ports: The MockPorts sentinels. + **kwargs: Extra keyword arguments forwarded to run_install. + + Returns: + The run_install coroutine. + """ + return svc.run_install( + names, + catalog=ports.catalog, + cache_mgr=ports.cache_mgr, + formula=ports.formula, + run_brew=_run_brew, + **kwargs, + ) + + +def _upgrade(names, old_kegs, ports, **kwargs): + """Call run_upgrade with the sentinel ports spread out. + + Args: + names: Formula names to upgrade. + old_kegs: The current active kegs, keyed by name. + ports: The MockPorts sentinels. + **kwargs: Extra keyword arguments forwarded to run_upgrade. + + Returns: + The run_upgrade coroutine. + """ + return svc.run_upgrade( + names, + old_kegs, + catalog=ports.catalog, + cache_mgr=ports.cache_mgr, + formula=ports.formula, + run_brew=_run_brew, + **kwargs, + ) + + +class TestRunInstall: + """Tests for run_install.""" + + async def test_returns_orchestrator_report(self, patched, ports, mock_env) -> None: + """Test that run_install returns the report produced by the orchestrator.""" + report = await _install(["wget", "curl"], ports, env=mock_env) + assert MockOrchestrator.last is not None + assert MockOrchestrator.last.installed_with == ["wget", "curl"] + assert report is MockOrchestrator.last.report # Passed through untouched + + async def test_client_is_closed(self, patched, ports, mock_env) -> None: + """Test that the HTTP client is closed after run_install completes.""" + await _install(["wget"], ports, env=mock_env) + assert patched.closed is True + + async def test_client_gets_the_streaming_timeout( + self, patched, ports, mock_env + ) -> None: + """Test the client is built with the pipeline timeout, not httpx's 5s default.""" + await _install(["wget"], ports, env=mock_env) + assert patched.kwargs["timeout"] is svc.PIPELINE_TIMEOUT + + # Pinned: a bottle body is streamed, so the read budget is a stall budget + assert svc.PIPELINE_TIMEOUT.read == 30.0 + assert svc.PIPELINE_TIMEOUT.connect == 10.0 + + async def test_downloader_built_with_env_cache_and_client( + self, patched, ports, mock_env + ) -> None: + """Test the Downloader is built with the env bottle_cache and the live client.""" + await _install(["wget"], ports, env=mock_env) + assert MockDownloader.last is not None + dl = MockDownloader.last + assert dl.cache_dir == mock_env.bottle_cache + assert dl.client is patched + + async def test_orchestrator_wired_with_adapters_and_config( + self, patched, ports, mock_env + ) -> None: + """Test the Orchestrator receives wired adapters and an env-derived config.""" + await _install(["wget"], ports, env=mock_env) + assert MockOrchestrator.last is not None + kw = MockOrchestrator.last.kwargs + + # Catalog port is the adapter over the catalog + installed-state cache + assert isinstance(kw["catalog"], CatalogAdapter) + assert kw["catalog"]._catalog is ports.catalog + assert kw["catalog"]._cache_mgr is ports.cache_mgr + + # Brew port wraps the formula backend + the injected runner + assert isinstance(kw["brew"], BrewAdapter) + assert kw["brew"]._backend is ports.formula + assert kw["brew"]._run_brew is _run_brew + + # Tab fetcher is fetch_bottle_tab bound to the live client + tf = kw["tab_fetcher"] + assert isinstance(tf, functools.partial) + assert tf.func is svc.fetch_bottle_tab + assert tf.args == (patched,) + + # Downloader forwarded; concurrency left to the Orchestrator's default + assert kw["downloader"] is MockDownloader.last + assert "install_concurrency" not in kw + + # Config derived from env + cfg = kw["config"] + assert isinstance(cfg, InstallConfig) + assert cfg.prefix == mock_env.prefix + assert cfg.repository == mock_env.repository + assert cfg.api_path == str(mock_env.api_path) + assert cfg.staging_root == mock_env.prefix / "var" / "homebrew" / ".staging" + + async def test_env_resolved_when_omitted( + self, patched, ports, mock_env, monkeypatch + ) -> None: + """Test that omitting env= falls back to get_brewery_env() automatically.""" + monkeypatch.setattr(svc, "get_brewery_env", lambda: mock_env) + await _install(["wget"], ports) # no env= + assert MockOrchestrator.last is not None + cfg = MockOrchestrator.last.kwargs["config"] + assert cfg.prefix == mock_env.prefix + + +class TestRunUpgrade: + """Tests for run_upgrade.""" + + async def test_returns_orchestrator_report(self, patched, ports, mock_env) -> None: + """Test run_upgrade returns the orchestrator's report and forwards old_kegs.""" + old = {"wget": Path("/p/Cellar/wget/1.0"), "curl": Path("/p/Cellar/curl/8.0")} + report = await _upgrade(["wget", "curl"], old, ports, env=mock_env) + assert MockOrchestrator.last is not None + assert MockOrchestrator.last.upgraded_with == (["wget", "curl"], old) + assert report is MockOrchestrator.last.report # Passed through untouched + + async def test_client_is_closed(self, patched, ports, mock_env) -> None: + """Test that the HTTP client is closed after run_upgrade completes.""" + await _upgrade(["wget"], {}, ports, env=mock_env) + assert patched.closed is True + + async def test_client_gets_the_streaming_timeout( + self, patched, ports, mock_env + ) -> None: + """Test that upgrade builds its client with the same timeout install uses.""" + await _upgrade(["wget"], {}, ports, env=mock_env) + assert patched.kwargs["timeout"] is svc.PIPELINE_TIMEOUT + + async def test_orchestrator_receives_the_same_ports_as_install( + self, patched, ports, mock_env + ) -> None: + """Test that upgrade wires the identical port set install does.""" + await _upgrade(["wget"], {}, ports, env=mock_env) + assert MockOrchestrator.last is not None + kw = MockOrchestrator.last.kwargs + assert kw["catalog"]._catalog is ports.catalog + assert kw["catalog"]._cache_mgr is ports.cache_mgr + assert kw["brew"]._backend is ports.formula + assert kw["downloader"].client is patched + + async def test_env_resolved_when_omitted( + self, patched, ports, mock_env, monkeypatch + ) -> None: + """Test that omitting env= falls back to get_brewery_env().""" + monkeypatch.setattr(svc, "get_brewery_env", lambda: mock_env) + await _upgrade(["wget"], {}, ports) # No env= + assert MockOrchestrator.last is not None + cfg = MockOrchestrator.last.kwargs["config"] + assert cfg.prefix == mock_env.prefix diff --git a/tests/unit/test_uninstall_service.py b/tests/unit/test_uninstall_service.py index 890972a..70cbceb 100644 --- a/tests/unit/test_uninstall_service.py +++ b/tests/unit/test_uninstall_service.py @@ -1,15 +1,9 @@ -"""Unit tests for the uninstall assembly function.""" +"""Unit tests for the native uninstall runner.""" from __future__ import annotations -import fcntl -import os - -import pytest - import brewery.providers.uninstall_service as svc from brewery.core.errors import BrewCommandError, OperationInProgressError -from brewery.core.locks import lock_path def _raise_os(c, p, name) -> None: @@ -38,12 +32,15 @@ def __init__(self, fail: bool = False) -> None: self.calls: list[list[str]] = [] self.fail = fail - async def uninstall(self, names: list[str]) -> None: + async def uninstall(self, names: list[str]) -> list[str]: """Record the uninstall call and optionally raise a failure. Args: names: The list of formula names to uninstall + Returns: + The names unchanged. + Raises: BrewCommandError: Always raised to force the brew fallback """ @@ -51,31 +48,21 @@ async def uninstall(self, names: list[str]) -> None: if self.fail: raise BrewCommandError("brew uninstall failed") - -class MockRepo: - """Repo stub exposing only the formula backend run_uninstall touches.""" - - def __init__(self, fail: bool = False) -> None: - """Initialise the repo with an optional failure mode. - - Args: - fail: Whether to fail on uninstall calls (default: False) - """ - self.formula = RecordingBackend(fail=fail) + return names async def test_native_success_takes_no_fallback(mock_env, monkeypatch) -> None: - """Every formula removed natively means the provider is never called.""" + """Test that every formula removed natively means the provider is never called.""" seen: list[str] = [] - monkeypatch.setattr(svc, "_remove_formula", lambda c, p, name: seen.append(name)) - repo = MockRepo() - await svc.run_uninstall(repo, ["yazi", "act"], env=mock_env) + monkeypatch.setattr(svc, "remove_rack", lambda c, p, name: seen.append(name)) + formula = RecordingBackend() + await svc.run_uninstall(["yazi", "act"], formula=formula, env=mock_env) assert seen == ["yazi", "act"] - assert repo.formula.calls == [] + assert formula.calls == [] async def test_native_failure_falls_back_per_formula(mock_env, monkeypatch) -> None: - """A native OSError for one formula falls back to brew for that one only.""" + """Test that a native OSError for one formula falls back to brew for that one only.""" def remove(c, p, name) -> None: """Raise OSError for 'act' to test native fallback, otherwise no-op. @@ -91,31 +78,31 @@ def remove(c, p, name) -> None: if name == "act": raise OSError("native failed") - monkeypatch.setattr(svc, "_remove_formula", remove) - repo = MockRepo() - await svc.run_uninstall(repo, ["yazi", "act"], env=mock_env) - assert repo.formula.calls == [["act"]] # yazi handled natively, only act fell back + monkeypatch.setattr(svc, "remove_rack", remove) + formula = RecordingBackend() + await svc.run_uninstall(["yazi", "act"], formula=formula, env=mock_env) + assert formula.calls == [["act"]] # yazi handled natively, only act fell back async def test_brew_fallback_failure_is_swallowed(mock_env, monkeypatch) -> None: - """A failing brew fallback does not propagate (verify reports the survivor).""" - monkeypatch.setattr(svc, "_remove_formula", _raise_os) - repo = MockRepo(fail=True) - await svc.run_uninstall(repo, ["yazi"], env=mock_env) # Should not raise - assert repo.formula.calls == [["yazi"]] + """Test that a failing brew fallback does not propagate (verify reports the survivor).""" + monkeypatch.setattr(svc, "remove_rack", _raise_os) + formula = RecordingBackend(fail=True) + await svc.run_uninstall(["yazi"], formula=formula, env=mock_env) # Should not raise + assert formula.calls == [["yazi"]] async def test_env_resolved_when_omitted(mock_env, monkeypatch) -> None: - """Omitting env= falls back to get_brewery_env() for the cellar/prefix paths.""" + """Test that omitting env= falls back to get_brewery_env() for the cellar/prefix paths.""" monkeypatch.setattr(svc, "get_brewery_env", lambda: mock_env) seen: list[tuple] = [] - monkeypatch.setattr(svc, "_remove_formula", lambda c, p, name: seen.append((c, p))) - await svc.run_uninstall(MockRepo(), ["yazi"]) # No env= + monkeypatch.setattr(svc, "remove_rack", lambda c, p, name: seen.append((c, p))) + await svc.run_uninstall(["yazi"], formula=RecordingBackend()) # No env= assert seen == [(mock_env.cellar / "yazi", mock_env.prefix)] async def test_locked_rack_skips_the_brew_fallback(mock_env, monkeypatch) -> None: - """brew locks the same rack, so falling back to it would fail identically.""" + """Test that brew locks the same rack, so falling back to it would fail identically.""" def remove(c, p, name) -> None: """Raise as though a peer process held the rack lock. @@ -130,49 +117,8 @@ def remove(c, p, name) -> None: """ raise OperationInProgressError(str(c)) - monkeypatch.setattr(svc, "_remove_formula", remove) - repo = MockRepo() - await svc.run_uninstall(repo, ["yazi"], env=mock_env) # Should not raise + monkeypatch.setattr(svc, "remove_rack", remove) + formula = RecordingBackend() + await svc.run_uninstall(["yazi"], formula=formula, env=mock_env) # Should not raise - assert repo.formula.calls == [] - - -def test_remove_formula_refuses_a_locked_rack(tmp_path) -> None: - """The rack lock is really taken: a peer's hold keeps the kegs in place.""" - cellar = tmp_path / "Cellar" / "tool" - (cellar / "1.0" / "bin").mkdir(parents=True) - prefix = tmp_path / "prefix" - - path = lock_path(prefix, "tool") - path.parent.mkdir(parents=True, exist_ok=True) - fd = os.open(path, os.O_RDWR | os.O_CREAT, 0o644) - fcntl.flock(fd, fcntl.LOCK_EX) - try: - with pytest.raises(OperationInProgressError): - svc._remove_formula(cellar, prefix, "tool") - - finally: - os.close(fd) - - assert cellar.exists() - - -def test_remove_formula_missing_dir_is_noop(tmp_path) -> None: - """A missing cellar dir is a clean no-op (already-removed success path).""" - svc._remove_formula(tmp_path / "Cellar" / "ghost", tmp_path / "prefix", "ghost") - - -def test_remove_formula_unlinks_all_versions_then_removes( - tmp_path, monkeypatch -) -> None: - """Every version keg is unlinked before the formula's cellar dir is removed.""" - cellar = tmp_path / "Cellar" / "tool" - (cellar / "1.0" / "bin").mkdir(parents=True) - (cellar / "2.0" / "bin").mkdir(parents=True) - seen: list[str] = [] - monkeypatch.setattr( - svc, "unlink_keg", lambda keg, *, prefix, name: seen.append(keg.name) - ) - svc._remove_formula(cellar, tmp_path / "prefix", "tool") - assert sorted(seen) == ["1.0", "2.0"] - assert not cellar.exists() + assert formula.calls == [] diff --git a/tests/unit/test_upgrade_service.py b/tests/unit/test_upgrade_service.py deleted file mode 100644 index 5170740..0000000 --- a/tests/unit/test_upgrade_service.py +++ /dev/null @@ -1,123 +0,0 @@ -"""Unit tests for the upgrade assembly function.""" - -from __future__ import annotations - -from pathlib import Path - -import pytest -from _stubs import MockClient, MockRepo, _run_brew, patch_httpx - -import brewery.providers.upgrade_service as svc -from brewery.providers.orchestrator import InstallReport, Outcome - - -class MockOrchestrator: - """Records the upgrade call and returns a sentinel report.""" - - last: MockOrchestrator | None = None - - def __init__(self) -> None: - """Initialise with no upgraded state.""" - MockOrchestrator.last = self - self.upgraded_with: tuple | None = None - self.report: InstallReport | None = None - - async def upgrade(self, names, old_kegs) -> InstallReport: - """Record the upgrade call and return a sentinel report. - - Args: - names: The names of the packages to upgrade. - old_kegs: The old kegs to upgrade from. - - Returns: - A report marking every name natively upgraded. - """ - self.upgraded_with = (names, old_kegs) - self.report = InstallReport(outcomes={n: Outcome.NATIVE for n in names}) - - return self.report - - -@pytest.fixture -def patched(monkeypatch) -> tuple[MockClient, dict]: - """Patch httpx and the shared build_orchestrator with recorders. - - Returns: - A tuple of the mock client and the built dictionary. - """ - client = patch_httpx(monkeypatch) - - built: dict = {} - - def _build(repo, *, client, env, run_brew, progress=None) -> MockOrchestrator: - """Record the build call and return a mock orchestrator. - - Args: - repo: The repository to build. - client: The HTTP client to use. - env: The environment variables to use. - run_brew: The brew run command to use. - progress: The optional progress sink. - - Returns: - A mock orchestrator. - """ - built.update( - repo=repo, - client=client, - env=env, - run_brew=run_brew, - progress=progress, - ) - - return MockOrchestrator() - - monkeypatch.setattr(svc, "build_orchestrator", _build) - - return client, built - - -async def test_returns_orchestrator_report(patched, mock_env) -> None: - """Test that run_upgrade returns the orchestrator's report and forwards names + old_kegs.""" - old = {"wget": Path("/p/Cellar/wget/1.0"), "curl": Path("/p/Cellar/curl/8.0")} - report = await svc.run_upgrade( - MockRepo(), ["wget", "curl"], old, run_brew=_run_brew, env=mock_env - ) - assert MockOrchestrator.last is not None - assert MockOrchestrator.last.upgraded_with == (["wget", "curl"], old) - assert report is MockOrchestrator.last.report # Passed through untouched - - -async def test_client_is_closed(patched, mock_env) -> None: - """Test that the HTTP client is closed after run_upgrade completes.""" - client, _ = patched - await svc.run_upgrade(MockRepo(), ["wget"], {}, run_brew=_run_brew, env=mock_env) - assert client.closed is True - - -async def test_client_gets_the_streaming_timeout(patched, mock_env) -> None: - """Test that upgrade builds its client with the same timeout install uses.""" - client, _ = patched - await svc.run_upgrade(MockRepo(), ["wget"], {}, run_brew=_run_brew, env=mock_env) - assert client.kwargs["timeout"] is svc.PIPELINE_TIMEOUT - - -async def test_build_orchestrator_receives_client_env_and_runner( - patched, mock_env -) -> None: - """Test that the shared assembler gets the open client, env, and runner.""" - client, built = patched - repo = MockRepo() - await svc.run_upgrade(repo, ["wget"], {}, run_brew=_run_brew, env=mock_env) - assert built["repo"] is repo - assert built["client"] is client - assert built["env"] is mock_env - assert built["run_brew"] is _run_brew - - -async def test_env_resolved_when_omitted(patched, mock_env, monkeypatch) -> None: - """Test that omitting env= falls back to get_brewery_env().""" - monkeypatch.setattr(svc, "get_brewery_env", lambda: mock_env) - _, built = patched - await svc.run_upgrade(MockRepo(), ["wget"], {}, run_brew=_run_brew) # No env= - assert built["env"] is mock_env From 548a589f9bca774e2300f20fe732f1097d10b91f Mon Sep 17 00:00:00 2001 From: Rob Webb Date: Wed, 19 Aug 2026 11:44:35 +0100 Subject: [PATCH 2/8] refactor(services): move pin/link command policy out of Repository into a services layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - New 'services/' package establishes a one-way layering — 'cli' -> 'services' -> {'core', 'providers'} - pin_packages()/unpin_packages()/'link_packages()/unlink_packages()/_resolve_installed_formulae() move out of repo.py into services/{pin/link/resolve}.py - Each take 'repo' as an argument and 'env=' becomes an overridable keyword rather than always being resolved internally - link_service.py moves to services/link.py with the new 'link_packages'/'unlink_packages' policy wrappers - The '(name, reason)' pair type 'Notes' is promoted out of 'link_service' to models.py and shared by every service - cli/commands/{link/pin}.py call the services directly instead of the repo methods - Pin/link test cases move from test_repo.py into a new test_pin_link_services.py - test_link_service.py is renamed to test_link.py --- src/brewery/cli/commands/link.py | 11 +- src/brewery/cli/commands/pin.py | 5 +- src/brewery/core/models.py | 4 + src/brewery/core/repo.py | 157 ------------------ src/brewery/services/__init__.py | 8 + .../link_service.py => services/link.py} | 74 ++++++++- src/brewery/services/pin.py | 79 +++++++++ src/brewery/services/resolve.py | 34 ++++ tests/integration/test_pin_link_services.py | 93 +++++++++++ tests/integration/test_repo.py | 91 +--------- .../{test_link_service.py => test_link.py} | 28 ++-- 11 files changed, 318 insertions(+), 266 deletions(-) create mode 100644 src/brewery/services/__init__.py rename src/brewery/{providers/link_service.py => services/link.py} (69%) create mode 100644 src/brewery/services/pin.py create mode 100644 src/brewery/services/resolve.py create mode 100644 tests/integration/test_pin_link_services.py rename tests/unit/{test_link_service.py => test_link.py} (86%) diff --git a/src/brewery/cli/commands/link.py b/src/brewery/cli/commands/link.py index dbb90b7..ece355b 100644 --- a/src/brewery/cli/commands/link.py +++ b/src/brewery/cli/commands/link.py @@ -12,6 +12,7 @@ print_failures, print_result, ) +from brewery.services import link as link_service if TYPE_CHECKING: from brewery.providers.linker import LinkResult @@ -21,7 +22,7 @@ def _preview_link(linked: list[tuple[str, LinkResult]]) -> None: """Print the paths a real `link` would create and, with --overwrite, delete. Args: - linked: The (name, result) pairs reported by the repository. + linked: The (name, result) pairs reported by the link service. """ for name, result in linked: print_result( @@ -62,8 +63,8 @@ def link( """ with _repository() as repo: sys.stdout.write("\n") - linked, advisories, failures = repo.link_packages( - names, overwrite=overwrite, force=force, dry_run=dry_run + linked, advisories, failures = link_service.link_packages( + repo, names, overwrite=overwrite, force=force, dry_run=dry_run ) print_advisories(advisories) @@ -104,7 +105,9 @@ def unlink( """ with _repository() as repo: sys.stdout.write("\n") - unlinked, advisories, failures = repo.unlink_packages(names, dry_run=dry_run) + unlinked, advisories, failures = link_service.unlink_packages( + repo, names, dry_run=dry_run + ) print_advisories(advisories) diff --git a/src/brewery/cli/commands/pin.py b/src/brewery/cli/commands/pin.py index 5e59a08..b692c7f 100644 --- a/src/brewery/cli/commands/pin.py +++ b/src/brewery/cli/commands/pin.py @@ -12,6 +12,7 @@ print_result, ) from brewery.core.errors import UserError +from brewery.services import pin as pin_service def _reject_casks(cask: bool, command: str, names: list[str]) -> None: @@ -77,7 +78,7 @@ def pin( with _repository() as repo: sys.stdout.write("\n") - pinned, advisories, failures = repo.pin_packages(names) + pinned, advisories, failures = pin_service.pin_packages(repo, names) _report("pin", pinned, advisories, failures) @@ -102,7 +103,7 @@ def unpin( with _repository() as repo: sys.stdout.write("\n") - unpinned, advisories, failures = repo.unpin_packages(names) + unpinned, advisories, failures = pin_service.unpin_packages(repo, names) _report("unpin", unpinned, advisories, failures) diff --git a/src/brewery/core/models.py b/src/brewery/core/models.py index d7d3bee..e9cbccc 100644 --- a/src/brewery/core/models.py +++ b/src/brewery/core/models.py @@ -7,6 +7,10 @@ from enum import Enum, Flag, auto from typing import Any +# (name, reason) pairs -- every service reports its advisories and failures this +# way, and the CLI's exit-code mapping keys off whether the failure list is empty +Notes = list[tuple[str, str]] + class PackageKind(Enum): """Enumeration of package kinds.""" diff --git a/src/brewery/core/repo.py b/src/brewery/core/repo.py index e0e1447..a662657 100644 --- a/src/brewery/core/repo.py +++ b/src/brewery/core/repo.py @@ -16,7 +16,6 @@ from brewery.providers import brew if TYPE_CHECKING: - from brewery.providers.linker import LinkResult, UnlinkResult from brewery.providers.orchestrator import ProgressPort log: BreweryLogger = get_logger(name=__name__) @@ -594,159 +593,3 @@ async def sweep_rack( self.cache_mgr.invalidate() return removed, failures - - def _resolve_installed_formulae( - self, names: list[str] - ) -> tuple[list[Package], list[tuple[str, str]]]: - """Resolve user-supplied names to installed formulae. - - Args: - names: Name(s) or alias(es) of the formulae. - - Returns: - The resolved packages, and (name, reason) pairs for those not installed. - """ - found: list[Package] = [] - failures: list[tuple[str, str]] = [] - - for name in names: - pkg: Package | None = self.cache_mgr.find_installed( - self.catalog.resolve_alias(name), PackageKind.FORMULA - ) - if pkg is None: - failures.append((name, "not installed")) - - else: - found.append(pkg) - - return found, failures - - @log_operation(event_prefix="pin_packages", log_args=["names"]) - def pin_packages( - self, names: list[str] - ) -> tuple[list[str], list[tuple[str, str]], list[tuple[str, str]]]: - """Pin formulae at their active keg, preventing upgrades. - - Args: - names: Name(s) of the formulae to pin. - - Returns: - Tuple of (pinned names, (name, reason) advisories, (name, reason) failures). - """ - from brewery.providers.pinning import pin - - env: BreweryENV = self.cache_mgr.env or get_brewery_env() - pkgs, failures = self._resolve_installed_formulae(names) - - pinned: list[str] = [] - advisories: list[tuple[str, str]] = [] - for pkg in pkgs: - if not pkg.path: - failures.append((pkg.name, "no active keg")) - - elif pin(prefix=env.prefix, name=pkg.name, keg=Path(pkg.path)): - pinned.append(pkg.name) - - else: - advisories.append((pkg.name, "already pinned")) - - if pinned: - self.cache_mgr.invalidate() - - return pinned, advisories, failures - - @log_operation(event_prefix="unpin_packages", log_args=["names"]) - def unpin_packages( - self, names: list[str] - ) -> tuple[list[str], list[tuple[str, str]], list[tuple[str, str]]]: - """Unpin formulae, allowing them to be upgraded again. - - Args: - names: Name(s) of the formulae to unpin. - - Returns: - Tuple of (unpinned names, (name, reason) advisories, (name, reason) failures). - """ - from brewery.providers.pinning import unpin - - env: BreweryENV = self.cache_mgr.env or get_brewery_env() - pkgs, failures = self._resolve_installed_formulae(names) - - unpinned: list[str] = [] - advisories: list[tuple[str, str]] = [] - for pkg in pkgs: - if unpin(prefix=env.prefix, name=pkg.name): - unpinned.append(pkg.name) - - else: - advisories.append((pkg.name, "not pinned")) - - if unpinned: - self.cache_mgr.invalidate() - - return unpinned, advisories, failures - - @log_operation(event_prefix="link_packages", log_args=["names"]) - def link_packages( - self, - names: list[str], - *, - overwrite: bool = False, - force: bool = False, - dry_run: bool = False, - ) -> tuple[ - list[tuple[str, LinkResult]], list[tuple[str, str]], list[tuple[str, str]] - ]: - """Symlink formulae into the prefix. - - Args: - names: Name(s) of the formulae to link. - overwrite: Delete conflicting prefix files while linking. - force: Allow keg-only formulae to be linked. - dry_run: Report what would be linked without touching the filesystem. - - Returns: - Tuple of ((name, LinkResult) pairs, advisories, failures). - """ - from brewery.providers.link_service import run_link - - env: BreweryENV = self.cache_mgr.env or get_brewery_env() - pkgs, failures = self._resolve_installed_formulae(names) - - linked, advisories, link_failures = run_link( - pkgs, env=env, overwrite=overwrite, force=force, dry_run=dry_run - ) - - if linked and not dry_run: - self.cache_mgr.invalidate() - - return linked, advisories, failures + link_failures - - @log_operation(event_prefix="unlink_packages", log_args=["names"]) - def unlink_packages( - self, names: list[str], *, dry_run: bool = False - ) -> tuple[ - list[tuple[str, UnlinkResult]], list[tuple[str, str]], list[tuple[str, str]] - ]: - """Remove formulae's symlinks from the prefix. - - Args: - names: Name(s) of the formulae to unlink. - dry_run: Report what would be removed without touching the filesystem. - - Returns: - Tuple of ((name, UnlinkResult) pairs, advisories, failures). - """ - from brewery.providers.link_service import run_unlink - - env: BreweryENV = self.cache_mgr.env or get_brewery_env() - pkgs, failures = self._resolve_installed_formulae(names) - - unlinked, advisories, unlink_failures = run_unlink( - pkgs, env=env, dry_run=dry_run - ) - - if unlinked and not dry_run: - self.cache_mgr.invalidate() - - return unlinked, advisories, failures + unlink_failures diff --git a/src/brewery/services/__init__.py b/src/brewery/services/__init__.py new file mode 100644 index 0000000..8a09eea --- /dev/null +++ b/src/brewery/services/__init__.py @@ -0,0 +1,8 @@ +"""Command-family services: the policy layer between the CLI and the data facade. + +The layering runs strictly one way: `cli` -> `services` -> {`core`, `providers`}. +Nothing in `core` or `providers` may import from here; `tests/unit/test_layering.py` +enforces that. +""" + +from __future__ import annotations diff --git a/src/brewery/providers/link_service.py b/src/brewery/services/link.py similarity index 69% rename from src/brewery/providers/link_service.py rename to src/brewery/services/link.py index 324ad61..60b9672 100644 --- a/src/brewery/providers/link_service.py +++ b/src/brewery/services/link.py @@ -4,17 +4,19 @@ from pathlib import Path -from brewery.core.config import BreweryENV +from brewery.core.config import BreweryENV, get_brewery_env +from brewery.core.decorators import log_operation from brewery.core.errors import LinkError, OperationInProgressError from brewery.core.fs_state import is_effectively_linked, linked_names from brewery.core.locks import formula_lock -from brewery.core.models import Package, PackageStatus +from brewery.core.models import Notes, Package, PackageStatus +from brewery.core.repo import Repository from brewery.providers.linker import LinkResult, UnlinkResult, link_keg, unlink_keg +from brewery.services.resolve import installed_formulae # Conflicting paths quoted back to the user before the list is elided _MAX_QUOTED_CONFLICTS = 3 -Notes = list[tuple[str, str]] # (name, reason) advisories or failures LinkOutcome = tuple[list[tuple[str, LinkResult]], Notes, Notes] UnlinkOutcome = tuple[list[tuple[str, UnlinkResult]], Notes, Notes] @@ -172,3 +174,69 @@ def _keg(pkg: Package) -> Path: raise ValueError(f"{pkg.name} has no keg path") return Path(pkg.path) + + +@log_operation(event_prefix="link_packages", log_args=["names"]) +def link_packages( + repo: Repository, + names: list[str], + *, + overwrite: bool = False, + force: bool = False, + dry_run: bool = False, + env: BreweryENV | None = None, +) -> LinkOutcome: + """Symlink formulae into the prefix. + + Args: + repo: The data facade to read installed state through. + names: Name(s) of the formulae to link. + overwrite: Delete conflicting prefix files while linking. + force: Allow keg-only formulae to be linked. + dry_run: Report what would be linked without touching the filesystem. + env: Brewery environment (paths), resolved if omitted. + + Returns: + Tuple of ((name, LinkResult) pairs, advisories, failures). + """ + env = env or repo.cache_mgr.env or get_brewery_env() + pkgs, failures = installed_formulae(repo, names) + + linked, advisories, link_failures = run_link( + pkgs, env=env, overwrite=overwrite, force=force, dry_run=dry_run + ) + + if linked and not dry_run: + repo.cache_mgr.invalidate() + + return linked, advisories, failures + link_failures + + +@log_operation(event_prefix="unlink_packages", log_args=["names"]) +def unlink_packages( + repo: Repository, + names: list[str], + *, + dry_run: bool = False, + env: BreweryENV | None = None, +) -> UnlinkOutcome: + """Remove formulae's symlinks from the prefix. + + Args: + repo: The data facade to read installed state through. + names: Name(s) of the formulae to unlink. + dry_run: Report what would be removed without touching the filesystem. + env: Brewery environment (paths), resolved if omitted. + + Returns: + Tuple of ((name, UnlinkResult) pairs, advisories, failures). + """ + env = env or repo.cache_mgr.env or get_brewery_env() + pkgs, failures = installed_formulae(repo, names) + + unlinked, advisories, unlink_failures = run_unlink(pkgs, env=env, dry_run=dry_run) + + if unlinked and not dry_run: + repo.cache_mgr.invalidate() + + return unlinked, advisories, failures + unlink_failures diff --git a/src/brewery/services/pin.py b/src/brewery/services/pin.py new file mode 100644 index 0000000..debbffa --- /dev/null +++ b/src/brewery/services/pin.py @@ -0,0 +1,79 @@ +"""Pin and unpin formulae at their active keg.""" + +from __future__ import annotations + +from pathlib import Path + +from brewery.core.config import BreweryENV, get_brewery_env +from brewery.core.decorators import log_operation +from brewery.core.models import Notes +from brewery.core.repo import Repository +from brewery.providers.pinning import pin, unpin +from brewery.services.resolve import installed_formulae + + +@log_operation(event_prefix="pin_packages", log_args=["names"]) +def pin_packages( + repo: Repository, names: list[str], *, env: BreweryENV | None = None +) -> tuple[list[str], Notes, Notes]: + """Pin formulae at their active keg, preventing upgrades. + + Args: + repo: The data facade to read installed state through. + names: Name(s) of the formulae to pin. + env: Brewery environment (paths), resolved if omitted. + + Returns: + Tuple of (pinned names, (name, reason) advisories, (name, reason) failures). + """ + env = env or repo.cache_mgr.env or get_brewery_env() + pkgs, failures = installed_formulae(repo, names) + + pinned: list[str] = [] + advisories: Notes = [] + for pkg in pkgs: + if not pkg.path: + failures.append((pkg.name, "no active keg")) + + elif pin(prefix=env.prefix, name=pkg.name, keg=Path(pkg.path)): + pinned.append(pkg.name) + + else: + advisories.append((pkg.name, "already pinned")) + + if pinned: + repo.cache_mgr.invalidate() + + return pinned, advisories, failures + + +@log_operation(event_prefix="unpin_packages", log_args=["names"]) +def unpin_packages( + repo: Repository, names: list[str], *, env: BreweryENV | None = None +) -> tuple[list[str], Notes, Notes]: + """Unpin formulae, allowing them to be upgraded again. + + Args: + repo: The data facade to read installed state through. + names: Name(s) of the formulae to unpin. + env: Brewery environment (paths), resolved if omitted. + + Returns: + Tuple of (unpinned names, (name, reason) advisories, (name, reason) failures). + """ + env = env or repo.cache_mgr.env or get_brewery_env() + pkgs, failures = installed_formulae(repo, names) + + unpinned: list[str] = [] + advisories: Notes = [] + for pkg in pkgs: + if unpin(prefix=env.prefix, name=pkg.name): + unpinned.append(pkg.name) + + else: + advisories.append((pkg.name, "not pinned")) + + if unpinned: + repo.cache_mgr.invalidate() + + return unpinned, advisories, failures diff --git a/src/brewery/services/resolve.py b/src/brewery/services/resolve.py new file mode 100644 index 0000000..95f8393 --- /dev/null +++ b/src/brewery/services/resolve.py @@ -0,0 +1,34 @@ +"""Resolve user-supplied names to installed formulae.""" + +from __future__ import annotations + +from brewery.core.models import Notes, Package, PackageKind +from brewery.core.repo import Repository + + +def installed_formulae( + repo: Repository, names: list[str] +) -> tuple[list[Package], Notes]: + """Resolve user-supplied names to installed formulae. + + Args: + repo: The data facade to read installed state and aliases through. + names: Name(s) or alias(es) of the formulae. + + Returns: + The resolved packages, and (name, reason) pairs for those not installed. + """ + found: list[Package] = [] + failures: Notes = [] + + for name in names: + pkg: Package | None = repo.cache_mgr.find_installed( + repo.catalog.resolve_alias(name), PackageKind.FORMULA + ) + if pkg is None: + failures.append((name, "not installed")) + + else: + found.append(pkg) + + return found, failures diff --git a/tests/integration/test_pin_link_services.py b/tests/integration/test_pin_link_services.py new file mode 100644 index 0000000..b16381b --- /dev/null +++ b/tests/integration/test_pin_link_services.py @@ -0,0 +1,93 @@ +"""Integration tests for the pin and link services over a real prefix.""" + +from __future__ import annotations + +import pytest + +from brewery.core.models import PackageStatus +from brewery.services.link import link_packages, unlink_packages +from brewery.services.pin import pin_packages, unpin_packages + +pytestmark = pytest.mark.integration + + +class TestPinAndUnpin: + """Tests for the pin service.""" + + def test_pin_writes_a_record_and_shows_as_pinned(self, repo, mock_env) -> None: + """Test that a pinned formula reads back as PINNED through the merge.""" + pinned, advisories, failures = pin_packages(repo, ["act"]) + + assert (pinned, advisories, failures) == (["act"], [], []) + assert (mock_env.prefix / "var" / "homebrew" / "pinned" / "act").is_symlink() + + pkg = next(p for p in repo.get_all_installed() if p.name == "act") + assert PackageStatus.PINNED in pkg.status + + def test_pinning_twice_is_an_advisory_not_a_failure(self, repo) -> None: + """Test that re-pinning warns and exits clean, as brew's `opoo` path does.""" + pin_packages(repo, ["act"]) + + pinned, advisories, failures = pin_packages(repo, ["act"]) + assert (pinned, advisories, failures) == ([], [("act", "already pinned")], []) + + def test_unpinning_an_unpinned_formula_is_an_advisory(self, repo) -> None: + """Test that unpinning what was never pinned warns rather than failing.""" + unpinned, advisories, failures = unpin_packages(repo, ["act"]) + + assert (unpinned, advisories, failures) == ([], [("act", "not pinned")], []) + + def test_pin_of_a_missing_formula_is_a_failure(self, repo) -> None: + """Test that a name that is not installed is a hard failure, as brew's `ofail` is.""" + pinned, advisories, failures = pin_packages(repo, ["ripgrep"]) + + assert (pinned, advisories, failures) == ( + [], + [], + [("ripgrep", "not installed")], + ) + + def test_pin_does_not_reach_for_casks(self, repo) -> None: + """Test that cask tokens are not resolvable as formulae, so they report not installed.""" + _, _, failures = pin_packages(repo, ["iina"]) + + assert failures == [("iina", "not installed")] + + +class TestLinkAndUnlink: + """Tests for the link service.""" + + def test_link_then_unlink_round_trips(self, repo, mock_env) -> None: + """Test that linking creates the bookkeeping record; unlinking removes it.""" + keg = mock_env.cellar / "act" / "0.2.88" + (keg / "bin").mkdir(parents=True) + (keg / "bin" / "act").write_text("#!/bin/sh\n") + + linked, _, failures = link_packages(repo, ["act"]) + assert [name for name, _ in linked] == ["act"] + assert not failures + assert (mock_env.prefix / "bin" / "act").is_symlink() + + unlinked, _, failures = unlink_packages(repo, ["act"]) + assert "bin/act" in unlinked[0][1].removed + assert not failures + assert not (mock_env.prefix / "bin" / "act").exists() + + def test_link_dry_run_leaves_the_prefix_alone(self, repo, mock_env) -> None: + """Test that a dry run previews the links without creating any.""" + keg = mock_env.cellar / "act" / "0.2.88" + (keg / "bin").mkdir(parents=True) + (keg / "bin" / "act").write_text("#!/bin/sh\n") + + linked, _, failures = link_packages(repo, ["act"], dry_run=True) + + assert "bin/act" in linked[0][1].linked + assert not failures + assert not (mock_env.prefix / "bin" / "act").exists() + + def test_link_of_a_missing_formula_is_a_failure(self, repo) -> None: + """Test that an uninstalled name fails rather than silently succeeding.""" + linked, _, failures = link_packages(repo, ["ripgrep"]) + + assert linked == [] + assert failures == [("ripgrep", "not installed")] diff --git a/tests/integration/test_repo.py b/tests/integration/test_repo.py index c193599..bdc1319 100644 --- a/tests/integration/test_repo.py +++ b/tests/integration/test_repo.py @@ -11,6 +11,7 @@ import pytest from brewery.core.models import PackageKind, PackageStatus +from brewery.services.pin import pin_packages, unpin_packages pytestmark = pytest.mark.integration @@ -635,7 +636,7 @@ async def test_pinned_package_skipped_on_upgrade_all(self, repo) -> None: 'pinned' reason and keep it out of the upgrade targets. A bulk upgrade skips pins without failing, matching `brew upgrade`. """ - assert repo.pin_packages(["act"])[0] == ["act"] + assert pin_packages(repo, ["act"])[0] == ["act"] upgraded, _current, advisories, failures = await repo.upgrade_packages() assert ("act", "pinned - not upgraded") in advisories @@ -649,7 +650,7 @@ async def test_pinned_named_package_skipped_on_upgrade( Naming a pinned package is an error, unlike skipping it in a bulk upgrade. """ - assert repo.pin_packages(["act"])[0] == ["act"] + assert pin_packages(repo, ["act"])[0] == ["act"] upgraded, _current, _advisories, failures = await repo.upgrade_packages(["act"]) assert ("act", "pinned - skipped") in failures @@ -660,8 +661,8 @@ async def test_pinned_named_package_skipped_on_upgrade( async def test_unpinned_package_upgrades_again(self, repo) -> None: """Test that unpinning restores a package to the upgrade targets.""" - repo.pin_packages(["act"]) - assert repo.unpin_packages(["act"])[0] == ["act"] + pin_packages(repo, ["act"]) + assert unpin_packages(repo, ["act"])[0] == ["act"] upgraded, _current, advisories, _failures = await repo.upgrade_packages() assert ("act", "pinned - not upgraded") not in advisories @@ -867,88 +868,6 @@ async def no_brew(args, *, output=None, check=None): assert pkg.versions[0] == "2.0" -class TestPinAndUnpin: - """Tests for Repository.pin_packages / unpin_packages.""" - - def test_pin_writes_a_record_and_shows_as_pinned(self, repo, mock_env) -> None: - """Test that a pinned formula reads back as PINNED through the merge.""" - pinned, advisories, failures = repo.pin_packages(["act"]) - - assert (pinned, advisories, failures) == (["act"], [], []) - assert (mock_env.prefix / "var" / "homebrew" / "pinned" / "act").is_symlink() - - pkg = next(p for p in repo.get_all_installed() if p.name == "act") - assert PackageStatus.PINNED in pkg.status - - def test_pinning_twice_is_an_advisory_not_a_failure(self, repo) -> None: - """Test that re-pinning warns and exits clean, as brew's `opoo` path does.""" - repo.pin_packages(["act"]) - - pinned, advisories, failures = repo.pin_packages(["act"]) - assert (pinned, advisories, failures) == ([], [("act", "already pinned")], []) - - def test_unpinning_an_unpinned_formula_is_an_advisory(self, repo) -> None: - """Test that unpinning what was never pinned warns rather than failing.""" - unpinned, advisories, failures = repo.unpin_packages(["act"]) - - assert (unpinned, advisories, failures) == ([], [("act", "not pinned")], []) - - def test_pin_of_a_missing_formula_is_a_failure(self, repo) -> None: - """Test that a name that is not installed is a hard failure, as brew's `ofail` is.""" - pinned, advisories, failures = repo.pin_packages(["ripgrep"]) - - assert (pinned, advisories, failures) == ( - [], - [], - [("ripgrep", "not installed")], - ) - - def test_pin_does_not_reach_for_casks(self, repo) -> None: - """Test that cask tokens are not resolvable as formulae, so they report not installed.""" - _, _, failures = repo.pin_packages(["iina"]) - - assert failures == [("iina", "not installed")] - - -class TestLinkAndUnlink: - """Tests for Repository.link_packages / unlink_packages.""" - - def test_link_then_unlink_round_trips(self, repo, mock_env) -> None: - """Test that linking creates the bookkeeping record; unlinking removes it.""" - keg = mock_env.cellar / "act" / "0.2.88" - (keg / "bin").mkdir(parents=True) - (keg / "bin" / "act").write_text("#!/bin/sh\n") - - linked, _, failures = repo.link_packages(["act"]) - assert [name for name, _ in linked] == ["act"] - assert not failures - assert (mock_env.prefix / "bin" / "act").is_symlink() - - unlinked, _, failures = repo.unlink_packages(["act"]) - assert "bin/act" in unlinked[0][1].removed - assert not failures - assert not (mock_env.prefix / "bin" / "act").exists() - - def test_link_dry_run_leaves_the_prefix_alone(self, repo, mock_env) -> None: - """Test that a dry run previews the links without creating any.""" - keg = mock_env.cellar / "act" / "0.2.88" - (keg / "bin").mkdir(parents=True) - (keg / "bin" / "act").write_text("#!/bin/sh\n") - - linked, _, failures = repo.link_packages(["act"], dry_run=True) - - assert "bin/act" in linked[0][1].linked - assert not failures - assert not (mock_env.prefix / "bin" / "act").exists() - - def test_link_of_a_missing_formula_is_a_failure(self, repo) -> None: - """Test that an uninstalled name fails rather than silently succeeding.""" - linked, _, failures = repo.link_packages(["ripgrep"]) - - assert linked == [] - assert failures == [("ripgrep", "not installed")] - - class TestCleanup: """Tests for Repository.cleanup_packages.""" diff --git a/tests/unit/test_link_service.py b/tests/unit/test_link.py similarity index 86% rename from tests/unit/test_link_service.py rename to tests/unit/test_link.py index e76b392..6bff40d 100644 --- a/tests/unit/test_link_service.py +++ b/tests/unit/test_link.py @@ -11,7 +11,7 @@ from brewery.core.locks import lock_path from brewery.core.models import Package, PackageKind, PackageStatus -from brewery.providers.link_service import run_link, run_unlink +from brewery.services.link import run_link, run_unlink pytestmark = pytest.mark.unit @@ -74,7 +74,7 @@ class TestRunLink: """Tests for linking formulae.""" def test_links_a_plain_formula(self, prefix, mock_env, make_pkg) -> None: - """A normal formula links and reports its symlink count.""" + """Test a normal formula links and reports its symlink count.""" pkg = make_pkg("wget") linked, advisories, failures = run_link([pkg], env=mock_env) @@ -85,7 +85,7 @@ def test_links_a_plain_formula(self, prefix, mock_env, make_pkg) -> None: assert (prefix / "bin" / "wget").is_symlink() def test_already_linked_is_an_advisory(self, prefix, mock_env, make_pkg) -> None: - """A second link warns and skips rather than relinking.""" + """Test a second link warns and skips rather than relinking.""" pkg = make_pkg("wget") run_link([pkg], env=mock_env) @@ -96,7 +96,7 @@ def test_already_linked_is_an_advisory(self, prefix, mock_env, make_pkg) -> None assert "already linked" in advisories[0][1] def test_keg_only_needs_force(self, prefix, mock_env, make_pkg) -> None: - """Keg-only formulae are skipped with an advisory naming --force.""" + """Test keg-only formulae are skipped with an advisory naming --force.""" pkg = make_pkg("icu4c", status=PackageStatus.KEG_ONLY) linked, advisories, failures = run_link([pkg], env=mock_env) @@ -106,7 +106,7 @@ def test_keg_only_needs_force(self, prefix, mock_env, make_pkg) -> None: assert not (prefix / "bin" / "icu4c").exists() def test_keg_only_links_with_force(self, prefix, mock_env, make_pkg) -> None: - """--force links a keg-only formula and writes its linked record.""" + """Test --force links a keg-only formula and writes its linked record.""" pkg = make_pkg("icu4c", status=PackageStatus.KEG_ONLY) linked, advisories, failures = run_link([pkg], env=mock_env, force=True) @@ -119,7 +119,7 @@ def test_keg_only_links_with_force(self, prefix, mock_env, make_pkg) -> None: def test_keg_only_dry_run_previews_what_force_would_do( self, prefix, mock_env, make_pkg ) -> None: - """A dry run still previews the links, as brew does, and says --force is needed.""" + """Test a dry run still previews the links, as brew does, and says --force is needed.""" pkg = make_pkg("icu4c", status=PackageStatus.KEG_ONLY) linked, advisories, failures = run_link([pkg], env=mock_env, dry_run=True) @@ -132,7 +132,7 @@ def test_keg_only_dry_run_previews_what_force_would_do( def test_conflict_becomes_a_failure_and_the_next_formula_still_links( self, prefix, mock_env, make_pkg ) -> None: - """One formula's conflict must not abort the rest of the batch.""" + """Test one formula's conflict must not abort the rest of the batch.""" blocked = make_pkg("wget") ok = make_pkg("curl") (prefix / "bin").mkdir() @@ -148,7 +148,7 @@ def test_conflict_becomes_a_failure_and_the_next_formula_still_links( def test_overwrite_replaces_the_conflicting_file( self, prefix, mock_env, make_pkg ) -> None: - """--overwrite links over a real file instead of failing.""" + """Test --overwrite links over a real file instead of failing.""" pkg = make_pkg("wget") (prefix / "bin").mkdir() (prefix / "bin" / "wget").write_text("a real file in the way") @@ -164,7 +164,7 @@ class TestRunUnlink: """Tests for unlinking formulae.""" def test_unlinks_a_linked_formula(self, prefix, mock_env, make_pkg) -> None: - """Unlinking removes the symlinks and reports the count.""" + """Test unlinking removes the symlinks and reports the count.""" pkg = make_pkg("wget") run_link([pkg], env=mock_env) @@ -177,7 +177,7 @@ def test_unlinks_a_linked_formula(self, prefix, mock_env, make_pkg) -> None: def test_keg_only_unlink_is_a_no_op_not_a_failure( self, prefix, mock_env, make_pkg ) -> None: - """Keg-only formulae own no symlinks, so unlinking removes nothing and succeeds. + """Test keg-only formulae own no symlinks, so unlinking removes nothing and succeeds. Matches brew, whose `unlink` never consults keg_only. """ @@ -189,7 +189,7 @@ def test_keg_only_unlink_is_a_no_op_not_a_failure( assert not advisories and not failures def test_dry_run_reports_without_removing(self, prefix, mock_env, make_pkg) -> None: - """A dry run names the symlinks and leaves them in place.""" + """Test a dry run names the symlinks and leaves them in place.""" pkg = make_pkg("wget") run_link([pkg], env=mock_env) @@ -201,10 +201,10 @@ def test_dry_run_reports_without_removing(self, prefix, mock_env, make_pkg) -> N class TestRackLock: - """A rack locked by another process is a per-formula failure.""" + """Tests for per-rack locking.""" def test_link_refuses_a_locked_rack(self, prefix, mock_env, make_pkg) -> None: - """The prefix is left untouched and the reason is reported.""" + """Test that the prefix is left untouched and the reason is reported.""" pkg = make_pkg("wget") fd = _hold_rack(prefix, "wget") try: @@ -219,7 +219,7 @@ def test_link_refuses_a_locked_rack(self, prefix, mock_env, make_pkg) -> None: assert not (prefix / "bin" / "wget").exists() def test_unlink_refuses_a_locked_rack(self, prefix, mock_env, make_pkg) -> None: - """An in-progress operation on the rack keeps the symlinks in place.""" + """Test that an in-progress operation on the rack keeps the symlinks in place.""" pkg = make_pkg("wget") run_link([pkg], env=mock_env) From fe72ae31c6277daba49054b3d96d0d05a75c49fa Mon Sep 17 00:00:00 2001 From: Rob Webb Date: Wed, 19 Aug 2026 13:54:22 +0100 Subject: [PATCH 3/8] refactor(services): move cleanup out of Repository into the services layer - cleanup_packages() moves to services/cleanup.py, alongside its previously-nested remove_rack()/sweep_rack() helpers, now module-level - '_CLEANUP_CONCURRENCY' moves with them - The function-local imports the old method needed to avoid a cycle become ordinary module-level imports now that the code sits above 'core' - 'env=' becomes an overridable keyword rather than always resolved internally, and the failure lists use the shared 'Notes' alias - cli/commands/cleanup.py and daemon/catalog_refresh._maybe_cleanup() call the service directly instead of the repo method - 'TestCleanup' moves from test_repo.py into a new test_cleanup_service.py, with the per-test local imports hoisted to module level --- src/brewery/cli/commands/cleanup.py | 3 +- src/brewery/core/repo.py | 122 ----------------- src/brewery/daemon/catalog_refresh.py | 3 +- src/brewery/services/cleanup.py | 141 +++++++++++++++++++ tests/integration/test_catalog_refresh.py | 21 ++- tests/integration/test_cleanup_service.py | 148 ++++++++++++++++++++ tests/integration/test_repo.py | 156 ---------------------- 7 files changed, 307 insertions(+), 287 deletions(-) create mode 100644 src/brewery/services/cleanup.py create mode 100644 tests/integration/test_cleanup_service.py diff --git a/src/brewery/cli/commands/cleanup.py b/src/brewery/cli/commands/cleanup.py index 5c7a4e4..73646e8 100644 --- a/src/brewery/cli/commands/cleanup.py +++ b/src/brewery/cli/commands/cleanup.py @@ -7,6 +7,7 @@ from brewery.cli.context import _repository, app, console, run_async from brewery.cli.error_formatting import CommandFailed, command_error from brewery.cli.output import print_failures, print_result, spinner +from brewery.services.cleanup import cleanup_packages @app.command(name="cleanup", aliases=["c", "clean"]) @@ -16,7 +17,7 @@ def cleanup() -> None: with _repository() as repo: sys.stdout.write("\n") with spinner("Cleaning up..."): - removed, failures = run_async(coro=repo.cleanup_packages()) + removed, failures = run_async(coro=cleanup_packages(repo)) if not removed and not failures: console.print("✓ Nothing to clean up\n", style="bold green") diff --git a/src/brewery/core/repo.py b/src/brewery/core/repo.py index a662657..ffccaa3 100644 --- a/src/brewery/core/repo.py +++ b/src/brewery/core/repo.py @@ -20,9 +20,6 @@ log: BreweryLogger = get_logger(name=__name__) -# Racks swept concurrently by `cleanup_packages`, matching the download/install bounds -_CLEANUP_CONCURRENCY = 8 - class Repository: """Repository for managing package data from various backends.""" @@ -474,122 +471,3 @@ async def upgrade_packages( current.append(pkg) return upgraded, current, advisories, failures - - @log_operation(event_prefix="cleanup") - async def cleanup_packages( - self, max_age_days: int | None = None - ) -> tuple[list[str], list[tuple[str, str]]]: - """Remove stale kegs replaced more than max_age_days ago. - - Args: - max_age_days: Age threshold in days, defaults to 30. - - Returns: - Tuple of (removed "name version" strings, (label, reason) failures). - """ - import asyncio - from collections import defaultdict - - from brewery.core.errors import OperationInProgressError - from brewery.core.locks import formula_lock - from brewery.core.settings import load_settings - from brewery.providers.cellar import rmtree - from brewery.providers.retention import CleanupCandidate, cleanup_candidates - - s = load_settings().retention - age = s.age_days if max_age_days is None else max_age_days - - env = self.cache_mgr.env or get_brewery_env() - - def remove_rack( - name: str, kegs: list[CleanupCandidate] - ) -> tuple[list[str], list[tuple[str, str]]]: - """Delete every stale keg of one formula under a single rack lock. - - One lock acquisition per rack, not per keg: the rack lock is - non-reentrant across threads, so two kegs of the same formula removed - concurrently would make one of them look locked by a peer process. - - Args: - name: The name of the formula. - kegs: That formula's stale kegs, in selection order. - - Returns: - Tuple of (removed "name version" strings, (label, reason) failures). - - Raises: - OperationInProgressError: Another process holds the rack lock. - """ - done: list[str] = [] - failed: list[tuple[str, str]] = [] - - with formula_lock(name, prefix=env.prefix): - for c in kegs: - label = f"{c.name} {c.version}" - try: - rmtree(c.keg) - done.append(label) - - except OSError as e: - failed.append((label, str(e))) - - return done, failed - - async def sweep_rack( - name: str, kegs: list[CleanupCandidate] - ) -> tuple[list[str], list[tuple[str, str]]]: - """Remove one rack's stale kegs off the event loop, bounded by `sem`. - - Args: - name: The name of the formula. - kegs: That formula's stale kegs, in selection order. - - Returns: - Tuple of (removed "name version" strings, (label, reason) failures). - """ - async with sem: - try: - return await asyncio.to_thread(remove_rack, name, kegs) - - except OperationInProgressError: - # Mid-install process on this rack; the next sweep picks it up - log.info(event="cleanup_skipped_locked", formula=name) - - return [], [] - - installed = self.cache_mgr.installed_packages(kind=PackageKind.FORMULA) - active = {Path(p.path) for p in installed if p.path} - # Reuse the already-attached size cache so it never re-measures the active cellar - active_sizes = { - Path(p.path): p.size_kb - for p in installed - if p.path and p.size_kb is not None - } - - by_rack: dict[str, list[CleanupCandidate]] = defaultdict(list) - for c in cleanup_candidates( - env.cellar, - active=active, - max_age_days=age, - max_versions=s.max_versions, - max_cellar_mb=s.max_cellar_mb, - active_sizes=active_sizes, - ): - by_rack[c.name].append(c) - - sem = asyncio.Semaphore(_CLEANUP_CONCURRENCY) - results = await asyncio.gather( - *(sweep_rack(name, kegs) for name, kegs in by_rack.items()) - ) - - removed: list[str] = [] - failures: list[tuple[str, str]] = [] - # Flattened in submission order, so the summary stays deterministic - for done, failed in results: - removed += done - failures += failed - - if removed: - self.cache_mgr.invalidate() - - return removed, failures diff --git a/src/brewery/daemon/catalog_refresh.py b/src/brewery/daemon/catalog_refresh.py index b9c91c7..367f8ce 100644 --- a/src/brewery/daemon/catalog_refresh.py +++ b/src/brewery/daemon/catalog_refresh.py @@ -69,6 +69,7 @@ async def _maybe_cleanup(catalog: Catalog) -> None: from brewery.core.config import ensure_cache_dir from brewery.core.repo import Repository from brewery.providers.retention import due_for_cleanup, mark_cleanup_run + from brewery.services.cleanup import cleanup_packages cache_dir = ensure_cache_dir() if not due_for_cleanup(cache_dir): @@ -77,7 +78,7 @@ async def _maybe_cleanup(catalog: Catalog) -> None: try: # Borrows the caller's catalog, so must not be closed here repo = Repository(catalog=catalog) - removed, _failures = await repo.cleanup_packages() + removed, _failures = await cleanup_packages(repo) mark_cleanup_run(cache_dir) if removed: log.info(event="daemon_cleanup", removed=len(removed)) diff --git a/src/brewery/services/cleanup.py b/src/brewery/services/cleanup.py new file mode 100644 index 0000000..2650b0c --- /dev/null +++ b/src/brewery/services/cleanup.py @@ -0,0 +1,141 @@ +"""Sweep stale kegs the retention policy no longer wants to keep.""" + +from __future__ import annotations + +import asyncio +from collections import defaultdict +from pathlib import Path + +from brewery.core.config import BreweryENV, get_brewery_env +from brewery.core.decorators import log_operation +from brewery.core.errors import OperationInProgressError +from brewery.core.locks import formula_lock +from brewery.core.logging import BreweryLogger, get_logger +from brewery.core.models import Notes, PackageKind +from brewery.core.repo import Repository +from brewery.core.settings import load_settings +from brewery.providers.cellar import rmtree +from brewery.providers.retention import CleanupCandidate, cleanup_candidates + +log: BreweryLogger = get_logger(name=__name__) + +# Racks swept concurrently, matching the download/install bounds +_CLEANUP_CONCURRENCY = 8 + + +def remove_rack( + name: str, kegs: list[CleanupCandidate], env: BreweryENV +) -> tuple[list[str], Notes]: + """Delete every stale keg of one formula under a single rack lock. + + One lock acquisition per rack, not per keg: the rack lock is + non-reentrant across threads, so two kegs of the same formula removed + concurrently would make one of them look locked by a peer process. + + Args: + name: The name of the formula. + kegs: That formula's stale kegs, in selection order. + env: The Brewery environment. + + Returns: + Tuple of (removed "name version" strings, (label, reason) failures). + + Raises: + OperationInProgressError: Another process holds the rack lock. + """ + done: list[str] = [] + failed: Notes = [] + + with formula_lock(name, prefix=env.prefix): + for c in kegs: + label = f"{c.name} {c.version}" + try: + rmtree(c.keg) + done.append(label) + + except OSError as e: + failed.append((label, str(e))) + + return done, failed + + +async def sweep_rack( + name: str, kegs: list[CleanupCandidate], env: BreweryENV +) -> tuple[list[str], Notes]: + """Remove one rack's stale kegs off the event loop, bounded by `sem`. + + Args: + name: The name of the formula. + kegs: That formula's stale kegs, in selection order. + + Returns: + Tuple of (removed "name version" strings, (label, reason) failures). + """ + sem = asyncio.Semaphore(_CLEANUP_CONCURRENCY) + + async with sem: + try: + return await asyncio.to_thread(remove_rack, name, kegs, env) + + except OperationInProgressError: + # Mid-install process on this rack; the next sweep picks it up + log.info(event="cleanup_skipped_locked", formula=name) + + return [], [] + + +@log_operation(event_prefix="cleanup") +async def cleanup_packages( + repo: Repository, + max_age_days: int | None = None, + *, + env: BreweryENV | None = None, +) -> tuple[list[str], Notes]: + """Remove stale kegs replaced more than max_age_days ago. + + Args: + repo: The data facade to read installed state through. + max_age_days: Age threshold in days, defaults to 30. + env: Brewery environment (paths), resolved if omitted. + + Returns: + Tuple of (removed "name version" strings, (label, reason) failures). + """ + s = load_settings().retention + age = s.age_days if max_age_days is None else max_age_days + + env = env or repo.cache_mgr.env or get_brewery_env() + + installed = repo.cache_mgr.installed_packages(kind=PackageKind.FORMULA) + active = {Path(p.path) for p in installed if p.path} + # Reuse the already-attached size cache so it never re-measures the active cellar + active_sizes = { + Path(p.path): p.size_kb for p in installed if p.path and p.size_kb is not None + } + + by_rack: dict[str, list[CleanupCandidate]] = defaultdict(list) + for c in cleanup_candidates( + env.cellar, + active=active, + max_age_days=age, + max_versions=s.max_versions, + max_cellar_mb=s.max_cellar_mb, + active_sizes=active_sizes, + ): + by_rack[c.name].append(c) + + results = await asyncio.gather( + *(sweep_rack(name, kegs, env) for name, kegs in by_rack.items()) + ) + + removed: list[str] = [] + failures: Notes = [] + # Flattened in submission order, so the summary stays deterministic + for done, failed in results: + removed += done + failures += failed + + if removed: + repo.cache_mgr.invalidate() + + return removed, failures diff --git a/tests/integration/test_catalog_refresh.py b/tests/integration/test_catalog_refresh.py index 2758637..e682542 100644 --- a/tests/integration/test_catalog_refresh.py +++ b/tests/integration/test_catalog_refresh.py @@ -13,6 +13,7 @@ from brewery.daemon import catalog_refresh as cr from brewery.daemon.catalog_refresh import refresh_catalog from brewery.providers import retention +from brewery.services import cleanup as cleanup_service pytestmark = pytest.mark.integration @@ -313,17 +314,23 @@ def __init__(self, catalog=None) -> None: """ calls.append("init") - async def cleanup_packages(self) -> tuple[list, list]: - """Simulates the cleanup_packages method, returning any cleanup results. + async def _sweep(repo, *a, **k) -> tuple[list, list]: + """Stands in for the cleanup service, returning any cleanup results. - Returns: - The cleanup results. - """ - calls.append("cleanup") + Args: + repo: The repository the daemon built. + *a: Ignored positional arguments. + **k: Ignored keyword arguments. + + Returns: + The cleanup results. + """ + calls.append("cleanup") - return cleanup() if cleanup is not None else ([], []) + return cleanup() if cleanup is not None else ([], []) monkeypatch.setattr("brewery.core.repo.Repository", MockRepo) + monkeypatch.setattr(cleanup_service, "cleanup_packages", _sweep) return calls, marks diff --git a/tests/integration/test_cleanup_service.py b/tests/integration/test_cleanup_service.py new file mode 100644 index 0000000..72f752a --- /dev/null +++ b/tests/integration/test_cleanup_service.py @@ -0,0 +1,148 @@ +"""Integration tests for the cleanup service's retention sweep.""" + +from __future__ import annotations + +import time + +import pytest + +from brewery.core import config +from brewery.core.repo import Repository +from brewery.providers.retention import mark_replaced +from brewery.services.cleanup import cleanup_packages + +pytestmark = pytest.mark.integration + + +class TestCleanup: + """Tests for the cleanup service.""" + + async def test_cleanup_removes_old_keeps_active_and_recent( + self, brew, empty_catalog, monkeypatch + ) -> None: + """Test that cleanup removes old stale versions, keeps active and recent versions.""" + DAY = 86400 + brew.formula( + "wget", + "2.0", + receipt={"source": {"tap": "homebrew/core"}, "runtime_dependencies": []}, + link_opt=True, # opt -> 2.0, so fs_state marks it active + ) + monkeypatch.setattr(config, "_env_cache", brew.env) + + cellar = brew.cellar + old = cellar / "wget" / "1.0" + old.mkdir(parents=True) + recent = cellar / "wget" / "3.0" + recent.mkdir(parents=True) + now = int(time.time()) + mark_replaced(old, by="2.0", at=now - 40 * DAY) + mark_replaced(recent, by="2.0", at=now - 2 * DAY) + + removed, failures = await cleanup_packages(Repository(catalog=empty_catalog)) + + assert removed == ["wget 1.0"] + assert failures == [] + assert not old.exists() # Old stale: removed + assert recent.exists() # Recent stale: kept + assert (cellar / "wget" / "2.0").exists() # Active: kept + + async def test_cleanup_removes_every_stale_keg_of_one_rack( + self, brew, empty_catalog, monkeypatch + ) -> None: + """Test that several stale kegs of one formula are all removed. + + The rack lock is per formula and not reentrant across threads, so a sweep + that parallelised per keg rather than per rack would find its own sibling + holding the lock and silently skip it. + """ + DAY = 86400 + brew.formula( + "wget", + "3.0", + receipt={"source": {"tap": "homebrew/core"}, "runtime_dependencies": []}, + link_opt=True, + ) + monkeypatch.setattr(config, "_env_cache", brew.env) + + now = int(time.time()) + stale = [] + for version in ("1.0", "1.5", "2.0"): + keg = brew.cellar / "wget" / version + keg.mkdir(parents=True) + mark_replaced(keg, by="3.0", at=now - 40 * DAY) + stale.append(keg) + + removed, failures = await cleanup_packages(Repository(catalog=empty_catalog)) + + assert failures == [] + assert sorted(removed) == ["wget 1.0", "wget 1.5", "wget 2.0"] + assert not any(keg.exists() for keg in stale) + assert (brew.cellar / "wget" / "3.0").exists() + + async def test_cleanup_sweeps_several_racks( + self, brew, empty_catalog, monkeypatch + ) -> None: + """Test that stale kegs across racks are all removed, one lock per rack.""" + DAY = 86400 + monkeypatch.setattr(config, "_env_cache", brew.env) + + now = int(time.time()) + stale = [] + for name in ("wget", "curl", "jq"): + brew.formula( + name, + "2.0", + receipt={ + "source": {"tap": "homebrew/core"}, + "runtime_dependencies": [], + }, + link_opt=True, + ) + keg = brew.cellar / name / "1.0" + keg.mkdir(parents=True) + mark_replaced(keg, by="2.0", at=now - 40 * DAY) + stale.append(keg) + + removed, failures = await cleanup_packages(Repository(catalog=empty_catalog)) + + assert failures == [] + assert sorted(removed) == ["curl 1.0", "jq 1.0", "wget 1.0"] + assert not any(keg.exists() for keg in stale) + + async def test_cleanup_skips_a_locked_rack( + self, brew, empty_catalog, monkeypatch + ) -> None: + """Test that a rack mid-install is left for the next sweep, not reported as a failure.""" + import fcntl + import os + + from brewery.core.locks import lock_path + + brew.formula( + "wget", + "2.0", + receipt={"source": {"tap": "homebrew/core"}, "runtime_dependencies": []}, + link_opt=True, + ) + monkeypatch.setattr(config, "_env_cache", brew.env) + + old = brew.cellar / "wget" / "1.0" + old.mkdir(parents=True) + mark_replaced(old, by="2.0", at=int(time.time()) - 40 * 86400) + + path = lock_path(brew.env.prefix, "wget") + path.parent.mkdir(parents=True, exist_ok=True) + fd = os.open(path, os.O_RDWR | os.O_CREAT, 0o644) + fcntl.flock(fd, fcntl.LOCK_EX) + try: + removed, failures = await cleanup_packages( + Repository(catalog=empty_catalog) + ) + + finally: + os.close(fd) + + assert removed == [] + assert failures == [] # Opportunistic: the daemon retries tomorrow + assert old.exists() diff --git a/tests/integration/test_repo.py b/tests/integration/test_repo.py index bdc1319..38c91e0 100644 --- a/tests/integration/test_repo.py +++ b/tests/integration/test_repo.py @@ -866,159 +866,3 @@ async def no_brew(args, *, output=None, check=None): # The rescan resolves the active version to 2.0 (1.0 is now a stale version) pkg = next(p for p in repo.get_all_installed() if p.name == "wget") assert pkg.versions[0] == "2.0" - - -class TestCleanup: - """Tests for Repository.cleanup_packages.""" - - async def test_cleanup_removes_old_keeps_active_and_recent( - self, brew, empty_catalog, monkeypatch - ) -> None: - """Test that cleanup removes old stale versions, keeps active and recent versions.""" - import time - - from brewery.core import config - from brewery.core.repo import Repository - from brewery.providers.retention import mark_replaced - - DAY = 86400 - brew.formula( - "wget", - "2.0", - receipt={"source": {"tap": "homebrew/core"}, "runtime_dependencies": []}, - link_opt=True, # opt -> 2.0, so fs_state marks it active - ) - monkeypatch.setattr(config, "_env_cache", brew.env) - - cellar = brew.cellar - old = cellar / "wget" / "1.0" - old.mkdir(parents=True) - recent = cellar / "wget" / "3.0" - recent.mkdir(parents=True) - now = int(time.time()) - mark_replaced(old, by="2.0", at=now - 40 * DAY) - mark_replaced(recent, by="2.0", at=now - 2 * DAY) - - removed, failures = await Repository(catalog=empty_catalog).cleanup_packages() - - assert removed == ["wget 1.0"] - assert failures == [] - assert not old.exists() # Old stale: removed - assert recent.exists() # Recent stale: kept - assert (cellar / "wget" / "2.0").exists() # Active: kept - - async def test_cleanup_removes_every_stale_keg_of_one_rack( - self, brew, empty_catalog, monkeypatch - ) -> None: - """Test that several stale kegs of one formula are all removed. - - The rack lock is per formula and not reentrant across threads, so a sweep - that parallelised per keg rather than per rack would find its own sibling - holding the lock and silently skip it. - """ - import time - - from brewery.core import config - from brewery.core.repo import Repository - from brewery.providers.retention import mark_replaced - - DAY = 86400 - brew.formula( - "wget", - "3.0", - receipt={"source": {"tap": "homebrew/core"}, "runtime_dependencies": []}, - link_opt=True, - ) - monkeypatch.setattr(config, "_env_cache", brew.env) - - now = int(time.time()) - stale = [] - for version in ("1.0", "1.5", "2.0"): - keg = brew.cellar / "wget" / version - keg.mkdir(parents=True) - mark_replaced(keg, by="3.0", at=now - 40 * DAY) - stale.append(keg) - - removed, failures = await Repository(catalog=empty_catalog).cleanup_packages() - - assert failures == [] - assert sorted(removed) == ["wget 1.0", "wget 1.5", "wget 2.0"] - assert not any(keg.exists() for keg in stale) - assert (brew.cellar / "wget" / "3.0").exists() - - async def test_cleanup_sweeps_several_racks( - self, brew, empty_catalog, monkeypatch - ) -> None: - """Test that stale kegs across racks are all removed, one lock per rack.""" - import time - - from brewery.core import config - from brewery.core.repo import Repository - from brewery.providers.retention import mark_replaced - - DAY = 86400 - monkeypatch.setattr(config, "_env_cache", brew.env) - - now = int(time.time()) - stale = [] - for name in ("wget", "curl", "jq"): - brew.formula( - name, - "2.0", - receipt={ - "source": {"tap": "homebrew/core"}, - "runtime_dependencies": [], - }, - link_opt=True, - ) - keg = brew.cellar / name / "1.0" - keg.mkdir(parents=True) - mark_replaced(keg, by="2.0", at=now - 40 * DAY) - stale.append(keg) - - removed, failures = await Repository(catalog=empty_catalog).cleanup_packages() - - assert failures == [] - assert sorted(removed) == ["curl 1.0", "jq 1.0", "wget 1.0"] - assert not any(keg.exists() for keg in stale) - - async def test_cleanup_skips_a_locked_rack( - self, brew, empty_catalog, monkeypatch - ) -> None: - """Test that a rack mid-install is left for the next sweep, not reported as a failure.""" - import fcntl - import os - import time - - from brewery.core import config - from brewery.core.locks import lock_path - from brewery.core.repo import Repository - from brewery.providers.retention import mark_replaced - - brew.formula( - "wget", - "2.0", - receipt={"source": {"tap": "homebrew/core"}, "runtime_dependencies": []}, - link_opt=True, - ) - monkeypatch.setattr(config, "_env_cache", brew.env) - - old = brew.cellar / "wget" / "1.0" - old.mkdir(parents=True) - mark_replaced(old, by="2.0", at=int(time.time()) - 40 * 86400) - - path = lock_path(brew.env.prefix, "wget") - path.parent.mkdir(parents=True, exist_ok=True) - fd = os.open(path, os.O_RDWR | os.O_CREAT, 0o644) - fcntl.flock(fd, fcntl.LOCK_EX) - try: - removed, failures = await Repository( - catalog=empty_catalog - ).cleanup_packages() - - finally: - os.close(fd) - - assert removed == [] - assert failures == [] # Opportunistic: the daemon retries tomorrow - assert old.exists() From c483f1aff3591734aba668e2eaf8e9b423da21f9 Mon Sep 17 00:00:00 2001 From: Rob Webb Date: Wed, 19 Aug 2026 17:42:49 +0100 Subject: [PATCH 4/8] refactor(services): move uninstall out of Repository into the services layer - uninstall_packages() and _verify_removed() move to services/uninstall.py as free functions taking the repo as an argument - The function-local imports the old method needed become ordinary module-level ones now that the code sits above 'core' - 'env=' becomes an overridable keyword - The 'formula'/'cask' backends become injectable parameters defaulting to 'brew.formula_backend'/'brew.cask_backend' instead of being read off the repo - _blocking_dependents() moves to deps.py, now a pure function over an already-merged package list with no repo access - The caller decides where the list comes from, reusing the 'kind=None' scan when there was one so a cask-only batch never scans the cellar at all - The failure lists use the shared 'Notes' alias - cli/commands/uninstall.py calls the service directly instead of the repo method - Uninstall and _verify_removed() test cases move from test_repo.py into a new test_uninstalling.py - Shared fixtures and helpers extracted to _repo_helpers.py --- src/brewery/cli/commands/uninstall.py | 3 +- src/brewery/core/deps.py | 38 ++++ src/brewery/core/repo.py | 167 +-------------- src/brewery/services/uninstall.py | 155 ++++++++++++++ tests/integration/_repo_helpers.py | 73 +++++++ tests/integration/test_repo.py | 278 +------------------------ tests/integration/test_uninstalling.py | 239 +++++++++++++++++++++ 7 files changed, 509 insertions(+), 444 deletions(-) create mode 100644 src/brewery/core/deps.py create mode 100644 src/brewery/services/uninstall.py create mode 100644 tests/integration/_repo_helpers.py create mode 100644 tests/integration/test_uninstalling.py diff --git a/src/brewery/cli/commands/uninstall.py b/src/brewery/cli/commands/uninstall.py index 951662e..710ee11 100644 --- a/src/brewery/cli/commands/uninstall.py +++ b/src/brewery/cli/commands/uninstall.py @@ -14,6 +14,7 @@ spinner, ) from brewery.core.models import PackageKind +from brewery.services.uninstall import uninstall_packages @app.command(aliases=["rm", "del"]) @@ -41,7 +42,7 @@ def uninstall( with _repository() as repo: with spinner("Uninstalling..."): - removed, failures = run_async(coro=repo.uninstall_packages(names, kind)) + removed, failures = run_async(coro=uninstall_packages(repo, names, kind)) print_result( f"✓ Uninstalled {len(removed)} package(s)\n", removed, style="bold green" diff --git a/src/brewery/core/deps.py b/src/brewery/core/deps.py new file mode 100644 index 0000000..6ccb351 --- /dev/null +++ b/src/brewery/core/deps.py @@ -0,0 +1,38 @@ +"""Dependency queries over already-merged installed packages.""" + +from __future__ import annotations + +from brewery.core.models import Package, PackageKind + + +def blocking_dependents( + packages: list[Package], removal: set[str] +) -> dict[str, list[str]]: + """Installed formulae outside `removal` that still require a target. + + Reads each target's receipt-derived reverse-deps and drops any dependent + that is itself being removed in the same batch. + + Args: + packages: Installed packages to search; non-formulae are ignored. + removal: Canonical formula names slated for removal. + + Returns: + target -> sorted installed formulae that require it (empty if none). + """ + if not removal: + return {} + + installed = {p.name: p for p in packages if p.kind == PackageKind.FORMULA} + + blockers: dict[str, list[str]] = {} + for name in removal: + pkg = installed.get(name) + if pkg is None: + continue + + deps = sorted(d for d in pkg.used_by if d not in removal) + if deps: + blockers[name] = deps + + return blockers diff --git a/src/brewery/core/repo.py b/src/brewery/core/repo.py index ffccaa3..bd6a3fd 100644 --- a/src/brewery/core/repo.py +++ b/src/brewery/core/repo.py @@ -7,7 +7,7 @@ from brewery.core.cache import Cache, CacheManager from brewery.core.catalog import Catalog -from brewery.core.config import BreweryENV, get_brewery_env +from brewery.core.config import BreweryENV from brewery.core.decorators import log_operation from brewery.core.errors import PackageNotFoundError from brewery.core.logging import BreweryLogger, get_logger @@ -181,171 +181,6 @@ async def install_packages( return installed, failures - @log_operation(event_prefix="uninstall_package", log_args=["name", "kind"]) - async def uninstall_packages( - self, names: list[str], kind: PackageKind | None = None - ) -> tuple[list[str], list[tuple[str, str]]]: - """Uninstall packages and refresh cache on success. - - Args: - names: Name(s) of the package(s) to uninstall. - kind: Kind of the package(s) (formula or cask). - - Returns: - List of successfully removed package names, and list of (name, reason) failures - - Raises: - BrewCommandError: Propagated from provider. - """ - resolved: dict[str, str] = {n: self.catalog.resolve_alias(n) for n in names} - - all_pkgs: list[Package] | None = None - - if kind is None: - # Resolve kinds and split into two lists - all_pkgs = self.get_all_installed() - kind_map: dict[str, PackageKind] = {p.name: p.kind for p in all_pkgs} - formula_names: list[str] = [ - resolved[n] - for n in names - if kind_map.get(resolved[n]) == PackageKind.FORMULA - ] - - cask_names: list[str] = [ - resolved[n] - for n in names - if kind_map.get(resolved[n]) == PackageKind.CASK - ] - - failures: list[tuple[str, str]] = [ - (n, "not found") for n in names if resolved[n] not in kind_map - ] - - else: - formula_names: list[str] = ( - [resolved[n] for n in names] if kind == PackageKind.FORMULA else [] - ) - cask_names: list[str] = ( - [resolved[n] for n in names] if kind == PackageKind.CASK else [] - ) - failures: list = [] - - blocked = self._blocking_dependents(set(formula_names), all_pkgs) - if blocked: - failures.extend( - (name, f"required by {', '.join(deps)}") - for name, deps in blocked.items() - ) - formula_names = [n for n in formula_names if n not in blocked] - - if formula_names: - from brewery.providers.uninstall_service import run_uninstall - - await run_uninstall(formula_names, formula=self.formula) - - if cask_names: - await self.cask.uninstall(names=cask_names) - - self.cache_mgr.invalidate() - - removed: list[str] = [] - failed: list[str] = [] - - for pkg_names, k in [ - (formula_names, PackageKind.FORMULA), - (cask_names, PackageKind.CASK), - ]: - if not pkg_names: - continue - - r, f = self._verify_removed(pkg_names, k) - removed += r - failed += f - - failures.extend((n, "uninstall failed") for n in failed) - - return removed, failures - - def _blocking_dependents( - self, removal: set[str], installed_pkgs: list[Package] | None = None - ) -> dict[str, list[str]]: - """Installed formulae outside `removal` that still require a target. - - Reads each target's receipt-derived reverse-deps and drops any dependent - that is itself being removed in the same batch. - - Args: - removal: Canonical formula names slated for removal. - installed_pkgs: Pre-fetched installed packages to reuse; when None, - the formula set is fetched here. - - Returns: - target -> sorted installed formulae that require it (empty if none). - """ - if not removal: - return {} - - source: list[Package] = ( - installed_pkgs - if installed_pkgs is not None - else self.cache_mgr.installed_packages(kind=PackageKind.FORMULA) - ) - installed = {p.name: p for p in source if p.kind == PackageKind.FORMULA} - - blockers: dict[str, list[str]] = {} - for name in removal: - pkg = installed.get(name) - if pkg is None: - continue - - deps = sorted(d for d in pkg.used_by if d not in removal) - if deps: - blockers[name] = deps - - return blockers - - def _verify_removed( - self, names: list[str], kind: PackageKind - ) -> tuple[list[str], list[str]]: - """Return (removed, failed) based on filesystem presence. - - A package counts as installed only while its directory still holds a version. - The lookup is case-insensitive so a mixed-case cask token still finds its - directory on a case-sensitive volume. - - Args: - names: Package names to verify. - kind: Package kind (formula or cask), selecting cellar or caskroom. - - Returns: - Tuple of (removed, failed) package names. - """ - import contextlib - - from brewery.core.fs_state import child_dirs - - env = self.cache_mgr.env or get_brewery_env() - - base_dir = env.cellar if kind == PackageKind.FORMULA else env.caskroom - index: dict[str, Path] = {d.name.casefold(): d for d in child_dirs(base_dir)} - - removed, failed = [], [] - for name in names: - survivor: Path | None = index.get(name.casefold()) - - if survivor is not None and child_dirs(survivor): - failed.append(name) - continue - - if survivor is not None: - # Nothing installed under it; drop the shell so the tree matches - with contextlib.suppress(OSError): - survivor.rmdir() - - removed.append(name) - - return removed, failed - @log_operation(event_prefix="upgrade_packages", log_args=["names", "kind"]) async def upgrade_packages( self, diff --git a/src/brewery/services/uninstall.py b/src/brewery/services/uninstall.py new file mode 100644 index 0000000..4d188ce --- /dev/null +++ b/src/brewery/services/uninstall.py @@ -0,0 +1,155 @@ +"""Uninstall formulae and casks, verifying removal against the filesystem.""" + +from __future__ import annotations + +import contextlib +from pathlib import Path + +from brewery.core.config import BreweryENV, get_brewery_env +from brewery.core.decorators import log_operation +from brewery.core.deps import blocking_dependents +from brewery.core.fs_state import child_dirs +from brewery.core.models import Notes, Package, PackageKind +from brewery.core.repo import Repository +from brewery.providers import brew +from brewery.providers.base import PackageBackend, UninstallBackend +from brewery.providers.uninstall_service import run_uninstall + + +@log_operation(event_prefix="uninstall_package", log_args=["name", "kind"]) +async def uninstall_packages( + repo: Repository, + names: list[str], + kind: PackageKind | None = None, + *, + env: BreweryENV | None = None, + formula: UninstallBackend = brew.formula_backend, + cask: PackageBackend = brew.cask_backend, +) -> tuple[list[str], Notes]: + """Uninstall packages and refresh cache on success. + + Args: + repo: The data facade to read installed state and aliases through. + names: Name(s) of the package(s) to uninstall. + kind: Kind of the package(s) (formula or cask). + env: Brewery environment (paths), resolved if omitted. + formula: Formula backend for the per-formula brew fallback. + cask: Cask backend, which handles casks wholesale. + + Returns: + List of successfully removed package names, and list of (name, reason) failures + + Raises: + BrewCommandError: Propagated from provider. + """ + env = env or repo.cache_mgr.env or get_brewery_env() + resolved: dict[str, str] = {n: repo.catalog.resolve_alias(n) for n in names} + + all_pkgs: list[Package] | None = None + + if kind is None: + # Resolve kinds and split into two lists + all_pkgs = repo.get_all_installed() + kind_map: dict[str, PackageKind] = {p.name: p.kind for p in all_pkgs} + formula_names: list[str] = [ + resolved[n] + for n in names + if kind_map.get(resolved[n]) == PackageKind.FORMULA + ] + + cask_names: list[str] = [ + resolved[n] for n in names if kind_map.get(resolved[n]) == PackageKind.CASK + ] + + failures: Notes = [ + (n, "not found") for n in names if resolved[n] not in kind_map + ] + + else: + formula_names: list[str] = ( + [resolved[n] for n in names] if kind == PackageKind.FORMULA else [] + ) + cask_names: list[str] = ( + [resolved[n] for n in names] if kind == PackageKind.CASK else [] + ) + failures: Notes = [] + + blocked: dict[str, list[str]] = {} + if formula_names: + # Reuse the scan above when there was one; a cask-only batch never scans + source = ( + all_pkgs + if all_pkgs is not None + else repo.cache_mgr.installed_packages(kind=PackageKind.FORMULA) + ) + blocked = blocking_dependents(source, set(formula_names)) + + if blocked: + failures.extend( + (name, f"required by {', '.join(deps)}") for name, deps in blocked.items() + ) + formula_names = [n for n in formula_names if n not in blocked] + + if formula_names: + await run_uninstall(formula_names, formula=formula, env=env) + + if cask_names: + await cask.uninstall(names=cask_names) + + repo.cache_mgr.invalidate() + + removed: list[str] = [] + failed: list[str] = [] + + for pkg_names, k in [ + (formula_names, PackageKind.FORMULA), + (cask_names, PackageKind.CASK), + ]: + if not pkg_names: + continue + + r, f = _verify_removed(pkg_names, k, env=env) + removed += r + failed += f + + failures.extend((n, "uninstall failed") for n in failed) + + return removed, failures + + +def _verify_removed( + names: list[str], kind: PackageKind, *, env: BreweryENV +) -> tuple[list[str], list[str]]: + """Return (removed, failed) based on filesystem presence. + + A package counts as installed only while its directory still holds a version. + The lookup is case-insensitive so a mixed-case cask token still finds its + directory on a case-sensitive volume. + + Args: + names: Package names to verify. + kind: Package kind (formula or cask), selecting cellar or caskroom. + env: Brewery environment (paths). + + Returns: + Tuple of (removed, failed) package names. + """ + base_dir = env.cellar if kind == PackageKind.FORMULA else env.caskroom + index: dict[str, Path] = {d.name.casefold(): d for d in child_dirs(base_dir)} + + removed, failed = [], [] + for name in names: + survivor: Path | None = index.get(name.casefold()) + + if survivor is not None and child_dirs(survivor): + failed.append(name) + continue + + if survivor is not None: + # Nothing installed under it; drop the shell so the tree matches + with contextlib.suppress(OSError): + survivor.rmdir() + + removed.append(name) + + return removed, failed diff --git a/tests/integration/_repo_helpers.py b/tests/integration/_repo_helpers.py new file mode 100644 index 0000000..b57f8d3 --- /dev/null +++ b/tests/integration/_repo_helpers.py @@ -0,0 +1,73 @@ +"""Helpers shared by the integration tests that drive a real prefix.""" + +from __future__ import annotations + +from pathlib import Path + + +class _NullSink: + """Stands in for StreamRelocator where the keg is already staged.""" + + def finish(self, keg: Path) -> None: + """Do nothing, as there is nothing staged to relocate. + + Args: + keg: The keg directory, ignored. + """ + + +def _provider_calls(mock_brew, subcommand: str) -> list[tuple[str, ...]]: + """Filter the mock_brew call log to brew invocations of a given subcommand. + + Args: + mock_brew: The mock brew call log. + subcommand: The subcommand to filter by. + + Returns: + A list of tuples representing the filtered brew calls. + """ + return [ + c for c in mock_brew if len(c) >= 2 and c[0] == "brew" and c[1] == subcommand + ] + + +def _add_alias(catalog, alias: str, name: str) -> None: + """Register an alias -> canonical name mapping in the catalog. + + Args: + catalog: The catalog to write to. + alias: The alias a user might type. + name: The canonical formula name it resolves to. + """ + with catalog._conn: + catalog._conn.execute( + "INSERT OR REPLACE INTO alias (alias, name) VALUES (?, ?)", (alias, name) + ) + + +def _install_formula(cellar, name, version="1.0", deps=()) -> Path: + """Write a minimal installed keg + receipt so the scan derives used_by. + + Args: + cellar: The cellar directory to write to + name: The name of the formula + version: The version of the formula (default: "1.0") + deps: The dependencies of the formula (default: ()) + + Returns: + The path to the installed keg + """ + import orjson + + keg = cellar / name / version + keg.mkdir(parents=True) + (keg / "INSTALL_RECEIPT.json").write_bytes( + orjson.dumps( + { + "source": {"tap": "homebrew/core"}, + "runtime_dependencies": [{"full_name": d} for d in deps], + } + ) + ) + + return keg diff --git a/tests/integration/test_repo.py b/tests/integration/test_repo.py index 38c91e0..28795b2 100644 --- a/tests/integration/test_repo.py +++ b/tests/integration/test_repo.py @@ -9,6 +9,7 @@ from brewery.core.repo import Repository import pytest +from _repo_helpers import _add_alias, _NullSink, _provider_calls from brewery.core.models import PackageKind, PackageStatus from brewery.services.pin import pin_packages, unpin_packages @@ -16,32 +17,6 @@ pytestmark = pytest.mark.integration -class _NullSink: - """Stands in for StreamRelocator where the keg is already staged.""" - - def finish(self, keg: Path) -> None: - """Do nothing, as there is nothing staged to relocate. - - Args: - keg: The keg directory, ignored. - """ - - -def _provider_calls(mock_brew, subcommand: str) -> list[tuple[str, ...]]: - """Filter the mock_brew call log to brew invocations of a given subcommand. - - Args: - mock_brew: The mock brew call log. - subcommand: The subcommand to filter by. - - Returns: - A list of tuples representing the filtered brew calls. - """ - return [ - c for c in mock_brew if len(c) >= 2 and c[0] == "brew" and c[1] == subcommand - ] - - def _repo_with_providers(catalog, *, formula=None, cask=None) -> Repository: """Build a Repository with per-test provider backends. @@ -89,48 +64,6 @@ async def _noop(names) -> list[str]: ) -def _add_alias(catalog, alias: str, name: str) -> None: - """Register an alias -> canonical name mapping in the catalog. - - Args: - catalog: The catalog to write to. - alias: The alias a user might type. - name: The canonical formula name it resolves to. - """ - with catalog._conn: - catalog._conn.execute( - "INSERT OR REPLACE INTO alias (alias, name) VALUES (?, ?)", (alias, name) - ) - - -def _install_formula(cellar, name, version="1.0", deps=()) -> Path: - """Write a minimal installed keg + receipt so the scan derives used_by. - - Args: - cellar: The cellar directory to write to - name: The name of the formula - version: The version of the formula (default: "1.0") - deps: The dependencies of the formula (default: ()) - - Returns: - The path to the installed keg - """ - import orjson - - keg = cellar / name / version - keg.mkdir(parents=True) - (keg / "INSTALL_RECEIPT.json").write_bytes( - orjson.dumps( - { - "source": {"tap": "homebrew/core"}, - "runtime_dependencies": [{"full_name": d} for d in deps], - } - ) - ) - - return keg - - class TestGetAllInstalled: """Tests for the get_all_installed method.""" @@ -364,215 +297,6 @@ async def test_install_via_alias_verified_by_canonical_name(self, repo) -> None: assert failures == [] -class TestUninstall: - """Tests for Repository.uninstall_packages.""" - - async def test_uninstall_still_present_is_failure(self, repo, monkeypatch) -> None: - """Test that a package still on disk after native & fallback uninstall is a failure. - - The mock does not delete the keg, so _verify_removed sees it still present - and reports failure rather than a phantom success. - """ - import brewery.providers.uninstall_service as svc - - def _boom(*a, **k) -> None: - """Raise OSError to simulate native uninstall failure. - - Args: - *a: Positional arguments - **k: Keyword arguments - - Raises: - OSError: Always raised to simulate native uninstall failure - """ - raise OSError("native failed") - - monkeypatch.setattr(svc, "remove_rack", _boom) - - # mock_brew logs but does not delete the keg, so _verify_removed sees it - removed, failures = await repo.uninstall_packages( - ["yazi"], kind=PackageKind.FORMULA - ) - assert removed == [] - assert failures == [("yazi", "uninstall failed")] - - async def test_uninstall_removed_package_is_success(self, repo, mock_env) -> None: - """Test that a keg removed during uninstall verifies as removed.""" - import shutil - - shutil.rmtree(mock_env.cellar / "yazi") - removed, failures = await repo.uninstall_packages( - ["yazi"], kind=PackageKind.FORMULA - ) - assert "yazi" in removed - assert failures == [] - - async def test_unknown_kind_resolves_via_installed(self, catalog, mock_env) -> None: - """Test that kind=None resolves each name's kind from installed state and - routes them to the correct backend: formula -> native, cask -> provider""" - import shutil - - async def mock_cask_uninstall(names) -> list[str]: - """Simulate brew uninstall removing the keg during the operation. - - Args: - names: The names to operate on. - - Returns: - The names unchanged. - """ - for name in names: - shutil.rmtree(mock_env.caskroom / name, ignore_errors=True) - - return names - - repo = _repo_with_providers(catalog, cask=mock_cask_uninstall) - removed, failures = await repo.uninstall_packages(["yazi", "iina"]) - assert len(removed) == 2 - assert failures == [] - - async def test_unknown_kind_not_installed_is_not_found(self, repo) -> None: - """Test that an uninstall target that is not installed is 'not found'.""" - removed, failures = await repo.uninstall_packages(["ripgrep"]) - assert removed == [] - assert failures == [("ripgrep", "not found")] - - async def test_uninstall_via_alias_resolves_to_canonical( - self, catalog, mock_env - ) -> None: - """Test that an alias is resolved before kind routing and verification. - - Uninstalling "yazi-cli" (an alias for installed "yazi") must route the - canonical name to the backend and verify its keg, reporting "yazi" removed. - """ - _add_alias(catalog, "yazi-cli", "yazi") - repo = _repo_with_providers(catalog) - removed, failures = await repo.uninstall_packages(["yazi-cli"]) - assert removed == ["yazi"] - assert failures == [] - - async def test_uninstall_routes_formula_native_and_cask_providers( - self, repo, mock_brew, mock_env - ) -> None: - """Test that formulae removed natively, casks routed to brew.""" - await repo.uninstall_packages(["yazi", "iina"]) - assert not (mock_env.cellar / "yazi").exists() # Formula: native - flat = [a for c in _provider_calls(mock_brew, "uninstall") for a in c] - assert "iina" in flat # Cask: brew provider - assert "yazi" not in flat # Formula should not hit brew - - async def test_uninstall_blocked_by_dependent(self, repo, mock_env) -> None: - """Test that a formula required by another installed formula is refused.""" - _install_formula(mock_env.cellar, "openssl") - _install_formula(mock_env.cellar, "curl", deps=["openssl"]) - repo.cache_mgr.invalidate() - removed, failures = await repo.uninstall_packages( - ["openssl"], kind=PackageKind.FORMULA - ) - assert removed == [] - assert failures == [("openssl", "required by curl")] - assert (mock_env.cellar / "openssl").exists() - - async def test_uninstall_both_in_batch_unblocks(self, repo, mock_env) -> None: - """Test that a dependent removed in the same batch does not block the target.""" - _install_formula(mock_env.cellar, "openssl") - _install_formula(mock_env.cellar, "curl", deps=["openssl"]) - repo.cache_mgr.invalidate() - removed, failures = await repo.uninstall_packages( - ["openssl", "curl"], kind=PackageKind.FORMULA - ) - assert len(removed) == 2 - assert failures == [] - assert not (mock_env.cellar / "openssl").exists() - - async def test_uninstall_lists_multiple_dependents(self, repo, mock_env) -> None: - """Test that multiple dependents are reported sorted and comma-joined.""" - _install_formula(mock_env.cellar, "openssl") - _install_formula(mock_env.cellar, "curl", deps=["openssl"]) - _install_formula(mock_env.cellar, "wget", deps=["openssl"]) - repo.cache_mgr.invalidate() - _, failures = await repo.uninstall_packages( - ["openssl"], kind=PackageKind.FORMULA - ) - assert failures == [("openssl", "required by curl, wget")] - - async def test_uninstall_removes_keg_natively( - self, repo, mock_brew, mock_env - ) -> None: - """Test that Formula uninstall removes the keg via the native path, not brew.""" - removed, _ = await repo.uninstall_packages(["yazi"], kind=PackageKind.FORMULA) - assert "yazi" in removed - assert not (mock_env.cellar / "yazi").exists() - assert _provider_calls(mock_brew, "uninstall") == [] - - async def test_uninstall_falls_back_to_brew( - self, repo, mock_brew, monkeypatch - ) -> None: - """Test that a native failure falls back to brew uninstall for that formula.""" - import brewery.providers.uninstall_service as svc - - def _boom(*a, **k) -> None: - """Raise OSError to simulate native uninstall failure. - - Args: - *a: Positional arguments - **k: Keyword arguments - - Raises: - OSError: Always raised to simulate native uninstall failure - """ - raise OSError("native failed") - - monkeypatch.setattr(svc, "remove_rack", _boom) - await repo.uninstall_packages(["yazi"], kind=PackageKind.FORMULA) - assert _provider_calls(mock_brew, "uninstall") - - -class TestVerifyRemoved: - """Tests for Repository._verify_removed's definition of "still installed".""" - - def test_rack_holding_a_keg_is_a_failure(self, repo, mock_env) -> None: - """Test that a formula whose rack still holds a keg counts as not removed.""" - assert repo._verify_removed(["yazi"], PackageKind.FORMULA) == ([], ["yazi"]) - - def test_absent_rack_is_removed(self, repo, mock_env) -> None: - """Test that a formula with no rack at all counts as removed.""" - assert repo._verify_removed(["ripgrep"], PackageKind.FORMULA) == ( - ["ripgrep"], - [], - ) - - def test_emptied_rack_is_removed_and_pruned(self, repo, mock_env) -> None: - """Test that a rack left behind holding no keg counts as removed. - - `fs_state` reports a rack with no keg directory as not installed, so - reporting it as an uninstall failure would contradict the next `list`. - """ - import shutil - - rack = mock_env.cellar / "yazi" - shutil.rmtree(rack / "26.5.6") - - assert repo._verify_removed(["yazi"], PackageKind.FORMULA) == (["yazi"], []) - - # The empty shell is swept up, so the tree agrees with the verdict - assert not rack.exists() - - def test_cask_token_matches_case_insensitively(self, repo, mock_env) -> None: - """Test that a differently-cased cask token still finds its Caskroom dir.""" - assert repo._verify_removed(["IINA"], PackageKind.CASK) == ([], ["IINA"]) - - def test_cask_token_with_only_metadata_is_removed(self, repo, mock_env) -> None: - """Test that a token directory holding no version directory counts as removed.""" - import shutil - - token = mock_env.caskroom / "iina" - shutil.rmtree(token / "1.4.1,160") - (token / ".metadata").mkdir(exist_ok=True) - - assert repo._verify_removed(["iina"], PackageKind.CASK) == (["iina"], []) - - class TestUpgrade: """Tests for Repository.upgrade_packages.""" diff --git a/tests/integration/test_uninstalling.py b/tests/integration/test_uninstalling.py new file mode 100644 index 0000000..3189756 --- /dev/null +++ b/tests/integration/test_uninstalling.py @@ -0,0 +1,239 @@ +"""Integration tests for the uninstall service over a real prefix.""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest +from _repo_helpers import _add_alias, _install_formula, _provider_calls + +from brewery.core.models import PackageKind +from brewery.core.repo import Repository +from brewery.services.uninstall import _verify_removed, uninstall_packages + +pytestmark = pytest.mark.integration + + +class TestUninstall: + """Tests for the uninstall service.""" + + async def test_uninstall_still_present_is_failure(self, repo, monkeypatch) -> None: + """Test that a package still on disk after native & fallback uninstall is a failure. + + The mock does not delete the keg, so _verify_removed sees it still present + and reports failure rather than a phantom success. + """ + import brewery.providers.uninstall_service as svc + + def _boom(*a, **k) -> None: + """Raise OSError to simulate native uninstall failure. + + Args: + *a: Positional arguments + **k: Keyword arguments + + Raises: + OSError: Always raised to simulate native uninstall failure + """ + raise OSError("native failed") + + monkeypatch.setattr(svc, "remove_rack", _boom) + + # mock_brew logs but does not delete the keg, so _verify_removed sees it + removed, failures = await uninstall_packages( + repo, ["yazi"], kind=PackageKind.FORMULA + ) + assert removed == [] + assert failures == [("yazi", "uninstall failed")] + + async def test_uninstall_removed_package_is_success(self, repo, mock_env) -> None: + """Test that a keg removed during uninstall verifies as removed.""" + import shutil + + shutil.rmtree(mock_env.cellar / "yazi") + removed, failures = await uninstall_packages( + repo, ["yazi"], kind=PackageKind.FORMULA + ) + assert "yazi" in removed + assert failures == [] + + async def test_unknown_kind_resolves_via_installed(self, catalog, mock_env) -> None: + """Test that kind=None resolves each name's kind from installed state and + routes them to the correct backend: formula -> native, cask -> provider""" + import shutil + + async def mock_cask_uninstall(names) -> list[str]: + """Simulate brew uninstall removing the keg during the operation. + + Args: + names: The names to operate on. + + Returns: + The names unchanged. + """ + for name in names: + shutil.rmtree(mock_env.caskroom / name, ignore_errors=True) + + return names + + removed, failures = await uninstall_packages( + Repository(catalog=catalog), + ["yazi", "iina"], + cask=SimpleNamespace(uninstall=mock_cask_uninstall), + ) + assert len(removed) == 2 + assert failures == [] + + async def test_unknown_kind_not_installed_is_not_found(self, repo) -> None: + """Test that an uninstall target that is not installed is 'not found'.""" + removed, failures = await uninstall_packages(repo, ["ripgrep"]) + assert removed == [] + assert failures == [("ripgrep", "not found")] + + async def test_uninstall_via_alias_resolves_to_canonical( + self, catalog, mock_env + ) -> None: + """Test that an alias is resolved before kind routing and verification. + + Uninstalling "yazi-cli" (an alias for installed "yazi") must route the + canonical name to the backend and verify its keg, reporting "yazi" removed. + """ + _add_alias(catalog, "yazi-cli", "yazi") + removed, failures = await uninstall_packages( + Repository(catalog=catalog), ["yazi-cli"] + ) + assert removed == ["yazi"] + assert failures == [] + + async def test_uninstall_routes_formula_native_and_cask_providers( + self, repo, mock_brew, mock_env + ) -> None: + """Test that formulae removed natively, casks routed to brew.""" + await uninstall_packages(repo, ["yazi", "iina"]) + assert not (mock_env.cellar / "yazi").exists() # Formula: native + flat = [a for c in _provider_calls(mock_brew, "uninstall") for a in c] + assert "iina" in flat # Cask: brew provider + assert "yazi" not in flat # Formula should not hit brew + + async def test_uninstall_blocked_by_dependent(self, repo, mock_env) -> None: + """Test that a formula required by another installed formula is refused.""" + _install_formula(mock_env.cellar, "openssl") + _install_formula(mock_env.cellar, "curl", deps=["openssl"]) + repo.cache_mgr.invalidate() + removed, failures = await uninstall_packages( + repo, ["openssl"], kind=PackageKind.FORMULA + ) + assert removed == [] + assert failures == [("openssl", "required by curl")] + assert (mock_env.cellar / "openssl").exists() + + async def test_uninstall_both_in_batch_unblocks(self, repo, mock_env) -> None: + """Test that a dependent removed in the same batch does not block the target.""" + _install_formula(mock_env.cellar, "openssl") + _install_formula(mock_env.cellar, "curl", deps=["openssl"]) + repo.cache_mgr.invalidate() + removed, failures = await uninstall_packages( + repo, ["openssl", "curl"], kind=PackageKind.FORMULA + ) + assert len(removed) == 2 + assert failures == [] + assert not (mock_env.cellar / "openssl").exists() + + async def test_uninstall_lists_multiple_dependents(self, repo, mock_env) -> None: + """Test that multiple dependents are reported sorted and comma-joined.""" + _install_formula(mock_env.cellar, "openssl") + _install_formula(mock_env.cellar, "curl", deps=["openssl"]) + _install_formula(mock_env.cellar, "wget", deps=["openssl"]) + repo.cache_mgr.invalidate() + _, failures = await uninstall_packages( + repo, ["openssl"], kind=PackageKind.FORMULA + ) + assert failures == [("openssl", "required by curl, wget")] + + async def test_uninstall_removes_keg_natively( + self, repo, mock_brew, mock_env + ) -> None: + """Test that Formula uninstall removes the keg via the native path, not brew.""" + removed, _ = await uninstall_packages(repo, ["yazi"], kind=PackageKind.FORMULA) + assert "yazi" in removed + assert not (mock_env.cellar / "yazi").exists() + assert _provider_calls(mock_brew, "uninstall") == [] + + async def test_uninstall_falls_back_to_brew( + self, repo, mock_brew, monkeypatch + ) -> None: + """Test that a native failure falls back to brew uninstall for that formula.""" + import brewery.providers.uninstall_service as svc + + def _boom(*a, **k) -> None: + """Raise OSError to simulate native uninstall failure. + + Args: + *a: Positional arguments + **k: Keyword arguments + + Raises: + OSError: Always raised to simulate native uninstall failure + """ + raise OSError("native failed") + + monkeypatch.setattr(svc, "remove_rack", _boom) + await uninstall_packages(repo, ["yazi"], kind=PackageKind.FORMULA) + assert _provider_calls(mock_brew, "uninstall") + + +class TestVerifyRemoved: + """Tests for _verify_removed's definition of "still installed".""" + + def test_rack_holding_a_keg_is_a_failure(self, repo, mock_env) -> None: + """Test that a formula whose rack still holds a keg counts as not removed.""" + assert _verify_removed(["yazi"], PackageKind.FORMULA, env=mock_env) == ( + [], + ["yazi"], + ) + + def test_absent_rack_is_removed(self, repo, mock_env) -> None: + """Test that a formula with no rack at all counts as removed.""" + assert _verify_removed(["ripgrep"], PackageKind.FORMULA, env=mock_env) == ( + ["ripgrep"], + [], + ) + + def test_emptied_rack_is_removed_and_pruned(self, repo, mock_env) -> None: + """Test that a rack left behind holding no keg counts as removed. + + `fs_state` reports a rack with no keg directory as not installed, so + reporting it as an uninstall failure would contradict the next `list`. + """ + import shutil + + rack = mock_env.cellar / "yazi" + shutil.rmtree(rack / "26.5.6") + + assert _verify_removed(["yazi"], PackageKind.FORMULA, env=mock_env) == ( + ["yazi"], + [], + ) + + # The empty shell is swept up, so the tree agrees with the verdict + assert not rack.exists() + + def test_cask_token_matches_case_insensitively(self, repo, mock_env) -> None: + """Test that a differently-cased cask token still finds its Caskroom dir.""" + assert _verify_removed(["IINA"], PackageKind.CASK, env=mock_env) == ( + [], + ["IINA"], + ) + + def test_cask_token_with_only_metadata_is_removed(self, repo, mock_env) -> None: + """Test that a token directory holding no version directory counts as removed.""" + import shutil + + token = mock_env.caskroom / "iina" + shutil.rmtree(token / "1.4.1,160") + (token / ".metadata").mkdir(exist_ok=True) + + assert _verify_removed(["iina"], PackageKind.CASK, env=mock_env) == ( + ["iina"], + [], + ) From 5bb09d7660d403749acf56fb93a66deb0eca0788 Mon Sep 17 00:00:00 2001 From: Rob Webb Date: Wed, 19 Aug 2026 19:50:10 +0100 Subject: [PATCH 5/8] refactor(services): move upgrade out of Repository into the services layer - upgrade_packages() moves to services/upgrade.py as a free function taking the repo as an argument - Target selection splits out into _select_targets(), so the entry point reads as select -> dispatch -> re-scan rather than one long branch - 'env=' becomes an overridable keyword - 'formula'/'cask' become injectable backends defaulting to 'brew.formula_backend'/'brew.cask_backend' instead of being read off the repo - The result tuple gets the 'UpgradeOutcome' alias and uses the shared 'Notes' alias for advisories/failures - cli/commands/upgrade.py calls the service directly instead of the repo method - pipeline.py drops the module docstring paragraph describing the layering, now stated once in services/__init__.py - 'TestUpgrade' moves from test_repo.py into a new test_upgrading.py --- src/brewery/cli/commands/upgrade.py | 3 +- src/brewery/core/repo.py | 127 ----------- src/brewery/providers/pipeline.py | 8 +- src/brewery/services/upgrade.py | 166 +++++++++++++++ tests/integration/test_repo.py | 299 +------------------------- tests/integration/test_upgrading.py | 316 ++++++++++++++++++++++++++++ 6 files changed, 486 insertions(+), 433 deletions(-) create mode 100644 src/brewery/services/upgrade.py create mode 100644 tests/integration/test_upgrading.py diff --git a/src/brewery/cli/commands/upgrade.py b/src/brewery/cli/commands/upgrade.py index b5ce07b..d83b23c 100644 --- a/src/brewery/cli/commands/upgrade.py +++ b/src/brewery/cli/commands/upgrade.py @@ -17,6 +17,7 @@ from brewery.cli.progress import make_reporter from brewery.core.errors import PinnedPackageWarning from brewery.core.models import Package, PackageKind +from brewery.services.upgrade import upgrade_packages @app.command(aliases=["u", "up"]) @@ -71,7 +72,7 @@ def upgrade( return upgraded, current, advisories, failures = run_async( - coro=repo.upgrade_packages(names, kind, progress=make_reporter(console)) + coro=upgrade_packages(repo, names, kind, progress=make_reporter(console)) ) if not upgraded and not advisories and not failures and not current: diff --git a/src/brewery/core/repo.py b/src/brewery/core/repo.py index bd6a3fd..e3c7b9a 100644 --- a/src/brewery/core/repo.py +++ b/src/brewery/core/repo.py @@ -2,7 +2,6 @@ from __future__ import annotations -from pathlib import Path from typing import TYPE_CHECKING from brewery.core.cache import Cache, CacheManager @@ -180,129 +179,3 @@ async def install_packages( ] return installed, failures - - @log_operation(event_prefix="upgrade_packages", log_args=["names", "kind"]) - async def upgrade_packages( - self, - names: list[str] | None = None, - kind: PackageKind | None = None, - *, - progress: ProgressPort | None = None, - ) -> tuple[ - list[Package], list[Package], list[tuple[str, str]], list[tuple[str, str]] - ]: - """Upgrade packages and report upgraded, up-to-date, advisories, and failures. - - Naming a formula that is already current reports it as up-to-date rather - than reinstalling it. - - Args: - names: Name(s) of the package(s) to upgrade. - kind: Kind of the package(s) (formula, cask, auto (default)) - progress: Optional progress sink for the native pipeline. - - Returns: - Tuple of (upgraded packages, already up-to-date packages, (name, reason) - advisories, (name, reason) failures). - - Raises: - BrewCommandError: Propagated from provider. - """ - installed: list[Package] = self.cache_mgr.installed_packages() - by_name: dict[str, Package] = {p.name: p for p in installed} - advisories: list[tuple[str, str]] = [] - satisfied: list[Package] = [] - - # Resolve the target set and any pinned skips - if names is None: - targets = [p for p in installed if PackageStatus.OUTDATED in p.status] - failures: list[tuple[str, str]] = [] - - # A bulk upgrade skips pins without failing - advisories += [ - (p.name, "pinned - not upgraded") - for p in targets - if PackageStatus.PINNED in p.status - ] - targets = [p for p in targets if PackageStatus.PINNED not in p.status] - - # Upgrade specified - else: - resolved: dict[str, str] = {n: self.catalog.resolve_alias(n) for n in names} - targets = [by_name[resolved[n]] for n in names if resolved[n] in by_name] - failures = [(n, "not found") for n in names if resolved[n] not in by_name] - - # Naming a pinned package explicitly is a failure - failures += [ - (p.name, "pinned - skipped") - for p in targets - if PackageStatus.PINNED in p.status - ] - targets = [p for p in targets if PackageStatus.PINNED not in p.status] - - # The orchestrator forces requested targets past `is_satisfied`, so a - # current formula would otherwise be re-poured in full; casks are - # exempt because nothing derives OUTDATED for them yet - satisfied = [ - p - for p in targets - if p.kind == PackageKind.FORMULA - and PackageStatus.OUTDATED not in p.status - ] - skip = {p.name for p in satisfied} - targets = [p for p in targets if p.name not in skip] - - if kind is not None: - targets = [p for p in targets if p.kind == kind] - satisfied = [p for p in satisfied if p.kind == kind] - - formula_names = [p.name for p in targets if p.kind == PackageKind.FORMULA] - cask_names = [p.name for p in targets if p.kind == PackageKind.CASK] - pre_versions: dict[str, str | None] = { - p.name: (p.versions[0] if p.versions else None) - for p in (*targets, *satisfied) - } - - if formula_names: - from brewery.providers.pipeline import run_upgrade - - old_kegs = { - p.name: Path(p.path) - for p in targets - if p.kind == PackageKind.FORMULA and p.path - } - await run_upgrade( - formula_names, - old_kegs, - catalog=self.catalog, - cache_mgr=self.cache_mgr, - formula=self.formula, - run_brew=run_brew, - progress=progress, - ) - - if cask_names: - await self.cask.upgrade(names=cask_names) - - # Only invalidate the cache if something actually changed - if formula_names or cask_names: - self.cache_mgr.invalidate() - - post: dict[str, Package] = { - p.name: p for p in self.cache_mgr.installed_packages() - } - - upgraded: list[Package] = [] - current: list[Package] = [] - for name in formula_names + cask_names + [p.name for p in satisfied]: - pkg = post.get(name) - if pkg is None: - continue - - new_version = pkg.versions[0] if pkg.versions else None - if new_version != pre_versions.get(name): - upgraded.append(pkg) - else: - current.append(pkg) - - return upgraded, current, advisories, failures diff --git a/src/brewery/providers/pipeline.py b/src/brewery/providers/pipeline.py index 811973a..94a57c1 100644 --- a/src/brewery/providers/pipeline.py +++ b/src/brewery/providers/pipeline.py @@ -1,10 +1,4 @@ -"""Assemble and run the native bottle pipeline for a set of formulae. - -Install and upgrade share one Orchestrator assembly; only the terminal call -differs. Nothing here knows about `Repository` -- the ports it needs are passed -in explicitly, so the command policy that chooses the formulae lives a layer up -in `brewery.services`. -""" +"""Assemble and run the native bottle pipeline for a set of formulae.""" from __future__ import annotations diff --git a/src/brewery/services/upgrade.py b/src/brewery/services/upgrade.py new file mode 100644 index 0000000..fdfef50 --- /dev/null +++ b/src/brewery/services/upgrade.py @@ -0,0 +1,166 @@ +"""Upgrade formulae and casks, reporting what moved and what was already current.""" + +from __future__ import annotations + +from pathlib import Path +from typing import TYPE_CHECKING + +from brewery.core.config import BreweryENV +from brewery.core.decorators import log_operation +from brewery.core.models import Notes, Package, PackageKind, PackageStatus +from brewery.core.repo import Repository +from brewery.core.shell import run_brew +from brewery.providers import brew +from brewery.providers.base import PackageBackend +from brewery.providers.pipeline import run_upgrade + +if TYPE_CHECKING: + from brewery.providers.orchestrator import ProgressPort + +UpgradeOutcome = tuple[list[Package], list[Package], Notes, Notes] + + +def _select_targets( + repo: Repository, installed: list[Package], names: list[str] | None +) -> tuple[list[Package], list[Package], Notes, Notes]: + """Resolve the packages to upgrade, and what to say about the ones skipped. + + Args: + repo: The data facade, for alias resolution. + installed: Every installed package, already merged. + names: Explicit targets, or None for "everything outdated". + + Returns: + Tuple of (targets, already-current packages, advisories, failures). + """ + advisories: Notes = [] + + # Bulk upgrade: everything outdated, skipping pins without failing + if names is None: + targets = [p for p in installed if PackageStatus.OUTDATED in p.status] + advisories += [ + (p.name, "pinned - not upgraded") + for p in targets + if PackageStatus.PINNED in p.status + ] + + return ( + [p for p in targets if PackageStatus.PINNED not in p.status], + [], + advisories, + [], + ) + + # Named upgrade + by_name: dict[str, Package] = {p.name: p for p in installed} + resolved: dict[str, str] = {n: repo.catalog.resolve_alias(n) for n in names} + targets = [by_name[resolved[n]] for n in names if resolved[n] in by_name] + failures: Notes = [(n, "not found") for n in names if resolved[n] not in by_name] + + # Naming a pinned package explicitly is a failure + failures += [ + (p.name, "pinned - skipped") + for p in targets + if PackageStatus.PINNED in p.status + ] + targets = [p for p in targets if PackageStatus.PINNED not in p.status] + + # The orchestrator forces requested targets past `is_satisfied`, so a current + # formula would otherwise be re-poured in full; casks are exempt because + # nothing derives OUTDATED for them yet (see dev/open.md #3 -- revisit this + # line when cask outdated lands) + satisfied = [ + p + for p in targets + if p.kind == PackageKind.FORMULA and PackageStatus.OUTDATED not in p.status + ] + skip = {p.name for p in satisfied} + + return [p for p in targets if p.name not in skip], satisfied, advisories, failures + + +@log_operation(event_prefix="upgrade_packages", log_args=["names", "kind"]) +async def upgrade_packages( + repo: Repository, + names: list[str] | None = None, + kind: PackageKind | None = None, + *, + env: BreweryENV | None = None, + formula: PackageBackend = brew.formula_backend, + cask: PackageBackend = brew.cask_backend, + progress: ProgressPort | None = None, +) -> UpgradeOutcome: + """Upgrade packages and report upgraded, up-to-date, advisories, and failures. + + Naming a formula that is already current reports it as up-to-date rather + than reinstalling it. + + Args: + repo: The data facade to read installed state and aliases through. + names: Name(s) of the package(s) to upgrade. + kind: Kind of the package(s) (formula, cask, auto (default)) + env: Brewery environment (paths), resolved by the pipeline if omitted. + formula: Formula backend for the per-formula brew fallback. + cask: Cask backend, which handles casks wholesale. + progress: Optional progress sink for the native pipeline. + + Returns: + Tuple of (upgraded packages, already up-to-date packages, (name, reason) + advisories, (name, reason) failures). + + Raises: + BrewCommandError: Propagated from provider. + """ + installed: list[Package] = repo.cache_mgr.installed_packages() + targets, satisfied, advisories, failures = _select_targets(repo, installed, names) + + if kind is not None: + targets = [p for p in targets if p.kind == kind] + satisfied = [p for p in satisfied if p.kind == kind] + + formula_names = [p.name for p in targets if p.kind == PackageKind.FORMULA] + cask_names = [p.name for p in targets if p.kind == PackageKind.CASK] + pre_versions: dict[str, str | None] = { + p.name: (p.versions[0] if p.versions else None) for p in (*targets, *satisfied) + } + + if formula_names: + old_kegs = { + p.name: Path(p.path) + for p in targets + if p.kind == PackageKind.FORMULA and p.path + } + await run_upgrade( + formula_names, + old_kegs, + catalog=repo.catalog, + cache_mgr=repo.cache_mgr, + formula=formula, + run_brew=run_brew, + env=env, + progress=progress, + ) + + if cask_names: + await cask.upgrade(names=cask_names) + + # Only invalidate the cache if something actually changed + if formula_names or cask_names: + repo.cache_mgr.invalidate() + + post: dict[str, Package] = {p.name: p for p in repo.cache_mgr.installed_packages()} + + upgraded: list[Package] = [] + current: list[Package] = [] + for name in formula_names + cask_names + [p.name for p in satisfied]: + pkg = post.get(name) + if pkg is None: + continue + + new_version = pkg.versions[0] if pkg.versions else None + if new_version != pre_versions.get(name): + upgraded.append(pkg) + else: + current.append(pkg) + + return upgraded, current, advisories, failures diff --git a/tests/integration/test_repo.py b/tests/integration/test_repo.py index 28795b2..1b30327 100644 --- a/tests/integration/test_repo.py +++ b/tests/integration/test_repo.py @@ -2,17 +2,15 @@ from __future__ import annotations -from pathlib import Path from typing import TYPE_CHECKING if TYPE_CHECKING: from brewery.core.repo import Repository import pytest -from _repo_helpers import _add_alias, _NullSink, _provider_calls +from _repo_helpers import _add_alias, _provider_calls from brewery.core.models import PackageKind, PackageStatus -from brewery.services.pin import pin_packages, unpin_packages pytestmark = pytest.mark.integration @@ -295,298 +293,3 @@ async def test_install_via_alias_verified_by_canonical_name(self, repo) -> None: ) assert [p.name for p in installed] == ["yazi"] assert failures == [] - - -class TestUpgrade: - """Tests for Repository.upgrade_packages.""" - - async def test_upgrade_all_targets_outdated(self, repo, mock_brew) -> None: - """Test that an upgrade with no names targets the outdated set. - - act is the only outdated package, so it is the upgrade target. - """ - await repo.upgrade_packages() - upgrades = _provider_calls(mock_brew, "upgrade") - flat = [arg for call in upgrades for arg in call] - assert "act" in flat - assert "yazi" not in flat # Up-to-date, not targeted - - async def test_upgrade_named_package(self, repo, mock_brew) -> None: - """Test that a named upgrade routes that package to the provider.""" - await repo.upgrade_packages(["act"]) - flat = [arg for call in _provider_calls(mock_brew, "upgrade") for arg in call] - assert "act" in flat - - async def test_named_up_to_date_formula_is_not_repoured( - self, repo, mock_brew - ) -> None: - """Test that naming a current formula reports it instead of re-pouring it. - - yazi is up to date, so it must never reach the pipeline or the provider. - """ - upgraded, current, _advisories, failures = await repo.upgrade_packages(["yazi"]) - flat = [arg for call in _provider_calls(mock_brew, "upgrade") for arg in call] - assert "yazi" not in flat - assert upgraded == [] - assert failures == [] - assert [p.name for p in current] == ["yazi"] - - async def test_named_outdated_formula_still_upgrades(self, repo, mock_brew) -> None: - """Test that the up-to-date filter leaves an outdated named target alone. - - Both are still reported: the mock provider changes no version on disk, so - act comes back as current rather than upgraded. - """ - _upgraded, current, _advisories, _failures = await repo.upgrade_packages( - ["act", "yazi"] - ) - flat = [arg for call in _provider_calls(mock_brew, "upgrade") for arg in call] - assert "act" in flat - assert "yazi" not in flat - assert sorted(p.name for p in current) == ["act", "yazi"] - - async def test_upgrade_unknown_name_is_failure(self, repo) -> None: - """Test that upgrading a non-installed name is reported as not found.""" - upgraded, _current, _advisories, failures = await repo.upgrade_packages( - ["ripgrep"] - ) - assert upgraded == [] - assert failures == [("ripgrep", "not found")] - - async def test_pinned_package_skipped_on_upgrade_all(self, repo) -> None: - """Test that a pinned outdated package is skipped, not upgraded. - - Pinning act (which is outdated) should move it to advisories with a - 'pinned' reason and keep it out of the upgrade targets. A bulk upgrade - skips pins without failing, matching `brew upgrade`. - """ - assert pin_packages(repo, ["act"])[0] == ["act"] - - upgraded, _current, advisories, failures = await repo.upgrade_packages() - assert ("act", "pinned - not upgraded") in advisories - assert failures == [] - assert all(p.name != "act" for p in upgraded) - - async def test_pinned_named_package_skipped_on_upgrade( - self, repo, mock_brew - ) -> None: - """Test that an explicitly named pinned package is refused, not upgraded. - - Naming a pinned package is an error, unlike skipping it in a bulk upgrade. - """ - assert pin_packages(repo, ["act"])[0] == ["act"] - - upgraded, _current, _advisories, failures = await repo.upgrade_packages(["act"]) - assert ("act", "pinned - skipped") in failures - assert all(p.name != "act" for p in upgraded) - - flat = [arg for call in _provider_calls(mock_brew, "upgrade") for arg in call] - assert "act" not in flat - - async def test_unpinned_package_upgrades_again(self, repo) -> None: - """Test that unpinning restores a package to the upgrade targets.""" - pin_packages(repo, ["act"]) - assert unpin_packages(repo, ["act"])[0] == ["act"] - - upgraded, _current, advisories, _failures = await repo.upgrade_packages() - assert ("act", "pinned - not upgraded") not in advisories - assert all(p.name != "act" for p in upgraded) - - async def test_upgrade_detects_version_change( - self, mock_brew, catalog, mock_env - ) -> None: - """Test that a version bump on the mock fs is reported as upgraded. - - Simulating brew replacing act 0.2.88 with 0.2.89 during the upgrade makes - the post-upgrade re-scan see a new version, classifying it as upgraded - rather than current. The swap happens inside an injected mock provider so - act is still present (at 0.2.88) when the pre-upgrade snapshot is taken. - """ - import shutil - - import orjson - - async def mock_formula_upgrade(names) -> list[str]: - """Simulate brew upgrade replacing the keg with a new version. - - Args: - names: The names to operate on. - - Returns: - The names unchanged. - """ - act_dir = mock_env.cellar / "act" - shutil.rmtree(act_dir) - new_keg = act_dir / "0.2.89" - new_keg.mkdir(parents=True) - (new_keg / "INSTALL_RECEIPT.json").write_bytes( - orjson.dumps({"source": {"tap": "homebrew/core"}}) - ) - - return names - - repo = _repo_with_providers(catalog, formula=mock_formula_upgrade) - upgraded, current, _advisories, _failures = await repo.upgrade_packages(["act"]) - assert [p.name for p in upgraded] == ["act"] - assert current == [] - - async def test_kind_filter_limits_targets(self, repo, mock_brew) -> None: - """Test that a kind filter restricts which providers are invoked. - - Upgrading with kind=CASK and no outdated casks should invoke no formula - upgrade for the outdated formula act. - """ - await repo.upgrade_packages(kind=PackageKind.CASK) - flat = [arg for call in _provider_calls(mock_brew, "upgrade") for arg in call] - assert "act" not in flat - - async def test_native_upgrade_bumps_version_and_retains_old( - self, brew, empty_catalog, monkeypatch - ) -> None: - """Test that a native upgrade links the new version and keeps the old as a stamped stale keg.""" - from pathlib import Path - - import orjson - - import brewery.providers.orchestrator as orch_mod - import brewery.providers.pipeline as install_svc - from brewery.core import config - from brewery.core.repo import Repository - from brewery.core.shell import BrewResult - from brewery.providers.manifest import BottleTabInfo - - # Installed state: wget 1.0, opt -> 1.0, minimal receipt - brew.formula( - "wget", - "1.0", - receipt={ - "source": {"tap": "homebrew/core"}, - "runtime_dependencies": [], - "installed_on_request": True, - }, - link_opt=True, - ) - monkeypatch.setattr(config, "_env_cache", brew.env) - - # Catalog: wget 2.0 WITH bottle fields - empty_catalog.write_formulae( - [ - { - "name": "wget", - "desc": None, - "homepage": None, - "tap": "homebrew/core", - "version": "2.0", - "revision": 0, - "version_scheme": 0, - "keg_only": 0, - "has_service": 0, - "post_install": 0, - "bottle_url": "https://ghcr.io/v2/homebrew/core/wget/blobs/sha256:dead", - "bottle_sha256": "d" * 64, - "bottle_cellar": ":any_skip_relocation", - "bottle_rebuild": 0, - "deprecated": 0, - "disabled": 0, - } - ], - [], - [], - ) - - # The keg the mocked download+extract hands back. - staged = brew.prefix.parent / "staged_wget" - (staged / "bin").mkdir(parents=True) - (staged / "bin" / "wget").write_text("v2") - - class MockDownloader: - def __init__(self, cache_dir, client): # build_orchestrator's call shape - """Initialise the mock downloader with a cache directory and client.""" - - async def fetch(self, ref, *, on_progress=None) -> Path: - """Return a mock Path for the wget tarball. - - Args: - ref: The reference to fetch (unused). - on_progress: Optional progress callback (unused). - - Returns: - A Path object pointing to the fake wget tarball. - """ - return Path("/fake/wget.tar.gz") - - async def mock_tab( - client, *, name, version, bottle_sha256, revision, rebuild - ) -> BottleTabInfo: - """Return a mock BottleTabInfo for the wget package. - - Args: - name: The package name. - version: The package version. - bottle_sha256: The SHA-256 hash of the bottle. - revision: The revision number. - rebuild: Whether the bottle needs to be rebuilt. - - Returns: - A BottleTabInfo object with mock data for the wget package. - """ - return BottleTabInfo( - homebrew_version="5.1", - changed_files=[], - source_modified_time=1, - compiler="clang", - runtime_dependencies=[], - arch="x86_64", - built_on={"os": "Macintosh"}, - path_exec_files=[], - installed_size=None, - ) - - monkeypatch.setattr(install_svc, "Downloader", MockDownloader) - monkeypatch.setattr(install_svc, "fetch_bottle_tab", mock_tab) - monkeypatch.setattr( - orch_mod, "extract_bottle", lambda bp, st, *, sink=None: staged - ) - monkeypatch.setattr(orch_mod, "StreamRelocator", lambda **kw: _NullSink()) - - # Defensive: a stray fallback must never reach the real brew binary - async def no_brew(args, *, output=None, check=None): - """Raise an exception to ensure no real brew call is made. - - Args: - args: The command-line arguments for the brew call. - output: The output file path (unused). - check: Whether to raise an exception on non-zero return code (unused). - - Returns: - A BrewResult object with empty stdout/stderr and returncode 0. - """ - return BrewResult(stdout="", stderr="", returncode=0) - - monkeypatch.setattr("brewery.providers.brew.run_brew", no_brew) - - repo = Repository(catalog=empty_catalog) - upgraded, _current, _advisories, failures = await repo.upgrade_packages( - ["wget"] - ) - - # Version bump reported - assert [p.name for p in upgraded] == ["wget"] - assert upgraded[0].versions[0] == "2.0" - assert failures == [] - - # New keg linked; opt and the prefix link point at 2.0 - new = brew.cellar / "wget" / "2.0" - assert new.exists() - assert Path((brew.prefix / "opt" / "wget").resolve()) == new - assert Path((brew.prefix / "bin" / "wget").resolve()) == new / "bin" / "wget" - - # Old keg retained as a stale version and stamped for cleanup - old = brew.cellar / "wget" / "1.0" - assert old.exists() - sidecar = orjson.loads((old / ".brewery_replaced.json").read_bytes()) - assert sidecar["replaced_by"] == "2.0" - - # The rescan resolves the active version to 2.0 (1.0 is now a stale version) - pkg = next(p for p in repo.get_all_installed() if p.name == "wget") - assert pkg.versions[0] == "2.0" diff --git a/tests/integration/test_upgrading.py b/tests/integration/test_upgrading.py new file mode 100644 index 0000000..d995d76 --- /dev/null +++ b/tests/integration/test_upgrading.py @@ -0,0 +1,316 @@ +"""Integration tests for the upgrade service over a real prefix.""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest +from _repo_helpers import _NullSink, _provider_calls + +from brewery.core.models import PackageKind +from brewery.core.repo import Repository +from brewery.services.pin import pin_packages, unpin_packages +from brewery.services.upgrade import upgrade_packages + +pytestmark = pytest.mark.integration + + +class TestUpgrade: + """Tests for the upgrade service.""" + + async def test_upgrade_all_targets_outdated(self, repo, mock_brew) -> None: + """Test that an upgrade with no names targets the outdated set. + + act is the only outdated package, so it is the upgrade target. + """ + await upgrade_packages(repo) + upgrades = _provider_calls(mock_brew, "upgrade") + flat = [arg for call in upgrades for arg in call] + assert "act" in flat + assert "yazi" not in flat # Up-to-date, not targeted + + async def test_upgrade_named_package(self, repo, mock_brew) -> None: + """Test that a named upgrade routes that package to the provider.""" + await upgrade_packages(repo, ["act"]) + flat = [arg for call in _provider_calls(mock_brew, "upgrade") for arg in call] + assert "act" in flat + + async def test_named_up_to_date_formula_is_not_repoured( + self, repo, mock_brew + ) -> None: + """Test that naming a current formula reports it instead of re-pouring it. + + yazi is up to date, so it must never reach the pipeline or the provider. + """ + upgraded, current, _advisories, failures = await upgrade_packages( + repo, ["yazi"] + ) + flat = [arg for call in _provider_calls(mock_brew, "upgrade") for arg in call] + assert "yazi" not in flat + assert upgraded == [] + assert failures == [] + assert [p.name for p in current] == ["yazi"] + + async def test_named_outdated_formula_still_upgrades(self, repo, mock_brew) -> None: + """Test that the up-to-date filter leaves an outdated named target alone. + + Both are still reported: the mock provider changes no version on disk, so + act comes back as current rather than upgraded. + """ + _upgraded, current, _advisories, _failures = await upgrade_packages( + repo, ["act", "yazi"] + ) + flat = [arg for call in _provider_calls(mock_brew, "upgrade") for arg in call] + assert "act" in flat + assert "yazi" not in flat + assert sorted(p.name for p in current) == ["act", "yazi"] + + async def test_upgrade_unknown_name_is_failure(self, repo) -> None: + """Test that upgrading a non-installed name is reported as not found.""" + upgraded, _current, _advisories, failures = await upgrade_packages( + repo, ["ripgrep"] + ) + assert upgraded == [] + assert failures == [("ripgrep", "not found")] + + async def test_pinned_package_skipped_on_upgrade_all(self, repo) -> None: + """Test that a pinned outdated package is skipped, not upgraded. + + Pinning act (which is outdated) should move it to advisories with a + 'pinned' reason and keep it out of the upgrade targets. A bulk upgrade + skips pins without failing, matching `brew upgrade`. + """ + assert pin_packages(repo, ["act"])[0] == ["act"] + + upgraded, _current, advisories, failures = await upgrade_packages(repo) + assert ("act", "pinned - not upgraded") in advisories + assert failures == [] + assert all(p.name != "act" for p in upgraded) + + async def test_pinned_named_package_skipped_on_upgrade( + self, repo, mock_brew + ) -> None: + """Test that an explicitly named pinned package is refused, not upgraded. + + Naming a pinned package is an error, unlike skipping it in a bulk upgrade. + """ + assert pin_packages(repo, ["act"])[0] == ["act"] + + upgraded, _current, _advisories, failures = await upgrade_packages( + repo, ["act"] + ) + assert ("act", "pinned - skipped") in failures + assert all(p.name != "act" for p in upgraded) + + flat = [arg for call in _provider_calls(mock_brew, "upgrade") for arg in call] + assert "act" not in flat + + async def test_unpinned_package_upgrades_again(self, repo) -> None: + """Test that unpinning restores a package to the upgrade targets.""" + pin_packages(repo, ["act"]) + assert unpin_packages(repo, ["act"])[0] == ["act"] + + upgraded, _current, advisories, _failures = await upgrade_packages(repo) + assert ("act", "pinned - not upgraded") not in advisories + assert all(p.name != "act" for p in upgraded) + + async def test_upgrade_detects_version_change( + self, mock_brew, catalog, mock_env + ) -> None: + """Test that a version bump on the mock fs is reported as upgraded. + + Simulating brew replacing act 0.2.88 with 0.2.89 during the upgrade makes + the post-upgrade re-scan see a new version, classifying it as upgraded + rather than current. The swap happens inside an injected mock provider so + act is still present (at 0.2.88) when the pre-upgrade snapshot is taken. + """ + import shutil + + import orjson + + async def mock_formula_upgrade(names) -> list[str]: + """Simulate brew upgrade replacing the keg with a new version. + + Args: + names: The names to operate on. + + Returns: + The names unchanged. + """ + act_dir = mock_env.cellar / "act" + shutil.rmtree(act_dir) + new_keg = act_dir / "0.2.89" + new_keg.mkdir(parents=True) + (new_keg / "INSTALL_RECEIPT.json").write_bytes( + orjson.dumps({"source": {"tap": "homebrew/core"}}) + ) + + return names + + upgraded, current, _advisories, _failures = await upgrade_packages( + Repository(catalog=catalog), + ["act"], + formula=SimpleNamespace(upgrade=mock_formula_upgrade), + ) + assert [p.name for p in upgraded] == ["act"] + assert current == [] + + async def test_kind_filter_limits_targets(self, repo, mock_brew) -> None: + """Test that a kind filter restricts which providers are invoked. + + Upgrading with kind=CASK and no outdated casks should invoke no formula + upgrade for the outdated formula act. + """ + await upgrade_packages(repo, kind=PackageKind.CASK) + flat = [arg for call in _provider_calls(mock_brew, "upgrade") for arg in call] + assert "act" not in flat + + async def test_native_upgrade_bumps_version_and_retains_old( + self, brew, empty_catalog, monkeypatch + ) -> None: + """Test that a native upgrade links the new version and keeps the old as a stamped stale keg.""" + from pathlib import Path + + import orjson + + import brewery.providers.orchestrator as orch_mod + import brewery.providers.pipeline as install_svc + from brewery.core import config + from brewery.core.shell import BrewResult + from brewery.providers.manifest import BottleTabInfo + + # Installed state: wget 1.0, opt -> 1.0, minimal receipt + brew.formula( + "wget", + "1.0", + receipt={ + "source": {"tap": "homebrew/core"}, + "runtime_dependencies": [], + "installed_on_request": True, + }, + link_opt=True, + ) + monkeypatch.setattr(config, "_env_cache", brew.env) + + # Catalog: wget 2.0 WITH bottle fields + empty_catalog.write_formulae( + [ + { + "name": "wget", + "desc": None, + "homepage": None, + "tap": "homebrew/core", + "version": "2.0", + "revision": 0, + "version_scheme": 0, + "keg_only": 0, + "has_service": 0, + "post_install": 0, + "bottle_url": "https://ghcr.io/v2/homebrew/core/wget/blobs/sha256:dead", + "bottle_sha256": "d" * 64, + "bottle_cellar": ":any_skip_relocation", + "bottle_rebuild": 0, + "deprecated": 0, + "disabled": 0, + } + ], + [], + [], + ) + + # The keg the mocked download+extract hands back. + staged = brew.prefix.parent / "staged_wget" + (staged / "bin").mkdir(parents=True) + (staged / "bin" / "wget").write_text("v2") + + class MockDownloader: + def __init__(self, cache_dir, client): # build_orchestrator's call shape + """Initialise the mock downloader with a cache directory and client.""" + + async def fetch(self, ref, *, on_progress=None) -> Path: + """Return a mock Path for the wget tarball. + + Args: + ref: The reference to fetch (unused). + on_progress: Optional progress callback (unused). + + Returns: + A Path object pointing to the fake wget tarball. + """ + return Path("/fake/wget.tar.gz") + + async def mock_tab( + client, *, name, version, bottle_sha256, revision, rebuild + ) -> BottleTabInfo: + """Return a mock BottleTabInfo for the wget package. + + Args: + name: The package name. + version: The package version. + bottle_sha256: The SHA-256 hash of the bottle. + revision: The revision number. + rebuild: Whether the bottle needs to be rebuilt. + + Returns: + A BottleTabInfo object with mock data for the wget package. + """ + return BottleTabInfo( + homebrew_version="5.1", + changed_files=[], + source_modified_time=1, + compiler="clang", + runtime_dependencies=[], + arch="x86_64", + built_on={"os": "Macintosh"}, + path_exec_files=[], + installed_size=None, + ) + + monkeypatch.setattr(install_svc, "Downloader", MockDownloader) + monkeypatch.setattr(install_svc, "fetch_bottle_tab", mock_tab) + monkeypatch.setattr( + orch_mod, "extract_bottle", lambda bp, st, *, sink=None: staged + ) + monkeypatch.setattr(orch_mod, "StreamRelocator", lambda **kw: _NullSink()) + + # Defensive: a stray fallback must never reach the real brew binary + async def no_brew(args, *, output=None, check=None): + """Raise an exception to ensure no real brew call is made. + + Args: + args: The command-line arguments for the brew call. + output: The output file path (unused). + check: Whether to raise an exception on non-zero return code (unused). + + Returns: + A BrewResult object with empty stdout/stderr and returncode 0. + """ + return BrewResult(stdout="", stderr="", returncode=0) + + monkeypatch.setattr("brewery.providers.brew.run_brew", no_brew) + + repo = Repository(catalog=empty_catalog) + upgraded, _current, _advisories, failures = await upgrade_packages( + repo, ["wget"] + ) + + # Version bump reported + assert [p.name for p in upgraded] == ["wget"] + assert upgraded[0].versions[0] == "2.0" + assert failures == [] + + # New keg linked; opt and the prefix link point at 2.0 + new = brew.cellar / "wget" / "2.0" + assert new.exists() + assert Path((brew.prefix / "opt" / "wget").resolve()) == new + assert Path((brew.prefix / "bin" / "wget").resolve()) == new / "bin" / "wget" + + # Old keg retained as a stale version and stamped for cleanup + old = brew.cellar / "wget" / "1.0" + assert old.exists() + sidecar = orjson.loads((old / ".brewery_replaced.json").read_bytes()) + assert sidecar["replaced_by"] == "2.0" + + # The rescan resolves the active version to 2.0 (1.0 is now a stale version) + pkg = next(p for p in repo.get_all_installed() if p.name == "wget") + assert pkg.versions[0] == "2.0" From c5a3b11df18e51a1a39075205605ad90b015ef30 Mon Sep 17 00:00:00 2001 From: Rob Webb Date: Thu, 20 Aug 2026 00:13:56 +0100 Subject: [PATCH 6/8] refactor(services): move install out of Repository, add a cask service seam - install_packages() moves to services/install.py as a free function taking the repo as an argument - 'Repository' is now purely the data facade, with every command-policy method relocated - 'env=' becomes an overridable keyword - 'formula'/'cask' become injectable backends defaulting to 'brew.formula_backend'/'brew.cask_backend', matching the uninstall/upgrade services - The failure list uses the shared 'Notes' alias - New 'services/cask.py' gives the three services one named seam for the 'pass to brew' path - cli/commands/install.py calls the service directly instead of the repo method - 'TestInstall' moves from test_repo.py into a new test_installing.py - The now-unused _repo_with_providers() helper is dropped, since services take their backends as arguments --- src/brewery/cli/commands/install.py | 3 +- src/brewery/core/repo.py | 63 -------------- src/brewery/services/cask.py | 38 +++++++++ src/brewery/services/install.py | 86 +++++++++++++++++++ src/brewery/services/uninstall.py | 3 +- src/brewery/services/upgrade.py | 3 +- tests/integration/test_installing.py | 76 +++++++++++++++++ tests/integration/test_repo.py | 120 +-------------------------- 8 files changed, 207 insertions(+), 185 deletions(-) create mode 100644 src/brewery/services/cask.py create mode 100644 src/brewery/services/install.py create mode 100644 tests/integration/test_installing.py diff --git a/src/brewery/cli/commands/install.py b/src/brewery/cli/commands/install.py index 5cae51b..a722a5c 100644 --- a/src/brewery/cli/commands/install.py +++ b/src/brewery/cli/commands/install.py @@ -16,6 +16,7 @@ from brewery.cli.progress import make_reporter from brewery.core.errors import AlreadyInstalledWarning from brewery.core.models import PackageKind +from brewery.services.install import install_packages @app.command(aliases=["add"]) @@ -48,7 +49,7 @@ def install( with _repository() as repo: installed, failures = run_async( - coro=repo.install_packages(names, target, progress=make_reporter(console)) + coro=install_packages(repo, names, target, progress=make_reporter(console)) ) print_result( diff --git a/src/brewery/core/repo.py b/src/brewery/core/repo.py index e3c7b9a..c6d53b1 100644 --- a/src/brewery/core/repo.py +++ b/src/brewery/core/repo.py @@ -2,8 +2,6 @@ from __future__ import annotations -from typing import TYPE_CHECKING - from brewery.core.cache import Cache, CacheManager from brewery.core.catalog import Catalog from brewery.core.config import BreweryENV @@ -11,12 +9,8 @@ from brewery.core.errors import PackageNotFoundError from brewery.core.logging import BreweryLogger, get_logger from brewery.core.models import Package, PackageKind, PackageStatus -from brewery.core.shell import run_brew from brewery.providers import brew -if TYPE_CHECKING: - from brewery.providers.orchestrator import ProgressPort - log: BreweryLogger = get_logger(name=__name__) @@ -122,60 +116,3 @@ def get_outdated(self) -> list[Package]: packages: list[Package] = self.cache_mgr.installed_packages() return [p for p in packages if PackageStatus.OUTDATED in p.status] - - @log_operation(event_prefix="install_package", log_args=["name", "kind"]) - async def install_packages( - self, - names: list[str], - kind: PackageKind = PackageKind.FORMULA, - *, - progress: ProgressPort | None = None, - ) -> tuple[list[Package], list[tuple[str, str]]]: - """Install a package or packages and return details. - - Args: - names: Name of the package(s) to install. - kind: Kind of the package(s) - formula (default or cask). - progress: Optional progress sink for the native pipeline. - - Returns: - Package(s) details on success. - - Raises: - BrewCommandError: Propagated from provider. - """ - if kind == PackageKind.CASK: - await self.cask.install(names=names) - - else: - from brewery.providers.pipeline import run_install - - await run_install( - names, - catalog=self.catalog, - cache_mgr=self.cache_mgr, - formula=self.formula, - run_brew=run_brew, - progress=progress, - ) - - self.cache_mgr.invalidate() - installed_by_name: dict[str, Package] = { - p.name: p for p in self.cache_mgr.installed_packages(kind=kind) - } - - resolved: dict[str, str] = {n: self.catalog.resolve_alias(n) for n in names} - - installed: list[Package] = [ - installed_by_name[resolved[n]] - for n in names - if resolved[n] in installed_by_name - ] - - failures: list[tuple[str, str]] = [ - (n, "install failed or not found") - for n in names - if resolved[n] not in installed_by_name - ] - - return installed, failures diff --git a/src/brewery/services/cask.py b/src/brewery/services/cask.py new file mode 100644 index 0000000..02be910 --- /dev/null +++ b/src/brewery/services/cask.py @@ -0,0 +1,38 @@ +"""Cask operations, handed to brew wholesale. + +Casks have no native path yet and are passed to brew as a batch. +""" + +from __future__ import annotations + +from brewery.providers.base import InstallBackend, UninstallBackend, UpgradeBackend + + +async def install_casks(names: list[str], *, backend: InstallBackend) -> None: + """Install every named cask. + + Args: + names: Cask tokens to install. + backend: The cask backend to delegate to. + """ + await backend.install(names=names) + + +async def uninstall_casks(names: list[str], *, backend: UninstallBackend) -> None: + """Uninstall every named cask. + + Args: + names: Cask tokens to uninstall. + backend: The cask backend to delegate to. + """ + await backend.uninstall(names=names) + + +async def upgrade_casks(names: list[str], *, backend: UpgradeBackend) -> None: + """Upgrade every named cask. + + Args: + names: Cask tokens to upgrade. + backend: The cask backend to delegate to. + """ + await backend.upgrade(names=names) diff --git a/src/brewery/services/install.py b/src/brewery/services/install.py new file mode 100644 index 0000000..333f6f3 --- /dev/null +++ b/src/brewery/services/install.py @@ -0,0 +1,86 @@ +"""Install formulae and casks, verifying what actually landed in the prefix.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from brewery.core.config import BreweryENV +from brewery.core.decorators import log_operation +from brewery.core.models import Notes, Package, PackageKind +from brewery.core.repo import Repository +from brewery.core.shell import run_brew +from brewery.providers import brew +from brewery.providers.base import PackageBackend +from brewery.providers.pipeline import run_install +from brewery.services.cask import install_casks + +if TYPE_CHECKING: + from brewery.providers.orchestrator import ProgressPort + + +@log_operation(event_prefix="install_package", log_args=["name", "kind"]) +async def install_packages( + repo: Repository, + names: list[str], + kind: PackageKind = PackageKind.FORMULA, + *, + env: BreweryENV | None = None, + formula: PackageBackend = brew.formula_backend, + cask: PackageBackend = brew.cask_backend, + progress: ProgressPort | None = None, +) -> tuple[list[Package], Notes]: + """Install packages and report what the re-scan found. + + Success is decided by the filesystem, not by the backend's exit status: a + name that is absent after the re-scan is a failure even if the install + reported none. + + Args: + repo: The data facade to read installed state and aliases through. + names: Name(s) of the package(s) to install. + kind: Kind of the package(s) - formula (default) or cask. + env: Brewery environment (paths), resolved by the pipeline if omitted. + formula: Formula backend for the per-formula brew fallback. + cask: Cask backend, which handles casks wholesale. + progress: Optional progress sink for the native pipeline. + + Returns: + Tuple of (installed packages, (name, reason) failures). + + Raises: + BrewCommandError: Propagated from provider. + """ + if kind == PackageKind.CASK: + await install_casks(names, backend=cask) + + else: + await run_install( + names, + catalog=repo.catalog, + cache_mgr=repo.cache_mgr, + formula=formula, + run_brew=run_brew, + env=env, + progress=progress, + ) + + repo.cache_mgr.invalidate() + installed_by_name: dict[str, Package] = { + p.name: p for p in repo.cache_mgr.installed_packages(kind=kind) + } + + resolved: dict[str, str] = {n: repo.catalog.resolve_alias(n) for n in names} + + installed: list[Package] = [ + installed_by_name[resolved[n]] + for n in names + if resolved[n] in installed_by_name + ] + + failures: Notes = [ + (n, "install failed or not found") + for n in names + if resolved[n] not in installed_by_name + ] + + return installed, failures diff --git a/src/brewery/services/uninstall.py b/src/brewery/services/uninstall.py index 4d188ce..0e25e3f 100644 --- a/src/brewery/services/uninstall.py +++ b/src/brewery/services/uninstall.py @@ -14,6 +14,7 @@ from brewery.providers import brew from brewery.providers.base import PackageBackend, UninstallBackend from brewery.providers.uninstall_service import run_uninstall +from brewery.services.cask import uninstall_casks @log_operation(event_prefix="uninstall_package", log_args=["name", "kind"]) @@ -94,7 +95,7 @@ async def uninstall_packages( await run_uninstall(formula_names, formula=formula, env=env) if cask_names: - await cask.uninstall(names=cask_names) + await uninstall_casks(cask_names, backend=cask) repo.cache_mgr.invalidate() diff --git a/src/brewery/services/upgrade.py b/src/brewery/services/upgrade.py index fdfef50..d8116c2 100644 --- a/src/brewery/services/upgrade.py +++ b/src/brewery/services/upgrade.py @@ -13,6 +13,7 @@ from brewery.providers import brew from brewery.providers.base import PackageBackend from brewery.providers.pipeline import run_upgrade +from brewery.services.cask import upgrade_casks if TYPE_CHECKING: from brewery.providers.orchestrator import ProgressPort @@ -142,7 +143,7 @@ async def upgrade_packages( ) if cask_names: - await cask.upgrade(names=cask_names) + await upgrade_casks(cask_names, backend=cask) # Only invalidate the cache if something actually changed if formula_names or cask_names: diff --git a/tests/integration/test_installing.py b/tests/integration/test_installing.py new file mode 100644 index 0000000..c731bb0 --- /dev/null +++ b/tests/integration/test_installing.py @@ -0,0 +1,76 @@ +"""Integration tests for the install service over a real prefix.""" + +from __future__ import annotations + +import pytest +from _repo_helpers import _add_alias, _provider_calls + +from brewery.core.models import PackageKind +from brewery.services.install import install_packages + +pytestmark = pytest.mark.integration + + +class TestInstall: + """Tests for the install service.""" + + async def test_install_calls_provider(self, repo, mock_brew) -> None: + """Test that installing a formula not already in the Cellar falls back to brew install.""" + await install_packages(repo, ["ripgrep"], kind=PackageKind.FORMULA) + assert _provider_calls(mock_brew, "install") + + async def test_install_reports_present_package_as_installed(self, repo) -> None: + """Test that a package present on the mock fs is reported installed. + + yazi already exists in the mock Cellar, so after the (mocked) install and + re-scan it is found and returned. + """ + installed, failures = await install_packages( + repo, ["yazi"], kind=PackageKind.FORMULA + ) + assert [p.name for p in installed] == ["yazi"] + assert failures == [] + + async def test_install_reports_absent_package_as_failure(self, repo) -> None: + """Test that a package absent from the fs after install is a failure. + + The mock does not create the keg, so a never-installed name re-scans as + missing and is reported as a failure rather than a success. + """ + installed, failures = await install_packages( + repo, ["ripgrep"], kind=PackageKind.FORMULA + ) + assert installed == [] + assert failures == [("ripgrep", "install failed or not found")] + + async def test_install_appearing_package_is_detected(self, repo, mock_env) -> None: + """Test that a keg created during install is detected on re-scan. + + Simulating brew creating the keg (plus a receipt) makes the package show + up after invalidation, exercising the cache-invalidate-then-rescan path. + """ + import orjson + + keg = mock_env.cellar / "ripgrep" / "14.1.0" + keg.mkdir(parents=True) + (keg / "INSTALL_RECEIPT.json").write_bytes( + orjson.dumps({"source": {"tap": "homebrew/core"}}) + ) + installed, failures = await install_packages( + repo, ["ripgrep"], kind=PackageKind.FORMULA + ) + assert [p.name for p in installed] == ["ripgrep"] + assert failures == [] + + async def test_install_via_alias_verified_by_canonical_name(self, repo) -> None: + """Test that an installed alias verifies against its canonical name. + + Requesting "yazi-cli" (an alias for the present "yazi") must report the + canonical package as installed. + """ + _add_alias(repo.catalog, "yazi-cli", "yazi") + installed, failures = await install_packages( + repo, ["yazi-cli"], kind=PackageKind.FORMULA + ) + assert [p.name for p in installed] == ["yazi"] + assert failures == [] diff --git a/tests/integration/test_repo.py b/tests/integration/test_repo.py index 1b30327..a69d399 100644 --- a/tests/integration/test_repo.py +++ b/tests/integration/test_repo.py @@ -1,67 +1,14 @@ -"""Integration tests for Repository orchestration over catalog + scanner + providers.""" +"""Integration tests for the Repository data facade over catalog + FS cache.""" from __future__ import annotations -from typing import TYPE_CHECKING - -if TYPE_CHECKING: - from brewery.core.repo import Repository - import pytest -from _repo_helpers import _add_alias, _provider_calls from brewery.core.models import PackageKind, PackageStatus pytestmark = pytest.mark.integration -def _repo_with_providers(catalog, *, formula=None, cask=None) -> Repository: - """Build a Repository with per-test provider backends. - - The default brew_formula/brew_cask backends are shared module singletons, so - a test must never mutate repo.formula/repo.cask in place. Injecting fresh - backends via the constructor keeps stateful mocks isolated to one test. - - Args: - catalog: The catalog to use for the repository. - formula: An optional formula backend to use. - cask: An optional cask backend to use. - - Returns: - A Repository instance with the specified backends. - """ - from types import SimpleNamespace - - from brewery.core.repo import Repository - from brewery.providers import brew - - async def _noop(names) -> list[str]: - """Simulate a no-op operation. - - Args: - names: The names to operate on. - - Returns: - The names unchanged. - """ - return names - - formula_backend = SimpleNamespace( - install=_noop, - uninstall=formula or _noop, - upgrade=formula or _noop, - ) - cask_backend = SimpleNamespace( - install=_noop, uninstall=cask or _noop, upgrade=cask or _noop - ) - - return Repository( - catalog=catalog, - formula_backend=formula_backend if formula else brew.formula_backend, - cask_backend=cask_backend if cask else brew.cask_backend, - ) - - class TestGetAllInstalled: """Tests for the get_all_installed method.""" @@ -228,68 +175,3 @@ async def test_no_match_returns_empty(self, repo) -> None: repo: The Repository instance to test with """ assert repo.search("zzzznomatch") == [] - - -class TestInstall: - """Tests for Repository.install_packages.""" - - async def test_install_calls_provider(self, repo, mock_brew) -> None: - """Test that installing a formula not already in the Cellar falls back to brew install.""" - await repo.install_packages(["ripgrep"], kind=PackageKind.FORMULA) - assert _provider_calls(mock_brew, "install") - - async def test_install_reports_present_package_as_installed(self, repo) -> None: - """Test that a package present on the mock fs is reported installed. - - yazi already exists in the mock Cellar, so after the (mocked) install and - re-scan it is found and returned. - """ - installed, failures = await repo.install_packages( - ["yazi"], kind=PackageKind.FORMULA - ) - assert [p.name for p in installed] == ["yazi"] - assert failures == [] - - async def test_install_reports_absent_package_as_failure(self, repo) -> None: - """Test that a package absent from the fs after install is a failure. - - The mock does not create the keg, so a never-installed name re-scans as - missing and is reported as a failure rather than a success. - """ - installed, failures = await repo.install_packages( - ["ripgrep"], kind=PackageKind.FORMULA - ) - assert installed == [] - assert failures == [("ripgrep", "install failed or not found")] - - async def test_install_appearing_package_is_detected(self, repo, mock_env) -> None: - """Test that a keg created during install is detected on re-scan. - - Simulating brew creating the keg (plus a receipt) makes the package show - up after invalidation, exercising the cache-invalidate-then-rescan path. - """ - import orjson - - keg = mock_env.cellar / "ripgrep" / "14.1.0" - keg.mkdir(parents=True) - (keg / "INSTALL_RECEIPT.json").write_bytes( - orjson.dumps({"source": {"tap": "homebrew/core"}}) - ) - installed, failures = await repo.install_packages( - ["ripgrep"], kind=PackageKind.FORMULA - ) - assert [p.name for p in installed] == ["ripgrep"] - assert failures == [] - - async def test_install_via_alias_verified_by_canonical_name(self, repo) -> None: - """Test that an installed alias verifies against its canonical name. - - Requesting "yazi-cli" (an alias for the present "yazi") must report the - canonical package as installed. - """ - _add_alias(repo.catalog, "yazi-cli", "yazi") - installed, failures = await repo.install_packages( - ["yazi-cli"], kind=PackageKind.FORMULA - ) - assert [p.name for p in installed] == ["yazi"] - assert failures == [] From 88ee7cf7a836842bf7c1f21272dec5d2c232c08a Mon Sep 17 00:00:00 2001 From: Rob Webb Date: Thu, 20 Aug 2026 14:48:24 +0100 Subject: [PATCH 7/8] refactor: enforce the one-way layering rule with import-linter - Add 'import-linter' as a dev dependency with two contracts in pyproject.toml - An exhaustive 'layers' contract pinning 'cli' > 'daemon' > 'services' > 'providers' > 'core', and a 'forbidden' contract keeping 'providers' off 'repo' - Wire 'lint-imports' into the 'just layers' recipe, 'just check', the pre-commit hooks, and the CI check job - 'Repository' drops its 'formula_backend'/'cask_backend' constructor arguments and the 'brewery.providers' import they needed - derive_local_status() moves into merge.py, its only caller, and the now-empty 'analysis' package is removed - run_uninstall() moves into services/uninstall.py alongside its caller, leaving 'providers' with the primitives and none of the policy - remove_rack() now calls the cellar.py's own rmtree() wrapper rather than 'shutil.rmtree' directly - 'log_args' on the install/uninstall services corrected from 'name' to 'names' - test_status.py merges into test_merge.py, and test_uninstall_service.py becomes test_uninstalling.py --- .github/workflows/ci.yaml | 4 +- .pre-commit-config.yaml | 7 ++ Justfile | 6 +- pyproject.toml | 27 +++++ src/brewery/analysis/status.py | 42 ------- src/brewery/core/merge.py | 39 ++++++- src/brewery/core/repo.py | 9 +- .../{analysis => providers}/__init__.py | 0 src/brewery/providers/cellar.py | 2 +- src/brewery/providers/uninstall_service.py | 44 -------- src/brewery/services/__init__.py | 5 +- src/brewery/services/install.py | 2 +- src/brewery/services/uninstall.py | 39 ++++++- tests/integration/test_uninstalling.py | 4 +- tests/unit/test_cellar.py | 14 +++ tests/unit/test_repo.py | 33 ++++++ tests/unit/test_status.py | 2 +- tests/unit/test_uninstall_service.py | 2 +- uv.lock | 105 ++++++++++++++++++ 19 files changed, 277 insertions(+), 109 deletions(-) delete mode 100644 src/brewery/analysis/status.py rename src/brewery/{analysis => providers}/__init__.py (100%) delete mode 100644 src/brewery/providers/uninstall_service.py create mode 100644 tests/unit/test_repo.py diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 41e9ec9..c2480d0 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -14,7 +14,7 @@ concurrency: jobs: check: - name: lint, format & type check + name: lint, format, type & layer check runs-on: macos-latest steps: - uses: actions/checkout@v7 @@ -28,7 +28,7 @@ jobs: - name: Sync dependencies run: uv sync --all-extras - - name: Lint, format & type check + - name: Lint, format, type & layer check run: uv run just check - name: Fail on uncommitted formatting changes diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 7948795..a9515fc 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -38,6 +38,13 @@ repos: entry: uv run ty check src tests language: system types: [python] + # Layering + - id: import-linter + name: Check import layering + entry: uv run lint-imports + language: system + pass_filenames: false + types: [python] # Syntax Checking - id: python-syntax-check name: Check for Python syntax errors diff --git a/Justfile b/Justfile index 3885f94..cf65124 100644 --- a/Justfile +++ b/Justfile @@ -34,8 +34,12 @@ format: type: uv run ty check src tests +# Check the one-way layering rule +layers: + uv run lint-imports + # Check code quality -check: lint format type +check: lint format type layers # Run all pre-commit hooks pre: diff --git a/pyproject.toml b/pyproject.toml index 71a2e48..d890d9f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -56,6 +56,7 @@ br = "brewery.cli.main:main" [project.optional-dependencies] dev = [ "icecream>=2.2.0,<3.0.0", + "import-linter>=2.3,<3.0.0", "prek>=0.4.14,<1.0.0", "pytest>=9.0.3,<10.0.0", "pytest-asyncio>=1.4.0,<2.0.0", @@ -72,6 +73,32 @@ Changelog = "https://github.com/rdawebb/brewery/blob/main/CHANGELOG.md" [tool.uv.build-backend] module-name = "brewery" +[tool.importlinter] +root_packages = ["brewery"] + +# One-way layering rule: a package may import anything below it, never above +[[tool.importlinter.contracts]] +name = "Layers" +type = "layers" +containers = ["brewery"] +layers = [ + "cli", + "daemon", + "services", + "providers", + "core", +] +exhaustive = true +exhaustive_ignores = ["scripts"] + +# Repository is a data facade for the layers above it; providers build on the +# core primitives directly +[[tool.importlinter.contracts]] +name = "Providers use core primitives, not the Repository facade" +type = "forbidden" +source_modules = ["brewery.providers"] +forbidden_modules = ["brewery.core.repo"] + [tool.pytest.ini_options] testpaths = ["tests"] asyncio_mode = "auto" diff --git a/src/brewery/analysis/status.py b/src/brewery/analysis/status.py deleted file mode 100644 index baa9e00..0000000 --- a/src/brewery/analysis/status.py +++ /dev/null @@ -1,42 +0,0 @@ -"""Derive package status from package info dictionary.""" - -from __future__ import annotations - -from brewery.core.models import PackageKind, PackageStatus - - -def derive_local_status( - *, - kind: PackageKind, - head: bool = False, - linked: bool = True, - pinned: bool = False, -) -> PackageStatus: - """Derive the filesystem-knowable half of a package's status. - - Returns only the flags the installed state can answer via filesystem state. - Keyword-only by design: the three flags are all booleans. - - Args: - kind: The package kind. Local flags apply to formulae only. - head: Whether the active keg is a HEAD build. - linked: Whether the formula is linked into the prefix. Defaults to True - so that a formula is not falsely flagged `NOT_LINKED`. - pinned: Whether the formula is pinned. - - Returns: - The locally-derived PackageStatus. - """ - status: PackageStatus = PackageStatus.NONE - - if kind == PackageKind.FORMULA: - if pinned: - status |= PackageStatus.PINNED - - if head: - status |= PackageStatus.HEAD - - if not linked: - status |= PackageStatus.NOT_LINKED - - return status diff --git a/src/brewery/core/merge.py b/src/brewery/core/merge.py index 596f167..6e135f9 100644 --- a/src/brewery/core/merge.py +++ b/src/brewery/core/merge.py @@ -2,7 +2,6 @@ from __future__ import annotations -from brewery.analysis.status import derive_local_status from brewery.core.catalog import CaskRow, Catalog, FormulaRow from brewery.core.fs_state import InstalledRecord from brewery.core.models import ( @@ -14,6 +13,44 @@ from brewery.core.version import PkgVersion +def derive_local_status( + *, + kind: PackageKind, + head: bool = False, + linked: bool = True, + pinned: bool = False, +) -> PackageStatus: + """Derive the filesystem-knowable half of a package's status. + + Returns only the flags the installed state can answer via filesystem state; + the merge layers the catalog-knowable flags on top. + Keyword-only by design: the three flags are all booleans. + + Args: + kind: The package kind. Local flags apply to formulae only. + head: Whether the active keg is a HEAD build. + linked: Whether the formula is linked into the prefix. Defaults to True + so that a formula is not falsely flagged `NOT_LINKED`. + pinned: Whether the formula is pinned. + + Returns: + The locally-derived PackageStatus. + """ + status: PackageStatus = PackageStatus.NONE + + if kind == PackageKind.FORMULA: + if pinned: + status |= PackageStatus.PINNED + + if head: + status |= PackageStatus.HEAD + + if not linked: + status |= PackageStatus.NOT_LINKED + + return status + + def merge_one(record: InstalledRecord, catalog: Catalog) -> Package: """Join a single installed record against the catalog into a Package. diff --git a/src/brewery/core/repo.py b/src/brewery/core/repo.py index c6d53b1..8ccb5d9 100644 --- a/src/brewery/core/repo.py +++ b/src/brewery/core/repo.py @@ -9,21 +9,18 @@ from brewery.core.errors import PackageNotFoundError from brewery.core.logging import BreweryLogger, get_logger from brewery.core.models import Package, PackageKind, PackageStatus -from brewery.providers import brew log: BreweryLogger = get_logger(name=__name__) class Repository: - """Repository for managing package data from various backends.""" + """Read-only access to installed packages and the catalog.""" def __init__( self, cache: Cache | None = None, catalog: Catalog | None = None, cache_mgr: CacheManager | None = None, - formula_backend=brew.formula_backend, - cask_backend=brew.cask_backend, env: BreweryENV | None = None, ) -> None: """Initialise the repository. @@ -32,8 +29,6 @@ def __init__( cache: Optional cache instance. catalog: Optional catalog instance. cache_mgr: Optional cache manager instance. - formula_backend: Backend for formulae. - cask_backend: Backend for casks. env: Optional Brewery environment. """ _cache = cache or Cache(namespace="repository") @@ -41,8 +36,6 @@ def __init__( self.cache_mgr: CacheManager = cache_mgr or CacheManager( _cache, self.catalog, env ) - self.formula = formula_backend - self.cask = cask_backend def close(self) -> None: """Close the catalog connection.""" diff --git a/src/brewery/analysis/__init__.py b/src/brewery/providers/__init__.py similarity index 100% rename from src/brewery/analysis/__init__.py rename to src/brewery/providers/__init__.py diff --git a/src/brewery/providers/cellar.py b/src/brewery/providers/cellar.py index 13f747a..a950ad9 100644 --- a/src/brewery/providers/cellar.py +++ b/src/brewery/providers/cellar.py @@ -117,7 +117,7 @@ def remove_rack(cellar_dir: Path, prefix: Path, name: str) -> None: for keg in sorted(p for p in cellar_dir.iterdir() if p.is_dir()): unlink_keg(keg, prefix=prefix, name=name) # realpath no-ops old kegs - shutil.rmtree(cellar_dir) + rmtree(cellar_dir) def _link_opt(prefix: Path, name: str, version: str) -> Path: diff --git a/src/brewery/providers/uninstall_service.py b/src/brewery/providers/uninstall_service.py deleted file mode 100644 index 4d86295..0000000 --- a/src/brewery/providers/uninstall_service.py +++ /dev/null @@ -1,44 +0,0 @@ -"""Run the native uninstall pipeline for a set of formulae.""" - -from __future__ import annotations - -import asyncio - -from brewery.core.config import BreweryENV, get_brewery_env -from brewery.core.errors import BrewCommandError, OperationInProgressError -from brewery.core.logging import BreweryLogger, get_logger -from brewery.providers.base import UninstallBackend -from brewery.providers.cellar import remove_rack - -log: BreweryLogger = get_logger(name=__name__) - - -async def run_uninstall( - names: list[str], - *, - formula: UninstallBackend, - env: BreweryENV | None = None, -) -> None: - """Unlink + remove each formula's kegs, brew-falling-back per formula. - - Args: - names: Canonical formula names to uninstall. - formula: Formula backend for the per-formula brew fallback. - env: Brewery environment (paths), resolved if omitted. - """ - env = env or get_brewery_env() - for name in names: - try: - await asyncio.to_thread(remove_rack, env.cellar / name, env.prefix, name) - - except OperationInProgressError as exc: - # brew locks the same rack, so falling back to it would fail too; - # the caller's removal verification reports the survivor as a failure - log.warning(event="uninstall_rack_locked", formula=name, error=str(exc)) - - except OSError: - try: - await formula.uninstall(names=[name]) - - except BrewCommandError: - pass # verification reports the survivor as a failure diff --git a/src/brewery/services/__init__.py b/src/brewery/services/__init__.py index 8a09eea..7a9d0ae 100644 --- a/src/brewery/services/__init__.py +++ b/src/brewery/services/__init__.py @@ -1,8 +1,7 @@ """Command-family services: the policy layer between the CLI and the data facade. -The layering runs strictly one way: `cli` -> `services` -> {`core`, `providers`}. -Nothing in `core` or `providers` may import from here; `tests/unit/test_layering.py` -enforces that. +The layering runs strictly one way: `cli` -> `services` -> {`core`, `providers`}, +enforced by import-linter contracts. """ from __future__ import annotations diff --git a/src/brewery/services/install.py b/src/brewery/services/install.py index 333f6f3..6094f19 100644 --- a/src/brewery/services/install.py +++ b/src/brewery/services/install.py @@ -18,7 +18,7 @@ from brewery.providers.orchestrator import ProgressPort -@log_operation(event_prefix="install_package", log_args=["name", "kind"]) +@log_operation(event_prefix="install_package", log_args=["names", "kind"]) async def install_packages( repo: Repository, names: list[str], diff --git a/src/brewery/services/uninstall.py b/src/brewery/services/uninstall.py index 0e25e3f..602f6c8 100644 --- a/src/brewery/services/uninstall.py +++ b/src/brewery/services/uninstall.py @@ -2,22 +2,27 @@ from __future__ import annotations +import asyncio import contextlib from pathlib import Path from brewery.core.config import BreweryENV, get_brewery_env from brewery.core.decorators import log_operation from brewery.core.deps import blocking_dependents +from brewery.core.errors import BrewCommandError, OperationInProgressError from brewery.core.fs_state import child_dirs +from brewery.core.logging import BreweryLogger, get_logger from brewery.core.models import Notes, Package, PackageKind from brewery.core.repo import Repository from brewery.providers import brew from brewery.providers.base import PackageBackend, UninstallBackend -from brewery.providers.uninstall_service import run_uninstall +from brewery.providers.cellar import remove_rack from brewery.services.cask import uninstall_casks +log: BreweryLogger = get_logger(name=__name__) -@log_operation(event_prefix="uninstall_package", log_args=["name", "kind"]) + +@log_operation(event_prefix="uninstall_package", log_args=["names", "kind"]) async def uninstall_packages( repo: Repository, names: list[str], @@ -118,6 +123,36 @@ async def uninstall_packages( return removed, failures +async def run_uninstall( + names: list[str], + *, + formula: UninstallBackend, + env: BreweryENV | None = None, +) -> None: + """Unlink + remove each formula's kegs, brew-falling-back per formula. + + Args: + names: Canonical formula names to uninstall. + formula: Formula backend for the per-formula brew fallback. + env: Brewery environment (paths), resolved if omitted. + """ + env = env or get_brewery_env() + for name in names: + try: + await asyncio.to_thread(remove_rack, env.cellar / name, env.prefix, name) + + except OperationInProgressError as exc: + # brew locks the same rack, so falling back to it would fail too + log.warning(event="uninstall_rack_locked", formula=name, error=str(exc)) + + except OSError: + try: + await formula.uninstall(names=[name]) + + except BrewCommandError: + pass # Verification reports the survivor as a failure + + def _verify_removed( names: list[str], kind: PackageKind, *, env: BreweryENV ) -> tuple[list[str], list[str]]: diff --git a/tests/integration/test_uninstalling.py b/tests/integration/test_uninstalling.py index 3189756..b7feecf 100644 --- a/tests/integration/test_uninstalling.py +++ b/tests/integration/test_uninstalling.py @@ -23,7 +23,7 @@ async def test_uninstall_still_present_is_failure(self, repo, monkeypatch) -> No The mock does not delete the keg, so _verify_removed sees it still present and reports failure rather than a phantom success. """ - import brewery.providers.uninstall_service as svc + import brewery.services.uninstall as svc def _boom(*a, **k) -> None: """Raise OSError to simulate native uninstall failure. @@ -163,7 +163,7 @@ async def test_uninstall_falls_back_to_brew( self, repo, mock_brew, monkeypatch ) -> None: """Test that a native failure falls back to brew uninstall for that formula.""" - import brewery.providers.uninstall_service as svc + import brewery.services.uninstall as svc def _boom(*a, **k) -> None: """Raise OSError to simulate native uninstall failure. diff --git a/tests/unit/test_cellar.py b/tests/unit/test_cellar.py index cc8760d..b65d54e 100644 --- a/tests/unit/test_cellar.py +++ b/tests/unit/test_cellar.py @@ -270,3 +270,17 @@ def test_unlinks_all_versions_then_removes(self, tmp_path, monkeypatch) -> None: remove_rack(cellar, tmp_path / "prefix", "tool") assert sorted(seen) == ["1.0", "2.0"] assert not cellar.exists() + + def test_removes_a_read_only_keg(self, tmp_path, monkeypatch) -> None: + """Bottles ship read-only files, so removal must go through cellar.rmtree.""" + cellar = tmp_path / "Cellar" / "tool" + share = cellar / "1.0" / "share" + share.mkdir(parents=True) + (share / "data").write_text("x") + os.chmod(share / "data", 0o444) + os.chmod(share, 0o555) + + monkeypatch.setattr(_cellar, "unlink_keg", lambda keg, *, prefix, name: None) + remove_rack(cellar, tmp_path / "prefix", "tool") + + assert not cellar.exists() diff --git a/tests/unit/test_repo.py b/tests/unit/test_repo.py new file mode 100644 index 0000000..5311b3b --- /dev/null +++ b/tests/unit/test_repo.py @@ -0,0 +1,33 @@ +"""Unit tests for the Repository facade's public shape.""" + +from __future__ import annotations + +import pytest + +pytestmark = pytest.mark.unit + +# Repository is a data facade; a mutating verb landing here is the regression +REPOSITORY_METHODS = { + "close", + "get_all_installed", + "get_details", + "get_outdated", + "search", +} + + +def test_repository_exposes_no_mutating_verbs() -> None: + """Test that Repository's public surface is still the frozen read-only set. + + Command policy belongs in `brewery.services`; a new public method here means + the facade is growing back into the god object the split removed. + """ + from brewery.core.repo import Repository + + public = { + name + for name in vars(Repository) + if not name.startswith("_") and callable(getattr(Repository, name)) + } + + assert public == REPOSITORY_METHODS diff --git a/tests/unit/test_status.py b/tests/unit/test_status.py index a167ee7..fd0ea29 100644 --- a/tests/unit/test_status.py +++ b/tests/unit/test_status.py @@ -4,7 +4,7 @@ import pytest -from brewery.analysis.status import derive_local_status +from brewery.core.merge import derive_local_status from brewery.core.models import PackageKind, PackageStatus pytestmark = pytest.mark.unit diff --git a/tests/unit/test_uninstall_service.py b/tests/unit/test_uninstall_service.py index 70cbceb..f46596e 100644 --- a/tests/unit/test_uninstall_service.py +++ b/tests/unit/test_uninstall_service.py @@ -2,7 +2,7 @@ from __future__ import annotations -import brewery.providers.uninstall_service as svc +import brewery.services.uninstall as svc from brewery.core.errors import BrewCommandError, OperationInProgressError diff --git a/uv.lock b/uv.lock index a6e44d7..43d4589 100644 --- a/uv.lock +++ b/uv.lock @@ -45,6 +45,7 @@ dependencies = [ [package.optional-dependencies] dev = [ { name = "icecream" }, + { name = "import-linter" }, { name = "prek" }, { name = "pytest" }, { name = "pytest-asyncio" }, @@ -58,6 +59,7 @@ dev = [ requires-dist = [ { name = "httpx", extras = ["brotli", "zstd"], specifier = ">=0.28.0,<1.0.0" }, { name = "icecream", marker = "extra == 'dev'", specifier = ">=2.2.0,<3.0.0" }, + { name = "import-linter", marker = "extra == 'dev'", specifier = ">=2.3,<3.0.0" }, { name = "orjson", specifier = ">=3.12.0,<4.0.0" }, { name = "prek", marker = "extra == 'dev'", specifier = ">=0.4.14,<1.0.0" }, { name = "pygments", specifier = ">=2.20.0" }, @@ -348,6 +350,94 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/c1/ea/53f2148663b321f21b5a606bd5f191517cf40b7072c0497d3c92c4a13b1e/executing-2.2.1-py2.py3-none-any.whl", hash = "sha256:760643d3452b4d777d295bb167ccc74c64a81df23fb5e08eff250c425a4b2017", size = 28317, upload-time = "2025-09-01T09:48:08.5Z" }, ] +[[package]] +name = "grimp" +version = "3.15" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/b2/73/ce58881177b003def779c87b5e10f396deef068933c97d6d206bd46d4cb7/grimp-3.15.tar.gz", hash = "sha256:91b57d4d801dc107ebfb5a7040d4777a152c579b5dc202426e1185e50931fe1e", size = 831734, upload-time = "2026-07-03T12:09:36.244Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/d4/06/dd37dd3e282e90856215ea406415ba0c18cb77cf3150ffa47bd1137c8daf/grimp-3.15-cp311-cp311-macosx_10_12_x86_64.whl", hash = "sha256:cc467bdf890b26545989994c5f91d99fb2b911bbc09d48cebf906082fb2c01d1", size = 2140928, upload-time = "2026-07-03T12:08:48.764Z" }, + { url = "https://files.pythonhosted.org/packages/5b/b0/9329b8df4916cd60a1b358e527fcb9239604a1eca88483deb68ab95d9d5a/grimp-3.15-cp311-cp311-macosx_11_0_arm64.whl", hash = "sha256:571d62c2f2e264ed83a9b283b0c1e322902ea3af8a9ad20d9f7cc5be3049be25", size = 2097876, upload-time = "2026-07-03T12:08:41.571Z" }, + { url = "https://files.pythonhosted.org/packages/91/ff/4352eb0a6549faefeaae1514f2994f6d9148b12db10e49d94935109cd4cd/grimp-3.15-cp311-cp311-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:e0c36369902389e080f708560621f29e2b64957a6bfa5afb06c13b74105039d6", size = 2262242, upload-time = "2026-07-03T12:07:33Z" }, + { url = "https://files.pythonhosted.org/packages/d9/3a/a7ddecd677891070277f92e71bc41c10c423a3317eb412f9636b4cbb32e3/grimp-3.15-cp311-cp311-manylinux_2_17_armv7l.manylinux2014_armv7l.whl", hash = "sha256:44a3a576bb0c5d0754108f0b6cedbeaee66a123112c71a8f4796656c9e52c5cd", size = 2196892, upload-time = "2026-07-03T12:07:43.574Z" }, + { url = "https://files.pythonhosted.org/packages/27/e2/8a7502011e324902853df7daecc3d6d651464920aad93a1a7ad9bd833105/grimp-3.15-cp311-cp311-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:80b833af82a197371c0b0b69ec600413d9b50109fec2f8e1e1317b1c226a3e07", size = 2348919, upload-time = "2026-07-03T12:08:16.589Z" }, + { url = "https://files.pythonhosted.org/packages/93/55/0570ff9ea16c8280de2e84b52006dd588b208716e73794cb332c43d1eb72/grimp-3.15-cp311-cp311-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:6167c3d6e30ac8ff0a32260edf51fad5c22ec77b04e85a8a0042c1ba78bf6846", size = 2609501, upload-time = "2026-07-03T12:07:53.841Z" }, + { url = "https://files.pythonhosted.org/packages/33/14/34482976316834848df5eb68ec2283c13f84bfab12b21b8c11e4f3442e77/grimp-3.15-cp311-cp311-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:e6c18582eb87e148da11ad6549ec006186460ed29959afb15e093c34696d0134", size = 2329567, upload-time = "2026-07-03T12:08:04.663Z" }, + { url = "https://files.pythonhosted.org/packages/78/1b/2803b1234cf5e663c00b9fb2318a93b8db3831942ff053276234fa6731ab/grimp-3.15-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:8c941ed53672f9d136f898bf41557062dc4ed4c4fd10c88e47499e7db1c436ec", size = 2277972, upload-time = "2026-07-03T12:08:29.175Z" }, + { url = "https://files.pythonhosted.org/packages/fa/96/fd25b4a61851db6ca82d4007e1981d9832dff7c34c7b02a975e0bf3da89f/grimp-3.15-cp311-cp311-musllinux_1_2_aarch64.whl", hash = "sha256:4d46417bd6fffa56814b1cf8ffe756fe761d8e5eb691f1d7aaa8a0aba37af05a", size = 2440215, upload-time = "2026-07-03T12:08:56.046Z" }, + { url = "https://files.pythonhosted.org/packages/7c/44/d065613d4cbdc108c33db5978351303cfcedfa2f6d55dcff432c722b3568/grimp-3.15-cp311-cp311-musllinux_1_2_armv7l.whl", hash = "sha256:1f39ca69733dc9cc2f4e7c6647ebbb9a85e1b615b6b808187b0fc6dc6f3a9354", size = 2471902, upload-time = "2026-07-03T12:09:06.502Z" }, + { url = "https://files.pythonhosted.org/packages/1c/01/1eb96d3b2f61d04a5bfe0502216e248cfa0dfe34697cf56ab74e762983d7/grimp-3.15-cp311-cp311-musllinux_1_2_i686.whl", hash = "sha256:dcbea03a4502291b6014530267a6447365557a7870524202034a9ceac4f26fdd", size = 2509411, upload-time = "2026-07-03T12:09:16.434Z" }, + { url = "https://files.pythonhosted.org/packages/d9/b6/89a69f121848b2324403ace3e3487c0074bf1257507b743fbebd281d270f/grimp-3.15-cp311-cp311-musllinux_1_2_x86_64.whl", hash = "sha256:c88386fac08a96933387d4058ef246b191872f7bd75a01c8655b70c8d1a0433a", size = 2519817, upload-time = "2026-07-03T12:09:26.999Z" }, + { url = "https://files.pythonhosted.org/packages/6a/18/9916c71ad9039e74148d9d3b8da4b6cf87900290edc0e7be1a891c840b66/grimp-3.15-cp311-cp311-win32.whl", hash = "sha256:788e1f019052fd6b4dc24ed9892de4f0f24e8e845bd01d146ee549515f70a9bc", size = 1856929, upload-time = "2026-07-03T12:09:46.611Z" }, + { url = "https://files.pythonhosted.org/packages/71/9f/6b349a4aee939b52a244c87e49f584ecad7ca4db67aea70d2d49c7709c26/grimp-3.15-cp311-cp311-win_amd64.whl", hash = "sha256:bfe2d28a94eb97db739382c4d377c8c014020eb6cc1599bcf950c9a2a60fce9d", size = 1988289, upload-time = "2026-07-03T12:09:38.9Z" }, + { url = "https://files.pythonhosted.org/packages/44/66/621abde26d8ece0d34ba611ca94bc62bf8c9c4389760d8909b0d96964878/grimp-3.15-cp312-cp312-macosx_10_12_x86_64.whl", hash = "sha256:915ea140cf55107fd6825c3e9eae2c4fda18aa19b87e8eee05d510b4d44ab928", size = 2143914, upload-time = "2026-07-03T12:08:50.423Z" }, + { url = "https://files.pythonhosted.org/packages/c8/76/a27fff8de84dbf46db9d6da937fe772f04a0e21e057a4863fd30e6fcaa55/grimp-3.15-cp312-cp312-macosx_11_0_arm64.whl", hash = "sha256:0ae2d4d958d871792a9686ad51845e9e1e0886e9db13ecc47a9475899f4b27db", size = 2089296, upload-time = "2026-07-03T12:08:42.802Z" }, + { url = "https://files.pythonhosted.org/packages/e1/3d/fe38a0881ce7e00ef8590745853bccff5d337a943dc0d1d0735b0eb605f9/grimp-3.15-cp312-cp312-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:19a1039d50ed6a9b221f44b32c7b07cb43dda70971e892fe8149995c0e9c1840", size = 2254829, upload-time = "2026-07-03T12:07:34.365Z" }, + { url = "https://files.pythonhosted.org/packages/4b/a2/ef18989048e8f0c92171eabb15dffe9cd72de6404b86e3a37553f7d16dd6/grimp-3.15-cp312-cp312-manylinux_2_17_armv7l.manylinux2014_armv7l.whl", hash = "sha256:5b7b649f2a34278897237670b6650073c9ab0fc1abf56821301bda28cb5c4256", size = 2193553, upload-time = "2026-07-03T12:07:45.06Z" }, + { url = "https://files.pythonhosted.org/packages/85/22/82303539d21068021cc28c526be5e1b1cc0b7a61704c1663909497dd9b8a/grimp-3.15-cp312-cp312-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:020a8875c0cb67f407eb019b7be65b574d49071653f155c243f801aa87a1fd4d", size = 2345941, upload-time = "2026-07-03T12:08:18.082Z" }, + { url = "https://files.pythonhosted.org/packages/bd/20/3c66e7c814ba2b6b0cd230ca2445825c605f28242f4f3a658e5bb9adda73/grimp-3.15-cp312-cp312-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:1f0a0705cf9a10648c4aea71edde8fe3dfbf1cf05bd434ef38c813ad7c544886", size = 2604369, upload-time = "2026-07-03T12:07:55.926Z" }, + { url = "https://files.pythonhosted.org/packages/d5/7d/2f642969950e096a67a43909ba66f33cb4750974e30c2c771e293aeb787c/grimp-3.15-cp312-cp312-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:c36373cd0a2d4c9b53fabefbaa9edcc4c511ad2d335fb1296484f6ca550e4f82", size = 2326619, upload-time = "2026-07-03T12:08:05.931Z" }, + { url = "https://files.pythonhosted.org/packages/a1/99/98d39545e54e239a52a54d8e96752780778b11b5ebc78096dfa090f9d2ac/grimp-3.15-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:477ac0abcd12c697a0bd01b40875605f7f0db97332df6148e4d4057ee3ad199d", size = 2272955, upload-time = "2026-07-03T12:08:30.488Z" }, + { url = "https://files.pythonhosted.org/packages/d6/02/fabd5ae2b12276530f4bae038ffcf3a556ac2c9b9fa271f83fbeb4036a08/grimp-3.15-cp312-cp312-musllinux_1_2_aarch64.whl", hash = "sha256:017e493fee9962d50db6f7a8b5d49ad2ae508484a06078d700adbef30f1ff1f0", size = 2431271, upload-time = "2026-07-03T12:08:57.606Z" }, + { url = "https://files.pythonhosted.org/packages/4d/c8/fa3ec84df9c3ccc2b08177be41a48b76178e9e5773f4471f1caff4fc5c46/grimp-3.15-cp312-cp312-musllinux_1_2_armv7l.whl", hash = "sha256:e77982dd5b0977945034fa328f65cfb62ff589cb0da97a0f6042de78525c5c73", size = 2466937, upload-time = "2026-07-03T12:09:07.857Z" }, + { url = "https://files.pythonhosted.org/packages/0a/7e/52d3acd2bbd6cebdf2ee6546c334f50f6358c25ae58624ae63d2ec3ad30b/grimp-3.15-cp312-cp312-musllinux_1_2_i686.whl", hash = "sha256:d129a8c57b7a19a44e8da94caf38a02753f1c9953aaba8c1a4633747f097164f", size = 2504734, upload-time = "2026-07-03T12:09:17.998Z" }, + { url = "https://files.pythonhosted.org/packages/33/53/ad27750eb8b4a0c3ecb5ca7d78c7230f0f5e814515ed6f8986be527117ff/grimp-3.15-cp312-cp312-musllinux_1_2_x86_64.whl", hash = "sha256:ba25002e92b1792f13391295a1f805d2e09781b846d92ec1a791ff9ed89298b6", size = 2514448, upload-time = "2026-07-03T12:09:28.401Z" }, + { url = "https://files.pythonhosted.org/packages/fe/c0/8cc474a24198c1c2936269c5854be92af41787bd76d3190af584a9cebca7/grimp-3.15-cp312-cp312-win32.whl", hash = "sha256:232bf7a4c7536f62a99478eeb01de63c455261add35ee00c6ec9f04f280f853e", size = 1855806, upload-time = "2026-07-03T12:09:48.396Z" }, + { url = "https://files.pythonhosted.org/packages/91/11/e46139fd43dd5714fae93f09f1c858fb2dc83a575d5f8cb1daf8a15a261b/grimp-3.15-cp312-cp312-win_amd64.whl", hash = "sha256:13be2285e358a7687c0f3b798ac9d4819f275976ad8d651297966e5a75bfafb9", size = 1985556, upload-time = "2026-07-03T12:09:40.786Z" }, + { url = "https://files.pythonhosted.org/packages/1e/6a/3e0a0760cc509cd09764d128de78a1f329740306c74e42dba82c58a99118/grimp-3.15-cp313-cp313-macosx_10_12_x86_64.whl", hash = "sha256:f19b957053d1c736aaa0015eed2d4855bb2511637c5360d32b3c5ea045904e7a", size = 2143197, upload-time = "2026-07-03T12:08:51.685Z" }, + { url = "https://files.pythonhosted.org/packages/c8/2e/127cbce04a5603d382c6a2bbc1a19b6889be49d60b88864bcdb174c8926d/grimp-3.15-cp313-cp313-macosx_11_0_arm64.whl", hash = "sha256:a6480472c2d1f7f6a903906c92e9897d4e3dee5d69ce3881f04368d4ec6d2dde", size = 2088342, upload-time = "2026-07-03T12:08:44.227Z" }, + { url = "https://files.pythonhosted.org/packages/b0/36/af9df683bf6c8711e0e9136876ca130f9971102d945ff3a36d0c45dae2ec/grimp-3.15-cp313-cp313-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:8c14b64dbf2e4397df2e35fa8aa7028533621ecf1f8ea7ec24bc296a9c695ea4", size = 2254509, upload-time = "2026-07-03T12:07:35.809Z" }, + { url = "https://files.pythonhosted.org/packages/02/f5/e712633b68ea14d04e7de84ede4f8ddbba763d5f2d29ae8d6af721f84870/grimp-3.15-cp313-cp313-manylinux_2_17_armv7l.manylinux2014_armv7l.whl", hash = "sha256:26364c2c9f7db88243365299b4d1c4d948b74304ff03c8a6e4f028147fed3b22", size = 2193831, upload-time = "2026-07-03T12:07:46.309Z" }, + { url = "https://files.pythonhosted.org/packages/13/74/151bdd73d6a60bd77d4b956f36d03dd558dd46a9b5ec8f5ad15921b503d7/grimp-3.15-cp313-cp313-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:69331e1be693415596c6084342059b9ba3ecf1cfe2e3b1c761598dd2fe14d522", size = 2345289, upload-time = "2026-07-03T12:08:19.458Z" }, + { url = "https://files.pythonhosted.org/packages/a1/b8/3fc950fa73b757cbc35f77542bd662f431b9a8f360e63196ded640771a33/grimp-3.15-cp313-cp313-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:fe397991c269868c2fb08114099b2aa4f1bc803d03fadaaf97e006019f9e5da2", size = 2604407, upload-time = "2026-07-03T12:07:57.217Z" }, + { url = "https://files.pythonhosted.org/packages/0a/d8/98916b9dc0b89a3e89e0d714ce0872be859fff40ceee3e2cb6886b106eb4/grimp-3.15-cp313-cp313-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:1d556bd62664ee044c47cff64d737be132408064d4ba68ca9f756cb29b41cc2d", size = 2326769, upload-time = "2026-07-03T12:08:08.773Z" }, + { url = "https://files.pythonhosted.org/packages/63/51/39595c5857f609e976e0bba1f19c1f45182ee4d8d2ea5cdfab72841eafbc/grimp-3.15-cp313-cp313-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:9061c5b6f01130ff8639c49c80176d29d921acc36741b2ae0b763a7668106082", size = 2272498, upload-time = "2026-07-03T12:08:32.03Z" }, + { url = "https://files.pythonhosted.org/packages/7d/d8/a44cc9db500ae80c45425238ee44d9556796aa88b8c0649cfa613fb88d14/grimp-3.15-cp313-cp313-musllinux_1_2_aarch64.whl", hash = "sha256:a9d04c195f0c6da4476361560d3d206a6a784b9eb0da7156fc6974511337e2e3", size = 2431075, upload-time = "2026-07-03T12:08:58.935Z" }, + { url = "https://files.pythonhosted.org/packages/a9/41/544f197ddb44990789683c25740ffa72474a57f0b16bc6b4a544edf9a2a9/grimp-3.15-cp313-cp313-musllinux_1_2_armv7l.whl", hash = "sha256:01fd68c74cfd08b110bfd338d77efaa470670530f7179e6977764f44cd74d5cc", size = 2467361, upload-time = "2026-07-03T12:09:09.275Z" }, + { url = "https://files.pythonhosted.org/packages/3b/b1/8261018b9f1ab47fffbb7739d7af3f04c8b529555b7fb2ae8b742d42d3db/grimp-3.15-cp313-cp313-musllinux_1_2_i686.whl", hash = "sha256:bd1ddd428a124d6730bed49ea60fb2ca6c8e0640c8f5abe1d0f6fc27a92fd8bc", size = 2503500, upload-time = "2026-07-03T12:09:19.585Z" }, + { url = "https://files.pythonhosted.org/packages/db/d6/99a2421c4c0de9f203a7e246030b13b676766e62e48504883863942646fb/grimp-3.15-cp313-cp313-musllinux_1_2_x86_64.whl", hash = "sha256:07e645e9d45ed43bb8ea8da5982c19eacf13ee685d8440e3dc280064c005599c", size = 2513834, upload-time = "2026-07-03T12:09:29.758Z" }, + { url = "https://files.pythonhosted.org/packages/62/a5/263729e23d64541cd99d36dbb592e29c1ecea803deb3bb5c0c463b43c2ec/grimp-3.15-cp313-cp313-win32.whl", hash = "sha256:473646e0a74a554b4ab071d7fcbf5f442eb8cf87561770dae269818636b8edf2", size = 1855743, upload-time = "2026-07-03T12:09:49.789Z" }, + { url = "https://files.pythonhosted.org/packages/1c/8c/a072bbea2e2e94da38f90bd8794037c90fb4eba389b49d01cdd2bb85e13c/grimp-3.15-cp313-cp313-win_amd64.whl", hash = "sha256:dbc2c15a1fbca2ff358f86cc90067176096dd73bec27d002515521b3125ba507", size = 1984546, upload-time = "2026-07-03T12:09:42.543Z" }, + { url = "https://files.pythonhosted.org/packages/d2/4f/311bd40c02d61eee0182cb8c9b6ded37d42bed9a334e5ba4dacbe1c4c997/grimp-3.15-cp314-cp314-macosx_10_12_x86_64.whl", hash = "sha256:6db0b30683612be4571b6b6208e1b39b77faa54566c5ca4f086d64cc783e4864", size = 2144799, upload-time = "2026-07-03T12:08:53.215Z" }, + { url = "https://files.pythonhosted.org/packages/42/ce/86e941cc26bd3419b5e2abb8b48fa35b850e5508f14e033f3ce28bfce608/grimp-3.15-cp314-cp314-macosx_11_0_arm64.whl", hash = "sha256:e731a1f8a192a802e04d4018d6cce9f4a12ce48b6b73f43014bd4e0a5d04a8e9", size = 2090802, upload-time = "2026-07-03T12:08:45.489Z" }, + { url = "https://files.pythonhosted.org/packages/6c/09/bcb4e380596e533ef9ebb3f164ad3cc113fde62318ca5af71a27886b1612/grimp-3.15-cp314-cp314-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:5cb607ee3e16440fc59ec2b49eef1b6dbbe701af9e99990b6491e6f120bd60b5", size = 2255870, upload-time = "2026-07-03T12:07:37.207Z" }, + { url = "https://files.pythonhosted.org/packages/c3/84/361cebbd7b14ce15d93bfb65e2b0ff30287252ffa6ab0b7fe08e029eae6c/grimp-3.15-cp314-cp314-manylinux_2_17_armv7l.manylinux2014_armv7l.whl", hash = "sha256:f0492b9f1120176146d81e51aa0691aa6c004cdf5192ab309a221f9b223dedfc", size = 2193359, upload-time = "2026-07-03T12:07:47.548Z" }, + { url = "https://files.pythonhosted.org/packages/5e/d6/c4dd785e149c3c66494822c8b46f0a36cbdf5a5400eeb2172e0a8b029d0f/grimp-3.15-cp314-cp314-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:1bb3dbb0471179e732400fdf31c8c8d0299c88d13dd3a3bbe77ce54fb3f1b545", size = 2346350, upload-time = "2026-07-03T12:08:20.653Z" }, + { url = "https://files.pythonhosted.org/packages/97/4b/470922cb9d0a4b0436bb298801f770c98c6953374ff84e545dd7a196aa66/grimp-3.15-cp314-cp314-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:1b578512dbaee7eba2900d8078eaf1f7f78aa7616c609b0849b8403012ffddda", size = 2604959, upload-time = "2026-07-03T12:07:58.924Z" }, + { url = "https://files.pythonhosted.org/packages/6b/e1/061dd80f5f0abb3ac53b29ade25ffac3e464e6853e8ce31dc6c6a33a06a9/grimp-3.15-cp314-cp314-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:d1a550a14cd2f8123fb06814b705f3af093fa1728b244f21ebcccc8d0da0f851", size = 2327185, upload-time = "2026-07-03T12:08:10.142Z" }, + { url = "https://files.pythonhosted.org/packages/7b/a3/32281e7b5fcd5622f4666b36003b9931ca0e112f2955f09458f198a30f65/grimp-3.15-cp314-cp314-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:db5ebb94ec356eaa5242145c993bc84c4b52d0d2c6980dfcd12b22d51c5b2c46", size = 2273241, upload-time = "2026-07-03T12:08:33.247Z" }, + { url = "https://files.pythonhosted.org/packages/15/4b/b4f6ae15484541decbe4f12cf39a4a769352cb06332e428cc8e64aed4a32/grimp-3.15-cp314-cp314-musllinux_1_2_aarch64.whl", hash = "sha256:e03331418d7fee746e2447ecf5296986e6f094ef15fd25125efad2328844edf7", size = 2433114, upload-time = "2026-07-03T12:09:00.351Z" }, + { url = "https://files.pythonhosted.org/packages/93/a2/7bdc628435a1e908aa53b351ce031a6134efd18245c4a5a4c28e1e6d19aa/grimp-3.15-cp314-cp314-musllinux_1_2_armv7l.whl", hash = "sha256:4cc91cb69e73e7899feb6ce73747f4dc092c2bfe2653f6cba69a398cd1dfc6ea", size = 2467235, upload-time = "2026-07-03T12:09:10.677Z" }, + { url = "https://files.pythonhosted.org/packages/5d/18/77853f5693d607d5f49c7e3cd58e482ef8362ea9cf86d0fcb108e4168195/grimp-3.15-cp314-cp314-musllinux_1_2_i686.whl", hash = "sha256:a848d5b4b656c1e3a28cce0d399f2790ecbeb2eeb78bb49896018614c87219f3", size = 2506552, upload-time = "2026-07-03T12:09:20.908Z" }, + { url = "https://files.pythonhosted.org/packages/0a/bd/a094d7dd7115e8764e8069d5d3a44045c333b41d98a5746e99ec712b4b18/grimp-3.15-cp314-cp314-musllinux_1_2_x86_64.whl", hash = "sha256:208ef56397cabfab52864b3d8a461fb5015a056fede1be4587e4453b13aa8c2f", size = 2514255, upload-time = "2026-07-03T12:09:31.383Z" }, + { url = "https://files.pythonhosted.org/packages/85/68/013c640df50968b5d25802a5f01b157514d4e0ccd48c37c63947610a0e05/grimp-3.15-cp314-cp314-win32.whl", hash = "sha256:74d32fae3d222888f6b61579998043579dd810ed948785896e552906cbfdfcb7", size = 1856182, upload-time = "2026-07-03T12:09:51.178Z" }, + { url = "https://files.pythonhosted.org/packages/cc/0b/648a1d77dfc5c2fd24fbdca0a45ee1c24669d981d12652424f5c34e3352c/grimp-3.15-cp314-cp314-win_amd64.whl", hash = "sha256:6b36e179d485c797e7fa234803620af752039fa27a8c7e3182333d7c96041f70", size = 1985451, upload-time = "2026-07-03T12:09:43.731Z" }, + { url = "https://files.pythonhosted.org/packages/6b/c6/fd88ea799d181c22ab47c6cb328e7be3e196c8395444af89fd8da401cf6b/grimp-3.15-cp314-cp314t-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:b9107d1037ee6e250ab7a15e1b758e0af76c841c19838b3cc688885a0b3817b5", size = 2253182, upload-time = "2026-07-03T12:07:39.351Z" }, + { url = "https://files.pythonhosted.org/packages/da/d9/25377ba259772edfa97e35e265d89eb89b966a82652967c8479902741067/grimp-3.15-cp314-cp314t-manylinux_2_17_armv7l.manylinux2014_armv7l.whl", hash = "sha256:b88975b2882edc55e8946640f7b36dfb83cce1303cff5f8528c3f4819d7fbb07", size = 2191822, upload-time = "2026-07-03T12:07:48.721Z" }, + { url = "https://files.pythonhosted.org/packages/23/64/cae8ca43e05aa5217ca2e17ffbda77e86cb215c1a9a03592c110c0162f27/grimp-3.15-cp314-cp314t-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:0fe01a5393e415e3d99b22c7dc6c6a9cc1b633714707d1c6b56ecbaccc2132f5", size = 2344413, upload-time = "2026-07-03T12:08:21.976Z" }, + { url = "https://files.pythonhosted.org/packages/39/32/feea8fec71e62394013865314854ccb3d7bb54f226564fd267e6662f509a/grimp-3.15-cp314-cp314t-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:dd4d70a0de010a9b452b59326a00574f7488dd1333d003df624114e5e00e877e", size = 2602856, upload-time = "2026-07-03T12:08:00.263Z" }, + { url = "https://files.pythonhosted.org/packages/63/ab/46ccdeb698398264d774273a9b8d9b013c8d23127d05371489b22dcf3ea5/grimp-3.15-cp314-cp314t-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:204beed673ff43ab4ebe52ff4efd29b80025ec777136a62109afb69792d7fae0", size = 2326095, upload-time = "2026-07-03T12:08:11.432Z" }, + { url = "https://files.pythonhosted.org/packages/69/ae/f98c2c964a850ab80341d02576ef8681e75178f893ba2c73dc03e6563925/grimp-3.15-cp314-cp314t-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:1602be339f8a4d368d71758814e5505200cf665818eb0f9a2cc74d71ecc8cf1c", size = 2274375, upload-time = "2026-07-03T12:08:34.598Z" }, + { url = "https://files.pythonhosted.org/packages/bb/38/9355cdb28fab458fb8defca6406de303d78af617aa0d8c9ae5253b4e5a8b/grimp-3.15-cp314-cp314t-musllinux_1_2_aarch64.whl", hash = "sha256:077c27a1e6ff3baf23a8caaa1d74cbbac27024b069956dd6ca5fc3acd43ddb4b", size = 2430307, upload-time = "2026-07-03T12:09:02.447Z" }, + { url = "https://files.pythonhosted.org/packages/7f/06/edfa7f8064c1a3502fe4aba56c07bd122e94e329188939d52c02bd7e85b6/grimp-3.15-cp314-cp314t-musllinux_1_2_armv7l.whl", hash = "sha256:5516fc38899f4396eff68e6b1997310b4de2d0155e3fad9f545e666c993985b3", size = 2464014, upload-time = "2026-07-03T12:09:12.185Z" }, + { url = "https://files.pythonhosted.org/packages/ec/c2/153b5cc3106814504b4195c7d4c178d85732d35b8895185a02112b8f9a03/grimp-3.15-cp314-cp314t-musllinux_1_2_i686.whl", hash = "sha256:dbe8e14cc61af4eea8e7ed6f2108828101f57fe86273d5b61cff044b69e6c6b5", size = 2502010, upload-time = "2026-07-03T12:09:22.431Z" }, + { url = "https://files.pythonhosted.org/packages/e8/3f/ae4ba51cf484030e3d15b3304eb7ab9039fe91fb35ff5e90d60fde135bff/grimp-3.15-cp314-cp314t-musllinux_1_2_x86_64.whl", hash = "sha256:1763b7b48ed6c9e6de838d0d64f19510a392235266cf2240c9be9cc25ca7c54b", size = 2514787, upload-time = "2026-07-03T12:09:33Z" }, + { url = "https://files.pythonhosted.org/packages/0f/5f/0fb2d2bc9fe6bce899947802c94531c24e581c715db19216b941c1b2422f/grimp-3.15-cp315-cp315-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:116c3a6dba61bd302919b82cb5d603f5977e321d80d7de3c7a1265357ab9dd66", size = 2345635, upload-time = "2026-07-03T12:08:23.406Z" }, + { url = "https://files.pythonhosted.org/packages/00/ac/05d859a62ed9282f43a0f0962da230c46a2eb3d3ecb5b39117b3b4888405/grimp-3.15-cp315-cp315-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:fcc2e5065d35047729e41a55c9c3eb99810cf322fe3b3e89fa126f306ce6b3b5", size = 2273647, upload-time = "2026-07-03T12:08:36.139Z" }, + { url = "https://files.pythonhosted.org/packages/6d/dd/f726316f54e29da4f24d545d6299e1ea0accfbbf52729077fd4a619de055/grimp-3.15-cp315-cp315t-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:b38327673df49203cff0ebcbbf078999a80f0b11bb654760059f6f00f56f64d6", size = 2344080, upload-time = "2026-07-03T12:08:25.044Z" }, + { url = "https://files.pythonhosted.org/packages/c1/56/523a8f969f0a0b0a8ce43fbe8bc7a04e90f52724efc85f3305f1e07ae626/grimp-3.15-cp315-cp315t-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:965b37dad2f73164a103381c9fd1255bb3b2f6021ca2f38ffe70dd00fdc0fc55", size = 2275041, upload-time = "2026-07-03T12:08:37.582Z" }, + { url = "https://files.pythonhosted.org/packages/8c/46/562e56ba1abecc40169aa40fbf62ba5dcda0f56d953b53eeeb5e43535fb2/grimp-3.15-pp311-pypy311_pp73-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:391bca4c4f1c4dc58f10fadd8e4bba1ca1dbf848a5adb4e66907258135aa0b26", size = 2263708, upload-time = "2026-07-03T12:07:40.836Z" }, + { url = "https://files.pythonhosted.org/packages/c0/6a/2fe608f847630675b06a4e635024771b897f4ea9d1c0d3063f83ed1b7fc8/grimp-3.15-pp311-pypy311_pp73-manylinux_2_17_armv7l.manylinux2014_armv7l.whl", hash = "sha256:08f7e663a465b524c5c0e121e6dd759f282ce35f406bc95bd68deda63a604361", size = 2198790, upload-time = "2026-07-03T12:07:50.314Z" }, + { url = "https://files.pythonhosted.org/packages/31/49/8365239abbe735d8e103dc19f485e57383f8a435a0ae3b2ba7576e2d8bb7/grimp-3.15-pp311-pypy311_pp73-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:5625a037e9fc04c6371effb213efb827d225f5a64e04bff5f448adf180193da9", size = 2352309, upload-time = "2026-07-03T12:08:26.377Z" }, + { url = "https://files.pythonhosted.org/packages/5b/76/c1679a9eb0ac4ac2fc943fdcf47c1d59a200fd8daac3db6bd72c63e45cf9/grimp-3.15-pp311-pypy311_pp73-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:4651d29d4ee69bd5a1d9aa963ac73e420da1386f0a2263b8acbc0ae495fda12c", size = 2611854, upload-time = "2026-07-03T12:08:01.912Z" }, + { url = "https://files.pythonhosted.org/packages/8e/cf/56e47ded5188088ad0b9455cd74182c536a2f6fed29f1d4c0d7b20617b5d/grimp-3.15-pp311-pypy311_pp73-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:f73dcb7fb94b3a450dea3a95d63e179a715f315e2e0ff5b78aedf8b3dad8707c", size = 2331437, upload-time = "2026-07-03T12:08:12.758Z" }, + { url = "https://files.pythonhosted.org/packages/e4/c8/47cb1dcf0819506915bb5a20dff70bddd518199808c67e1aba730a2fedbc/grimp-3.15-pp311-pypy311_pp73-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:228e044bc458ee840ab20e1839efbecdecdd151d22ae3902a0f03aa94de772df", size = 2279755, upload-time = "2026-07-03T12:08:38.982Z" }, + { url = "https://files.pythonhosted.org/packages/38/90/6202e5fa8ffd3648b2fbcd1d6028c6224a278f557073cc3f8094f0a01cd5/grimp-3.15-pp311-pypy311_pp73-musllinux_1_2_aarch64.whl", hash = "sha256:bbaa9053652d29733ff88277b554e716e5acc40b37d12841c972779196d2c0ef", size = 2441806, upload-time = "2026-07-03T12:09:03.85Z" }, + { url = "https://files.pythonhosted.org/packages/b8/b6/2d82c6b6161169b0cb87ccb746e3a590c5a9fb54cb7c59d6c657ed731394/grimp-3.15-pp311-pypy311_pp73-musllinux_1_2_armv7l.whl", hash = "sha256:12fd9e675e6fcf633a5d14f6cbeee4b667a3d3307c91653ccc1a5aef03f53ec6", size = 2472941, upload-time = "2026-07-03T12:09:13.481Z" }, + { url = "https://files.pythonhosted.org/packages/b7/36/fb2906ed60288b236867829f9b3ced558a2725225ec0ed2354a7124a0474/grimp-3.15-pp311-pypy311_pp73-musllinux_1_2_i686.whl", hash = "sha256:0012653561d80b4e4972ac246deb569051a1fbf3880d620a9475e7159722bbab", size = 2513392, upload-time = "2026-07-03T12:09:24.122Z" }, + { url = "https://files.pythonhosted.org/packages/5e/d2/ec5f501d9623fe8eb2d517ab7528708b672dba51322a931515b58b659f6f/grimp-3.15-pp311-pypy311_pp73-musllinux_1_2_x86_64.whl", hash = "sha256:e438ae00dfd554c6262e92c945ec233c5e311cc99fdb0d602c6063b4f5fc3d9c", size = 2522530, upload-time = "2026-07-03T12:09:34.518Z" }, +] + [[package]] name = "h11" version = "0.16.0" @@ -418,6 +508,21 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/1e/5e/d4e9f1a599fb8e573b7b87160658329fbf28d19eac2718f51fc3def3aa5a/idna-3.18-py3-none-any.whl", hash = "sha256:7f952cbe720b688055e3f87de14f5c3e5fdaa8bc3928985c4077ca689de849a2", size = 65455, upload-time = "2026-06-02T14:34:06.319Z" }, ] +[[package]] +name = "import-linter" +version = "2.13" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "click" }, + { name = "grimp" }, + { name = "rich" }, + { name = "typing-extensions" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/97/c6/42962eb043df4d6984c1540220735b20442572fe37dab5e65ac807c939b9/import_linter-2.13.tar.gz", hash = "sha256:13af4a1d6b06044c58ea784e8732fd7fe48eec821a75feb4d6a1a2de36dd5c27", size = 1279761, upload-time = "2026-07-03T14:00:31.285Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/88/13/e7725e6eb32607fd4af51ccf3edfe835826e5cdf783b4d7fdc8f459196ae/import_linter-2.13-py3-none-any.whl", hash = "sha256:c0372e7ee5e15657bc06a8e841445e13237afd738a672d26863dc927af9f0bf5", size = 638185, upload-time = "2026-07-03T14:00:29.676Z" }, +] + [[package]] name = "iniconfig" version = "2.3.0" From 03be50644d0dc52995213ebf5197e1c0a7c8ed9d Mon Sep 17 00:00:00 2001 From: Rob Webb Date: Thu, 20 Aug 2026 21:42:35 +0100 Subject: [PATCH 8/8] test: rename integration test_repo.py to test_querying.py - unit/test_repo.py and integration/test_repo.py shared a basename, which pytest can't import as two distinct modules - The integration file only covers the facade's read paths, so 'test_querying.py' matches the naming of its siblings --- tests/integration/{test_repo.py => test_querying.py} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename tests/integration/{test_repo.py => test_querying.py} (98%) diff --git a/tests/integration/test_repo.py b/tests/integration/test_querying.py similarity index 98% rename from tests/integration/test_repo.py rename to tests/integration/test_querying.py index a69d399..2a8be40 100644 --- a/tests/integration/test_repo.py +++ b/tests/integration/test_querying.py @@ -1,4 +1,4 @@ -"""Integration tests for the Repository data facade over catalog + FS cache.""" +"""Integration tests for the Repository facade's reads over catalog + FS cache.""" from __future__ import annotations