Repository: python-wheel-build/fromager

Issue:
source finders should be aware of download config settings
If a user configures special downloader instructions using the settings file, especially if rename instructions are included, then it should not be necessary to create override functions for the finders to use to find those names.

Discussion:

Patch:
diff --git a/docs/customization.md b/docs/customization.md
index 72a5343..e411da6 100644
--- a/docs/customization.md
+++ b/docs/customization.md
@@ -448,7 +448,10 @@ def post_build(
 
 ## Customizations using settings.yaml
 
-To use predefined urls to download sources from, instead of overriding the entire `download_source` function, a mapping of package to download source url can be provided directly in settings.yaml. Optionally the downloaded sdist can be renamed. Both the url and the destination filename support templating. The only supported template variable is `version` - it is replaced by the version returned by the resolver.
+To use predefined urls to download sources from, instead of overriding the entire `download_source` function, a mapping of package to download source url can be provided directly in settings.yaml. Optionally the downloaded sdist can be renamed. Both the url and the destination filename support templating. The only supported template variable are:  
+
+- `version` - it is replaced by the version returned by the resolver  
+- `canonicalized_name` - it is replaced by the canonicalized name of the package specified in the requirement, specifically it applies `canonicalize_name(req.nam)`  
 
 Additionally, the source distribution index server used by the package resolver can be overriden for a particular package. The resolver can also be told to whether include wheels or sdist sources while trying to resolve the package. Templating is not supported here.
 
@@ -457,7 +460,7 @@ packages:
     torch:
         download_source:
             url: "https://github.com/pytorch/pytorch/releases/download/v${version}/pytorch-v${version}.tar.gz"
-            destination_filename: "torch-${version}.tar.gz"
+            destination_filename: "${canonicalized_name}-${version}.tar.gz"
         resolver_dist:
             sdist_server_url: "https://pypi.org/simple"
             include_wheels: true
diff --git a/src/fromager/commands/step.py b/src/fromager/commands/step.py
index 111125b..1e043ac 100644
--- a/src/fromager/commands/step.py
+++ b/src/fromager/commands/step.py
@@ -61,7 +61,9 @@ def prepare_source(
     """
     req = Requirement(f"{dist_name}=={dist_version}")
     sdists_downloads = pathlib.Path(wkctx.sdists_repo) / "downloads"
-    source_filename = finders.find_sdist(wkctx.sdists_downloads, req, str(dist_version))
+    source_filename = finders.find_sdist(
+        wkctx, wkctx.sdists_downloads, req, str(dist_version)
+    )
     if source_filename is None:
         dir_contents: list[str] = []
         for ext in ["*.tar.gz", "*.zip"]:
@@ -94,7 +96,7 @@ def build_sdist(
 
     """
     req = Requirement(f"{dist_name}=={dist_version}")
-    source_root_dir = _find_source_root_dir(wkctx.work_dir, req, dist_version)
+    source_root_dir = _find_source_root_dir(wkctx, wkctx.work_dir, req, dist_version)
     build_env = wheels.BuildEnvironment(wkctx, source_root_dir.parent, None)
     sdist_filename = sources.build_sdist(
         ctx=wkctx,
@@ -107,11 +109,12 @@ def build_sdist(
 
 
 def _find_source_root_dir(
+    wkctx: context.WorkContext,
     work_dir: pathlib.Path,
     req: Requirement,
     dist_version: Version,
 ) -> pathlib.Path:
-    source_root_dir = finders.find_source_dir(work_dir, req, str(dist_version))
+    source_root_dir = finders.find_source_dir(wkctx, work_dir, req, str(dist_version))
     if source_root_dir:
         return source_root_dir
     work_dir_contents = list(str(e) for e in work_dir.glob("*"))
@@ -138,7 +141,7 @@ def prepare_build(
     """
     server.start_wheel_server(wkctx)
     req = Requirement(f"{dist_name}=={dist_version}")
-    source_root_dir = _find_source_root_dir(wkctx.work_dir, req, dist_version)
+    source_root_dir = _find_source_root_dir(wkctx, wkctx.work_dir, req, dist_version)
     sdist.prepare_build_environment(wkctx, req, source_root_dir)
 
 
@@ -159,7 +162,7 @@ def build_wheel(
 
     """
     req = Requirement(f"{dist_name}=={dist_version}")
-    source_root_dir = _find_source_root_dir(wkctx.work_dir, req, dist_version)
+    source_root_dir = _find_source_root_dir(wkctx, wkctx.work_dir, req, dist_version)
     build_env = wheels.BuildEnvironment(wkctx, source_root_dir.parent, None)
     wheel_filename = wheels.build_wheel(
         ctx=wkctx,
diff --git a/src/fromager/finders.py b/src/fromager/finders.py
index b480ef2..5622d79 100644
--- a/src/fromager/finders.py
+++ b/src/fromager/finders.py
@@ -7,7 +7,7 @@ import re
 from packaging.requirements import Requirement
 from packaging.utils import canonicalize_name
 
-from . import overrides
+from . import context, overrides
 
 logger = logging.getLogger(__name__)
 
@@ -21,20 +21,35 @@ def _dist_name_to_filename(dist_name: str) -> str:
     return re.sub(r"[^\w\d.]+", "_", canonical_name, flags=re.UNICODE)
 
 
+def _check_archive_name_in_settings(
+    ctx: context.WorkContext,
+    req: Requirement,
+    dist_version: str,
+):
+    destination_filename_from_settings = (
+        ctx.settings.download_source_destination_filename(
+            req.name, req=req, version=dist_version
+        )
+    )
+    return destination_filename_from_settings
+
+
 def find_sdist(
+    ctx: context.WorkContext,
     downloads_dir: pathlib.Path,
     req: Requirement,
     dist_version: str,
 ) -> pathlib.Path | None:
-    sdist_name_func = overrides.find_override_method(
-        req.name, "expected_source_archive_name"
+    sdist_file_name = overrides.find_and_invoke(
+        req.name,
+        "expected_source_archive_name",
+        _check_archive_name_in_settings,
+        req=req,
+        dist_version=dist_version,
+        ctx=ctx,
     )
 
-    if sdist_name_func:
-        # The file must exist exactly as given.
-        sdist_file_name = overrides.invoke(
-            sdist_name_func, req=req, dist_version=dist_version
-        )
+    if sdist_file_name:
         sdist_file = downloads_dir / sdist_file_name
         if sdist_file.exists():
             return sdist_file
@@ -113,6 +128,7 @@ def find_wheel(
 
 
 def find_source_dir(
+    ctx: context.WorkContext,
     work_dir: pathlib.Path,
     req: Requirement,
     dist_version: str,
@@ -130,14 +146,17 @@ def find_source_dir(
             return source_dir
         raise ValueError(f"looked for {source_dir} and did not find")
 
-    sdist_name_func = overrides.find_override_method(
-        req.name, "expected_source_archive_name"
+    sdist_name = overrides.find_and_invoke(
+        req.name,
+        "expected_source_archive_name",
+        _check_archive_name_in_settings,
+        req=req,
+        dist_version=dist_version,
+        ctx=ctx,
     )
-    if sdist_name_func:
+
+    if sdist_name:
         # The directory must exist exactly as given.
-        sdist_name = overrides.invoke(
-            sdist_name_func, req=req, dist_version=dist_version
-        )
         if sdist_name.endswith(".tar.gz"):
             ext_to_strip = ".tar.gz"
         elif sdist_name.endswith(".zip"):
diff --git a/src/fromager/sdist.py b/src/fromager/sdist.py
index 804a2b4..a02f2cb 100644
--- a/src/fromager/sdist.py
+++ b/src/fromager/sdist.py
@@ -214,7 +214,7 @@ def handle_requirement(
             )
             try:
                 find_sdist_result = finders.find_sdist(
-                    ctx.sdists_builds, req, resolved_version
+                    ctx, ctx.sdists_builds, req, resolved_version
                 )
                 if not find_sdist_result:
                     sources.build_sdist(
diff --git a/src/fromager/settings.py b/src/fromager/settings.py
index dfa31b5..cceda3f 100644
--- a/src/fromager/settings.py
+++ b/src/fromager/settings.py
@@ -1,8 +1,12 @@
 import logging
 import pathlib
+import string
 import typing
 
 import yaml
+from packaging.requirements import Requirement
+from packaging.utils import canonicalize_name
+from packaging.version import Version
 
 from . import overrides
 
@@ -24,17 +28,35 @@ class Settings:
             overrides.pkgname_to_override_module(key): value for key, value in p.items()
         }
 
-    def download_source_url(self, pkg: str, default: str | None = None) -> str | None:
+    def download_source_url(
+        self,
+        pkg: str,
+        req: Requirement | None = None,
+        version: Version | str | None = None,
+        default: str | None = None,
+        resolve_template: bool = True,
+    ) -> str | None:
         download_source = self._get_package_download_source_settings(pkg)
-        return self._return_value_or_default(download_source.get("url"), default)
+        url = self._return_value_or_default(download_source.get("url"), default)
+        if url and resolve_template:
+            url = _resolve_template(url, req, version)
+        return url
 
     def download_source_destination_filename(
-        self, pkg: str, default: str | None = None
+        self,
+        pkg: str,
+        req: Requirement | None = None,
+        version: Version | str | None = None,
+        default: str | None = None,
+        resolve_template: bool = True,
     ) -> str | None:
         download_source = self._get_package_download_source_settings(pkg)
-        return self._return_value_or_default(
+        destination_filename = self._return_value_or_default(
             download_source.get("destination_filename"), default
         )
+        if destination_filename and resolve_template:
+            destination_filename = _resolve_template(destination_filename, req, version)
+        return destination_filename
 
     def resolver_sdist_server_url(
         self, pkg: str, default: str | None = None
@@ -79,6 +101,34 @@ class Settings:
         return value if value is not None else default
 
 
+def _resolve_template(
+    template: str | None,
+    req: Requirement | None = None,
+    version: Version | str | None = None,
+):
+    if not template:
+        return None
+
+    template_env = {}
+    if version:
+        template_env["version"] = str(version)
+    if req:
+        template_env["canonicalized_name"] = str(canonicalize_name(req.name))
+
+    try:
+        return string.Template(template).substitute(template_env)
+    except KeyError:
+        if req:
+            logger.warning(
+                f"{req.name}: Couldn't resolve url or name for {req} using the template: {template_env}"
+            )
+        else:
+            logger.warning(
+                f"Couldn't resolve {template} using the template: {template_env}"
+            )
+        raise
+
+
 def _parse(content: str) -> Settings:
     data = yaml.safe_load(content)
     return Settings(data)
diff --git a/src/fromager/sources.py b/src/fromager/sources.py
index 796a63c..705b2d7 100644
--- a/src/fromager/sources.py
+++ b/src/fromager/sources.py
@@ -4,7 +4,6 @@ import logging
 import os.path
 import pathlib
 import shutil
-import string
 import subprocess
 import tarfile
 import typing
@@ -37,7 +36,7 @@ def download_source(
     source_type = "override"
     if not downloader:
         downloader = default_download_source
-        if not ctx.settings.download_source_url(req.name):
+        if not ctx.settings.download_source_url(req.name, resolve_template=False):
             source_type = "sdist"
     for url in sdist_server_urls:
         try:
@@ -158,10 +157,12 @@ def default_download_source(
         ctx, req, override_sdist_server_url, include_sdists, include_wheels
     )
 
-    dest_filename_template = ctx.settings.download_source_destination_filename(req.name)
-    destination_filename = _resolve_template(dest_filename_template, req, version)
-    url_template = ctx.settings.download_source_url(req.name, org_url)
-    url = _resolve_template(url_template, req, version)
+    destination_filename = ctx.settings.download_source_destination_filename(
+        req.name, req=req, version=version
+    )
+    url = ctx.settings.download_source_url(
+        req.name, default=org_url, req=req, version=version
+    )
 
     source_filename = _download_source_check(
         ctx.sdists_downloads, url, destination_filename
@@ -222,19 +223,6 @@ def download_url(
     return outfile
 
 
-def _resolve_template(template: str | None, req: Requirement, version: str):
-    if not template:
-        return None
-    template_env = {"version": version}
-    try:
-        return string.Template(template).substitute(template_env)
-    except KeyError:
-        logger.warning(
-            f"{req.name}: Couldn't resolve url or name for {req} using the template: {template_env}"
-        )
-        raise
-
-
 def _sdist_root_name(source_filename: pathlib.Path) -> str:
     base_name = pathlib.Path(source_filename).name
     if base_name.endswith(".tar.gz"):


Test patch:
diff --git a/tests/test_finders.py b/tests/test_finders.py
index b892a78..9b3c905 100644
--- a/tests/test_finders.py
+++ b/tests/test_finders.py
@@ -17,7 +17,7 @@ from fromager import finders
         ("ruamel-yaml", "0.18.6", "ruamel.yaml-0.18.6.tar.gz"),
     ],
 )
-def test_find_sdist(tmp_path, dist_name, version_string, expected_base):
+def test_find_sdist(tmp_path, tmp_context, dist_name, version_string, expected_base):
     sdists_repo = pathlib.Path(tmp_path)
     downloads = sdists_repo / "downloads"
     downloads.mkdir()
@@ -26,7 +26,7 @@ def test_find_sdist(tmp_path, dist_name, version_string, expected_base):
 
     req = Requirement(dist_name)
     ver = Version(version_string)
-    actual = finders.find_sdist(downloads, req, ver)
+    actual = finders.find_sdist(tmp_context, downloads, req, ver)
     assert str(archive) == str(actual)
 
 
@@ -68,7 +68,9 @@ def test_find_wheel(tmp_path, dist_name, version_string, expected_base):
         ("ruamel-yaml", "0.18.6", "ruamel.yaml-0.18.6", "ruamel.yaml-0.18.6"),
     ],
 )
-def test_find_source_dir(tmp_path, dist_name, version_string, unpack_base, source_base):
+def test_find_source_dir(
+    tmp_path, tmp_context, dist_name, version_string, unpack_base, source_base
+):
     work_dir = pathlib.Path(tmp_path)
     unpack_dir = work_dir / unpack_base
     unpack_dir.mkdir()
@@ -78,5 +80,5 @@ def test_find_source_dir(tmp_path, dist_name, version_string, unpack_base, sourc
 
     req = Requirement(dist_name)
     ver = Version(version_string)
-    actual = finders.find_source_dir(work_dir, req, ver)
+    actual = finders.find_source_dir(tmp_context, work_dir, req, ver)
     assert str(source_dir) == str(actual)
diff --git a/tests/test_settings.py b/tests/test_settings.py
index b4bd423..a8afb27 100644
--- a/tests/test_settings.py
+++ b/tests/test_settings.py
@@ -1,5 +1,8 @@
 import textwrap
 
+import pytest
+from packaging.requirements import Requirement
+
 from fromager import settings
 
 
@@ -86,3 +89,19 @@ def test_no_resolver_dist():
     assert s.resolver_include_sdists("foo") is None
     assert s.resolver_include_wheels("foo") is None
     assert s.resolver_sdist_server_url("foo") is None
+
+
+def test_resolve_template_with_no_template():
+    req = Requirement("foo==1.0")
+    assert settings._resolve_template(None, req, "1.0") is None
+
+
+def test_resolve_template_with_version():
+    req = Requirement("foo==1.0")
+    assert settings._resolve_template("url-${version}", req, "1.0") == "url-1.0"
+
+
+def test_resolve_template_with_no_matching_template():
+    req = Requirement("foo==1.0")
+    with pytest.raises(KeyError):
+        settings._resolve_template("url-${flag}", req, "1.0")
diff --git a/tests/test_sources.py b/tests/test_sources.py
index c18b7a4..a882e7e 100644
--- a/tests/test_sources.py
+++ b/tests/test_sources.py
@@ -19,37 +19,21 @@ def test_invalid_tarfile(mock_download_url, tmp_path: pathlib.Path):
         sources._download_source_check(fake_dir, fake_url)
 
 
-def test_resolve_template_with_no_template():
-    req = Requirement("foo==1.0")
-    assert sources._resolve_template(None, req, "1.0") is None
-
-
-def test_resolve_template_with_version():
-    req = Requirement("foo==1.0")
-    assert sources._resolve_template("url-${version}", req, "1.0") == "url-1.0"
-
-
-def test_resolve_template_with_no_matching_template():
-    req = Requirement("foo==1.0")
-    with pytest.raises(KeyError):
-        sources._resolve_template("url-${flag}", req, "1.0")
-
-
 @patch("fromager.sources.resolve_dist")
 @patch("fromager.sources._download_source_check")
-@patch.object(settings.Settings, "download_source_url")
-@patch.object(settings.Settings, "download_source_destination_filename")
+@patch.object(settings.Settings, "_get_package_download_source_settings")
 def test_default_download_source_from_settings(
-    download_source_destination_filename: Mock,
-    download_source_url: Mock,
+    get_package_download_source_settings: Mock,
     download_source_check: Mock,
     resolve_dist: Mock,
     tmp_context: context.WorkContext,
 ):
     resolve_dist.return_value = ("url", "1.0")
     download_source_check.return_value = pathlib.Path("filename.zip")
-    download_source_url.return_value = "predefined_url-${version}"
-    download_source_destination_filename.return_value = "foo-${version}"
+    get_package_download_source_settings.return_value = {
+        "url": "predefined_url-${version}",
+        "destination_filename": "${canonicalized_name}-${version}",
+    }
     req = Requirement("foo==1.0")
     sdist_server_url = sources.PYPI_SERVER_URL