Specs: propagated variants affect == equality (#47376)

This PR changes the semantic of == for spec so that:

hdf5++mpi == hdf5+mpi

won't hold true anymore. It also changes the constrain semantic, so that a
non-propagating variant always override a propagating variant. This means:

(hdf5++mpi).constrain(hdf5+mpi) -> hdf5+mpi

Before we had a very weird semantic, that was supposed to be tested by unit-tests:

(libelf++debug).constrain(libelf+debug+foo) -> libelf++debug++foo

This semantic has been dropped, as it was never really tested due to the == bug.

lib/spack/spack/spec.py

@@ -877,8 +877,9 @@ def constrain(self, other):
                 # Next, if any flags in other propagate, we force them to propagate in our case
                 shared = list(sorted(set(other[flag_type]) - extra_other))
                 for x, y in _shared_subset_pair_iterate(shared, sorted(self[flag_type])):
-                    if x.propagate:
+                    if y.propagate is True and x.propagate is False:
-                        y.propagate = True
+                        changed = True
+                        y.propagate = False
 
         # TODO: what happens if flag groups with a partial (but not complete)
         # intersection specify different behaviors for flag propagation?
@@ -933,6 +934,7 @@ def _cmp_iter(self):
             def flags():
                 for flag in v:
                     yield flag
+                    yield flag.propagate
 
             yield flags
 

lib/spack/spack/test/spec_semantics.py

@@ -231,7 +231,7 @@ class TestSpecSemantics:
             ("mpich+foo", "mpich foo=True", "mpich+foo"),
             ("mpich++foo", "mpich foo=True", "mpich+foo"),
             ("mpich foo=true", "mpich+foo", "mpich+foo"),
-            ("mpich foo==true", "mpich++foo", "mpich+foo"),
+            ("mpich foo==true", "mpich++foo", "mpich++foo"),
             ("mpich~foo", "mpich foo=FALSE", "mpich~foo"),
             ("mpich~~foo", "mpich foo=FALSE", "mpich~foo"),
             ("mpich foo=False", "mpich~foo", "mpich~foo"),
@@ -271,17 +271,17 @@ class TestSpecSemantics:
             ("mpich+foo", "mpich", "mpich+foo"),
             ("mpich~foo", "mpich", "mpich~foo"),
             ("mpich foo=1", "mpich", "mpich foo=1"),
-            ("mpich", "mpich++foo", "mpich+foo"),
+            ("mpich", "mpich++foo", "mpich++foo"),
             ("libelf+debug", "libelf+foo", "libelf+debug+foo"),
             ("libelf+debug", "libelf+debug+foo", "libelf+debug+foo"),
             ("libelf debug=2", "libelf foo=1", "libelf debug=2 foo=1"),
             ("libelf debug=2", "libelf debug=2 foo=1", "libelf debug=2 foo=1"),
             ("libelf+debug", "libelf~foo", "libelf+debug~foo"),
             ("libelf+debug", "libelf+debug~foo", "libelf+debug~foo"),
-            ("libelf++debug", "libelf+debug+foo", "libelf++debug++foo"),
+            ("libelf++debug", "libelf+debug+foo", "libelf+debug+foo"),
-            ("libelf debug==2", "libelf foo=1", "libelf debug==2 foo==1"),
+            ("libelf debug==2", "libelf foo=1", "libelf debug==2 foo=1"),
-            ("libelf debug==2", "libelf debug=2 foo=1", "libelf debug==2 foo==1"),
+            ("libelf debug==2", "libelf debug=2 foo=1", "libelf debug=2 foo=1"),
-            ("libelf++debug", "libelf++debug~foo", "libelf++debug~~foo"),
+            ("libelf++debug", "libelf++debug~foo", "libelf++debug~foo"),
             ("libelf foo=bar,baz", "libelf foo=*", "libelf foo=bar,baz"),
             ("libelf foo=*", "libelf foo=bar,baz", "libelf foo=bar,baz"),
             (
@@ -367,19 +367,24 @@ def test_abstract_specs_can_constrain_each_other(self, lhs, rhs, expected):
                 'mpich cflags="-O3 -g"',
                 'mpich cflags=="-O3"',
                 'mpich cflags="-O3 -g"',
+                'mpich cflags="-O3 -g"',
+                [],
+                [],
+            ),
+            (
                 'mpich cflags=="-O3 -g"',
-                [("cflags", "-O3")],
+                'mpich cflags=="-O3"',
-                [("cflags", "-O3")],
+                'mpich cflags=="-O3 -g"',
+                'mpich cflags=="-O3 -g"',
+                [("cflags", "-O3"), ("cflags", "-g")],
+                [("cflags", "-O3"), ("cflags", "-g")],
             ),
         ],
     )
     def test_constrain_compiler_flags(
         self, lhs, rhs, expected_lhs, expected_rhs, propagated_lhs, propagated_rhs
     ):
-        """Constraining is asymmetric for compiler flags. Also note that
+        """Constraining is asymmetric for compiler flags."""
-        Spec equality does not account for flag propagation, so the checks
-        here are manual.
-        """
         lhs, rhs, expected_lhs, expected_rhs = (
             Spec(lhs),
             Spec(rhs),
@@ -1904,3 +1909,20 @@ def test_old_format_strings_trigger_error(default_mock_concretization):
     s = Spec("pkg-a").concretized()
     with pytest.raises(SpecFormatStringError):
         s.format("${PACKAGE}-${VERSION}-${HASH}")
+
+
+@pytest.mark.regression("47362")
+@pytest.mark.parametrize(
+    "lhs,rhs",
+    [
+        ("hdf5 +mpi", "hdf5++mpi"),
+        ("hdf5 cflags==-g", "hdf5 cflags=-g"),
+        ("hdf5 +mpi ++shared", "hdf5+mpi +shared"),
+        ("hdf5 +mpi cflags==-g", "hdf5++mpi cflag=-g"),
+    ],
+)
+def test_equality_discriminate_on_propagation(lhs, rhs):
+    """Tests that == can discriminate abstract specs based on their 'propagation' status"""
+    s, t = Spec(lhs), Spec(rhs)
+    assert s != t
+    assert len({s, t}) == 2

lib/spack/spack/variant.py

@@ -251,7 +251,7 @@ def implicit_variant_conversion(method):
     def convert(self, other):
         # We don't care if types are different as long as I can convert other to type(self)
         try:
-            other = type(self)(other.name, other._original_value)
+            other = type(self)(other.name, other._original_value, propagate=other.propagate)
         except (error.SpecError, ValueError):
             return False
         return method(self, other)
@@ -379,6 +379,7 @@ def _value_setter(self, value: ValueType) -> None:
     def _cmp_iter(self) -> Iterable:
         yield self.name
         yield from (str(v) for v in self.value_as_tuple)
+        yield self.propagate
 
     def copy(self) -> "AbstractVariant":
         """Returns an instance of a variant equivalent to self
@@ -453,6 +454,7 @@ def constrain(self, other: "AbstractVariant") -> bool:
             values.remove("*")
 
         self._value_setter(",".join(str(v) for v in values))
+        self.propagate = self.propagate and other.propagate
         return old_value != self.value
 
     def __contains__(self, item: Union[str, bool]) -> bool:
@@ -557,6 +559,7 @@ def constrain(self, other: "AbstractVariant") -> bool:
 
         if self.value != other.value:
             raise UnsatisfiableVariantSpecError(other.value, self.value)
+        self.propagate = self.propagate and other.propagate
         return False
 
     def __contains__(self, item: ValueType) -> bool: