Merge pull request #784 from tiran/normalize-sdist-metadata

fix: normalize name in default_get_install_dependencies_of_sdist

Author: mergify[bot]

src/fromager/dependencies.py

@@ -12,7 +12,7 @@
 import tomlkit
 from packaging.metadata import Metadata
 from packaging.requirements import Requirement
-from packaging.utils import canonicalize_name
+from packaging.utils import NormalizedName, canonicalize_name
 from packaging.version import Version
 
 from . import (
@@ -318,7 +318,8 @@ def default_get_install_dependencies_of_sdist(
         req=req,
         version=version,
         what="sdist metadata",
-        dist_name=metadata.name,
+        # Metadata name is a non-normalized string
+        dist_name=canonicalize_name(metadata.name),
         dist_version=metadata.version,
     )
     if not metadata.requires_dist:
@@ -371,17 +372,30 @@ def pep517_metadata_of_sdist(
 
 
 def validate_dist_name_version(
-    req: Requirement, version: Version, what: str, dist_name: str, dist_version: Version
+    req: Requirement,
+    version: Version,
+    what: str,
+    dist_name: NormalizedName,
+    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}")
+        if dist_name != canonicalize_name(dist_name):
+            # API misuse
+            raise RuntimeError("dist_name argument {dist_name!r} is not normalized")
         else:
-            logger.warning(f"{what} has different local version than {version!r}")
+            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}"
+        )
 
 
 def get_install_dependencies_of_wheel(

tests/test_dependencies.py

@@ -2,11 +2,15 @@
 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
 
@@ -206,3 +210,72 @@ 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()