Repository: python-wheel-build/fromager

Issue:
improve patch management
We have encountered a case where the same wheel is being built from different source archives for different variants. Those source archives use a different path name as the source root when unpacked, but they have a common prefix (one is `package-version-variant` the other is `package-version`). That means that any patches for `package-version` will be applied to `package-version-variant`, even if they should not be.

We can at the very least move patches into subdirectories based on the source directory name. That will not be a backwards-compatible change.

We could also introduce an optional variant part of the path (`patches/variant/source-root`). It's not clear that's absolutely needed (it doesn't do anything for the case at hand, and we haven't encountered other cases where it's needed).

Discussion:

Patch:
diff --git a/docs/customization.md b/docs/customization.md
index c849486..1d2afe8 100644
--- a/docs/customization.md
+++ b/docs/customization.md
@@ -36,7 +36,7 @@ organized with a subdirectory per variant.
 Environment files are named using the [canonical distribution
 name](#canonical-distribution-names) and the suffix `.env`.
 
-```
+```console
 $ tree overrides/envs/
 overrides/envs/
 ├── cpu
@@ -54,7 +54,7 @@ around the 3 parts will be ignored, but whitespace inside the value is
 preserved. It is not necessary to quote values, and quotes inside the
 value will be passed through.
 
-```
+```console
 $ cat overrides/envs/cuda/llama_cpp_python.env
 CMAKE_ARGS=-DLLAMA_CUBLAS=on -DCMAKE_CUDA_ARCHITECTURES=all-major -DLLAMA_NATIVE=off
 CFLAGS=-mno-avx
@@ -63,38 +63,41 @@ FORCE_CMAKE=1
 
 ## Patching source
 
-The `--patches-dir` command line argument specifies a directory
-containing patches to be applied after the source code is in place and
-before evaluating any further dependencies or building the
-project. The default directory is `overrides/patches`.
-
-Patch files should be prefixed with the [canonical distribution
-name](#canonical-distribution-names) followed by a hyphen (`-`) and
-the version number of the package and use the suffix `.patch`. The
-filenames are sorted lexicographically, so any text between the prefix
-and suffix can be used to ensure the patches are applied in a specific
-order.
-
-Patches are applied by running `patch -p1 filename` while inside the
-root of the source tree.
-
-```
-$ ls -1 overrides/patches/
-clarifai-10.2.1-fix-sdist.patch
-flash_attn-2.5.7-pyproject-toml.patch
-jupyterlab_pygments-0.3.0-pyproject-remove-jupyterlab.patch
-ninja-1.11.1.1-wrap-system-ninja.patch
-pytorch-v2.2.1-001-remove-cmake-build-requirement.patch
-pytorch-v2.2.1-002-dist-info-no-run-build-deps.patch
-pytorch-v2.2.1-003-fbgemm-no-maybe-uninitialized.patch
-pytorch-v2.2.1-004-fix-release-version.patch
-pytorch-v2.2.2-001-remove-cmake-build-requirement.patch
-pytorch-v2.2.2-002-dist-info-no-run-build-deps.patch
-pytorch-v2.2.2-003-fbgemm-no-maybe-uninitialized.patch
-pytorch-v2.2.2-004-fix-release-version.patch
-xformers-0.0.26.post1-pyproject.toml.patch
+The `--patches-dir` command line argument specifies a directory containing
+patches to be applied after the source code is in place and before evaluating
+any further dependencies or building the project. The default directory is
+`overrides/patches`.
+
+Patch files should be placed in a subdirectory matching the source directory
+name and use the suffix `.patch`. The filenames are sorted lexicographically, so
+any text between the prefix and suffix can be used to ensure the patches are
+applied in a specific order.
+
+Patches are applied by running `patch -p1 filename` while inside the root of the
+source tree.
+
+```console
+$ ls -1 overrides/patches/*
+clarifai-10.2.1/fix-sdist.patch
+flash_attn-2.5.7/pyproject-toml.patch
+jupyterlab_pygments-0.3.0/pyproject-remove-jupyterlab.patch
+ninja-1.11.1.1/wrap-system-ninja.patch
+pytorch-v2.2.1/001-remove-cmake-build-requirement.patch
+pytorch-v2.2.1/002-dist-info-no-run-build-deps.patch
+pytorch-v2.2.1/003-fbgemm-no-maybe-uninitialized.patch
+pytorch-v2.2.1/004-fix-release-version.patch
+pytorch-v2.2.2/001-remove-cmake-build-requirement.patch
+pytorch-v2.2.2/002-dist-info-no-run-build-deps.patch
+pytorch-v2.2.2/003-fbgemm-no-maybe-uninitialized.patch
+pytorch-v2.2.2/004-fix-release-version.patch
+xformers-0.0.26.post1/pyproject.toml.patch
 ```
 
+Note: A legacy patch organization with the patches in the patches directory, not
+in subdirectories, with the filenames prefixed with the source directory name is
+also supported. The newer format, using subdirectories, is preferred because it
+avoids name collisions between variant source trees.
+
 ## Override plugins
 
 For more complex customization requirements, create an override plugin.
@@ -107,7 +110,7 @@ configure the entry point in the
 link the [canonical distribution name](#canonical-distribution-names)
 to an importable module.
 
-```
+```toml
 [project.entry-points."fromager.project_overrides"]
 flit_core = "package_plugins.flit_core"
 pyarrow = "package_plugins.pyarrow"
@@ -137,7 +140,7 @@ The function may be invoked more than once if multiple sdist servers
 are being used. Returning a valid response prevents multiple
 invocations.
 
-```
+```python
 def download_source(ctx, req, sdist_server_url):
     ...
     return source_filename, version
@@ -155,7 +158,7 @@ evaluated, the `Path` to the source archive, and the version.
 The return value should be the `Path` to the root of the source tree,
 ideally inside the `ctx.work_dir` directory.
 
-```
+```python
 def prepare_source(ctx, req, source_filename, version):
     ...
     return output_dir_name
@@ -173,7 +176,7 @@ look for.
 The return value should be a string with the base filename (no paths)
 for the archive.
 
-```
+```python
 def expected_source_archive_name(req, dist_version):
     return f'apache-arrow-{dist_version}.tar.gz'
 ```
@@ -190,7 +193,7 @@ look for.
 The return value should be a string with the name of the source root
 directory relative to the `ctx.work_dir` where it was prepared.
 
-```
+```python
 def expected_source_directory_name(req, dist_version):
     return f'apache-arrow-{dist_version}/arrow-apache-arrow-{dist_version}'
 ```
@@ -208,7 +211,7 @@ for build system dependencies for the package. The caller is
 responsible for evaluating the requirements with the current build
 environment settings to determine if they are actually needed.
 
-```
+```python
 # pyarrow.py
 def get_build_system_dependencies(ctx, req, sdist_root_dir):
     # The _actual_ directory with our requirements is different than
@@ -233,7 +236,7 @@ for build backend dependencies for the package. The caller is
 responsible for evaluating the requirements with the current build
 environment settings to determine if they are actually needed.
 
-```
+```python
 # pyarrow.py
 def get_build_backend_dependencies(ctx, req, sdist_root_dir):
     # The _actual_ directory with our requirements is different than
@@ -258,7 +261,7 @@ for runtime and installation dependencies for the package. The caller
 is responsible for evaluating the requirements with the current build
 environment settings to determine if they are actually needed.
 
-```
+```python
 # pyarrow.py
 def get_install_dependencies(ctx, req, sdist_root_dir):
     # The _actual_ directory with our requirements is different than
@@ -284,7 +287,7 @@ the `Path` to the root of the source tree.
 
 The return value is ignored.
 
-```
+```python
 def build_wheel(ctx, build_env, extra_environ, req, sdist_root_dir):
     ...
 ```
@@ -300,7 +303,7 @@ canonical version of the name, computed using
 with underscores (`_`). For convenience, the `canonicalize`
 command will print the correct form of a name.
 
-```
+```console
 $ tox -e cli -- canonicalize flit-core
 flit_core
 ```
diff --git a/src/fromager/overrides.py b/src/fromager/overrides.py
index 9d36371..2571564 100644
--- a/src/fromager/overrides.py
+++ b/src/fromager/overrides.py
@@ -1,3 +1,4 @@
+import itertools
 import logging
 import pathlib
 import typing
@@ -45,7 +46,10 @@ def patches_for_source_dir(
     the filenames.
 
     """
-    return sorted(patches_dir.glob(source_dir_name + "*.patch"))
+    return itertools.chain(
+        sorted(patches_dir.glob(source_dir_name + "*.patch")),  # legacy format
+        sorted((patches_dir / source_dir_name).glob("*.patch")),  # safer
+    )
 
 
 def extra_environ_for_pkg(


Test patch:
diff --git a/tests/test_overrides.py b/tests/test_overrides.py
new file mode 100644
index 0000000..de12a72
--- /dev/null
+++ b/tests/test_overrides.py
@@ -0,0 +1,40 @@
+import pathlib
+
+from fromager import overrides
+
+
+def test_patches_for_source_dir(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()
+
+    # legacy form
+    p1 = patches_dir / "project-1.2.3-001.patch"
+    np1 = patches_dir / "project-1.2.3.txt"
+    p2 = patches_dir / "project-1.2.3-variant-002.patch"
+
+    # new form with project dir
+    p3 = project_patch_dir / "003.patch"
+    p4 = project_patch_dir / "004.patch"
+    np2 = project_patch_dir / "not-a-patch.txt"
+    p5 = project_variant_patch_dir / "005.patch"
+    np3 = project_variant_patch_dir / "not-a-patch.txt"
+
+    # Create all of the test files
+    for p in [p1, p2, p3, p4, p5]:
+        p.write_text("this is a patch file")
+    for f in [np1, np2, np3]:
+        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, p3, p4]
+
+    results = list(
+        overrides.patches_for_source_dir(patches_dir, "project-1.2.3-variant")
+    )
+    assert results == [p2, p5]