Merge pull request #424 from dhellmann/patch-dir-lookup

standardize patch directories using override module names

Author: mergify[bot]

e2e/test_bootstrap_extras.sh

@@ -41,8 +41,8 @@ $OUTDIR/work-dir/setuptools-*/build.log
 
 $OUTDIR/wheels-repo/downloads/PySocks-*.whl
 $OUTDIR/sdists-repo/downloads/PySocks-*.tar.gz
-$OUTDIR/sdists-repo/builds/PySocks-*.tar.gz
-$OUTDIR/work-dir/PySocks-*/build.log
+$OUTDIR/sdists-repo/builds/pysocks-*.tar.gz
+$OUTDIR/work-dir/pysocks-*/build.log
 "
 
 for pattern in $EXPECTED_FILES; do

src/fromager/overrides.py

@@ -1,11 +1,13 @@
 import inspect
+import itertools
 import logging
 import pathlib
-import re
 import typing
 from importlib import metadata
 
+from packaging.requirements import Requirement
 from packaging.utils import canonicalize_name
+from packaging.version import Version
 from stevedore import extension
 
 # An interface for reretrieving per-package information which influences
@@ -88,45 +90,39 @@ def log_overrides() -> None:
         )
 
 
-def patches_for_source_dir(
-    patches_dir: pathlib.Path, source_dir_name: str
+def patches_for_requirement(
+    patches_dir: pathlib.Path,
+    req: Requirement,
+    version: Version,
 ) -> typing.Iterable[pathlib.Path]:
-    """Iterator producing patches to apply to the source dir.
+    """Iterator producing patches to apply to the source for a given version of a requirement.
 
-    Input should be the base directory name, not a full path.
-
-    Yields pathlib.Path() references to patches in the order they
-    should be applied, which is controlled through lexical sorting of
-    the filenames.
+    Yields pathlib.Path() references to patches in the order they should be
+    applied, which is controlled through lexical sorting of the filenames.
 
     """
-    return sorted((patches_dir / source_dir_name).glob("*.patch"))
-
-
-def get_patch_directories(
-    patches_dir: pathlib.Path, source_root_dir: pathlib.Path
-) -> list[pathlib.Path]:
+    override_name = pkgname_to_override_module(req.name)
+    unversioned_patch_dir = patches_dir / override_name
+    versioned_patch_dir = patches_dir / f"{override_name}-{version}"
+    return itertools.chain(
+        # Apply all of the unversioned patches first, in order based on
+        # filename.
+        sorted(unversioned_patch_dir.glob("*.patch")),
+        # Then apply any for this specific version, in order based on filename.
+        sorted(versioned_patch_dir.glob("*.patch")),
+    )
+
+
+def get_versioned_patch_directories(
+    patches_dir: pathlib.Path,
+    req: Requirement,
+) -> typing.Generator[pathlib.Path, None, None]:
     """
-    This function will return directories that may contain patches for a specific requirement.
-    It takes in patches directory and a source root directory as input.
-    The output will be a list of all directories containing patches for that requirement
+    This function will return directories that may contain patches for any version of a specific requirement.
     """
     # Get the req name as per the source_root_dir naming conventions
-    req_name = source_root_dir.name.rsplit("-", 1)[0]
-    patches = sorted((patches_dir).glob(f"{req_name}*"))
-    filtered_patches = _filter_patches_based_on_req(patches, req_name)
-    return filtered_patches
-
-
-# Helper method to filter the unwanted patches using a regex
-def _filter_patches_based_on_req(
-    patches: list[pathlib.Path], req_name: str
-) -> list[pathlib.Path]:
-    # Set up regex to filter out unwanted patches.
-    pattern = re.compile(rf"^{req_name}-v?(\d+\.)+\d+")
-    filtered_patches = [s for s in patches if pattern.match(s.name)]
-    # filtered_patches won't contain patches for current version of req
-    return filtered_patches
+    override_name = pkgname_to_override_module(req.name)
+    return patches_dir.glob(f"{override_name}-*")
 
 
 def pkgname_to_override_module(pkgname: str) -> str:

src/fromager/sources.py

@@ -213,21 +213,28 @@ def _takes_arg(f: typing.Callable, arg_name: str) -> bool:
 
 def unpack_source(
     ctx: context.WorkContext,
+    req: Requirement,
+    version: Version,
     source_filename: pathlib.Path,
 ) -> tuple[pathlib.Path, bool]:
-    sdist_root_name = _sdist_root_name(source_filename)
-    unpack_dir = ctx.work_dir / sdist_root_name
+    # sdist names are less standardized and the names of the directories they
+    # contain are also not very standard. Force the names into a predictable
+    # form based on the override module name for the requirement.
+    req_name = overrides.pkgname_to_override_module(req.name)
+    expected_name = f"{req_name}-{version}"
+
+    # The unpack_dir is a parent dir where we put temporary outputs during the
+    # build process, including the unpacked source in a subdirectory.
+    unpack_dir = ctx.work_dir / expected_name
     if unpack_dir.exists():
         if ctx.cleanup:
             logger.debug("cleaning up %s", unpack_dir)
             shutil.rmtree(unpack_dir)
         else:
             logger.info("reusing %s", unpack_dir)
             return (unpack_dir / unpack_dir.name, False)
-    # We create a unique directory based on the sdist name, but that
-    # may not be the same name as the root directory of the content in
-    # the sdist (due to case, punctuation, etc.), so after we unpack
-    # it look for what was created.
+
+    # sdists might be tarballs or zip files.
     logger.debug("unpacking %s to %s", source_filename, unpack_dir)
     if str(source_filename).endswith(".tar.gz"):
         with tarfile.open(source_filename, "r") as t:
@@ -242,45 +249,51 @@ def unpack_source(
     else:
         raise ValueError(f"Do not know how to unpack source archive {source_filename}")
 
-    # if tarball named foo-2.3.1.tar.gz was downloaded, then ensure that after unpacking, the source directory's path is foo-2.3.1/foo-2.3.1
+    # We create a unique directory based on the requirement name, but that may
+    # not be the same name as the root directory of the content in the sdist
+    # (due to case, punctuation, etc.), so after we unpack it look for what was
+    # created and ensure the extracted directory matches the override module
+    # name and version of the requirement.
     unpacked_root_dir = next(iter(unpack_dir.glob("*")))
-    new_unpacked_root_dir = unpacked_root_dir.parent / sdist_root_name
-    if unpacked_root_dir.name != new_unpacked_root_dir.name:
+    if unpacked_root_dir.name != expected_name:
+        desired_name = unpacked_root_dir.parent / expected_name
         try:
-            shutil.move(str(unpacked_root_dir), str(new_unpacked_root_dir))
+            shutil.move(
+                str(unpacked_root_dir),
+                str(desired_name),
+            )
         except Exception as err:
             raise Exception(
-                f"Could not rename {unpacked_root_dir.name} to {new_unpacked_root_dir.name}: {err}"
+                f"Could not rename {unpacked_root_dir.name} to {desired_name}: {err}"
             ) from err
+        unpacked_root_dir = desired_name
 
-    return (new_unpacked_root_dir, True)
+    return (unpacked_root_dir, True)
 
 
 def patch_source(
     ctx: context.WorkContext,
     source_root_dir: pathlib.Path,
     req: Requirement,
+    version: Version,
 ) -> None:
-    # Flag to check whether patch has been applied
-    has_applied = False
-    # apply any unversioned patch first
-    for p in overrides.patches_for_source_dir(
-        ctx.settings.patches_dir, overrides.pkgname_to_override_module(req.name)
+    patch_count = 0
+    for p in overrides.patches_for_requirement(
+        patches_dir=ctx.settings.patches_dir,
+        req=req,
+        version=version,
     ):
         _apply_patch(p, source_root_dir)
-        has_applied = True
-
-    # make sure that we don't apply the generic unversioned patch again
-    if source_root_dir.name != overrides.pkgname_to_override_module(req.name):
-        for p in overrides.patches_for_source_dir(
-            ctx.settings.patches_dir, source_root_dir.name
-        ):
-            _apply_patch(p, source_root_dir)
-            has_applied = True
+        patch_count += 1
 
+    logger.debug("%s: applied %d patches", req.name, patch_count)
     # If no patch has been applied, call warn for old patch
-    if not has_applied:
-        _warn_for_old_patch(source_root_dir, ctx.settings.patches_dir)
+    if not patch_count:
+        _warn_for_old_patch(
+            req=req,
+            version=version,
+            patches_dir=ctx.settings.patches_dir,
+        )
 
 
 def _apply_patch(patch: pathlib.Path, source_root_dir: pathlib.Path):
@@ -290,18 +303,20 @@ def _apply_patch(patch: pathlib.Path, source_root_dir: pathlib.Path):
 
 
 def _warn_for_old_patch(
-    source_root_dir: pathlib.Path,
+    req: Requirement,
+    version: Version,
     patches_dir: pathlib.Path,
 ) -> None:
-    # Get the req name as per the source_root_dir naming conventions
-    req_name = source_root_dir.name.rsplit("-", 1)[0]
-
     # Filter the patch directories using regex
-    patch_directories = overrides.get_patch_directories(patches_dir, source_root_dir)
+    patch_directories = overrides.get_versioned_patch_directories(
+        patches_dir=patches_dir, req=req
+    )
 
     for dirs in patch_directories:
         for p in dirs.iterdir():
-            logger.warning(f"{req_name}: patch {p} exists but will not be applied")
+            logger.warning(
+                f"{req.name}: patch {p} exists but will not be applied for version {version}"
+            )
 
 
 def write_build_meta(
@@ -366,7 +381,12 @@ def default_prepare_source(
     source_filename: pathlib.Path,
     version: Version,
 ) -> tuple[pathlib.Path, bool]:
-    source_root_dir, is_new = unpack_source(ctx, source_filename)
+    source_root_dir, is_new = unpack_source(
+        ctx=ctx,
+        req=req,
+        version=version,
+        source_filename=source_filename,
+    )
     if is_new:
         prepare_new_source(
             ctx=ctx,
@@ -387,7 +407,7 @@ def prepare_new_source(
 
     `default_prepare_source` runs this function when the sources are new.
     """
-    patch_source(ctx, source_root_dir, req)
+    patch_source(ctx, source_root_dir, req, version)
     pyproject.apply_project_override(
         ctx=ctx,
         req=req,

tests/test_overrides.py

@@ -1,42 +1,39 @@
 import pathlib
-from importlib import metadata
 from unittest import mock
 from unittest.mock import patch
 
 import pytest
+from packaging.requirements import Requirement
+from packaging.version import Version
 
 from fromager import overrides
 
 
-def test_patches_for_source_dir(tmp_path: pathlib.Path):
+def test_patches_for_requirement(tmp_path: pathlib.Path):
     patches_dir = tmp_path / "patches"
     patches_dir.mkdir()
 
     project_patch_dir = patches_dir / "project-1.2.3"
     project_patch_dir.mkdir()
 
-    project_variant_patch_dir = patches_dir / "project-1.2.3-variant"
-    project_variant_patch_dir.mkdir()
-
     p1 = project_patch_dir / "001.patch"
     p2 = project_patch_dir / "002.patch"
     np1 = project_patch_dir / "not-a-patch.txt"
-    p3 = project_variant_patch_dir / "003.patch"
-    np2 = project_variant_patch_dir / "not-a-patch.txt"
 
     # Create all of the test files
-    for p in [p1, p2, p3]:
+    for p in [p1, p2]:
         p.write_text("this is a patch file")
-    for f in [np1, np2]:
+    for f in [np1]:
         f.write_text("this is not a patch file")
 
-    results = list(overrides.patches_for_source_dir(patches_dir, "project-1.2.3"))
-    assert results == [p1, p2]
-
     results = list(
-        overrides.patches_for_source_dir(patches_dir, "project-1.2.3-variant")
+        overrides.patches_for_requirement(
+            patches_dir=patches_dir,
+            req=Requirement("project"),
+            version=Version("1.2.3"),
+        )
     )
-    assert results == [p3]
+    assert results == [p1, p2]
 
 
 def test_invoke_override_with_exact_args():
@@ -73,86 +70,3 @@ def default_foo(arg1):
     assert overrides.find_and_invoke(
         "pkg", "foo", default_foo, arg1="value1", arg2="value2"
     )
-
-
-def test_regex_dummy_package(tmp_path: pathlib.Path):
-    req_name = "foo"
-    patches_dir = tmp_path / "patches_dir"
-    patches_dir.mkdir()
-
-    lst = [
-        patches_dir / "foo-1.1.0",
-        patches_dir / "foo-bar-2.0.0",
-        patches_dir / "foo-v2.3.0",
-        patches_dir / "foo-bar-bar-v2.3.1",
-        patches_dir / "foo-bar-v5.5.5",
-        patches_dir / "foo-3.4.4",
-        patches_dir / "foo-v2.3.0.1",
-    ]
-
-    expected = [
-        patches_dir / "foo-1.1.0",
-        patches_dir / "foo-v2.3.0",
-        patches_dir / "foo-3.4.4",
-        patches_dir / "foo-v2.3.0.1",
-    ]
-
-    actual = overrides._filter_patches_based_on_req(lst, req_name)
-    assert len(expected) == len(actual)
-    assert expected == actual
-
-
-def test_regex_for_deepspeed(tmp_path: pathlib.Path):
-    req_name = "deepspeed"
-    patches_dir = tmp_path / "patches_dir"
-    patches_dir.mkdir()
-
-    lst = [
-        patches_dir / "deepspeed-1.1.0",
-        patches_dir / "deepspeed-deep-2.0.0",
-        patches_dir / "deepspeed-v2.3.0.post1",
-        patches_dir / "deepspeed-v5.5.5",
-        patches_dir / "deepspeed-3.4.4",
-        patches_dir / "deepspeed-sdg-3.4.4",
-    ]
-
-    expected = [
-        patches_dir / "deepspeed-1.1.0",
-        patches_dir / "deepspeed-v2.3.0.post1",
-        patches_dir / "deepspeed-v5.5.5",
-        patches_dir / "deepspeed-3.4.4",
-    ]
-
-    actual = overrides._filter_patches_based_on_req(lst, req_name)
-    assert len(expected) == len(actual)
-    assert expected == actual
-
-
-def test_regex_for_vllm(tmp_path: pathlib.Path):
-    req_name = "vllm"
-    patches_dir = tmp_path / "patches_dir"
-    patches_dir.mkdir()
-
-    lst = [
-        patches_dir / "vllm-1.1.0.9",
-        patches_dir / "vllm-llm-2.1.0.0",
-        patches_dir / "vllm-v2.3.5.0.post1",
-        patches_dir / "vllm-v5.5.5.1",
-    ]
-
-    expected = [
-        patches_dir / "vllm-1.1.0.9",
-        patches_dir / "vllm-v2.3.5.0.post1",
-        patches_dir / "vllm-v5.5.5.1",
-    ]
-
-    actual = overrides._filter_patches_based_on_req(lst, req_name)
-    assert len(expected) == len(actual)
-    assert expected == actual
-
-
-def test_get_dist_info():
-    fromager_version = metadata.version("fromager")
-    plugin_dist, plugin_version = overrides._get_dist_info("fromager.submodule")
-    assert plugin_dist == "fromager"
-    assert plugin_version == fromager_version