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

5 files changed, 59 insertions, 42 deletions

diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 0cf392a7ce..399f58461f 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -1828,6 +1828,18 @@ class Spec(object):
         # 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 @@ class Spec(object):
                 if not_existing:
                     raise UnknownVariantError(spec.name, not_existing)
 
-                for name, v in [(x, y) for (x, y) in spec.variants.items()]:
-                    # 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)
-                    )
+                substitute_single_valued_variants(spec)
 
     def constrain(self, other, deps=True):
         """Merge the constraints of other with self.
@@ -2290,25 +2291,19 @@ class Spec(object):
         # 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)
-                try:
-                    pkg_variant = other.package.variants[name]
-                    pkg_variant.validate_or_raise(v, pkg_cls)
-                except (SpecError, KeyError):
-                    # Catch the two things that could go wrong above:
-                    # 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)
-                )
+                substitute_single_valued_variants(other)
+            except (SpecError, KeyError):
+                # Catch the two things that could go wrong above:
+                # 1. name is not a valid variant (KeyError)
+                # 2. the variant is not validated (SpecError)
+                return False
 
         var_strict = strict
         if (not self.name) or (not other.name):
diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py
index 306a6ad98f..80a2f63bde 100644
--- a/lib/spack/spack/test/spec_semantics.py
+++ b/lib/spack/spack/test/spec_semantics.py
@@ -280,6 +280,18 @@ class TestSpecSematics(object):
 
         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 @@ class TestSpecSematics(object):
         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',
diff --git a/lib/spack/spack/variant.py b/lib/spack/spack/variant.py
index 2d02ab1253..fe08e4529e 100644
--- a/lib/spack/spack/variant.py
+++ b/lib/spack/spack/variant.py
@@ -546,6 +546,22 @@ class VariantMap(lang.HashableMap):
         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."""
 
diff --git a/var/spack/repos/builtin.mock/packages/a/package.py b/var/spack/repos/builtin.mock/packages/a/package.py
index b697f4d2a9..e39ad13bd1 100644
--- a/var/spack/repos/builtin.mock/packages/a/package.py
+++ b/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'
diff --git a/var/spack/repos/builtin/packages/wget/package.py b/var/spack/repos/builtin/packages/wget/package.py
index 10b8c33d31..ff704ca37c 100644
--- a/var/spack/repos/builtin/packages/wget/package.py
+++ b/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):