diff --git a/docs/concepts/hooks-and-overrides.rst b/docs/concepts/hooks-and-overrides.rst index 3fcac836..9d527499 100644 --- a/docs/concepts/hooks-and-overrides.rst +++ b/docs/concepts/hooks-and-overrides.rst @@ -56,6 +56,12 @@ a Python module. See :doc:`/reference/hooks` for the complete list of hooks and their arguments. +.. versionchanged:: 0.96.0 + + Configured ``hook-sdist`` and ``hook-prebuilt`` profiles support typed + download results with explicit artifact kinds. See :ref:`download_source_hook` + for the contract, legacy path defaults, and a migration example. + Global Hooks ------------ diff --git a/docs/concepts/resolver-architecture.rst b/docs/concepts/resolver-architecture.rst index 4db347fb..06899f79 100644 --- a/docs/concepts/resolver-architecture.rst +++ b/docs/concepts/resolver-architecture.rst @@ -97,7 +97,8 @@ CLI commands interact with providers through a common .. versionchanged:: 0.96.0 The ``hook-sdist`` and ``hook-prebuilt`` profiles create providers through the required ``get_resolver_provider`` override hook. They do not fall back - to PyPI, and hook-backed artifact downloading remains a separate feature. + to PyPI. Their ``download()`` methods invoke the package's ``download_source`` + hook when present and return a validated ``DownloadedSource``. Per-package settings in YAML can select which provider to use and configure its parameters (index URL, tag pattern, etc.). When a package @@ -109,6 +110,14 @@ Otherwise, override plugins can replace the provider for a specific package via the ``get_resolver_provider`` hook, after which the legacy resolver settings are used. +Hook downloads accept explicit artifact kinds and adapt legacy paths using +profile-specific defaults. If no download hook exists, the profile selects the +standard sdist or wheel downloader and its destination directory. Hook errors +propagate with context instead of triggering a fallback. The typed result makes +the artifact kind available for source preparation; consuming it in preparation +and integrating prebuilt detection into the bootstrapper are separate steps. +See :ref:`download_source_hook` for the return contract and compatibility rules. + Version Filtering Window ------------------------- diff --git a/docs/reference/config-reference.rst b/docs/reference/config-reference.rst index f9c1df4d..4b0432f1 100644 --- a/docs/reference/config-reference.rst +++ b/docs/reference/config-reference.rst @@ -77,6 +77,9 @@ Source Resolver .. autoattribute:: pep517 .. autoattribute:: tarball +.. autoclass:: DownloadedSource + :members: path, kind + .. autoclass:: DownloadKind .. autoattribute:: sdist @@ -86,6 +89,13 @@ Source Resolver .. autoattribute:: any_source .. autoattribute:: not_available +.. versionchanged:: 0.96.0 + + Configured hook downloads require a concrete result kind supported by the + selected profile. ``any_source`` and ``not_available`` are not accepted as + result kinds. See :ref:`download_source_hook` for legacy path defaults and + the typed hook contract. + Global Settings --------------- diff --git a/docs/reference/hooks.rst b/docs/reference/hooks.rst index 5e10f0a8..b8e90ef3 100644 --- a/docs/reference/hooks.rst +++ b/docs/reference/hooks.rst @@ -219,6 +219,8 @@ Source hooks were removed. Define a :ref:`resolver_provider ` hook to resolve sources. +.. _download_source_hook: + .. autofromagerhook:: default_download_source The ``download_source()`` function is responsible for downloading the @@ -228,7 +230,73 @@ Source hooks evaluated, version of the package being downloaded, the download URL, and the output directory in which the source should be downloaded. - The return value should be a ``pathlib.Path`` file path to the downloaded source. + For packages without ``source:`` configuration, return a ``pathlib.Path`` + to the downloaded source. This legacy contract is unchanged. + + .. versionchanged:: 0.96.0 + + The ``hook-sdist`` and ``hook-prebuilt`` source profiles accept + :class:`~fromager.packagesettings.DownloadedSource` with a local + ``pathlib.Path`` and an explicit + :class:`~fromager.packagesettings.DownloadKind`: + + * ``hook-sdist`` accepts ``sdist``, ``tarball``, or ``git_checkout``. + * ``hook-prebuilt`` accepts only ``prebuilt_wheel``. + + A bare ``Path`` remains supported for legacy hooks: ``hook-sdist`` + treats ``.tar.gz`` and ``.zip`` paths as ``sdist``; ``hook-prebuilt`` + treats ``.whl`` paths as ``prebuilt_wheel``. Bare directories and other + path forms are rejected. The sdist default preserves legacy archive + semantics; it does not infer the artifact kind from archive contents. + Hooks needing tarball or git-checkout semantics must return a typed + result with the explicit kind. ``any_source`` and ``not_available`` + are not valid concrete result kinds. + + Invalid return types, malformed typed fields, and kinds incompatible + with the profile raise errors identifying the package and hook. + Hook exceptions are chained to a ``RuntimeError`` with that context; + neither failures nor invalid results trigger a fallback download. + + If the download hook is absent, ``hook-sdist`` downloads the candidate + URL as an sdist into ``ctx.sdists_downloads``. ``hook-prebuilt`` downloads + it as a wheel into ``ctx.wheels_prebuilt``. These defaults use the + standard archive and wheel validation. Custom tarball URLs and git + downloads require a download hook. The ``get_resolver_provider`` hook + remains required for both profiles. + + To migrate a raw-tarball hook to ``source: {provider: hook-sdist}``, return + its artifact kind explicitly: + + .. code-block:: python + + import pathlib + + from packaging.requirements import Requirement + from packaging.version import Version + + from fromager import downloads + from fromager.context import WorkContext + from fromager.packagesettings import DownloadedSource, DownloadKind + + def download_source( + ctx: WorkContext, + req: Requirement, + version: Version, + download_url: str, + sdists_downloads_dir: pathlib.Path, + ) -> DownloadedSource: + """Download a raw source tarball and identify its kind.""" + path = downloads.download_url( + destination_dir=sdists_downloads_dir, + url=download_url, + destination_filename=f"{req.name}-{version}.tar.gz", + ) + return DownloadedSource(path=path, kind=DownloadKind.tarball) + + A hook that clones sources returns the checkout directory with + ``kind=DownloadKind.git_checkout``. Typed results are supported through + the configured hook profiles; packages using legacy settings still require + a bare ``Path``. .. autofromagerhook:: default_prepare_source diff --git a/src/fromager/bootstrapper/_bootstrapper.py b/src/fromager/bootstrapper/_bootstrapper.py index fa245101..a22464b5 100644 --- a/src/fromager/bootstrapper/_bootstrapper.py +++ b/src/fromager/bootstrapper/_bootstrapper.py @@ -18,6 +18,7 @@ from .. import ( bootstrap_requirement_resolver, + packagesettings, progress, sources, threading_utils, @@ -483,14 +484,13 @@ def _download_source( source_url: str, ) -> pathlib.Path: """Download source for a package.""" - download_result = sources.download_source( + result: packagesettings.DownloadedSource = sources.download_source( ctx=self.ctx, req=req, version=resolved_version, download_url=source_url, ) - source_path: pathlib.Path = download_result[0] - return source_path + return result.path def _prepare_source( self, diff --git a/src/fromager/bootstrapper/_prepare_source.py b/src/fromager/bootstrapper/_prepare_source.py index 6076fdd2..67df1546 100644 --- a/src/fromager/bootstrapper/_prepare_source.py +++ b/src/fromager/bootstrapper/_prepare_source.py @@ -43,9 +43,9 @@ def _bg_prepare_source( sdist_root_dir=unpacked / unpacked.stem, cached_wheel_filename=cached_wheel, ) - source_filename, _ = sources.download_source( + source_filename = sources.download_source( ctx=ctx, req=req, version=resolved_version, download_url=source_url - ) + ).path sdist_root_dir = sources.prepare_source( ctx=ctx, req=req, source_filename=source_filename, version=resolved_version ) diff --git a/src/fromager/commands/build.py b/src/fromager/commands/build.py index 42aaaf48..1fe34257 100644 --- a/src/fromager/commands/build.py +++ b/src/fromager/commands/build.py @@ -384,12 +384,12 @@ def _build( # If we get here and still don't have a wheel filename, then we need to # build the wheel. if not wheel_filename: - source_filename, _ = sources.download_source( + source_filename = sources.download_source( ctx=wkctx, req=req, version=resolved_version, download_url=source_download_url, - ) + ).path logger.debug( "saved sdist of version %s from %s to %s", resolved_version, diff --git a/src/fromager/commands/step.py b/src/fromager/commands/step.py index 197d06a1..95280934 100644 --- a/src/fromager/commands/step.py +++ b/src/fromager/commands/step.py @@ -50,12 +50,12 @@ def download_source_archive( source_url, version = sources.resolve_source( ctx=wkctx, req=req, sdist_server_url=sdist_server_url ) - filename, _ = sources.download_source( + filename = sources.download_source( ctx=wkctx, req=req, version=version, download_url=source_url, - ) + ).path requirement_ctxvar.reset(token) print(filename) diff --git a/src/fromager/packagesettings/__init__.py b/src/fromager/packagesettings/__init__.py index 7dafd8f5..5703c758 100644 --- a/src/fromager/packagesettings/__init__.py +++ b/src/fromager/packagesettings/__init__.py @@ -17,6 +17,7 @@ from ._pbi import PackageBuildInfo from ._resolver import ( BuildSDist, + DownloadedSource, DownloadKind, GitHubTagCloneResolver, GitHubTagDownloadResolver, @@ -58,6 +59,7 @@ "BuildSDist", "DownloadKind", "DownloadSource", + "DownloadedSource", "EnvKey", "EnvVars", "ExternalCommands", diff --git a/src/fromager/packagesettings/_resolver.py b/src/fromager/packagesettings/_resolver.py index 27b98026..8c049449 100644 --- a/src/fromager/packagesettings/_resolver.py +++ b/src/fromager/packagesettings/_resolver.py @@ -1,5 +1,6 @@ from __future__ import annotations +import dataclasses import datetime import enum import inspect @@ -47,6 +48,20 @@ def __bool__(self) -> bool: DownloadKindSet = frozenset[DownloadKind] +@dataclasses.dataclass(frozen=True, slots=True) +class DownloadedSource: + """Immutable result of downloading an artifact. + + ``path`` is the local artifact path. ``kind`` identifies how the artifact + should be handled; hook resolvers require a concrete, supported kind. + + .. versionadded:: 0.96.0 + """ + + path: pathlib.Path + kind: DownloadKind + + class AbstractResolver(pydantic.BaseModel): """Abstract base class for resolvers""" @@ -79,7 +94,7 @@ def download( ctx: context.WorkContext, req: Requirement, candidate: Candidate, - ) -> tuple[pathlib.Path, DownloadKind]: + ) -> DownloadedSource: """Download the resolved artifact for *candidate*.""" raise NotImplementedError @@ -89,7 +104,7 @@ def _download( req: Requirement, candidate: Candidate, download_kind: DownloadKind, - ) -> tuple[pathlib.Path, DownloadKind]: + ) -> DownloadedSource: """Download the resolved artifact for *candidate*. Returns the local path and the kind of artifact that was @@ -144,7 +159,7 @@ def _download( ) case _: # includes any_source and not_available typing.assert_never(download_kind) # type: ignore[arg-type] - return path, download_kind + return DownloadedSource(path=path, kind=download_kind) class CooldownMixin(pydantic.BaseModel): @@ -219,7 +234,7 @@ def download( ctx: context.WorkContext, req: Requirement, candidate: Candidate, - ) -> tuple[pathlib.Path, DownloadKind]: + ) -> DownloadedSource: """Download an sdist from PyPI.""" return self._download(ctx, req, candidate, DownloadKind.sdist) @@ -272,7 +287,7 @@ def download( ctx: context.WorkContext, req: Requirement, candidate: Candidate, - ) -> tuple[pathlib.Path, DownloadKind]: + ) -> DownloadedSource: """Download a pre-built wheel from PyPI.""" return self._download(ctx, req, candidate, DownloadKind.prebuilt_wheel) @@ -350,7 +365,7 @@ def download( ctx: context.WorkContext, req: Requirement, candidate: Candidate, - ) -> tuple[pathlib.Path, DownloadKind]: + ) -> DownloadedSource: """Download an sdist or tarball from a custom URL.""" return self._download(ctx, req, candidate, self.download_kind) @@ -433,7 +448,7 @@ def download( ctx: context.WorkContext, req: Requirement, candidate: Candidate, - ) -> tuple[pathlib.Path, DownloadKind]: + ) -> DownloadedSource: """Clone a git repository at a specific tag.""" return self._download(ctx, req, candidate, DownloadKind.git_checkout) @@ -596,7 +611,7 @@ def download( ctx: context.WorkContext, req: Requirement, candidate: Candidate, - ) -> tuple[pathlib.Path, DownloadKind]: + ) -> DownloadedSource: """Download a tarball from a GitHub tag.""" return self._download(ctx, req, candidate, DownloadKind.tarball) @@ -639,7 +654,7 @@ def download( ctx: context.WorkContext, req: Requirement, candidate: Candidate, - ) -> tuple[pathlib.Path, DownloadKind]: + ) -> DownloadedSource: """Clone a git repository from a GitHub tag.""" return self._download(ctx, req, candidate, DownloadKind.git_checkout) @@ -680,7 +695,7 @@ def download( ctx: context.WorkContext, req: Requirement, candidate: Candidate, - ) -> tuple[pathlib.Path, DownloadKind]: + ) -> DownloadedSource: """Download a tarball from a GitLab tag.""" return self._download(ctx, req, candidate, DownloadKind.tarball) @@ -723,7 +738,7 @@ def download( ctx: context.WorkContext, req: Requirement, candidate: Candidate, - ) -> tuple[pathlib.Path, DownloadKind]: + ) -> DownloadedSource: """Clone a git repository from a GitLab tag.""" return self._download(ctx, req, candidate, DownloadKind.git_checkout) @@ -748,7 +763,7 @@ def download( ctx: context.WorkContext, req: Requirement, candidate: Candidate, - ) -> tuple[pathlib.Path, DownloadKind]: + ) -> DownloadedSource: """Raise because package is not available.""" raise ValueError(f"package {req.name} is not available") @@ -812,14 +827,70 @@ def resolver_provider( ) -> resolver.BaseProvider: raise NotImplementedError - def download( + def _download_from_hook( self, ctx: context.WorkContext, req: Requirement, candidate: Candidate, - ) -> tuple[pathlib.Path, DownloadKind]: - """Raise because hook-based download is not yet implemented.""" - raise NotImplementedError("Hook resolver download needs a hook") + legacy_kind: DownloadKind, + ) -> DownloadedSource: + """Download with the package hook and normalize its result.""" + hook = overrides.find_override_method(req.name, "download_source") + if hook is None: + return self._download(ctx, req, candidate, legacy_kind) + + error_context = f"{req.name}: {self.provider!r} download_source hook" + try: + result: object = overrides.invoke( + hook, + ctx=ctx, + req=req, + version=candidate.version, + download_url=candidate.url, + sdists_downloads_dir=ctx.sdists_downloads, + ) + except Exception as err: + raise RuntimeError(f"{error_context} failed") from err + + logger.info("%s returned %s", error_context, result) + if isinstance(result, DownloadedSource): + downloaded = result + elif isinstance(result, pathlib.Path): + suffixes = ( + (".whl",) + if legacy_kind is DownloadKind.prebuilt_wheel + else (".tar.gz", ".zip") + ) + if result.is_dir() or not result.name.endswith(suffixes): + raise ValueError( + f"{error_context} returned legacy Path {result}; expected a " + f"{' or '.join(suffixes)} file. Return DownloadedSource with " + "an explicit supported DownloadKind for other artifact forms" + ) + downloaded = DownloadedSource(path=result, kind=legacy_kind) + else: + raise TypeError( + f"{error_context} returned {type(result).__name__}, " + "expected pathlib.Path or DownloadedSource" + ) + + if not isinstance(downloaded.path, pathlib.Path): + raise TypeError( + f"{error_context}: DownloadedSource.path must be pathlib.Path, " + f"got {type(downloaded.path).__name__}" + ) + if not isinstance(downloaded.kind, DownloadKind): + raise TypeError( + f"{error_context}: DownloadedSource.kind must be DownloadKind, " + f"got {type(downloaded.kind).__name__}" + ) + if downloaded.kind not in self.download_kinds: + supported = ", ".join(sorted(kind.value for kind in self.download_kinds)) + raise ValueError( + f"{error_context} returned unsupported kind {downloaded.kind.value!r}; " + f"expected DownloadedSource with one of: {supported}" + ) + return downloaded class HookSDistResolver(AbstractHookResolver): @@ -827,7 +898,9 @@ class HookSDistResolver(AbstractHookResolver): The ``hook-sdist`` provider delegates resolution and download to plugin hooks. The downloaded artifact can be a source distribution, - a tarball, or a git checkout. + a tarball, or a git checkout. Download hooks can return ``DownloadedSource`` + with a concrete kind, or a legacy archive ``Path`` treated as an sdist. + Without a download hook, the candidate is downloaded as an sdist. Example:: @@ -859,12 +932,24 @@ def resolver_provider( ignore_platform=False, ) + def download( + self, + ctx: context.WorkContext, + req: Requirement, + candidate: Candidate, + ) -> DownloadedSource: + """Download source via the ``download_source`` override hook.""" + return self._download_from_hook(ctx, req, candidate, DownloadKind.sdist) + class HookPrebuiltResolver(AbstractHookResolver): """Call resolver_provider and download_source hook, use pre-built wheel The ``hook-prebuilt`` provider delegates resolution and download to - plugin hooks. The downloaded artifact must be a pre-built wheel. + plugin hooks. The downloaded artifact must be a pre-built wheel. Download + hooks can return ``DownloadedSource`` with kind ``prebuilt_wheel`` or a + legacy wheel ``Path``. Without a download hook, the candidate is downloaded + into ``ctx.wheels_prebuilt``. Example:: @@ -897,6 +982,17 @@ def resolver_provider( ignore_platform=False, ) + def download( + self, + ctx: context.WorkContext, + req: Requirement, + candidate: Candidate, + ) -> DownloadedSource: + """Download a pre-built wheel via the ``download_source`` override hook.""" + return self._download_from_hook( + ctx, req, candidate, DownloadKind.prebuilt_wheel + ) + SourceResolver = typing.Annotated[ PyPISDistResolver diff --git a/src/fromager/sources.py b/src/fromager/sources.py index b5f674c0..12051259 100644 --- a/src/fromager/sources.py +++ b/src/fromager/sources.py @@ -86,7 +86,7 @@ def download_source( req: Requirement, version: Version, download_url: str, -) -> tuple[pathlib.Path, packagesettings.DownloadKind]: +) -> packagesettings.DownloadedSource: """Download a resolved source and return its path and artifact kind. Configured source resolvers receive a minimal candidate. Legacy downloads @@ -115,7 +115,9 @@ def download_source( raise ValueError( f"expected a Path back to downloaded source. got {source_path}" ) - return source_path, packagesettings.DownloadKind.sdist + return packagesettings.DownloadedSource( + path=source_path, kind=packagesettings.DownloadKind.sdist + ) def get_source_provider( diff --git a/tests/test_bootstrapper.py b/tests/test_bootstrapper.py index 1a9cfd1e..00b8a004 100644 --- a/tests/test_bootstrapper.py +++ b/tests/test_bootstrapper.py @@ -37,7 +37,7 @@ from fromager.bootstrapper._work_item import WorkItem from fromager.context import WorkContext from fromager.dependency_graph import ROOT -from fromager.packagesettings import DownloadKind, WheelSettings +from fromager.packagesettings import DownloadedSource, DownloadKind, WheelSettings from fromager.requirements_file import RequirementType, SourceType @@ -1170,9 +1170,9 @@ def test_bg_prepare_source_log_prefix_includes_version( ), patch( "fromager.sources.download_source", - return_value=( - source_filename, - DownloadKind.sdist, + return_value=DownloadedSource( + path=source_filename, + kind=DownloadKind.sdist, ), ), patch( @@ -1222,7 +1222,7 @@ def test_bootstrapper_download_source_returns_downloaded_path( with patch( "fromager.bootstrapper._bootstrapper.sources.download_source", - return_value=(source_filename, DownloadKind.sdist), + return_value=DownloadedSource(path=source_filename, kind=DownloadKind.sdist), ) as download_source: result = bt._download_source(req, version, source_url) diff --git a/tests/test_commands.py b/tests/test_commands.py index 1f3ba7cd..17572993 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -46,7 +46,9 @@ def test_download_source_archive_prints_downloaded_path( patch.object( step.sources, "download_source", - return_value=(expected, packagesettings.DownloadKind.sdist), + return_value=packagesettings.DownloadedSource( + path=expected, kind=packagesettings.DownloadKind.sdist + ), ), ): result = cli_runner.invoke( @@ -83,7 +85,9 @@ def test_build_passes_downloaded_path_to_prepare_source( patch.object( build.sources, "download_source", - return_value=(source_filename, packagesettings.DownloadKind.sdist), + return_value=packagesettings.DownloadedSource( + path=source_filename, kind=packagesettings.DownloadKind.sdist + ), ), patch.object( build.sources, diff --git a/tests/test_packagesettings_resolver.py b/tests/test_packagesettings_resolver.py index 2dffa570..54adfa56 100644 --- a/tests/test_packagesettings_resolver.py +++ b/tests/test_packagesettings_resolver.py @@ -1,6 +1,8 @@ from __future__ import annotations +import dataclasses import datetime +import pathlib import re import textwrap import typing @@ -11,12 +13,14 @@ import yaml from packaging.requirements import Requirement from packaging.version import Version +from wheel.wheelfile import WheelFile from fromager import resolver from fromager.candidate import Candidate, Cooldown from fromager.context import WorkContext from fromager.packagesettings._resolver import ( BuildSDist, + DownloadedSource, DownloadKind, GitHubTagCloneResolver, GitHubTagDownloadResolver, @@ -190,9 +194,10 @@ def test_download(self, mock_dl: mock.MagicMock, tmp_context: WorkContext) -> No expected = tmp_context.sdists_downloads / "test-pkg-1.2.3.tar.gz" mock_dl.return_value = expected r = _parse(self.YAML) - path, kind = r.download(tmp_context, _REQ, _CANDIDATE_SDIST) - assert path == expected - assert kind is DownloadKind.sdist + result = r.download(tmp_context, _REQ, _CANDIDATE_SDIST) + assert isinstance(result, DownloadedSource) + assert result.path == expected + assert result.kind is DownloadKind.sdist mock_dl.assert_called_once_with( destination_dir=tmp_context.sdists_downloads, url=_CANDIDATE_SDIST.url, @@ -231,9 +236,10 @@ def test_download(self, mock_dl: mock.MagicMock, tmp_context: WorkContext) -> No expected = tmp_context.wheels_prebuilt / "test_pkg-1.2.3-py3-none-any.whl" mock_dl.return_value = expected r = _parse(self.YAML) - path, kind = r.download(tmp_context, _REQ, _CANDIDATE_WHEEL) - assert path == expected - assert kind is DownloadKind.prebuilt_wheel + result = r.download(tmp_context, _REQ, _CANDIDATE_WHEEL) + assert isinstance(result, DownloadedSource) + assert result.path == expected + assert result.kind is DownloadKind.prebuilt_wheel mock_dl.assert_called_once_with( destination_dir=tmp_context.wheels_prebuilt, url=_CANDIDATE_WHEEL.url, @@ -304,9 +310,10 @@ def test_download(self, mock_dl: mock.MagicMock, tmp_context: WorkContext) -> No expected = tmp_context.sdists_downloads / "test-pkg-1.2.3.tar.gz" mock_dl.return_value = expected r = _parse(self.YAML) - path, kind = r.download(tmp_context, _REQ, _CANDIDATE_TARBALL) - assert path == expected - assert kind is DownloadKind.tarball + result = r.download(tmp_context, _REQ, _CANDIDATE_TARBALL) + assert isinstance(result, DownloadedSource) + assert result.path == expected + assert result.kind is DownloadKind.tarball mock_dl.assert_called_once_with( destination_dir=tmp_context.sdists_downloads, url=_CANDIDATE_TARBALL.url, @@ -374,9 +381,10 @@ def test_download(self, mock_dl: mock.MagicMock, tmp_context: WorkContext) -> No ) mock_dl.return_value = expected_dest r = _parse(self.YAML) - path, kind = r.download(tmp_context, _REQ, _CANDIDATE_GIT) - assert path == expected_dest - assert kind is DownloadKind.git_checkout + result = r.download(tmp_context, _REQ, _CANDIDATE_GIT) + assert isinstance(result, DownloadedSource) + assert result.path == expected_dest + assert result.kind is DownloadKind.git_checkout mock_dl.assert_called_once_with( destination_dir=expected_dest, vcs_url=_CANDIDATE_GIT.url, @@ -493,9 +501,10 @@ def test_download(self, mock_dl: mock.MagicMock, tmp_context: WorkContext) -> No expected = tmp_context.sdists_downloads / "test-pkg-1.2.3.tar.gz" mock_dl.return_value = expected r = _parse(self.YAML) - path, kind = r.download(tmp_context, _REQ, _CANDIDATE_GITHUB_TARBALL) - assert path == expected - assert kind is DownloadKind.tarball + result = r.download(tmp_context, _REQ, _CANDIDATE_GITHUB_TARBALL) + assert isinstance(result, DownloadedSource) + assert result.path == expected + assert result.kind is DownloadKind.tarball mock_dl.assert_called_once_with( destination_dir=tmp_context.sdists_downloads, url=_CANDIDATE_GITHUB_TARBALL.url, @@ -539,9 +548,10 @@ def test_download(self, mock_dl: mock.MagicMock, tmp_context: WorkContext) -> No ) mock_dl.return_value = expected_dest r = _parse(self.YAML) - path, kind = r.download(tmp_context, _REQ, _CANDIDATE_GITHUB_CLONE) - assert path == expected_dest - assert kind is DownloadKind.git_checkout + result = r.download(tmp_context, _REQ, _CANDIDATE_GITHUB_CLONE) + assert isinstance(result, DownloadedSource) + assert result.path == expected_dest + assert result.kind is DownloadKind.git_checkout mock_dl.assert_called_once_with( destination_dir=expected_dest, vcs_url=_CANDIDATE_GITHUB_CLONE.url, @@ -580,9 +590,10 @@ def test_download(self, mock_dl: mock.MagicMock, tmp_context: WorkContext) -> No expected = tmp_context.sdists_downloads / "test-pkg-1.2.3.tar.gz" mock_dl.return_value = expected r = _parse(self.YAML) - path, kind = r.download(tmp_context, _REQ, _CANDIDATE_GITLAB_TARBALL) - assert path == expected - assert kind is DownloadKind.tarball + result = r.download(tmp_context, _REQ, _CANDIDATE_GITLAB_TARBALL) + assert isinstance(result, DownloadedSource) + assert result.path == expected + assert result.kind is DownloadKind.tarball mock_dl.assert_called_once_with( destination_dir=tmp_context.sdists_downloads, url=_CANDIDATE_GITLAB_TARBALL.url, @@ -626,9 +637,10 @@ def test_download(self, mock_dl: mock.MagicMock, tmp_context: WorkContext) -> No ) mock_dl.return_value = expected_dest r = _parse(self.YAML) - path, kind = r.download(tmp_context, _REQ, _CANDIDATE_GITLAB_CLONE) - assert path == expected_dest - assert kind is DownloadKind.git_checkout + result = r.download(tmp_context, _REQ, _CANDIDATE_GITLAB_CLONE) + assert isinstance(result, DownloadedSource) + assert result.path == expected_dest + assert result.kind is DownloadKind.git_checkout mock_dl.assert_called_once_with( destination_dir=expected_dest, vcs_url=_CANDIDATE_GITLAB_CLONE.url, @@ -729,11 +741,6 @@ def hook( "ignore_platform": False, } - def test_download(self, tmp_context: WorkContext) -> None: - r = _parse(self.YAML) - with pytest.raises(NotImplementedError, match="hook"): - r.download(tmp_context, _REQ, _CANDIDATE_SDIST) - class TestHookPrebuiltResolver: YAML = """\ @@ -784,10 +791,282 @@ def hook( "get_resolver_provider", ) - def test_download(self, tmp_context: WorkContext) -> None: - r = _parse(self.YAML) - with pytest.raises(NotImplementedError, match="hook"): - r.download(tmp_context, _REQ, _CANDIDATE_WHEEL) + +@pytest.mark.parametrize( + ("provider", "kind"), + [ + ("hook-sdist", DownloadKind.sdist), + ("hook-sdist", DownloadKind.tarball), + ("hook-sdist", DownloadKind.git_checkout), + ("hook-prebuilt", DownloadKind.prebuilt_wheel), + ], +) +def test_hook_download_preserves_typed_result_and_arguments( + tmp_context: WorkContext, provider: str, kind: DownloadKind +) -> None: + """Preserve the hook's explicit kind and forward the legacy arguments.""" + r = _parse(f"source:\n provider: {provider}") + candidate = _CANDIDATE_WHEEL if r.resolves_prebuilt_wheel else _CANDIDATE_SDIST + expected = DownloadedSource(path=tmp_context.work_dir / "artifact", kind=kind) + captured: dict[str, object] = {} + + def hook( + ctx: WorkContext, + req: Requirement, + version: Version, + download_url: str, + sdists_downloads_dir: pathlib.Path, + ) -> DownloadedSource: + captured.update( + ctx=ctx, + req=req, + version=version, + download_url=download_url, + sdists_downloads_dir=sdists_downloads_dir, + ) + return expected + + with ( + mock.patch( + "fromager.overrides.find_override_method", return_value=hook + ) as find_hook, + mock.patch.object(type(r), "_download") as fallback, + ): + result = r.download(tmp_context, _REQ, candidate) + + assert result is expected + assert captured == { + "ctx": tmp_context, + "req": _REQ, + "version": candidate.version, + "download_url": candidate.url, + "sdists_downloads_dir": tmp_context.sdists_downloads, + } + find_hook.assert_called_once_with(_REQ.name, "download_source") + fallback.assert_not_called() + + +@pytest.mark.parametrize( + ("provider", "filename", "kind"), + [ + ("hook-sdist", "test-pkg-1.2.3.tar.gz", DownloadKind.sdist), + ("hook-sdist", "test-pkg-1.2.3.zip", DownloadKind.sdist), + ( + "hook-prebuilt", + "test_pkg-1.2.3-py3-none-any.whl", + DownloadKind.prebuilt_wheel, + ), + ], +) +def test_hook_download_adapts_legacy_path_and_narrow_signature( + tmp_context: WorkContext, provider: str, filename: str, kind: DownloadKind +) -> None: + """Adapt bare paths without requiring hooks to accept every argument.""" + r = _parse(f"source:\n provider: {provider}") + expected = tmp_context.work_dir / filename + + def hook(req: Requirement, version: Version) -> pathlib.Path: + assert req == _REQ + assert version == _VERSION + return expected + + with mock.patch("fromager.overrides.find_override_method", return_value=hook): + result = r.download(tmp_context, _REQ, _CANDIDATE_SDIST) + + assert result == DownloadedSource(path=expected, kind=kind) + + +@pytest.mark.parametrize("provider", ["hook-sdist", "hook-prebuilt"]) +@pytest.mark.parametrize( + ("hook_result", "error_type", "message"), + [ + (None, TypeError, "expected pathlib.Path or DownloadedSource"), + ("not-a-path", TypeError, "expected pathlib.Path or DownloadedSource"), + (42, TypeError, "expected pathlib.Path or DownloadedSource"), + ( + (pathlib.Path("source"), DownloadKind.sdist), + TypeError, + "expected pathlib.Path or DownloadedSource", + ), + ( + DownloadedSource( + path=typing.cast(pathlib.Path, "source"), kind=DownloadKind.sdist + ), + TypeError, + "DownloadedSource.path must be pathlib.Path", + ), + ( + DownloadedSource( + path=pathlib.Path("source"), kind=typing.cast(DownloadKind, "sdist") + ), + TypeError, + "DownloadedSource.kind must be DownloadKind", + ), + ( + DownloadedSource(path=pathlib.Path("source"), kind=DownloadKind.any_source), + ValueError, + "unsupported kind 'any_source'", + ), + ( + DownloadedSource( + path=pathlib.Path("source"), kind=DownloadKind.not_available + ), + ValueError, + "unsupported kind 'n/a'", + ), + ], +) +def test_hook_download_rejects_invalid_results_without_fallback( + tmp_context: WorkContext, + provider: str, + hook_result: object, + error_type: type[Exception], + message: str, +) -> None: + """Reject malformed hook results with package and provider context.""" + r = _parse(f"source:\n provider: {provider}") + + def hook() -> object: + return hook_result + + with ( + mock.patch("fromager.overrides.find_override_method", return_value=hook), + mock.patch.object(type(r), "_download") as fallback, + pytest.raises(error_type, match=re.escape(message)) as exc_info, + ): + r.download(tmp_context, _REQ, _CANDIDATE_SDIST) + + assert f"test-pkg: '{provider}' download_source hook" in str(exc_info.value) + fallback.assert_not_called() + + +@pytest.mark.parametrize( + ("provider", "kind"), + [ + ("hook-sdist", DownloadKind.prebuilt_wheel), + ("hook-prebuilt", DownloadKind.sdist), + ("hook-prebuilt", DownloadKind.tarball), + ("hook-prebuilt", DownloadKind.git_checkout), + ], +) +def test_hook_download_rejects_kind_incompatible_with_profile( + tmp_context: WorkContext, provider: str, kind: DownloadKind +) -> None: + """Do not relabel an explicit kind to fit the selected profile.""" + r = _parse(f"source:\n provider: {provider}") + + def hook() -> DownloadedSource: + return DownloadedSource(path=tmp_context.work_dir, kind=kind) + + with ( + mock.patch("fromager.overrides.find_override_method", return_value=hook), + pytest.raises(ValueError, match=f"unsupported kind '{kind.value}'") as exc_info, + ): + r.download(tmp_context, _REQ, _CANDIDATE_SDIST) + + for allowed_kind in r.download_kinds: + assert allowed_kind.value in str(exc_info.value) + + +@pytest.mark.parametrize( + ("provider", "filename", "is_directory"), + [ + ("hook-sdist", "checkout", True), + ("hook-sdist", "source.tar.gz", True), + ("hook-sdist", "source.tar.xz", False), + ("hook-sdist", "test_pkg-1.2.3-py3-none-any.whl", False), + ("hook-prebuilt", "test_pkg-1.2.3-py3-none-any.whl", True), + ("hook-prebuilt", "source.tar.gz", False), + ], +) +def test_hook_download_requires_kind_for_unsupported_legacy_paths( + tmp_context: WorkContext, provider: str, filename: str, is_directory: bool +) -> None: + """Require an explicit kind when a path cannot use the legacy default.""" + r = _parse(f"source:\n provider: {provider}") + path = tmp_context.work_dir / filename + if is_directory: + path.mkdir() + else: + path.touch() + + def hook() -> pathlib.Path: + return path + + with ( + mock.patch("fromager.overrides.find_override_method", return_value=hook), + pytest.raises(ValueError, match="Return DownloadedSource"), + ): + r.download(tmp_context, _REQ, _CANDIDATE_SDIST) + + +@pytest.mark.parametrize("provider", ["hook-sdist", "hook-prebuilt"]) +def test_hook_download_uses_profile_default_when_hook_is_missing( + tmp_context: WorkContext, provider: str +) -> None: + """Select the correct download helper and destination without a hook.""" + r = _parse(f"source:\n provider: {provider}") + prebuilt = r.resolves_prebuilt_wheel + candidate = _CANDIDATE_WHEEL if prebuilt else _CANDIDATE_SDIST + destination = ( + tmp_context.wheels_prebuilt if prebuilt else tmp_context.sdists_downloads + ) + expected = destination / candidate.url.rsplit("/", 1)[1] + kind = DownloadKind.prebuilt_wheel if prebuilt else DownloadKind.sdist + + with ( + mock.patch("fromager.overrides.find_override_method", return_value=None), + mock.patch("fromager.downloads.download_sdist", return_value=expected) as sdist, + mock.patch("fromager.downloads.download_wheel", return_value=expected) as wheel, + ): + result = r.download(tmp_context, _REQ, candidate) + + selected, unused = (wheel, sdist) if prebuilt else (sdist, wheel) + selected.assert_called_once_with(destination_dir=destination, url=candidate.url) + unused.assert_not_called() + assert result == DownloadedSource(path=expected, kind=kind) + + +@pytest.mark.parametrize("provider", ["hook-sdist", "hook-prebuilt"]) +def test_hook_download_chains_failure_without_fallback( + tmp_context: WorkContext, provider: str +) -> None: + """Preserve the hook failure and do not attempt a default download.""" + r = _parse(f"source:\n provider: {provider}") + cause = OSError("download failed") + + def hook() -> pathlib.Path: + raise cause + + with ( + mock.patch("fromager.overrides.find_override_method", return_value=hook), + mock.patch.object(type(r), "_download") as fallback, + pytest.raises( + RuntimeError, match=f"'{provider}' download_source hook failed" + ) as exc_info, + ): + r.download(tmp_context, _REQ, _CANDIDATE_SDIST) + + assert exc_info.value.__cause__ is cause + fallback.assert_not_called() + + +def test_hook_prebuilt_download_fallback_accepts_valid_wheel( + tmp_context: WorkContext, +) -> None: + """Use wheel validation, rather than sdist validation, for a cached wheel.""" + path = tmp_context.wheels_prebuilt / "test_pkg-1.2.3-py3-none-any.whl" + with WheelFile(path, "w") as wheel: + wheel.writestr( + f"{wheel.dist_info_path}/METADATA", + "Metadata-Version: 2.1\nName: test-pkg\nVersion: 1.2.3\n", + ) + r = HookPrebuiltResolver(provider="hook-prebuilt") + + with mock.patch("fromager.overrides.find_override_method", return_value=None): + result = r.download(tmp_context, _REQ, _CANDIDATE_WHEEL) + + assert result == DownloadedSource(path=path, kind=DownloadKind.prebuilt_wheel) @mock.patch("fromager.packagesettings._resolver.overrides.find_override_method") @@ -911,3 +1190,16 @@ def test_missing_provider() -> None: source: index_url: https://pypi.test/simple """) + + +# -- DownloadedSource --------------------------------------------------------- + + +def test_downloaded_source_is_frozen() -> None: + ds = DownloadedSource( + path=pathlib.Path("/tmp/test.tar.gz"), kind=DownloadKind.sdist + ) + assert ds.path == pathlib.Path("/tmp/test.tar.gz") + assert ds.kind is DownloadKind.sdist + with pytest.raises(dataclasses.FrozenInstanceError): + ds.path = pathlib.Path("/other") # type: ignore[misc] diff --git a/tests/test_sources.py b/tests/test_sources.py index 40cdb7af..94d0ca63 100644 --- a/tests/test_sources.py +++ b/tests/test_sources.py @@ -861,7 +861,9 @@ def test_download_source_regular_package( download_url="https://pkg.test/pkg-1.0.tar.gz", ) - assert result == (expected, packagesettings.DownloadKind.sdist) + assert result == packagesettings.DownloadedSource( + path=expected, kind=packagesettings.DownloadKind.sdist + ) mock_invoke.assert_called_once() @@ -875,11 +877,14 @@ def test_download_source_uses_configured_source_resolver( req = Requirement("pkg==1.0") version = Version("1.0") download_url = "https://pkg.test/pkg-1.0.tar.gz" + expected_result = packagesettings.DownloadedSource( + path=expected, kind=packagesettings.DownloadKind.sdist + ) with ( patch.object( packagesettings.PyPISDistResolver, "_download", - return_value=(expected, packagesettings.DownloadKind.sdist), + return_value=expected_result, ) as mock_download, patch.object( packagesettings.PackageBuildInfo, @@ -894,7 +899,7 @@ def test_download_source_uses_configured_source_resolver( ) expected_candidate = Candidate(name=req.name, version=version, url=download_url) - assert result == (expected, packagesettings.DownloadKind.sdist) + assert result == expected_result mock_download.assert_called_once_with( tmp_context, req, @@ -904,11 +909,15 @@ def test_download_source_uses_configured_source_resolver( mock_invoke.assert_not_called() -def test_download_source_returns_configured_result_without_validation( +def test_download_source_passes_configured_result_through( tmp_context: context.WorkContext, + tmp_path: pathlib.Path, ) -> None: - """Leave configured resolver result validation to the follow-up phase.""" - configured_result = "resolver-owned-result" + """Pass the configured resolver result through without modification.""" + configured_result = packagesettings.DownloadedSource( + path=tmp_path / "pkg-1.0.tar.gz", + kind=packagesettings.DownloadKind.tarball, + ) source_resolver = Mock() source_resolver.download.return_value = configured_result @@ -925,7 +934,49 @@ def test_download_source_returns_configured_result_without_validation( download_url="https://pkg.test/pkg-1.0.tar.gz", ) - assert result == configured_result + assert result is configured_result + + +def test_download_source_preserves_configured_hook_result( + tmp_context: context.WorkContext, +) -> None: + """Carry the explicit hook artifact kind through source dispatch.""" + req = Requirement("pkg==1.0") + version = Version("1.0") + download_url = "git+https://git.test/pkg.git@v1.0" + expected = packagesettings.DownloadedSource( + path=tmp_context.work_dir, + kind=packagesettings.DownloadKind.git_checkout, + ) + + def hook( + ctx: context.WorkContext, + req: Requirement, + version: Version, + download_url: str, + sdists_downloads_dir: pathlib.Path, + ) -> packagesettings.DownloadedSource: + assert ctx is tmp_context + assert req == Requirement("pkg==1.0") + assert version == Version("1.0") + assert download_url == "git+https://git.test/pkg.git@v1.0" + assert sdists_downloads_dir == tmp_context.sdists_downloads + return expected + + with ( + patch.object( + packagesettings.PackageBuildInfo, + "source_resolver", + new_callable=PropertyMock, + return_value=packagesettings.HookSDistResolver(provider="hook-sdist"), + ), + patch("fromager.overrides.find_override_method", return_value=hook), + ): + result = sources.download_source( + ctx=tmp_context, req=req, version=version, download_url=download_url + ) + + assert result is expected @patch("fromager.overrides.find_and_invoke", return_value="not-a-path")