From 1b790d8e19e8b1413ca58ac60058a4ee628aa2d5 Mon Sep 17 00:00:00 2001 From: Christian Heimes Date: Fri, 3 Jul 2026 08:42:29 +0200 Subject: [PATCH] refactor(downloads): centralize download helpers in new module Create `fromager.downloads` as a single import point for all download helpers, avoiding cyclic import issues between `sources`, `wheels`, and `resolver`. Move `download_url` and `extract_filename_from_url` from `sources` and `resolver` into the new module. Add `download_sdist` and `download_wheel` that validate filenames with `parse_sdist_filename` / `parse_wheel_filename` before downloading and verify archives are non-empty afterwards. Add `download_git_source` for cloning from pip VCS URLs using `git_clone_fast` with destination directory conflict detection. Improve `download_url` temp file handling: use `NamedTemporaryFile` in the target directory with `os.rename` for atomic moves. Replace `_download_source_check` and `_download_wheel_check` with the new `download_sdist` and `download_wheel` functions. Co-Authored-By: Claude Signed-off-by: Christian Heimes --- pyproject.toml | 2 +- src/fromager/commands/build.py | 4 +- src/fromager/downloads.py | 225 ++++++++++++++++++++++++++++++++ src/fromager/gitutils.py | 7 +- src/fromager/resolver.py | 10 +- src/fromager/sources.py | 159 +++++------------------ src/fromager/wheels.py | 23 +--- tests/test_downloads.py | 229 +++++++++++++++++++++++++++++++++ tests/test_sources.py | 37 ++---- tests/test_wheels.py | 27 +--- 10 files changed, 515 insertions(+), 208 deletions(-) create mode 100644 src/fromager/downloads.py create mode 100644 tests/test_downloads.py diff --git a/pyproject.toml b/pyproject.toml index 67b789597..ea3ce5f45 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -208,7 +208,7 @@ exclude = [ [[tool.mypy.overrides]] # packages without typing annotations and stubs -module = ["hatchling", "hatchling.build", "license_expression", "pyproject_hooks", "requests_mock", "resolver", "spdx_tools.*", "stevedore"] +module = ["hatchling", "hatchling.build", "license_expression", "pyproject_hooks", "requests_mock", "resolver", "spdx_tools.*", "stevedore", "wheel.*"] ignore_missing_imports = true [tool.basedpyright] diff --git a/src/fromager/commands/build.py b/src/fromager/commands/build.py index 204f01ac8..58bf2f6bd 100644 --- a/src/fromager/commands/build.py +++ b/src/fromager/commands/build.py @@ -23,6 +23,7 @@ clickext, context, dependency_graph, + downloads, hooks, metrics, overrides, @@ -33,7 +34,6 @@ wheels, ) -from .. import resolver from ..log import VERBOSE_LOG_FMT, ThreadLogFilter, req_ctxvar_context logger = logging.getLogger(__name__) @@ -495,7 +495,7 @@ def _is_wheel_built( pbi = wkctx.package_build_info(req) build_tag_from_settings = pbi.build_tag(resolved_version) build_tag = build_tag_from_settings if build_tag_from_settings else (0, "") - wheel_basename = resolver.extract_filename_from_url(url) + wheel_basename = downloads.extract_filename_from_url(url) _, _, build_tag_from_name, _ = parse_wheel_filename(wheel_basename) existing_build_tag = build_tag_from_name if build_tag_from_name else (0, "") if ( diff --git a/src/fromager/downloads.py b/src/fromager/downloads.py new file mode 100644 index 000000000..82dd7b8da --- /dev/null +++ b/src/fromager/downloads.py @@ -0,0 +1,225 @@ +from __future__ import annotations + +import logging +import os +import pathlib +import tarfile +import tempfile +import typing +import zipfile +from urllib.parse import unquote, urlparse + +from packaging.utils import parse_sdist_filename, parse_wheel_filename +from wheel import wheelfile + +from . import gitutils +from .http_retry import RETRYABLE_EXCEPTIONS, retry_on_exception +from .request_session import session + +logger = logging.getLogger(__name__) + + +def extract_filename_from_url(url: str) -> str: + """Extract filename from URL and decode it.""" + path = urlparse(url).path + filename = os.path.basename(path) + return unquote(filename) + + +def download_url( + *, + destination_dir: pathlib.Path, + url: str, + destination_filename: str | None = None, +) -> pathlib.Path: + """Download a URL to destination_dir, returning the local path. + + Returns immediately if the file already exists. Downloads to a + temporary file in the same directory first and uses :func:`os.rename` + for an atomic move on success, avoiding partial files. Retries on + transient network errors. + + .. versionadded:: 0.90.0 + """ + basename = ( + destination_filename if destination_filename else extract_filename_from_url(url) + ) + outfile = destination_dir / basename + logger.debug( + "looking for %s %s", + outfile, + "(exists)" if outfile.exists() else "(not there)", + ) + if outfile.exists(): + logger.debug("already have %s", outfile) + return outfile + + destination_dir.mkdir(parents=True, exist_ok=True) + + @retry_on_exception( + exceptions=RETRYABLE_EXCEPTIONS, + max_attempts=5, + backoff_factor=1.5, + max_backoff=120.0, + ) + def _download_with_retry() -> pathlib.Path: + """Download to a temporary file, then atomically rename.""" + logger.debug("reading from %s", url) + # NamedTemporaryFile in the same directory ensures os.rename() is + # atomic (same filesystem). delete=False so we control cleanup. + # Temp filename looks like ".tmp.foo-1.2.tar.gz.abc123def". + tmp = tempfile.NamedTemporaryFile( + dir=destination_dir, + prefix=f".tmp.{basename}.", + delete=False, + ) + temp_path = pathlib.Path(tmp.name) + try: + with session.get(url, stream=True) as r: + r.raise_for_status() + logger.debug("writing to %s", temp_path) + for chunk in r.iter_content(chunk_size=64 * 1024): + if chunk: + tmp.write(chunk) + tmp.close() + # Atomic rename on the same filesystem + os.rename(temp_path, outfile) + logger.info("saved %s", outfile) + return outfile + except BaseException: + tmp.close() + temp_path.unlink(missing_ok=True) + raise + + result: pathlib.Path = _download_with_retry() + return result + + +def download_sdist( + *, + destination_dir: pathlib.Path, + url: str, + destination_filename: str | None = None, +) -> pathlib.Path: + """Download a source distribution and verify it. + + Accepts ``.tar.gz`` and ``.zip`` sdists. Validates the filename + with :func:`packaging.utils.parse_sdist_filename` before + downloading and checks that the archive is non-empty afterwards. + + .. note:: + + The function does not validate ``PKG-INFO`` metadata or the + required top-level directory described in the `source distribution + file format + `_ + specification. + + .. versionadded:: 0.90.0 + """ + basename = ( + destination_filename if destination_filename else extract_filename_from_url(url) + ) + # Validates name, version, and extension (.tar.gz or .zip); + # raises InvalidSdistFilename on malformed names. + parse_sdist_filename(basename) + + filepath = download_url( + destination_dir=destination_dir, + url=url, + destination_filename=basename, + ) + + if basename.endswith(".tar.gz"): + with tarfile.open(filepath, mode="r:gz") as tar: + if tar.next() is None: + raise tarfile.TarError(f"empty tar file: {filepath}") + elif basename.endswith(".zip"): + with zipfile.ZipFile(filepath) as zf: + if not zf.namelist(): + raise zipfile.BadZipFile(f"empty zip file: {filepath}") + else: + typing.assert_never(basename) # type: ignore[arg-type] + + return filepath + + +def download_wheel( + *, + destination_dir: pathlib.Path, + url: str, + destination_filename: str | None = None, +) -> pathlib.Path: + """Download a wheel and verify it. + + Validates that the filename is a valid wheel name (using + :func:`packaging.utils.parse_wheel_filename`) before downloading, + then checks that the zip archive is non-empty. + + .. note:: + + The function does not validate dist-info ``METADATA`` and other + files described in the `binary distribution format + `_ + specification. + + .. versionadded:: 0.90.0 + """ + basename = ( + destination_filename if destination_filename else extract_filename_from_url(url) + ) + # Validates name, version, build tag, tags, and .whl extension; + # raises InvalidWheelFilename on malformed names. + parse_wheel_filename(basename) + + filepath = download_url( + destination_dir=destination_dir, + url=url, + destination_filename=basename, + ) + + # validate wheel file: WheelFile checks for dist-info dir and RECORD. + # NOTE: Does not validate that METADATA file is valid or consistent with + # wheel's distribution name and version. + # https://packaging.python.org/en/latest/specifications/binary-distribution-format/ + with wheelfile.WheelFile(filepath) as wf: + di_metadata = f"{wf.dist_info_path}/METADATA" + if di_metadata not in wf.namelist(): + raise wheelfile.WheelError(f"{di_metadata!r} missing") + + return filepath + + +def download_git_source( + *, + destination_dir: pathlib.Path, + vcs_url: str, + require_ref: bool = True, +) -> pathlib.Path: + """Clone a git repository from a pip VCS URL. + + Parses *vcs_url* with :func:`~fromager.gitutils.parse_vcs_url` + and clones using :func:`~fromager.gitutils.git_clone_fast` + with recursive submodules. + + Raises ``FileExistsError`` if the destination directory already + exists. + + .. versionadded:: 0.90.0 + """ + repo_url, ref = gitutils.parse_vcs_url(vcs_url, require_ref=require_ref) + + # git clone fails when the target directory is not empty. + try: + destination_dir.mkdir(parents=True, exist_ok=False) + except FileExistsError: + raise FileExistsError( + f"destination directory {destination_dir} already exists, " + f"cannot clone {vcs_url}" + ) from None + gitutils.git_clone_fast( + output_dir=destination_dir, + repo_url=repo_url, + ref=ref, + ) + return destination_dir diff --git a/src/fromager/gitutils.py b/src/fromager/gitutils.py index af5aa7cd0..2d554a6a1 100644 --- a/src/fromager/gitutils.py +++ b/src/fromager/gitutils.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import logging import pathlib import typing @@ -5,7 +7,10 @@ from packaging.requirements import Requirement -from fromager import context, external_commands +from fromager import external_commands + +if typing.TYPE_CHECKING: + from fromager import context logger = logging.getLogger(__name__) diff --git a/src/fromager/resolver.py b/src/fromager/resolver.py index d51b9decc..574c01317 100644 --- a/src/fromager/resolver.py +++ b/src/fromager/resolver.py @@ -14,7 +14,7 @@ from collections.abc import Iterable from operator import attrgetter from platform import python_version -from urllib.parse import quote, unquote, urlparse +from urllib.parse import quote, urlparse import pypi_simple import resolvelib @@ -33,6 +33,7 @@ from . import overrides from .candidate import Candidate, Cooldown from .constraints import Constraints +from .downloads import extract_filename_from_url as extract_filename_from_url from .extras_provider import ExtrasProvider from .http_retry import RETRYABLE_EXCEPTIONS, retry_on_exception from .request_session import session @@ -204,13 +205,6 @@ def _compute_max_age_cutoff( return bootstrap_time - ctx.max_release_age -def extract_filename_from_url(url: str) -> str: - """Extract filename from URL and decode it.""" - path = urlparse(url).path - filename = os.path.basename(path) - return unquote(filename) - - class LogReporter(resolvelib.BaseReporter): """Report resolution events diff --git a/src/fromager/sources.py b/src/fromager/sources.py index af165ce03..cfb600b8f 100644 --- a/src/fromager/sources.py +++ b/src/fromager/sources.py @@ -7,6 +7,7 @@ import shutil import tarfile import typing +import warnings import zipfile import resolvelib @@ -15,12 +16,11 @@ parse_sdist_filename, ) from packaging.version import Version -from requests.exceptions import ChunkedEncodingError, ConnectionError -from urllib3.exceptions import IncompleteRead, ProtocolError from . import ( build_environment, dependencies, + downloads, external_commands, gitutils, metrics, @@ -31,8 +31,6 @@ tarballs, vendor_rust, ) -from .http_retry import RETRYABLE_EXCEPTIONS, retry_on_exception -from .request_session import session from .requirements_file import RequirementType, SourceType if typing.TYPE_CHECKING: @@ -41,6 +39,32 @@ logger = logging.getLogger(__name__) +def download_url( + *, + req: Requirement, + destination_dir: pathlib.Path, + url: str, + destination_filename: str | None = None, +) -> pathlib.Path: + """Download a URL to destination_dir, returning the local path. + + .. deprecated:: + Use :func:`fromager.downloads.download_url` instead. + The ``req`` parameter is unused and will be removed. + """ + warnings.warn( + "fromager.sources.download_url() is deprecated, " + "use fromager.downloads.download_url() instead", + DeprecationWarning, + stacklevel=2, + ) + return downloads.download_url( + destination_dir=destination_dir, + url=url, + destination_filename=destination_filename, + ) + + def get_source_type(ctx: context.WorkContext, req: Requirement) -> SourceType: source_type = SourceType.SDIST if req.url: @@ -194,7 +218,7 @@ def default_download_source( if url is None: raise ValueError(f"Could not determine download URL for {req}") if destination_filename is None: - url_filename = resolver.extract_filename_from_url(url) + url_filename = downloads.extract_filename_from_url(url) if url_filename.endswith(".zip"): destination_filename = f"{pbi.override_module_name}-{version}.zip" else: @@ -202,8 +226,7 @@ def default_download_source( logger.debug( "config has no destination_filename, use default %r", destination_filename ) - source_filename = _download_source_check( - req=req, + source_filename = downloads.download_sdist( destination_dir=sdists_downloads_dir, url=url, destination_filename=destination_filename, @@ -244,128 +267,6 @@ def download_git_source( ) -# Helper method to check whether .zip /.tar / .tgz is able to extract and check its content. -# It will throw exception if any other file is encountered. Eg: index.html -def _download_source_check( - req: Requirement, - destination_dir: pathlib.Path, - url: str, - destination_filename: str | None = None, -) -> pathlib.Path: - source_filename = download_url( - req=req, - destination_dir=destination_dir, - url=url, - destination_filename=destination_filename, - ) - if source_filename.suffix == ".zip": - with zipfile.ZipFile(source_filename) as zip_file: - source_file_contents = zip_file.namelist() - if not source_file_contents: - raise zipfile.BadZipFile( - f"Empty zip file encountered: {source_filename}" - ) - elif ( - source_filename.suffix == ".tgz" - or source_filename.suffix == ".gz" - or str(source_filename).endswith(".tar.gz") - ): - with tarfile.open(source_filename) as tar: - if not tar.next(): - raise tarfile.TarError(f"Empty tar file encountered: {source_filename}") - else: - raise ValueError( - f"The source file encountered is not a zip or tar file: {source_filename}" - ) - return source_filename - - -def download_url( - *, - req: Requirement, - destination_dir: pathlib.Path, - url: str, - destination_filename: str | None = None, -) -> pathlib.Path: - """Download a URL to destination_dir, returning the local path. - - Returns immediately if the file already exists. Downloads to a - temporary file first and renames it on success to avoid leaving - partial files. Retries on transient network errors. - """ - basename = ( - destination_filename - if destination_filename - else resolver.extract_filename_from_url(url) - ) - outfile = pathlib.Path(destination_dir) / basename - logger.debug( - "looking for %s %s", - outfile, - "(exists)" if outfile.exists() else "(not there)", - ) - if outfile.exists(): - logger.debug("already have %s", outfile) - return outfile - - # Create a temporary file to avoid partial downloads - temp_file = outfile.with_suffix(outfile.suffix + ".tmp") - - def _download_with_retry() -> pathlib.Path: - """Internal function that performs the actual download with retry logic.""" - logger.debug(f"reading from {url}") - try: - with session.get(url, stream=True) as r: - r.raise_for_status() - with open(temp_file, "wb") as f: - logger.debug("writing to %s", temp_file) - # Use smaller chunk size for better error recovery - for chunk in r.iter_content(chunk_size=64 * 1024): - if chunk: # Filter out keep-alive chunks - f.write(chunk) - - # Only move to final location if download completed successfully - temp_file.rename(outfile) - logger.info("saved %s", outfile) - return outfile - - except ( - ChunkedEncodingError, - IncompleteRead, - ProtocolError, - ConnectionError, - ) as e: - # Clean up partial file on failure - if temp_file.exists(): - temp_file.unlink() - logger.warning(f"Download failed for {url}: {e}") - raise - except Exception: - # Clean up partial file on any other failure - if temp_file.exists(): - temp_file.unlink() - raise - - # Apply retry logic specifically for download operations - @retry_on_exception( - exceptions=RETRYABLE_EXCEPTIONS, - max_attempts=5, - backoff_factor=1.5, - max_backoff=120.0, - ) - def download_with_retry() -> pathlib.Path: - return _download_with_retry() - - try: - result: pathlib.Path = download_with_retry() - return result - except Exception: - # Ensure temp file is cleaned up if it still exists - if temp_file.exists(): - temp_file.unlink() - raise - - def _takes_arg(f: typing.Callable, arg_name: str) -> bool: sig = inspect.signature(f) return arg_name in sig.parameters diff --git a/src/fromager/wheels.py b/src/fromager/wheels.py index 0998a9dea..45d95eef4 100644 --- a/src/fromager/wheels.py +++ b/src/fromager/wheels.py @@ -12,7 +12,6 @@ import elfdeps import tomlkit -import wheel.wheelfile # type: ignore from packaging.requirements import Requirement from packaging.tags import Tag from packaging.utils import ( @@ -24,6 +23,7 @@ from . import ( dependencies, + downloads, external_commands, metrics, overrides, @@ -31,7 +31,6 @@ requirements_file, resolver, sbom, - sources, ) if typing.TYPE_CHECKING: @@ -454,10 +453,13 @@ def download_wheel( wheel_url: str, output_directory: pathlib.Path, ) -> pathlib.Path: - wheel_filename = output_directory / resolver.extract_filename_from_url(wheel_url) + wheel_filename = output_directory / downloads.extract_filename_from_url(wheel_url) if not wheel_filename.exists(): logger.info(f"downloading pre-built wheel {wheel_url}") - wheel_filename = _download_wheel_check(req, output_directory, wheel_url) + wheel_filename = downloads.download_wheel( + destination_dir=output_directory, + url=wheel_url, + ) logger.info(f"saved wheel to {wheel_filename}") else: logger.info(f"have existing wheel {wheel_filename}") @@ -465,19 +467,6 @@ def download_wheel( return wheel_filename -def _download_wheel_check( - req: Requirement, destination_dir: pathlib.Path, wheel_url: str -) -> pathlib.Path: - wheel_filename = sources.download_url( - req=req, - destination_dir=destination_dir, - url=wheel_url, - ) - # validates whether the wheel is correct or not. will raise an error in the wheel is invalid - wheel.wheelfile.WheelFile(wheel_filename) - return wheel_filename - - def get_wheel_server_urls( ctx: context.WorkContext, req: Requirement, *, cache_wheel_server_url: str | None ) -> list[str]: diff --git a/tests/test_downloads.py b/tests/test_downloads.py new file mode 100644 index 000000000..27ad1d97f --- /dev/null +++ b/tests/test_downloads.py @@ -0,0 +1,229 @@ +from __future__ import annotations + +import io +import pathlib +import tarfile +import typing +import zipfile +from unittest.mock import Mock, patch + +import pytest +import requests_mock +from packaging.utils import InvalidSdistFilename, InvalidWheelFilename +from wheel.wheelfile import WheelFile + +from fromager.downloads import ( + download_git_source, + download_sdist, + download_url, + download_wheel, + extract_filename_from_url, +) + +# -- extract_filename_from_url ------------------------------------------------ + + +@pytest.mark.parametrize( + "url, expected", + [ + ("https://pkg.test/simple/pkg-1.0.tar.gz", "pkg-1.0.tar.gz"), + ("https://pkg.test/path/to/file.zip", "file.zip"), + ("https://pkg.test/pkg-1.0%2Blocal.tar.gz", "pkg-1.0+local.tar.gz"), + ], +) +def test_extract_filename_from_url(url: str, expected: str) -> None: + assert extract_filename_from_url(url) == expected + + +# -- download_url ------------------------------------------------------------- + +_PKG_URL = "https://pkg.test/pkg-1.0.tar.gz" + + +def test_download_url_creates_file( + requests_mock: requests_mock.Mocker, tmp_path: pathlib.Path +) -> None: + requests_mock.get(_PKG_URL, content=b"data") + result = download_url(destination_dir=tmp_path, url=_PKG_URL) + assert result == tmp_path / "pkg-1.0.tar.gz" + assert result.read_bytes() == b"data" + + +def test_download_url_skips_existing(tmp_path: pathlib.Path) -> None: + (tmp_path / "pkg-1.0.tar.gz").write_bytes(b"old") + result = download_url(destination_dir=tmp_path, url=_PKG_URL) + assert result.read_bytes() == b"old" + + +def test_download_url_destination_filename( + requests_mock: requests_mock.Mocker, tmp_path: pathlib.Path +) -> None: + requests_mock.get(_PKG_URL, content=b"data") + result = download_url( + destination_dir=tmp_path, url=_PKG_URL, destination_filename="custom.tar.gz" + ) + assert result.name == "custom.tar.gz" + + +def test_download_url_creates_parent_dirs( + requests_mock: requests_mock.Mocker, tmp_path: pathlib.Path +) -> None: + requests_mock.get(_PKG_URL, content=b"data") + assert download_url(destination_dir=tmp_path / "a" / "b", url=_PKG_URL).exists() + + +def test_download_url_cleans_up_on_failure( + requests_mock: requests_mock.Mocker, tmp_path: pathlib.Path +) -> None: + requests_mock.get(_PKG_URL, exc=ConnectionError("fail")) + with pytest.raises(ConnectionError): + download_url(destination_dir=tmp_path, url=_PKG_URL) + assert list(tmp_path.glob(".*")) == [] + + +# -- download_sdist ----------------------------------------------------------- + +_MOCK_DOWNLOAD_URL = "fromager.downloads.download_url" + + +def _make_targz(path: pathlib.Path) -> None: + with tarfile.open(path, "w:gz") as tar: + info = tarfile.TarInfo(name="dummy.txt") + info.size = 4 + tar.addfile(info, io.BytesIO(b"test")) + + +def _make_zip(path: pathlib.Path) -> None: + with zipfile.ZipFile(path, "w") as zf: + zf.writestr("dummy.txt", "test") + + +def _patch_download(filepath: pathlib.Path) -> typing.Any: + return patch(_MOCK_DOWNLOAD_URL, return_value=filepath) + + +@pytest.mark.parametrize( + "filename, maker", + [ + ("pkg-1.0.tar.gz", _make_targz), + ("pkg-1.0.zip", _make_zip), + ], +) +def test_download_sdist_valid( + tmp_path: pathlib.Path, + filename: str, + maker: typing.Callable[[pathlib.Path], None], +) -> None: + f = tmp_path / filename + maker(f) + with _patch_download(f): + result = download_sdist( + destination_dir=tmp_path, url=f"https://pkg.test/{filename}" + ) + assert result == f + + +def test_download_sdist_rejects_invalid_name(tmp_path: pathlib.Path) -> None: + with pytest.raises(InvalidSdistFilename): + download_sdist( + destination_dir=tmp_path, + url="https://pkg.test/pkg.exe", + ) + + +@pytest.mark.parametrize( + "filename, maker, exc", + [ + ("pkg-1.0.tar.gz", lambda p: tarfile.open(p, "w:gz").close(), tarfile.TarError), + ("pkg-1.0.zip", lambda p: zipfile.ZipFile(p, "w").close(), zipfile.BadZipFile), + ], +) +def test_download_sdist_empty( + tmp_path: pathlib.Path, + filename: str, + maker: typing.Callable[[pathlib.Path], None], + exc: type[Exception], +) -> None: + f = tmp_path / filename + maker(f) + with _patch_download(f): + with pytest.raises(exc, match="empty"): + download_sdist(destination_dir=tmp_path, url=f"https://pkg.test/{filename}") + + +# -- download_wheel ----------------------------------------------------------- + +_WHEEL_FILENAME = "pkg-1.0-py3-none-any.whl" +_WHEEL_URL = f"https://pkg.test/{_WHEEL_FILENAME}" + + +def _make_wheel(path: pathlib.Path, name: str = "pkg", version: str = "1.0") -> None: + """Create a minimal valid wheel with METADATA, WHEEL, and RECORD.""" + with WheelFile(path, "w") as wf: + wf.writestr( + f"{wf.dist_info_path}/METADATA", + f"Metadata-Version: 1.0\nName: {name}\nVersion: {version}\n", + ) + wf.writestr( + f"{wf.dist_info_path}/WHEEL", + "Wheel-Version: 1.0\n", + ) + + +def test_download_wheel_valid(tmp_path: pathlib.Path) -> None: + f = tmp_path / _WHEEL_FILENAME + _make_wheel(f) + with _patch_download(f): + result = download_wheel( + destination_dir=tmp_path, + url=_WHEEL_URL, + ) + assert result == f + + +def test_download_wheel_rejects_invalid_name(tmp_path: pathlib.Path) -> None: + with pytest.raises(InvalidWheelFilename): + download_wheel( + destination_dir=tmp_path, + url="https://pkg.test/bad-name.whl", + ) + + +def test_download_wheel_missing_metadata(tmp_path: pathlib.Path) -> None: + f = tmp_path / _WHEEL_FILENAME + with WheelFile(f, "w") as wf: + wf.writestr(f"{wf.dist_info_path}/WHEEL", "Wheel-Version: 1.0\n") + with _patch_download(f): + with pytest.raises(Exception, match="METADATA"): + download_wheel( + destination_dir=tmp_path, + url=_WHEEL_URL, + ) + + +# -- download_git_source ------------------------------------------------------ + +_MOCK_GIT_CLONE = "fromager.downloads.gitutils.git_clone_fast" +_GIT_BASE = "git+https://github.test/org/repo.git" + + +@patch(_MOCK_GIT_CLONE) +def test_git_source_clones(mock_clone: Mock, tmp_path: pathlib.Path) -> None: + dest = tmp_path / "repo" + download_git_source( + destination_dir=dest, + vcs_url=f"{_GIT_BASE}@refs/tags/v1.0", + ) + assert dest.is_dir() + mock_clone.assert_called_once_with( + output_dir=dest, + repo_url="https://github.test/org/repo.git", + ref="refs/tags/v1.0", + ) + + +def test_git_source_dest_exists(tmp_path: pathlib.Path) -> None: + dest = tmp_path / "repo" + dest.mkdir() + with pytest.raises(FileExistsError, match="already exists"): + download_git_source(destination_dir=dest, vcs_url=f"{_GIT_BASE}@v1.0") diff --git a/tests/test_sources.py b/tests/test_sources.py index 239ce9ca1..38746d02e 100644 --- a/tests/test_sources.py +++ b/tests/test_sources.py @@ -1,7 +1,6 @@ import pathlib import sys import tarfile -import typing import zipfile from unittest.mock import Mock, patch @@ -12,28 +11,15 @@ from fromager import context, packagesettings, resolver, sources -@patch("fromager.sources.download_url") -def test_invalid_tarfile(mock_download_url: typing.Any, tmp_path: pathlib.Path) -> None: - mock_download_url.return_value = pathlib.Path(tmp_path / "test" / "fake_wheel.txt") - fake_url = "https://www.thisisafakeurl.com" - fake_dir = tmp_path / "test" - fake_dir.mkdir() - text_file = fake_dir / "fake_wheel.txt" - text_file.write_text("This is a test file") - req = Requirement("test_pkg==42.1.2") - with pytest.raises(ValueError): - sources._download_source_check(req=req, destination_dir=fake_dir, url=fake_url) - - @patch("fromager.resolver.find_all_matching_from_provider") -@patch("fromager.sources._download_source_check") +@patch("fromager.downloads.download_sdist") def test_resolve_source_from_settings( - download_source_check: Mock, + download_sdist: Mock, find_all_matching_from_provider: Mock, testdata_context: context.WorkContext, ) -> None: find_all_matching_from_provider.return_value = [("url", Version("42.1.2"))] - download_source_check.return_value = pathlib.Path("filename.zip") + download_sdist.return_value = pathlib.Path("filename.zip") req = Requirement("test_pkg==42.1.2") sdist_server_url = "https://sdist.test/egg" @@ -41,7 +27,6 @@ def test_resolve_source_from_settings( ctx=testdata_context, req=req, sdist_server_url=sdist_server_url ) - # Verify we got the expected result assert url == "url" assert version == Version("42.1.2") @@ -53,8 +38,7 @@ def test_resolve_source_from_settings( testdata_context.sdists_downloads, ) - download_source_check.assert_called_with( - req=req, + download_sdist.assert_called_with( destination_dir=testdata_context.sdists_downloads, url="https://egg.test/test-pkg/v42.1.2.tar.gz", destination_filename="test-pkg-42.1.2.tar.gz", @@ -62,7 +46,7 @@ def test_resolve_source_from_settings( @patch("fromager.resolver.find_all_matching_from_provider") -@patch("fromager.sources._download_source_check") +@patch("fromager.downloads.download_sdist") @patch.multiple( packagesettings.PackageBuildInfo, resolver_include_sdists=False, @@ -70,12 +54,12 @@ def test_resolve_source_from_settings( resolver_sdist_server_url=Mock(return_value="url"), ) def test_resolve_source_with_predefined_resolve_dist( - download_source_check: Mock, + download_sdist: Mock, find_all_matching_from_provider: Mock, tmp_context: context.WorkContext, ) -> None: find_all_matching_from_provider.return_value = [("url", Version("1.0"))] - download_source_check.return_value = pathlib.Path("filename") + download_sdist.return_value = pathlib.Path("filename") req = Requirement("foo==1.0") url, version = sources.resolve_source( @@ -185,9 +169,9 @@ def test_patch_sources_apply_only_unversioned( ), ], ) -@patch("fromager.sources._download_source_check") +@patch("fromager.downloads.download_sdist") def test_default_download_source_no_destination_filename( - download_source_check: Mock, + download_sdist: Mock, tmp_context: context.WorkContext, pkg: str, version_str: str, @@ -202,8 +186,7 @@ def test_default_download_source_no_destination_filename( tmp_context, req, version, url, tmp_context.sdists_downloads ) - download_source_check.assert_called_with( - req=req, + download_sdist.assert_called_with( destination_dir=tmp_context.sdists_downloads, url=url, destination_filename=expected_filename, diff --git a/tests/test_wheels.py b/tests/test_wheels.py index 2b5f7f33d..6dd948aa1 100644 --- a/tests/test_wheels.py +++ b/tests/test_wheels.py @@ -3,30 +3,14 @@ from unittest.mock import Mock, patch import pytest -import wheel.wheelfile # type: ignore from conftest import make_sbom_ctx from packaging.requirements import Requirement from packaging.version import Version -from fromager import build_environment, context, sources, wheels +from fromager import build_environment, context, downloads, wheels from fromager.packagesettings import SbomSettings -@patch("fromager.sources.download_url") -def test_invalid_wheel_file_exception( - mock_download_url: Mock, tmp_path: pathlib.Path -) -> None: - mock_download_url.return_value = pathlib.Path(tmp_path / "test" / "fake_wheel.txt") - fake_url = "https://www.thisisafakeurl.com" - fake_dir = tmp_path / "test" - fake_dir.mkdir() - text_file = fake_dir / "fake_wheel.txt" - text_file.write_text("This is a test file") - req = Requirement("test_pkg") - with pytest.raises(wheel.wheelfile.WheelError): - wheels._download_wheel_check(req, fake_dir, fake_url) - - @patch("pyproject_hooks.BuildBackendHookCaller.build_wheel") def test_default_build_wheel( mock_build_wheel: Mock, @@ -247,9 +231,8 @@ def test_download_wheel_unquotes_url_encoded_filenames(tmp_path: pathlib.Path) - assert result_filename == expected_filename -def test_sources_download_url_unquotes_filenames(tmp_path: pathlib.Path) -> None: - """Test that sources.download_url properly unquotes URL-encoded characters in filenames.""" - req = Requirement("test_pkg") +def test_download_url_unquotes_filenames(tmp_path: pathlib.Path) -> None: + """Test that downloads.download_url properly unquotes URL-encoded characters in filenames.""" # URL with encoded plus sign (%2B) url = "https://example.test/test_pkg-1.0%2Blocal.tar.gz" @@ -260,9 +243,7 @@ def test_sources_download_url_unquotes_filenames(tmp_path: pathlib.Path) -> None mock_response.iter_content.return_value = [b"test content"] mock_get.return_value.__enter__.return_value = mock_response - result_filename = sources.download_url( - req=req, destination_dir=tmp_path, url=url - ) + result_filename = downloads.download_url(destination_dir=tmp_path, url=url) # The filename should be unquoted, containing actual + character expected_filename = tmp_path / "test_pkg-1.0+local.tar.gz"