SV variants are evaluated correctly in "when=" (#4118)

* SV variants are evaluated correctly in `when=` statements fixes #4113

The problem here was tricky:
```python
spec.satisfies(other)
```
changes already the MV variants in others into SV variants (where
necessary) if spec is concrete. If it is not concrete it does
nothing because we may be acting at a pure syntactical level.

When evaluating a `when=` keyword spec is for sure not concrete
as it is in the middle of the concretization process. In this case we
have to trigger manually the substitution in other to not end up
comparing a MV variant "foo=bar" to a SV variant "foo=bar" and having
False in return. Which is wrong.

* sv variants: improved error message for typos in "when=" statements

lib/spack/spack/spec.py

@@ -1828,6 +1828,18 @@ def _evaluate_dependency_conditions(self, name):
         # evaluate when specs to figure out constraints on the dependency.
         dep = None
         for when_spec, dep_spec in conditions.items():
+            # If self was concrete it would have changed the variants in
+            # when_spec automatically. As here we are for sure during the
+            # concretization process, self is not concrete and we must change
+            # the variants in when_spec on our own to avoid using a
+            # MultiValuedVariant whe it is instead a SingleValuedVariant
+            try:
+                substitute_single_valued_variants(when_spec)
+            except SpecError as e:
+                msg = 'evaluating a `when=` statement gives ' + e.message
+                e.message = msg
+                raise
+
             sat = self.satisfies(when_spec, strict=True)
             if sat:
                 if dep is None:
@@ -2064,18 +2076,7 @@ def validate_or_raise(self):
                 if not_existing:
                     raise UnknownVariantError(spec.name, not_existing)
 
-                for name, v in [(x, y) for (x, y) in spec.variants.items()]:
+                substitute_single_valued_variants(spec)
-                    # When parsing a spec every variant of the form
-                    # 'foo=value' will be interpreted by default as a
-                    # multi-valued variant. During validation of the
-                    # variants we use the information in the package
-                    # to turn any variant that needs it to a single-valued
-                    # variant.
-                    pkg_variant = pkg_variants[name]
-                    pkg_variant.validate_or_raise(v, pkg_cls)
-                    spec.variants.substitute(
-                        pkg_variant.make_variant(v._original_value)
-                    )
 
     def constrain(self, other, deps=True):
         """Merge the constraints of other with self.
@@ -2290,25 +2291,19 @@ def satisfies(self, other, deps=True, strict=False, strict_deps=False):
         # to substitute every multi-valued variant that needs it with a
         # single-valued variant.
         if self.concrete:
-            for name, v in [(x, y) for (x, y) in other.variants.items()]:
+            try:
                 # When parsing a spec every variant of the form
                 # 'foo=value' will be interpreted by default as a
                 # multi-valued variant. During validation of the
                 # variants we use the information in the package
                 # to turn any variant that needs it to a single-valued
                 # variant.
-                pkg_cls = type(other.package)
+                substitute_single_valued_variants(other)
-                try:
+            except (SpecError, KeyError):
-                    pkg_variant = other.package.variants[name]
+                # Catch the two things that could go wrong above:
-                    pkg_variant.validate_or_raise(v, pkg_cls)
+                # 1. name is not a valid variant (KeyError)
-                except (SpecError, KeyError):
+                # 2. the variant is not validated (SpecError)
-                    # Catch the two things that could go wrong above:
+                return False
-                    # 1. name is not a valid variant (KeyError)
-                    # 2. the variant is not validated (SpecError)
-                    return False
-                other.variants.substitute(
-                    pkg_variant.make_variant(v._original_value)
-                )
 
         var_strict = strict
         if (not self.name) or (not other.name):

lib/spack/spack/test/spec_semantics.py

@@ -280,6 +280,18 @@ def test_satisfies_single_valued_variant(self):
 
         assert a.satisfies('foobar=bar')
 
+        # Assert that an autospec generated from a literal
+        # gives the right result for a single valued variant
+        assert 'foobar=bar' in a
+        assert 'foobar=baz' not in a
+        assert 'foobar=fee' not in a
+
+        # ... and for a multi valued variant
+        assert 'foo=bar' in a
+
+        # Check that conditional dependencies are treated correctly
+        assert '^b' in a
+
     def test_unsatisfiable_multi_value_variant(self):
 
         # Semantics for a multi-valued variant is different
@@ -337,22 +349,6 @@ def test_unsatisfiable_multi_value_variant(self):
         with pytest.raises(MultipleValuesInExclusiveVariantError):
             a.concretize()
 
-        # FIXME: remove after having checked the correctness of the semantics
-        # check_unsatisfiable('multivalue_variant foo="bar,baz"',
-        #                     'multivalue_variant foo="bar,baz,quux"',
-        #                     concrete=True)
-        # check_unsatisfiable('multivalue_variant foo="bar,baz"',
-        #                     'multivalue_variant foo="bar,baz,quux"',
-        #                     concrete=True)
-
-        # but succeed for abstract ones (b/c they COULD satisfy the
-        # constraint if constrained)
-        # check_satisfies('multivalue_variant foo="bar"',
-        #                 'multivalue_variant foo="bar,baz"')
-
-        # check_satisfies('multivalue_variant foo="bar,baz"',
-        #                 'multivalue_variant foo="bar,baz,quux"')
-
     def test_unsatisfiable_variant_types(self):
         # These should fail due to incompatible types
         check_unsatisfiable('multivalue_variant +foo',

lib/spack/spack/variant.py

@@ -546,6 +546,22 @@ def __str__(self):
         return string.getvalue()
 
 
+def substitute_single_valued_variants(spec):
+    """Uses the information in `spec.package` to turn any variant that needs
+    it into a SingleValuedVariant.
+
+    Args:
+        spec: spec on which to operate the substitution
+    """
+    for name, v in spec.variants.items():
+        pkg_cls = type(spec.package)
+        pkg_variant = spec.package.variants[name]
+        pkg_variant.validate_or_raise(v, pkg_cls)
+        spec.variants.substitute(
+            pkg_variant.make_variant(v._original_value)
+        )
+
+
 class DuplicateVariantError(error.SpecError):
     """Raised when the same variant occurs in a spec twice."""
 

var/spack/repos/builtin.mock/packages/a/package.py

@@ -49,6 +49,8 @@ class A(AutotoolsPackage):
         multi=False
     )
 
+    depends_on('b', when='foobar=bar')
+
     def with_or_without_fee(self, activated):
         if not activated:
             return '--no-fee'

var/spack/repos/builtin/packages/wget/package.py

@@ -38,7 +38,15 @@ class Wget(Package):
     version('1.17', 'c4c4727766f24ac716936275014a0536')
     version('1.16', '293a37977c41b5522f781d3a3a078426')
 
-    depends_on("openssl")
+    variant(
+        'ssl',
+        default='openssl',
+        values=('gnutls', 'openssl'),
+        description='Specify SSL backend'
+    )
+
+    depends_on('gnutls',  when='ssl=gnutls')
+    depends_on('openssl', when='ssl=openssl')
     depends_on("perl@5.12.0:", type='build')
 
     def install(self, spec, prefix):