From 2f5db1220b0a603091cda2d878541f3328a8bb14 Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Wed, 24 Sep 2025 18:07:06 -0400 Subject: [PATCH 1/4] Revert "feat(lint): add requirement resolution to lint-requirements command" This reverts commit 0664d8dbfcc2ac17501ddff7aa7e35356dc25a0f. This change introduces new runtime requirements that we can't absorb downstream. --- src/fromager/commands/lint_requirements.py | 39 ++-------------------- 1 file changed, 3 insertions(+), 36 deletions(-) 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 From e2b26c7f9e7300e91f8e19163927f0be47c8b290 Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Wed, 24 Sep 2025 18:09:38 -0400 Subject: [PATCH 2/4] Revert "fix: normalize name in default_get_install_dependencies_of_sdist" This reverts commit bb9ee8eb03caa59aedd940d06b72eb725f0c8549. We have to revert the feature addition that this depends on. --- src/fromager/dependencies.py | 30 ++++----------- tests/test_dependencies.py | 73 ------------------------------------ 2 files changed, 8 insertions(+), 95 deletions(-) diff --git a/src/fromager/dependencies.py b/src/fromager/dependencies.py index d244d9be..dd160489 100644 --- a/src/fromager/dependencies.py +++ b/src/fromager/dependencies.py @@ -12,7 +12,7 @@ import tomlkit from packaging.metadata import Metadata from packaging.requirements import Requirement -from packaging.utils import NormalizedName, canonicalize_name +from packaging.utils import canonicalize_name from packaging.version import Version from . import ( @@ -318,8 +318,7 @@ def default_get_install_dependencies_of_sdist( req=req, version=version, what="sdist metadata", - # Metadata name is a non-normalized string - dist_name=canonicalize_name(metadata.name), + dist_name=metadata.name, dist_version=metadata.version, ) if not metadata.requires_dist: @@ -372,30 +371,17 @@ def pep517_metadata_of_sdist( def validate_dist_name_version( - req: Requirement, - version: Version, - what: str, - dist_name: NormalizedName, - dist_version: Version, + req: Requirement, version: Version, what: str, dist_name: str, 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") + raise ValueError(f"{what} does not match requirement {req_name!r}") + if dist_version != version: + if dist_version.public != version.public: + raise ValueError(f"{what} does not match public version {version!r}") 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}" - ) + logger.warning(f"{what} has different local version than {version!r}") def get_install_dependencies_of_wheel( 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() From 80f0bd8905bbd6e4b9beb20f2d96783fa4b75fca Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Wed, 24 Sep 2025 18:11:13 -0400 Subject: [PATCH 3/4] Revert "feat: validate name and version of sdist and wheel" This reverts commit 86a39db845bcfae6dbb13e8eca8c62b29c3a405d. Validating the name and version of the packages happens too early, so packages with plugins that override those settings via the metadata hook end up reporting the wrong versions. --- docs/hooks.rst | 8 ++-- src/fromager/context.py | 6 +-- src/fromager/dependencies.py | 74 ++++++------------------------------ src/fromager/sources.py | 38 ++---------------- src/fromager/wheels.py | 50 ++++++------------------ tests/test_sources.py | 28 -------------- tests/test_wheels.py | 42 -------------------- 7 files changed, 32 insertions(+), 214 deletions(-) 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/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 dd160489..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 canonicalize_name from packaging.version import Version from . import ( @@ -303,24 +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", - dist_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) @@ -336,54 +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: str, dist_version: Version -) -> None: - """Validate that dist name and version matches expected values""" - req_name = canonicalize_name(req.name) - if dist_name != req_name: - raise ValueError(f"{what} does not match requirement {req_name!r}") - if dist_version != version: - if dist_version.public != version.public: - raise ValueError(f"{what} does not match public version {version!r}") - else: - logger.warning(f"{what} has different local version than {version!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..9a47b4b1 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,34 @@ 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) + # invalidate uv's cache + build_env.clean_cache(req) - # 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 +380,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_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) From bbacffa11d3ec925e5c1c771a676f57baf58d730 Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Wed, 24 Sep 2025 18:19:52 -0400 Subject: [PATCH 4/4] clean up revert --- src/fromager/wheels.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/fromager/wheels.py b/src/fromager/wheels.py index 9a47b4b1..57092bbd 100644 --- a/src/fromager/wheels.py +++ b/src/fromager/wheels.py @@ -323,9 +323,6 @@ def build_wheel( f"Expected 1 built wheel in {ctx.wheels_build}, got {len(wheels)}" ) - # invalidate uv's cache - build_env.clean_cache(req) - wheel = add_extra_metadata_to_wheels( ctx=ctx, req=req,