feat: combine and validator constraints

Fromager's `Constraints` class now supports combining multiple
constraints and performs stricter validation of constraints. The class
now refuses constraints without a version specifier, extras, or an URL.
It packaging's new `SpecifierSet.is_unsatisfiable()` feature to detect
constraints that are not satisfiable.

Example:
The constraints `foo>=1.0`, `foo<2.0`, and `foo!=1.1.0` are combined
into to `foo<2.0,>=1.0,!=1.1.0`. A combination of `foo<1.0` and
`foo>=2.0` would result in a `InvalidConstraintError` instead.

See: #1096
Signed-off-by: Christian Heimes <cheimes@redhat.com>

Author: tiran

pyproject.toml

@@ -34,7 +34,7 @@ dependencies = [
     "click>=8.1.7",
     "elfdeps>=0.2.0",
     "license-expression",
-    "packaging",
+    "packaging>=26.1",
     "packageurl-python",
     "psutil",
     "pydantic>=2.12",

src/fromager/constraints.py

@@ -11,6 +11,10 @@
 logger = logging.getLogger(__name__)
 
 
+class InvalidConstraintError(ValueError):
+    pass
+
+
 class Constraints:
     def __init__(self) -> None:
         # mapping of canonical names to requirements
@@ -20,23 +24,50 @@ def __init__(self) -> None:
     def __iter__(self) -> Generator[NormalizedName, None, None]:
         yield from self._data
 
+    def __len__(self) -> int:
+        return len(self._data)
+
     def add_constraint(self, unparsed: str) -> None:
-        """Add new constraint, must not conflict with any existing constraints"""
+        """Add new constraint, must not conflict with any existing constraints
+
+        .. versionchanged: 0.83.0
+           Non-conflicting constraints are now combined. Constraints with
+           conflicts now raise :exc:`InvalidConstraintError`. Inputs without a
+           version specifier or with extras or url are also refused.
+        """
         req = Requirement(unparsed)
         canon_name = canonicalize_name(req.name)
         previous = self._data.get(canon_name)
 
+        # validator properties: must have a specifier, must not have extras or URL
+        if req.extras:
+            raise InvalidConstraintError(f"Constraint {unparsed!r} has extras")
+        if req.url:
+            raise InvalidConstraintError(f"Constraint {unparsed!r} has an url")
+        if not req.specifier:
+            raise InvalidConstraintError(f"Constraint {unparsed!r} has no specifiers")
+
+        # verify that incoming constraint is okay by itself
+        if req.specifier.is_unsatisfiable():
+            raise InvalidConstraintError(f"Constraint {unparsed!r} is unsatisfiable")
+
         if not requirements_file.evaluate_marker(req, req):
             logger.debug(f"Constraint {req} does not match environment")
             return
 
         if previous is not None:
-            raise KeyError(
-                f"{canon_name}: new constraint '{req}' conflicts with '{previous}'"
-            )
-        if requirements_file.evaluate_marker(req, req):
+            logger.debug("combining constraints %s and %s", previous, req)
+            new_specifier = req.specifier & previous.specifier
+            if new_specifier.is_unsatisfiable():
+                raise InvalidConstraintError(
+                    f"Combined specifier '{new_specifier}' is not satisfiable "
+                    f"(existing: {previous}, new: {req})"
+                )
+            req.specifier = new_specifier
+        else:
             logger.debug(f"adding constraint {req}")
-            self._data[canon_name] = req
+
+        self._data[canon_name] = req
 
     def load_constraints_file(self, constraints_file: str | pathlib.Path) -> None:
         """Load constraints from a constraints file"""

tests/test_constraints.py

@@ -1,24 +1,24 @@
 import pathlib
-import typing
+from unittest import mock
 
 import pytest
 from packaging import markers
 from packaging.requirements import Requirement
 from packaging.version import Version
 
-from fromager import constraints
+from fromager.constraints import Constraints, InvalidConstraintError
 
 
 def test_constraint_is_satisfied_by() -> None:
-    c = constraints.Constraints()
+    c = Constraints()
     c.add_constraint("foo<=1.1")
     assert c.is_satisfied_by("foo", Version("1.1"))
     assert c.is_satisfied_by("foo", Version("1.0"))
     assert c.is_satisfied_by("bar", Version("2.0"))
 
 
 def test_constraint_canonical_name() -> None:
-    c = constraints.Constraints()
+    c = Constraints()
     c.add_constraint("flash_attn<=1.1")
     assert c.is_satisfied_by("flash_attn", Version("1.1"))
     assert c.is_satisfied_by("flash-attn", Version("1.1"))
@@ -27,78 +27,72 @@ def test_constraint_canonical_name() -> None:
 
 
 def test_constraint_not_is_satisfied_by() -> None:
-    c = constraints.Constraints()
+    c = Constraints()
     c.add_constraint("foo<=1.1")
     c.add_constraint("bar>=2.0")
     assert not c.is_satisfied_by("foo", Version("1.2"))
     assert not c.is_satisfied_by("foo", Version("2.0"))
     assert not c.is_satisfied_by("bar", Version("1.0"))
 
 
+@mock.patch("platform.machine", mock.Mock(return_value="atari"))
 def test_add_constraint_conflict() -> None:
-    c = constraints.Constraints()
+    assert markers.default_environment()["platform_machine"] == "atari"
+
+    c = Constraints()
     c.add_constraint("foo<=1.1")
     c.add_constraint("flit_core==2.0rc3")
 
-    # Exact duplicate should raise error (same package, same marker)
-    with pytest.raises(KeyError):
-        c.add_constraint("foo<=1.1")
-
-    # Different version, same marker (no marker) should raise error
-    with pytest.raises(KeyError):
+    # Conflicting version, same marker (no marker) should raise error
+    with pytest.raises(InvalidConstraintError):
         c.add_constraint("foo>1.1")
 
-    # Different version for flit_core should raise error
-    with pytest.raises(KeyError):
+    # Conflicting version for flit_core should raise error
+    with pytest.raises(InvalidConstraintError):
         c.add_constraint("flit_core>2.0.0")
 
     # Normalized name conflict should raise error
-    with pytest.raises(KeyError):
+    with pytest.raises(InvalidConstraintError):
         c.add_constraint("flit-core>2.0.0")
 
-    # Different, but equivalent markers should raise KeyError
-    with pytest.raises(KeyError):
-        # arm64 -> macos; aarch64 -> linux
-        for arch in ["x86_64", "arm64", "aarch64"]:
-            c.add_constraint(
-                f"bar==1.0; python_version >= '3.11' and platform_machine == '{arch}'"
-            )
-            c.add_constraint(
-                f"bar==1.1; platform_machine == '{arch}' and python_version >= '3.11'"
-            )
+    # Constraints for other platforms are ignored
+    c.add_constraint(
+        "bar==1.0; python_version >= '3.11' and platform_machine == 'amiga'"
+    )
+    assert c.get_constraint("bar") is None
+
+    c.add_constraint(
+        "bar==1.0; python_version >= '3.11' and platform_machine == 'atari'"
+    )
+    # Make sure correct constraint is added
+    constraint = c.get_constraint("bar")
+    assert constraint
+    assert constraint.name == "bar"
+    assert constraint.specifier == "==1.0"
+    assert constraint.marker == markers.Marker(
+        'python_version >= "3.11" and platform_machine == "atari"'
+    )
+
+    # Different, but equivalent markers should raise error
+    with pytest.raises(InvalidConstraintError):
+        c.add_constraint(
+            "bar==1.1; platform_machine == 'atari' and python_version >= '3.11'"
+        )
 
     # Same package with different markers should NOT raise error
-    c.add_constraint("baz==1.0; platform_machine != 'ppc64le'")
-    c.add_constraint("baz==1.1; platform_machine == 'ppc64le'")
+    c.add_constraint("baz==1.0; platform_machine != 'amiga'")
+    c.add_constraint("baz==1.1; platform_machine == 'amiga'")
 
     # But same package with same marker should raise error
-    with pytest.raises(KeyError):
-        c.add_constraint("foo==1.2; platform_machine != 'ppc64le'")
+    with pytest.raises(InvalidConstraintError):
+        c.add_constraint("foo==1.2; platform_machine != 'amiga'")
 
     # Verify multiple constraints for same package are stored
-    assert len(c._data) == 4  # flit_core, foo, bar, and baz
-
-    # Make sure correct constraint is added
-    env = typing.cast(dict[str, str], markers.default_environment())
-    constraint = c.get_constraint("bar")
-
-    if env.get("platform_machine") == "x86_64" and constraint is not None:
-        assert constraint.name == "bar"
-        assert constraint.specifier == "==1.0"
-        assert constraint.marker == markers.Marker(
-            'python_version >= "3.11" and platform_machine == "x86_64"'
-        )
-
-    if env.get("platform_machine") == "arm64" and constraint is not None:
-        assert constraint.name == "bar"
-        assert constraint.specifier == "==1.0"
-        assert constraint.marker == markers.Marker(
-            'python_version >= "3.11" and platform_machine == "arm64"'
-        )
+    assert len(c) == 4  # flit_core, foo, bar, and baz
 
 
 def test_allow_prerelease() -> None:
-    c = constraints.Constraints()
+    c = Constraints()
     c.add_constraint("foo>=1.1")
     assert not c.allow_prerelease("foo")
     c.add_constraint("bar>=1.1a0")
@@ -109,15 +103,40 @@ def test_allow_prerelease() -> None:
 
 def test_load_non_existant_constraints_file(tmp_path: pathlib.Path) -> None:
     non_existant_file = tmp_path / "non_existant.txt"
-    c = constraints.Constraints()
+    c = Constraints()
     with pytest.raises(FileNotFoundError):
         c.load_constraints_file(non_existant_file)
 
 
 def test_load_constraints_file(tmp_path: pathlib.Path) -> None:
     constraint_file = tmp_path / "constraint.txt"
-    constraint_file.write_text("egg\ntorch==3.1.0 # comment\n")
-    c = constraints.Constraints()
+    constraint_file.write_text("egg==1.0\ntorch==3.1.0 # comment\n")
+    c = Constraints()
     c.load_constraints_file(constraint_file)
     assert list(c) == ["egg", "torch"]  # type: ignore
     assert c.get_constraint("torch") == Requirement("torch==3.1.0")
+
+
+def test_invalid_constraints() -> None:
+    c = Constraints()
+    with pytest.raises(InvalidConstraintError, match=r".*no specifier"):
+        c.add_constraint("foo")
+    with pytest.raises(InvalidConstraintError, match=r".*has extras"):
+        c.add_constraint("foo[extra]>=1.0")
+    with pytest.raises(InvalidConstraintError, match=r".*has an url"):
+        c.add_constraint("foo@https://foo.test")
+
+
+def test_unsatisfiable() -> None:
+    c = Constraints()
+    with pytest.raises(InvalidConstraintError):
+        c.add_constraint("foo<1.0,>2.0")
+
+
+def test_combine_constraints() -> None:
+    c = Constraints()
+    c.add_constraint("foo>=1.0")
+    c.add_constraint("foo<2.0")
+    assert c.get_constraint("foo") == Requirement("foo<2.0,>=1.0")
+    c.add_constraint("foo!=1.1.0")
+    assert c.get_constraint("foo") == Requirement("foo<2.0,>=1.0,!=1.1.0")