Fix satisfaction checks for excluding variants from matrices (#16893)

Because of the way abstract variants are implemented, the following 
spec matrix does not work as intended:
```
matrix:
- [foo]
- [bar=a, bar=b]
exclude:
- bar=a
```
because abstract variants always satisfy any variant of the same
name, regardless of values.

This PR converts abstract variants to whatever their appropriate 
type is before running satisfaction checks for the excludes clause 
in a matrix.

fixes #16841

8 files changed, 80 insertions, 50 deletions

diff --git a/lib/spack/spack/spec_list.py b/lib/spack/spack/spec_list.py
index de75458f32..4468df7c35 100644
--- a/lib/spack/spack/spec_list.py
+++ b/lib/spack/spack/spec_list.py
@@ -5,6 +5,7 @@
 import itertools
 from six import string_types
 
+import spack.variant
 from spack.spec import Spec
 from spack.error import SpackError
 
@@ -189,7 +190,18 @@ def _expand_matrix_constraints(object, specify=True):
         # Construct a combined spec to test against excludes
         flat_combo = [constraint for list in combo for constraint in list]
         ordered_combo = sorted(flat_combo, key=spec_ordering_key)
+
         test_spec = Spec(' '.join(ordered_combo))
+        # Abstract variants don't have normal satisfaction semantics
+        # Convert all variants to concrete types.
+        # This method is best effort, so all existing variants will be
+        # converted before any error is raised.
+        # Catch exceptions because we want to be able to operate on
+        # abstract specs without needing package information
+        try:
+            spack.variant.substitute_abstract_variants(test_spec)
+        except spack.variant.UnknownVariantError:
+            pass
         if any(test_spec.satisfies(x) for x in excludes):
             continue
 
diff --git a/lib/spack/spack/test/cmd/dependents.py b/lib/spack/spack/test/cmd/dependents.py
index 1001e8764f..22a3acd0c3 100644
--- a/lib/spack/spack/test/cmd/dependents.py
+++ b/lib/spack/spack/test/cmd/dependents.py
@@ -26,7 +26,7 @@ def test_transitive_dependents(mock_packages):
     out = dependents('--transitive', 'libelf')
     actual = set(re.split(r'\s+', out.strip()))
     assert actual == set(
-        ['callpath', 'dyninst', 'libdwarf', 'mpileaks', 'multivalue_variant',
+        ['callpath', 'dyninst', 'libdwarf', 'mpileaks', 'multivalue-variant',
          'singlevalue-variant-dependent',
          'patch-a-dependency', 'patch-several-dependencies'])
 
@@ -36,8 +36,8 @@ def test_immediate_installed_dependents(mock_packages, database):
     with color_when(False):
         out = dependents('--installed', 'libelf')
 
-    lines = [l for l in out.strip().split('\n') if not l.startswith('--')]
-    hashes = set([re.split(r'\s+', l)[0] for l in lines])
+    lines = [li for li in out.strip().split('\n') if not li.startswith('--')]
+    hashes = set([re.split(r'\s+', li)[0] for li in lines])
 
     expected = set([spack.store.db.query_one(s).dag_hash(7)
                     for s in ['dyninst', 'libdwarf']])
@@ -53,8 +53,8 @@ def test_transitive_installed_dependents(mock_packages, database):
     with color_when(False):
         out = dependents('--installed', '--transitive', 'fake')
 
-    lines = [l for l in out.strip().split('\n') if not l.startswith('--')]
-    hashes = set([re.split(r'\s+', l)[0] for l in lines])
+    lines = [li for li in out.strip().split('\n') if not li.startswith('--')]
+    hashes = set([re.split(r'\s+', li)[0] for li in lines])
 
     expected = set([spack.store.db.query_one(s).dag_hash(7)
                     for s in ['zmpi', 'callpath^zmpi', 'mpileaks^zmpi']])
diff --git a/lib/spack/spack/test/spec_list.py b/lib/spack/spack/test/spec_list.py
index 9bbbc435e2..ff45096c3e 100644
--- a/lib/spack/spack/test/spec_list.py
+++ b/lib/spack/spack/test/spec_list.py
@@ -156,3 +156,10 @@ class TestSpecList(object):
                                                 ['+shared', '~shared'])
         expected = [Spec(' '.join(combo)) for combo in expected_components]
         assert set(speclist.specs) == set(expected)
+
+    def test_spec_list_matrix_exclude(self, mock_packages):
+        # Test on non-boolean variants for regression for #16841
+        matrix = [{'matrix': [['multivalue-variant'], ['foo=bar', 'foo=baz']],
+                   'exclude': ['foo=bar']}]
+        speclist = SpecList('specs', matrix)
+        assert len(speclist.specs) == 1
diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py
index b55fa27ae1..d908ce7d89 100644
--- a/lib/spack/spack/test/spec_semantics.py
+++ b/lib/spack/spack/test/spec_semantics.py
@@ -275,27 +275,27 @@ class TestSpecSematics(object):
 
     def test_satisfies_multi_value_variant(self):
         # Check quoting
-        check_satisfies('multivalue_variant foo="bar,baz"',
-                        'multivalue_variant foo="bar,baz"')
-        check_satisfies('multivalue_variant foo=bar,baz',
-                        'multivalue_variant foo=bar,baz')
-        check_satisfies('multivalue_variant foo="bar,baz"',
-                        'multivalue_variant foo=bar,baz')
+        check_satisfies('multivalue-variant foo="bar,baz"',
+                        'multivalue-variant foo="bar,baz"')
+        check_satisfies('multivalue-variant foo=bar,baz',
+                        'multivalue-variant foo=bar,baz')
+        check_satisfies('multivalue-variant foo="bar,baz"',
+                        'multivalue-variant foo=bar,baz')
 
         # A more constrained spec satisfies a less constrained one
-        check_satisfies('multivalue_variant foo="bar,baz"',
-                        'multivalue_variant foo="bar"')
+        check_satisfies('multivalue-variant foo="bar,baz"',
+                        'multivalue-variant foo="bar"')
 
-        check_satisfies('multivalue_variant foo="bar,baz"',
-                        'multivalue_variant foo="baz"')
+        check_satisfies('multivalue-variant foo="bar,baz"',
+                        'multivalue-variant foo="baz"')
 
-        check_satisfies('multivalue_variant foo="bar,baz,barbaz"',
-                        'multivalue_variant foo="bar,baz"')
+        check_satisfies('multivalue-variant foo="bar,baz,barbaz"',
+                        'multivalue-variant foo="bar,baz"')
 
-        check_satisfies('multivalue_variant foo="bar,baz"',
+        check_satisfies('multivalue-variant foo="bar,baz"',
                         'foo="bar,baz"')
 
-        check_satisfies('multivalue_variant foo="bar,baz"',
+        check_satisfies('multivalue-variant foo="bar,baz"',
                         'foo="bar"')
 
     def test_satisfies_single_valued_variant(self):
@@ -325,7 +325,7 @@ class TestSpecSematics(object):
         a.concretize()
         assert '^b' not in a
 
-        mv = Spec('multivalue_variant')
+        mv = Spec('multivalue-variant')
         mv.concretize()
         assert 'a@1.0' not in mv
 
@@ -340,9 +340,9 @@ class TestSpecSematics(object):
         # Depending on whether the spec is concrete or not
 
         a = make_spec(
-            'multivalue_variant foo="bar"', concrete=True
+            'multivalue-variant foo="bar"', concrete=True
         )
-        spec_str = 'multivalue_variant foo="bar,baz"'
+        spec_str = 'multivalue-variant foo="bar,baz"'
         b = Spec(spec_str)
         assert not a.satisfies(b)
         assert not a.satisfies(spec_str)
@@ -350,8 +350,8 @@ class TestSpecSematics(object):
         with pytest.raises(UnsatisfiableSpecError):
             a.constrain(b)
 
-        a = Spec('multivalue_variant foo="bar"')
-        spec_str = 'multivalue_variant foo="bar,baz"'
+        a = Spec('multivalue-variant foo="bar"')
+        spec_str = 'multivalue-variant foo="bar,baz"'
         b = Spec(spec_str)
         # The specs are abstract and they **could** be constrained
         assert a.satisfies(b)
@@ -360,9 +360,9 @@ class TestSpecSematics(object):
         assert a.constrain(b)
 
         a = make_spec(
-            'multivalue_variant foo="bar,baz"', concrete=True
+            'multivalue-variant foo="bar,baz"', concrete=True
         )
-        spec_str = 'multivalue_variant foo="bar,baz,quux"'
+        spec_str = 'multivalue-variant foo="bar,baz,quux"'
         b = Spec(spec_str)
         assert not a.satisfies(b)
         assert not a.satisfies(spec_str)
@@ -370,8 +370,8 @@ class TestSpecSematics(object):
         with pytest.raises(UnsatisfiableSpecError):
             a.constrain(b)
 
-        a = Spec('multivalue_variant foo="bar,baz"')
-        spec_str = 'multivalue_variant foo="bar,baz,quux"'
+        a = Spec('multivalue-variant foo="bar,baz"')
+        spec_str = 'multivalue-variant foo="bar,baz,quux"'
         b = Spec(spec_str)
         # The specs are abstract and they **could** be constrained
         assert a.satisfies(b)
@@ -384,8 +384,8 @@ class TestSpecSematics(object):
             a.concretize()
 
         # This time we'll try to set a single-valued variant
-        a = Spec('multivalue_variant fee="bar"')
-        spec_str = 'multivalue_variant fee="baz"'
+        a = Spec('multivalue-variant fee="bar"')
+        spec_str = 'multivalue-variant fee="baz"'
         b = Spec(spec_str)
         # The specs are abstract and they **could** be constrained,
         # as before concretization I don't know which type of variant
@@ -405,20 +405,20 @@ class TestSpecSematics(object):
 
         # FIXME: these needs to be checked as the new relaxed
         # FIXME: semantic makes them fail (constrain does not raise)
-        # check_unsatisfiable('multivalue_variant +foo',
-        #                     'multivalue_variant foo="bar"')
-        # check_unsatisfiable('multivalue_variant ~foo',
-        #                     'multivalue_variant foo="bar"')
+        # check_unsatisfiable('multivalue-variant +foo',
+        #                     'multivalue-variant foo="bar"')
+        # check_unsatisfiable('multivalue-variant ~foo',
+        #                     'multivalue-variant foo="bar"')
 
         check_unsatisfiable(
-            target_spec='multivalue_variant foo="bar"',
-            constraint_spec='multivalue_variant +foo',
+            target_spec='multivalue-variant foo="bar"',
+            constraint_spec='multivalue-variant +foo',
             target_concrete=True
         )
 
         check_unsatisfiable(
-            target_spec='multivalue_variant foo="bar"',
-            constraint_spec='multivalue_variant ~foo',
+            target_spec='multivalue-variant foo="bar"',
+            constraint_spec='multivalue-variant ~foo',
             target_concrete=True
         )
 
@@ -597,15 +597,15 @@ class TestSpecSematics(object):
 
     def test_constrain_multi_value_variant(self):
         check_constrain(
-            'multivalue_variant foo="bar,baz"',
-            'multivalue_variant foo="bar"',
-            'multivalue_variant foo="baz"'
+            'multivalue-variant foo="bar,baz"',
+            'multivalue-variant foo="bar"',
+            'multivalue-variant foo="baz"'
         )
 
         check_constrain(
-            'multivalue_variant foo="bar,baz,barbaz"',
-            'multivalue_variant foo="bar,barbaz"',
-            'multivalue_variant foo="baz"'
+            'multivalue-variant foo="bar,baz,barbaz"',
+            'multivalue-variant foo="bar,barbaz"',
+            'multivalue-variant foo="baz"'
         )
 
     def test_constrain_compiler_flags(self):
@@ -734,7 +734,7 @@ class TestSpecSematics(object):
             Spec('libelf foo')
 
     def test_spec_formatting(self):
-        spec = Spec("multivalue_variant cflags=-O2")
+        spec = Spec("multivalue-variant cflags=-O2")
         spec.concretize()
 
         # Since the default is the full spec see if the string rep of
@@ -806,7 +806,7 @@ class TestSpecSematics(object):
             assert expected == actual
 
     def test_spec_formatting_escapes(self):
-        spec = Spec('multivalue_variant cflags=-O2')
+        spec = Spec('multivalue-variant cflags=-O2')
         spec.concretize()
 
         sigil_mismatches = [
@@ -895,7 +895,7 @@ class TestSpecSematics(object):
 
     def test_any_combination_of(self):
         # Test that using 'none' and another value raise during concretization
-        spec = Spec('multivalue_variant foo=none,bar')
+        spec = Spec('multivalue-variant foo=none,bar')
         with pytest.raises(spack.error.SpecError) as exc_info:
             spec.concretize()
 
diff --git a/lib/spack/spack/test/spec_yaml.py b/lib/spack/spack/test/spec_yaml.py
index 03129a7eb9..98fb1e68fe 100644
--- a/lib/spack/spack/test/spec_yaml.py
+++ b/lib/spack/spack/test/spec_yaml.py
@@ -69,7 +69,7 @@ def test_concrete_spec(config, mock_packages):
 
 
 def test_yaml_multivalue(config, mock_packages):
-    spec = Spec('multivalue_variant foo="bar,baz"')
+    spec = Spec('multivalue-variant foo="bar,baz"')
     spec.concretize()
     check_yaml_round_trip(spec)
 
diff --git a/lib/spack/spack/variant.py b/lib/spack/spack/variant.py
index 0443b68ec3..e43a002182 100644
--- a/lib/spack/spack/variant.py
+++ b/lib/spack/spack/variant.py
@@ -593,19 +593,30 @@ def substitute_abstract_variants(spec):
     """Uses the information in `spec.package` to turn any variant that needs
     it into a SingleValuedVariant.
 
+    This method is best effort. All variants that can be substituted will be
+    substituted before any error is raised.
+
     Args:
         spec: spec on which to operate the substitution
     """
+    # This method needs to be best effort so that it works in matrix exlusion
+    # in $spack/lib/spack/spack/spec_list.py
+    failed = []
     for name, v in spec.variants.items():
         if name in spack.directives.reserved_names:
             continue
         pkg_variant = spec.package_class.variants.get(name, None)
         if not pkg_variant:
-            raise UnknownVariantError(spec, [name])
+            failed.append(name)
+            continue
         new_variant = pkg_variant.make_variant(v._original_value)
         pkg_variant.validate_or_raise(new_variant, spec.package_class)
         spec.variants.substitute(new_variant)
 
+    # Raise all errors at once
+    if failed:
+        raise UnknownVariantError(spec, failed)
+
 
 # The class below inherit from Sequence to disguise as a tuple and comply
 # with the semantic expected by the 'values' argument of the variant directive
diff --git a/var/spack/repos/builtin.mock/packages/multivalue_variant/package.py b/var/spack/repos/builtin.mock/packages/multivalue-variant/package.py
index 22d0ea1d97..22d0ea1d97 100644
--- a/var/spack/repos/builtin.mock/packages/multivalue_variant/package.py
+++ b/var/spack/repos/builtin.mock/packages/multivalue-variant/package.py
diff --git a/var/spack/repos/builtin.mock/packages/singlevalue-variant-dependent/package.py b/var/spack/repos/builtin.mock/packages/singlevalue-variant-dependent/package.py
index 5507fbdc21..de14faa51f 100644
--- a/var/spack/repos/builtin.mock/packages/singlevalue-variant-dependent/package.py
+++ b/var/spack/repos/builtin.mock/packages/singlevalue-variant-dependent/package.py
@@ -14,4 +14,4 @@ class SinglevalueVariantDependent(Package):
 
     version('1.0', '0123456789abcdef0123456789abcdef')
 
-    depends_on('multivalue_variant fee=baz')
+    depends_on('multivalue-variant fee=baz')