From e8fc64f38dacb4edc211898458ef6926a4a48ed3 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Thu, 7 Nov 2024 18:11:10 +0100 Subject: [PATCH] Revert "Specs: propagated variants affect `==` equality (#47376)" This reverts commit 395c911689d7a3ee426660f3fab334ccee04da1a. --- lib/spack/spack/spec.py | 6 ++-- lib/spack/spack/test/spec_semantics.py | 46 +++++++------------------- lib/spack/spack/variant.py | 5 +-- 3 files changed, 15 insertions(+), 42 deletions(-) diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index 3d92879484f..257eb2f66db 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -877,9 +877,8 @@ 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 y.propagate is True and x.propagate is False: - changed = True - y.propagate = False + if x.propagate: + y.propagate = True # TODO: what happens if flag groups with a partial (but not complete) # intersection specify different behaviors for flag propagation? @@ -934,7 +933,6 @@ def _cmp_iter(self): def flags(): for flag in v: yield flag - yield flag.propagate yield flags diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py index 6342325364a..c49df1a7b27 100644 --- a/lib/spack/spack/test/spec_semantics.py +++ b/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==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++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 debug=2 foo=1", "libelf debug==2 foo==1"), + ("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,24 +367,19 @@ 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"', - 'mpich cflags=="-O3"', - 'mpich cflags=="-O3 -g"', - 'mpich cflags=="-O3 -g"', - [("cflags", "-O3"), ("cflags", "-g")], - [("cflags", "-O3"), ("cflags", "-g")], + [("cflags", "-O3")], + [("cflags", "-O3")], ), ], ) def test_constrain_compiler_flags( self, lhs, rhs, expected_lhs, expected_rhs, propagated_lhs, propagated_rhs ): - """Constraining is asymmetric for compiler flags.""" + """Constraining is asymmetric for compiler flags. Also note that + Spec equality does not account for flag propagation, so the checks + here are manual. + """ lhs, rhs, expected_lhs, expected_rhs = ( Spec(lhs), Spec(rhs), @@ -1958,20 +1953,3 @@ 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 diff --git a/lib/spack/spack/variant.py b/lib/spack/spack/variant.py index bce2015c120..8a5a03c06cd 100644 --- a/lib/spack/spack/variant.py +++ b/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, propagate=other.propagate) + other = type(self)(other.name, other._original_value) except (error.SpecError, ValueError): return False return method(self, other) @@ -379,7 +379,6 @@ 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 @@ -454,7 +453,6 @@ 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: @@ -559,7 +557,6 @@ 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: