diff --git a/docs/hooks.rst b/docs/hooks.rst index fb2a6616..653ded97 100644 --- a/docs/hooks.rst +++ b/docs/hooks.rst @@ -230,9 +230,7 @@ Source hooks .. autofromagerhook:: default_build_sdist The ``build_sdist()`` function is responsible for creating a new source - distribution from the prepared source tree and placing it in - ``ctx.sdists_build``. The dist name and version of the sdist file must - match the ``Requirement`` and ``Version``. + distribution from the prepared source tree and placing it in ``ctx.sdists_build``. The arguments are the ``WorkContext``, the ``Requirement`` being evaluated, and the `Path` to the root of the source tree. @@ -249,8 +247,8 @@ Wheel hooks The ``build_wheel()`` function is responsible for creating a wheel from the prepared source tree and placing it in ``ctx.wheels_build``. The - default implementation uses :pep:`517` pyproject hook. The dist name - and version of the wheel must match the ``Requirement`` and ``Version``. + default implementation invokes ``pip wheel`` in a temporary directory + and passes the path to the source tree as argument. The arguments are the ``WorkContext``, the ``Path`` to a virtualenv prepared with the build dependencies, a ``dict`` with extra environment diff --git a/src/fromager/commands/lint_requirements.py b/src/fromager/commands/lint_requirements.py index 3aa54ea5..3ad731b1 100644 --- a/src/fromager/commands/lint_requirements.py +++ b/src/fromager/commands/lint_requirements.py @@ -5,9 +5,7 @@ import click from packaging.requirements import InvalidRequirement, Requirement -from fromager import bootstrapper, context, progress, requirements_file -from fromager.log import requirement_ctxvar -from fromager.requirements_file import RequirementType +from fromager import requirements_file logger = logging.getLogger(__name__) @@ -19,17 +17,13 @@ required=True, type=click.Path(exists=False, path_type=pathlib.Path), ) -@click.pass_obj -def lint_requirements( - wkctx: context.WorkContext, input_files_path: list[pathlib.Path] -) -> None: +def lint_requirements(input_files_path: list[pathlib.Path]) -> None: """ Command to lint the constraints.txt and requirements.txt files This command takes a single wildcard path string for constraints.txt and requirements.txt. It checks the formatting of these files and reports issues if found. Files with names that end with constraints.txt (e.g. constraints.txt, global-constraints.txt, etc.) are not allowed - to contain extra dependencies. Additionally, it resolves valid input requirements to ensure - we can find a matching version of each package. + to contain extra dependencies. """ if len(input_files_path) == 0: @@ -38,15 +32,6 @@ def lint_requirements( flag = True - # Create bootstrapper for requirement resolution - bt = bootstrapper.Bootstrapper( - ctx=wkctx, - progressbar=progress.Progressbar(None), - prev_graph=None, - cache_wheel_server_url=None, - sdist_only=True, - ) - for path in input_files_path: parsed_lines = requirements_file.parse_requirements_file(path) unique_entries: dict[str, Requirement] = {} @@ -62,24 +47,6 @@ def lint_requirements( raise InvalidRequirement( "Constraints files cannot contain extra dependencies" ) - - # Resolve the requirement to ensure it can be found - # Skip resolution for constraints files as they should only specify versions - if not path.name.endswith("constraints.txt"): - token = requirement_ctxvar.set(requirement) - try: - _, version = bt.resolve_version( - req=requirement, - req_type=RequirementType.TOP_LEVEL, - ) - logger.info(f"{requirement} resolves to {version}") - except Exception as resolve_err: - logger.error( - f"{path}: {line}: Failed to resolve requirement: {resolve_err}" - ) - flag = False - finally: - requirement_ctxvar.reset(token) except InvalidRequirement as err: logger.error(f"{path}: {line}: {err}") flag = False diff --git a/src/fromager/context.py b/src/fromager/context.py index 02c94f8f..7c317d94 100644 --- a/src/fromager/context.py +++ b/src/fromager/context.py @@ -63,15 +63,15 @@ def __init__( self.constraints.load_constraints_file(constraints_file) else: self.input_constraints_uri = None - self.sdists_repo = pathlib.Path(sdists_repo).resolve() + self.sdists_repo = pathlib.Path(sdists_repo).absolute() self.sdists_downloads = self.sdists_repo / "downloads" self.sdists_builds = self.sdists_repo / "builds" - self.wheels_repo = pathlib.Path(wheels_repo).resolve() + self.wheels_repo = pathlib.Path(wheels_repo).absolute() self.wheels_build_base = self.wheels_repo / "build" self.wheels_downloads = self.wheels_repo / "downloads" self.wheels_prebuilt = self.wheels_repo / "prebuilt" self.wheel_server_dir = self.wheels_repo / "simple" - self.work_dir = pathlib.Path(work_dir).resolve() + self.work_dir = pathlib.Path(work_dir).absolute() self.graph_file = self.work_dir / "graph.json" self.uv_cache = self.work_dir / "uv-cache" self.wheel_server_url = wheel_server_url diff --git a/src/fromager/dependencies.py b/src/fromager/dependencies.py index d244d9be..d60bd968 100644 --- a/src/fromager/dependencies.py +++ b/src/fromager/dependencies.py @@ -12,7 +12,6 @@ import tomlkit from packaging.metadata import Metadata from packaging.requirements import Requirement -from packaging.utils import NormalizedName, canonicalize_name from packaging.version import Version from . import ( @@ -303,25 +302,21 @@ def default_get_install_dependencies_of_sdist( Uses PEP 517 prepare_metadata_for_build_wheel() API. """ - metadata = pep517_metadata_of_sdist( + hook_caller = get_build_backend_hook_caller( ctx=ctx, req=req, - version=version, - sdist_root_dir=sdist_root_dir, - build_env=build_env, - extra_environ=extra_environ, build_dir=build_dir, - config_settings=config_settings, - validate=True, - ) - validate_dist_name_version( - req=req, - version=version, - what="sdist metadata", - # Metadata name is a non-normalized string - dist_name=canonicalize_name(metadata.name), - dist_version=metadata.version, + override_environ=extra_environ, + build_env=build_env, ) + with tempfile.TemporaryDirectory() as tmp_dir: + distinfo_name = hook_caller.prepare_metadata_for_build_wheel( + tmp_dir, + config_settings=config_settings, + ) + metadata_file = pathlib.Path(tmp_dir) / distinfo_name / "METADATA" + # ignore minor metadata issues + metadata = parse_metadata(metadata_file, validate=False) if not metadata.requires_dist: return set() return set(metadata.requires_dist) @@ -337,67 +332,6 @@ def parse_metadata(metadata_file: pathlib.Path, *, validate: bool = True) -> Met return Metadata.from_email(metadata_file.read_bytes(), validate=validate) -def pep517_metadata_of_sdist( - *, - ctx: context.WorkContext, - req: Requirement, - version: Version, - sdist_root_dir: pathlib.Path, - build_env: build_environment.BuildEnvironment, - extra_environ: dict[str, str], - build_dir: pathlib.Path, - config_settings: dict[str, str], - validate: bool = True, -) -> Metadata: - """Get wheel metadata from a source distribution - - Uses PEP 517 prepare_metadata_for_build_wheel() API. - """ - hook_caller = get_build_backend_hook_caller( - ctx=ctx, - req=req, - build_dir=build_dir, - override_environ=extra_environ, - build_env=build_env, - ) - with tempfile.TemporaryDirectory() as tmp_dir: - distinfo_name = hook_caller.prepare_metadata_for_build_wheel( - tmp_dir, - config_settings=config_settings, - ) - metadata_file = pathlib.Path(tmp_dir) / distinfo_name / "METADATA" - metadata = parse_metadata(metadata_file, validate=validate) - - return metadata - - -def validate_dist_name_version( - req: Requirement, - version: Version, - what: str, - dist_name: NormalizedName, - dist_version: Version, -) -> None: - """Validate that dist name and version matches expected values""" - req_name = canonicalize_name(req.name) - if dist_name != req_name: - if dist_name != canonicalize_name(dist_name): - # API misuse - raise RuntimeError("dist_name argument {dist_name!r} is not normalized") - else: - raise ValueError( - f"{what} {dist_name!r} does not match requirement {req_name!r}" - ) - if dist_version.public != version.public: - raise ValueError( - f"{what} {dist_version.public!r} does not match public version {version.public!r}" - ) - if dist_version.local != version.local: - logger.warning( - f"{what} {dist_version.local!r} has different local version than {version.local!r}" - ) - - def get_install_dependencies_of_wheel( req: Requirement, wheel_filename: pathlib.Path, requirements_file_dir: pathlib.Path ) -> set[Requirement]: diff --git a/src/fromager/sources.py b/src/fromager/sources.py index 94ece4a7..fe5d6fb1 100644 --- a/src/fromager/sources.py +++ b/src/fromager/sources.py @@ -11,9 +11,6 @@ import resolvelib from packaging.requirements import Requirement -from packaging.utils import ( - parse_sdist_filename, -) from packaging.version import Version from requests.exceptions import ChunkedEncodingError, ConnectionError from urllib3.exceptions import IncompleteRead, ProtocolError @@ -587,7 +584,7 @@ def build_sdist( # NOT produce a valid sdist, so we can use the PEP-517 approach # instead. logger.info("using PEP-517 sdist build, ignoring any plugins") - sdist_file = pep517_build_sdist( + sdist_filename = pep517_build_sdist( ctx=ctx, extra_environ=extra_environ, req=req, @@ -596,7 +593,7 @@ def build_sdist( build_env=build_env, ) else: - sdist_file = overrides.find_and_invoke( + sdist_filename = overrides.find_and_invoke( req.name, "build_sdist", default_build_sdist, @@ -608,19 +605,8 @@ def build_sdist( build_dir=build_dir, build_env=build_env, ) - logger.info(f"built source distribution {sdist_file}") - - # validate location and file name - if not sdist_file.is_file(): - raise FileNotFoundError(sdist_file) - sdist_file = sdist_file.resolve() - if sdist_file.parent != ctx.sdists_builds: - raise ValueError( - f"{sdist_file!r} is not in sdists build directory {ctx.sdists_builds!r}" - ) - validate_sdist_filename(req=req, version=version, sdist_file=sdist_file) - - return sdist_file + logger.info(f"built source distribution {sdist_filename}") + return sdist_filename def default_build_sdist( @@ -730,19 +716,3 @@ def ensure_pkg_info( ) had_pkg_info = False return had_pkg_info - - -def validate_sdist_filename( - req: Requirement, - version: Version, - sdist_file: pathlib.Path, -) -> None: - """Check that sdist matches requirement name and version""" - sdist_name, sdist_version = parse_sdist_filename(sdist_file.name) - dependencies.validate_dist_name_version( - req=req, - version=version, - what=sdist_file.name, - dist_name=sdist_name, - dist_version=sdist_version, - ) diff --git a/src/fromager/wheels.py b/src/fromager/wheels.py index dc99f767..57092bbd 100644 --- a/src/fromager/wheels.py +++ b/src/fromager/wheels.py @@ -16,11 +16,7 @@ import wheel.wheelfile # type: ignore from packaging.requirements import Requirement from packaging.tags import Tag -from packaging.utils import ( - BuildTag, - canonicalize_name, - parse_wheel_filename, -) +from packaging.utils import BuildTag, canonicalize_name, parse_wheel_filename from packaging.version import Version from . import ( @@ -264,22 +260,6 @@ def add_extra_metadata_to_wheels( raise FileNotFoundError("Could not locate new wheels file") -def validate_wheel_filename( - req: Requirement, - version: Version, - wheel_file: pathlib.Path, -) -> None: - """Check that wheel matches requirement name and version""" - wheel_name, wheel_version, _, _ = parse_wheel_filename(wheel_file.name) - dependencies.validate_dist_name_version( - req=req, - version=version, - what=wheel_file.name, - dist_name=wheel_name, - dist_version=wheel_version, - ) - - @metrics.timeit(description="build wheels") def build_wheel( *, @@ -300,8 +280,8 @@ def build_wheel( extra_environ = packagesettings.get_extra_environ( ctx=ctx, req=req, - sdist_root_dir=sdist_root_dir, version=version, + sdist_root_dir=sdist_root_dir, build_env=build_env, ) @@ -331,42 +311,31 @@ def build_wheel( build_env=build_env, extra_environ=extra_environ, req=req, - version=version, sdist_root_dir=sdist_root_dir, build_dir=pbi.build_dir(sdist_root_dir), + version=version, ) + # End the timer wheels = list(ctx.wheels_build.glob("*.whl")) if len(wheels) != 1: raise FileNotFoundError( f"Expected 1 built wheel in {ctx.wheels_build}, got {len(wheels)}" ) - tmp_wheel_file = wheels[0] - - # validate location and file name - if tmp_wheel_file.parent != ctx.wheels_build: - raise ValueError( - f"{tmp_wheel_file!r} is not in wheels build directory {ctx.wheels_build!r}" - ) - validate_wheel_filename(req=req, version=version, wheel_file=tmp_wheel_file) - # add extra metadata and validate again - new_wheel_file = add_extra_metadata_to_wheels( + wheel = add_extra_metadata_to_wheels( ctx=ctx, req=req, version=version, extra_environ=extra_environ, sdist_root_dir=sdist_root_dir, - wheel_file=tmp_wheel_file, + wheel_file=wheels[0], ) - validate_wheel_filename(req=req, version=version, wheel_file=new_wheel_file) - # invalidate uv cache, so the new wheel is picked up. The step is only # relevant for local development when a package is rebuilt multiple times # without bumping the build tag. ctx.uv_clean_cache(req) - - return new_wheel_file + return wheel def pep517_build_wheel( @@ -408,8 +377,8 @@ def default_build_wheel( sdist_root_dir: pathlib.Path, version: Version, build_dir: pathlib.Path, -) -> pathlib.Path: - return pep517_build_wheel( +) -> None: + pep517_build_wheel( ctx=ctx, build_env=build_env, extra_environ=extra_environ, diff --git a/tests/test_dependencies.py b/tests/test_dependencies.py index 4dfb71db..7cd120e5 100644 --- a/tests/test_dependencies.py +++ b/tests/test_dependencies.py @@ -2,15 +2,11 @@ import itertools import pathlib import shutil -import textwrap import typing from unittest.mock import Mock, patch import pytest -from packaging.metadata import Metadata from packaging.requirements import Requirement -from packaging.utils import NormalizedName -from packaging.version import Version from fromager import build_environment, context, dependencies @@ -210,72 +206,3 @@ def test_get_build_sdist_dependencies_cached( build_env=build_env, ) assert results == set([Requirement("foo==1.0")]) - - -@patch("fromager.dependencies.pep517_metadata_of_sdist") -def test_default_get_install_dependencies_of_sdist( - m_pep517_metadata_of_sdist: Mock, - tmp_context: context.WorkContext, - tmp_path: pathlib.Path, -) -> None: - req = Requirement("huggingface-hub") - version = Version("1.2.3") - # sdist metadata name may not be normalized - metadata_txt = textwrap.dedent( - """\ - Metadata-Version: 2.3 - Name: HuggingFace_Hub - Version: 1.2.3 - Requires-Dist: filelock - Requires-Dist: requests - """ - ) - metadata = Metadata.from_email(metadata_txt) - m_pep517_metadata_of_sdist.return_value = metadata - - requirements = dependencies.default_get_install_dependencies_of_sdist( - ctx=tmp_context, - req=req, - version=version, - sdist_root_dir=tmp_path, - build_env=Mock(), - extra_environ={}, - build_dir=tmp_path, - config_settings={}, - ) - assert requirements == {Requirement("filelock"), Requirement("requests")} - - -@pytest.mark.parametrize( - "req_str,version_str,dist_name_str,dist_version_str,exc", - [ - ("mypkg", "1.0", "mypkg", "1.0", None), - ("MyPKG", "1.0", "mypkg", "1.0", None), - ("mypkg", "1.0", "MyPKG", "1.0", RuntimeError), - ("mypkg", "1.0", "otherpkg", "1.0", ValueError), - ("mypkg", "1.0", "mypkg", "1.1", ValueError), - ("mypkg", "1.0+local", "mypkg", "1.0+local", None), - ("mypkg", "1.0", "mypkg", "1.0+local", None), - ("mypkg", "1.0+local", "mypkg", "1.0", None), - ], -) -def test_validate_dist_name_version( - req_str: str, - version_str: str, - dist_name_str: str, - dist_version_str: str, - exc: type[Exception] | None, -) -> None: - validate = functools.partial( - dependencies.validate_dist_name_version, - req=Requirement(req_str), - version=Version(version_str), - what="test", - dist_name=typing.cast(NormalizedName, dist_name_str), - dist_version=Version(dist_version_str), - ) - if exc is None: - validate() - else: - with pytest.raises(exc): - validate() diff --git a/tests/test_sources.py b/tests/test_sources.py index 1386e282..84fe4a3a 100644 --- a/tests/test_sources.py +++ b/tests/test_sources.py @@ -187,31 +187,3 @@ def test_patch_sources_apply_only_unversioned( version=Version("0.6.0"), ) apply_patch.assert_called_once_with(req, unversioned_patch_file, source_root_dir) - - -@pytest.mark.parametrize( - "dist_name,version_string,sdist_filename,okay", - [ - ("mypkg", "1.2", "mypkg-1.2.tar.gz", True), - ("mypkg", "1.2", "unknown-1.2.tar.gz", False), - ("mypkg", "1.2", "mypkg-1.2.1.tar.gz", False), - ("oslo.messaging", "14.7.0", "oslo.messaging-14.7.0.tar.gz", True), - ("cython", "3.0.10", "Cython-3.0.10.tar.gz", True), - # parse_sdist_filename() accepts a dash in the name - ("fromage_test", "9.9.9", "fromage-test-9.9.9.tar.gz", True), - ("fromage-test", "9.9.9", "fromage-test-9.9.9.tar.gz", True), - ("fromage_test", "9.9.9", "fromage_test-9.9.9.tar.gz", True), - ("ruamel-yaml", "0.18.6", "ruamel.yaml-0.18.6.tar.gz", True), - ], -) -def test_validate_sdist_file( - dist_name: str, version_string: str, sdist_filename: pathlib.Path, okay: bool -) -> None: - req = Requirement(dist_name) - version = Version(version_string) - sdist_file = pathlib.Path(sdist_filename) - if okay: - sources.validate_sdist_filename(req, version, sdist_file) - else: - with pytest.raises(ValueError): - sources.validate_sdist_filename(req, version, sdist_file) diff --git a/tests/test_wheels.py b/tests/test_wheels.py index 7d851f00..cb2759bb 100644 --- a/tests/test_wheels.py +++ b/tests/test_wheels.py @@ -214,45 +214,3 @@ def test_add_extra_metadata_blocks_path_traversal( sdist_root_dir=sdist_dir, wheel_file=wheel_file, ) - - -@pytest.mark.parametrize( - "dist_name,version_string,wheel_filename,okay", - [ - ("mypkg", "1.2", "mypkg-1.2-py2.py3-none-any.whl", True), - ("mypkg", "1.2", "unknown-1.2-py2.py3-none-any.whl", False), - ("mypkg", "1.2", "mypkg-1.2.1-py2.py3-none-any.whl", False), - ( - "oslo.messaging", - "14.7.0", - "oslo.messaging-14.7.0-py2.py3-none-any.whl", - True, - ), - ("cython", "3.0.10", "Cython-3.0.10-cp311-cp311-linux_aarch64.whl", True), - ( - "fromage_test", - "9.9.9", - "fromage_test-9.9.9-cp311-cp311-linux_aarch64.whl", - True, - ), - # parse_wheel_filename() does NOT accept a dash in the name - ( - "fromage_test", - "9.9.9", - "fromage-test-9.9.9-cp311-cp311-linux_aarch64.whl", - False, - ), - ("ruamel-yaml", "0.18.6", "ruamel.yaml-0.18.6-py3-none-any.whl", True), - ], -) -def test_validate_wheel_file( - dist_name: str, version_string: str, wheel_filename: str, okay: bool -) -> None: - req = Requirement(dist_name) - version = Version(version_string) - wheel_file = pathlib.Path(wheel_filename) - if okay: - wheels.validate_wheel_filename(req, version, wheel_file) - else: - with pytest.raises(ValueError): - wheels.validate_wheel_filename(req, version, wheel_file)