SPACK-56: fix Variant concretization.

- Variant concretization is tricky:

  - During concretization, a spec without variants (e.g., mpich) means
    "don't care".  So, Spec('mpich').satisfies('mpich+debug') is true
    because it *could* still be built that way.

  - After concretization, a spec without a particular variant means
    "don't know", as that wasn't part of the spec, so the opposite
    relationship is true.  Assume 'spec' is already installed:

      spec.satisfies('mpich+debug')

    this is false beacuse the `debug` variant didn't exist when spec
    was built, so we can't satisfy the explicit request for +debug.

5 files changed, 107 insertions, 23 deletions

diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py
index 3f569f9dce..15e886ad3c 100644
--- a/lib/spack/spack/concretize.py
+++ b/lib/spack/spack/concretize.py
@@ -101,6 +101,16 @@ class DefaultConcretizer(object):
             spec.architecture = spack.architecture.sys_type()
 
 
+    def concretize_variants(self, spec):
+        """If the spec already has variants filled in, return.  Otherwise, add
+           the default variants from the package specification.
+        """
+        for name, variant in spec.package.variants.items():
+            if name not in spec.variants:
+                spec.variants[name] = spack.spec.VariantSpec(
+                    name, variant.default)
+
+
     def concretize_compiler(self, spec):
         """If the spec already has a compiler, we're done.  If not, then take
            the compiler used for the nearest ancestor with a compiler
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index fca14f97db..4639aea452 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -270,7 +270,7 @@ class CompilerSpec(object):
 
 
 @key_ordering
-class Variant(object):
+class VariantSpec(object):
     """Variants are named, build-time options for a package.  Names depend
        on the particular package being built, and each named variant can
        be enabled or disabled.
@@ -285,7 +285,7 @@ class Variant(object):
 
 
     def copy(self):
-        return Variant(self.name, self.enabled)
+        return VariantSpec(self.name, self.enabled)
 
 
     def __str__(self):
@@ -294,9 +294,27 @@ class Variant(object):
 
 
 class VariantMap(HashableMap):
-    def satisfies(self, other):
-        return all(self[key].enabled == other[key].enabled
-                   for key in other if key in self)
+    def satisfies(self, other, self_is_concrete):
+        if self_is_concrete:
+            return all(k in self and self[k].enabled == other[k].enabled
+                       for k in other)
+        else:
+            return all(self[k].enabled == other[k].enabled
+                       for k in other if k in self)
+
+
+    def constrain(self, other, other_is_concrete):
+        if other_is_concrete:
+            for k in self:
+                if k not in other:
+                    raise UnsatisfiableVariantSpecError(self[k], '<absent>')
+
+        for k in other:
+            if k in self:
+                if self[k].enabled != other[k].enabled:
+                    raise UnsatisfiableVariantSpecError(self[k], other[k])
+            else:
+                self[k] = other[k].copy()
 
 
     def __str__(self):
@@ -375,7 +393,7 @@ class Spec(object):
         """Called by the parser to add a variant."""
         if name in self.variants: raise DuplicateVariantError(
                 "Cannot specify variant '%s' twice" % name)
-        self.variants[name] = Variant(name, enabled)
+        self.variants[name] = VariantSpec(name, enabled)
 
 
     def _set_compiler(self, compiler):
@@ -607,6 +625,7 @@ class Spec(object):
                 spack.concretizer.concretize_architecture(self)
                 spack.concretizer.concretize_compiler(self)
                 spack.concretizer.concretize_version(self)
+                spack.concretizer.concretize_variants(self)
             presets[self.name] = self
 
         visited.add(self.name)
@@ -789,8 +808,7 @@ class Spec(object):
                     else:
                         required = index.providers_for(vspec.name)
                         if required:
-                            raise UnsatisfiableProviderSpecError(
-                                required[0], pkg_dep)
+                            raise UnsatisfiableProviderSpecError(required[0], pkg_dep)
                 provider_index.update(pkg_dep)
 
             if name not in spec_deps:
@@ -929,7 +947,7 @@ class Spec(object):
             self.compiler = other.compiler
 
         self.versions.intersect(other.versions)
-        self.variants.update(other.variants)
+        self.variants.constrain(other.variants, other._concrete)
         self.architecture = self.architecture or other.architecture
 
         if constrain_deps:
@@ -998,11 +1016,13 @@ class Spec(object):
         # All these attrs have satisfies criteria of their own,
         # but can be None to indicate no constraints.
         for s, o in ((self.versions, other.versions),
-                     (self.variants, other.variants),
                      (self.compiler, other.compiler)):
             if s and o and not s.satisfies(o):
                 return False
 
+        if not self.variants.satisfies(other.variants, self._concrete):
+            return False
+
         # Architecture satisfaction is currently just string equality.
         # Can be None for unconstrained, though.
         if (self.architecture and other.architecture and
diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py
index fb67aa8a8d..ecbc46981c 100644
--- a/lib/spack/spack/test/spec_dag.py
+++ b/lib/spack/spack/test/spec_dag.py
@@ -242,12 +242,6 @@ class SpecDagTest(MockPackagesTest):
         self.assertRaises(spack.spec.UnsatisfiableCompilerSpecError, spec.normalize)
 
 
-    def test_unsatisfiable_variant(self):
-        set_pkg_dep('mpileaks', 'mpich+debug')
-        spec = Spec('mpileaks ^mpich~debug ^callpath ^dyninst ^libelf ^libdwarf')
-        self.assertRaises(spack.spec.UnsatisfiableVariantSpecError, spec.normalize)
-
-
     def test_unsatisfiable_architecture(self):
         set_pkg_dep('mpileaks', 'mpich=bgqos_0')
         spec = Spec('mpileaks ^mpich=sles_10_ppc64 ^callpath ^dyninst ^libelf ^libdwarf')
diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py
index 1db7956f04..8614b74c7a 100644
--- a/lib/spack/spack/test/spec_semantics.py
+++ b/lib/spack/spack/test/spec_semantics.py
@@ -33,8 +33,8 @@ class SpecSematicsTest(MockPackagesTest):
     # ================================================================================
     # Utility functions to set everything up.
     # ================================================================================
-    def check_satisfies(self, spec, anon_spec):
-        left = Spec(spec)
+    def check_satisfies(self, spec, anon_spec, concrete=False):
+        left = Spec(spec, concrete=concrete)
         right = parse_anonymous_spec(anon_spec, left.name)
 
         # Satisfies is one-directional.
@@ -46,8 +46,8 @@ class SpecSematicsTest(MockPackagesTest):
         right.copy().constrain(left)
 
 
-    def check_unsatisfiable(self, spec, anon_spec):
-        left = Spec(spec)
+    def check_unsatisfiable(self, spec, anon_spec, concrete=False):
+        left = Spec(spec, concrete=concrete)
         right = parse_anonymous_spec(anon_spec, left.name)
 
         self.assertFalse(left.satisfies(right))
@@ -150,9 +150,33 @@ class SpecSematicsTest(MockPackagesTest):
         self.check_unsatisfiable('mpileaks^mpi@3:', '^mpich@1.0')
 
 
-    def test_satisfies_variant(self):
-        self.check_satisfies('foo %gcc@4.7.3', '%gcc@4.7')
-        self.check_unsatisfiable('foo %gcc@4.7', '%gcc@4.7.3')
+    def test_satisfies_matching_variant(self):
+        self.check_satisfies('mpich+foo', 'mpich+foo')
+        self.check_satisfies('mpich~foo', 'mpich~foo')
+
+
+    def test_satisfies_unconstrained_variant(self):
+        # only asked for mpich, no constraints.  Either will do.
+        self.check_satisfies('mpich+foo', 'mpich')
+        self.check_satisfies('mpich~foo', 'mpich')
+
+
+    def test_unsatisfiable_variants(self):
+        # This case is different depending on whether the specs are concrete.
+
+        # 'mpich' is not concrete:
+        self.check_satisfies('mpich', 'mpich+foo', False)
+        self.check_satisfies('mpich', 'mpich~foo', False)
+
+        # 'mpich' is concrete:
+        self.check_unsatisfiable('mpich', 'mpich+foo', True)
+        self.check_unsatisfiable('mpich', 'mpich~foo', True)
+
+
+    def test_unsatisfiable_variant_mismatch(self):
+        # No matchi in specs
+        self.check_unsatisfiable('mpich~foo', 'mpich+foo')
+        self.check_unsatisfiable('mpich+foo', 'mpich~foo')
 
 
 
diff --git a/lib/spack/spack/variant.py b/lib/spack/spack/variant.py
new file mode 100644
index 0000000000..3d3e2b0f6d
--- /dev/null
+++ b/lib/spack/spack/variant.py
@@ -0,0 +1,36 @@
+##############################################################################
+# Copyright (c) 2013, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Written by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://scalability-llnl.github.io/spack
+# Please also see the LICENSE file for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License (as published by
+# the Free Software Foundation) version 2.1 dated February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with this program; if not, write to the Free Software Foundation,
+# Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+"""Variant is a class describing flags on builds, or "variants".
+
+Could be generalized later to describe aribitrary parameters, but
+currently variants are just flags.
+
+"""
+
+class Variant(object):
+    """Represents a variant on a build.  Can be either on or off."""
+    def __init__(self, default, description):
+        self.default     = bool(default)
+        self.description = str(description)