look for patches in canonical locations

Require patch directories be named using the
override module form of the name of the package
being patched. In addition to being more
predictable, this allows us to simplify the logic
for detecting patches for other versions of a
project that are not being applied.

Author: dhellmann

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

@@ -275,27 +275,25 @@ def patch_source(
     ctx: context.WorkContext,
     source_root_dir: pathlib.Path,
     req: Requirement,
+    version: Version,
 ) -> None:
     patch_count = 0
-    # apply any unversioned patch first
-    for p in overrides.patches_for_source_dir(
-        ctx.settings.patches_dir, overrides.pkgname_to_override_module(req.name)
+    for p in overrides.patches_for_requirement(
+        patches_dir=ctx.settings.patches_dir,
+        req=req,
+        version=version,
     ):
         _apply_patch(p, source_root_dir)
         patch_count += 1
 
-    # 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)
-            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 patch_count:
-        _warn_for_old_patch(source_root_dir, ctx.settings.patches_dir)
+        _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):
@@ -305,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(
@@ -407,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

tests/test_sources.py

@@ -121,7 +121,12 @@ def test_patch_sources_apply_unversioned_and_versioned(
     source_root_dir = tmp_path / "deepspeed-0.5.0"
     source_root_dir.mkdir()
 
-    sources.patch_source(tmp_context, source_root_dir, Requirement("deepspeed==0.5.0"))
+    sources.patch_source(
+        ctx=tmp_context,
+        source_root_dir=source_root_dir,
+        req=Requirement("deepspeed==0.5.0"),
+        version=Version("0.5.0"),
+    )
     assert apply_patch.call_count == 2
     apply_patch.assert_has_calls(
         [
@@ -151,10 +156,15 @@ def test_patch_sources_apply_only_unversioned(
     unversioned_patch_file = deepspeed_unversioned_patch / "deepspeed-update.patch"
     unversioned_patch_file.write_text("This is a test patch")
 
-    source_root_dir = tmp_path / "deepspeed"
+    source_root_dir = tmp_path / "deepspeed-0.5.0"
     source_root_dir.mkdir()
 
-    sources.patch_source(tmp_context, source_root_dir, Requirement("deepspeed==0.5.0"))
+    sources.patch_source(
+        ctx=tmp_context,
+        source_root_dir=source_root_dir,
+        req=Requirement("deepspeed"),
+        version=Version("0.6.0"),
+    )
     assert apply_patch.call_count == 1
     apply_patch.assert_has_calls(
         [
@@ -179,7 +189,11 @@ def test_warning_for_older_patch(mock, tmp_path: pathlib.Path):
     source_root_dir = tmp_path / "deepspeed-0.6.0"
     source_root_dir.mkdir()
 
-    sources._warn_for_old_patch(source_root_dir, patches_dir)
+    sources._warn_for_old_patch(
+        req=Requirement("deepspeed"),
+        version=Version("0.6.0"),
+        patches_dir=patches_dir,
+    )
     mock.assert_called()
 
 
@@ -199,5 +213,9 @@ def test_warning_for_older_patch_different_req(mock, tmp_path: pathlib.Path):
     source_root_dir = tmp_path / "deepspeed-0.5.0"
     source_root_dir.mkdir()
 
-    sources._warn_for_old_patch(source_root_dir, patches_dir)
+    sources._warn_for_old_patch(
+        req=Requirement("deepspeed"),
+        version=Version("0.5.0"),
+        patches_dir=patches_dir,
+    )
     mock.assert_not_called()