Update concretize to check for more changes and iterate further.

3 files changed, 65 insertions, 29 deletions

diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py
index 2e1d5d7f03..66002492cb 100644
--- a/lib/spack/spack/concretize.py
+++ b/lib/spack/spack/concretize.py
@@ -63,9 +63,9 @@ class DefaultConcretizer(object):
         """
         # return if already concrete.
         if spec.versions.concrete:
-            return
+            return False
 
-        # If there are known avaialble versions, return the most recent
+        # If there are known available versions, return the most recent
         # version that satisfies the spec
         pkg = spec.package
         valid_versions = sorted(
@@ -93,6 +93,8 @@ class DefaultConcretizer(object):
                 else:
                     spec.versions = ver([last])
 
+        return True   # Things changed
+
 
     def concretize_architecture(self, spec):
         """If the spec already had an architecture, return.  Otherwise if
@@ -109,22 +111,27 @@ class DefaultConcretizer(object):
            they're not explicit.
         """
         if spec.architecture is not None:
-            return
+            return False
 
         if spec.root.architecture:
             spec.architecture = spec.root.architecture
         else:
             spec.architecture = spack.architecture.sys_type()
 
+        assert(spec.architecture is not None)
+        return True   # changed
+
 
     def concretize_variants(self, spec):
         """If the spec already has variants filled in, return.  Otherwise, add
            the default variants from the package specification.
         """
+        changed = False
         for name, variant in spec.package.variants.items():
             if name not in spec.variants:
-                spec.variants[name] = spack.spec.VariantSpec(
-                    name, variant.default)
+                spec.variants[name] = spack.spec.VariantSpec(name, variant.default)
+                changed = True
+        return changed
 
 
     def concretize_compiler(self, spec):
@@ -144,7 +151,7 @@ class DefaultConcretizer(object):
         if (spec.compiler and
             spec.compiler.concrete and
             spec.compiler in all_compilers):
-            return
+            return False
 
         try:
             nearest = next(p for p in spec.traverse(direction='parents')
@@ -165,6 +172,8 @@ class DefaultConcretizer(object):
         except StopIteration:
             spec.compiler = spack.compilers.default_compiler().copy()
 
+        return True  # things changed.
+
 
     def choose_provider(self, spec, providers):
         """This is invoked for virtual specs.  Given a spec with a virtual name,
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 5876fc6cf8..e1fbb84423 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -736,26 +736,31 @@ class Spec(object):
         if visited is None: visited = set()
 
         if self.name in visited:
-            return
+            return False
+
+        changed = False
 
         # Concretize deps first -- this is a bottom-up process.
         for name in sorted(self.dependencies.keys()):
-            self.dependencies[name]._concretize_helper(presets, visited)
+            changed |= self.dependencies[name]._concretize_helper(presets, visited)
 
         if self.name in presets:
-            self.constrain(presets[self.name])
+            changed |= self.constrain(presets[self.name])
+
         else:
             # Concretize virtual dependencies last.  Because they're added
             # to presets below, their constraints will all be merged, but we'll
             # still need to select a concrete package later.
             if not self.virtual:
-                spack.concretizer.concretize_architecture(self)
-                spack.concretizer.concretize_compiler(self)
-                spack.concretizer.concretize_version(self)
-                spack.concretizer.concretize_variants(self)
+                changed |= any(
+                    (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)
+        return changed
 
 
     def _replace_with(self, concrete):
@@ -783,20 +788,22 @@ class Spec(object):
               this are infrequent, but should implement this before it is
               a problem.
         """
+        changed = False
         while True:
             virtuals =[v for v in self.traverse() if v.virtual]
             if not virtuals:
-                return
+                return changed
 
             for spec in virtuals:
                 providers = spack.db.providers_for(spec)
                 concrete = spack.concretizer.choose_provider(spec, providers)
                 concrete = concrete.copy()
                 spec._replace_with(concrete)
+                changed = True
 
             # If there are duplicate providers or duplicate provider deps, this
             # consolidates them and merge constraints.
-            self.normalize(force=True)
+            changed |= self.normalize(force=True)
 
 
     def concretize(self):
@@ -814,9 +821,16 @@ class Spec(object):
         if self._concrete:
             return
 
-        self.normalize()
-        self._expand_virtual_packages()
-        self._concretize_helper()
+        changed = True
+        force = False
+
+        while changed:
+            changes = (self.normalize(force=force),
+                       self._expand_virtual_packages(),
+                       self._concretize_helper())
+            changed = any(changes)
+            force=True
+
         self._concrete = True
 
 
@@ -982,6 +996,7 @@ class Spec(object):
         # it from the package description.
         if dep.name not in spec_deps:
             spec_deps[dep.name] = dep.copy()
+            changed = True
 
         # Constrain package information with spec info
         try:
@@ -998,7 +1013,6 @@ class Spec(object):
         dependency = spec_deps[dep.name]
         if dep.name not in self.dependencies:
             self._add_dependency(dependency)
-            changed = True
 
         changed |= dependency._normalize_helper(visited, spec_deps, provider_index)
         return changed
@@ -1031,13 +1045,12 @@ class Spec(object):
                 if pkg_dep:
                     changed |= self._merge_dependency(
                         pkg_dep, visited, spec_deps, provider_index)
-
             any_change |= changed
 
         return any_change
 
 
-    def normalize(self, **kwargs):
+    def normalize(self, force=False):
         """When specs are parsed, any dependencies specified are hanging off
            the root, and ONLY the ones that were explicitly provided are there.
            Normalization turns a partial flat spec into a DAG, where:
@@ -1050,13 +1063,13 @@ class Spec(object):
                 package that provides a virtual package that is in the spec,
                 then we replace the virtual package with the non-virtual one.
 
-           4. The spec DAG matches package DAG.
+           4. The spec DAG matches package DAG, including default variant values.
 
            TODO: normalize should probably implement some form of cycle detection,
            to ensure that the spec is actually a DAG.
         """
-        if self._normal and not kwargs.get('force', False):
-            return
+        if self._normal and not force:
+            return False
 
         # Ensure first that all packages & compilers in the DAG exist.
         self.validate_names()
@@ -1070,19 +1083,18 @@ class Spec(object):
         # traverse the package DAG and fill out dependencies according
         # to package files & their 'when' specs
         visited = set()
-        self._normalize_helper(visited, spec_deps, index)
+        any_change = self._normalize_helper(visited, spec_deps, index)
 
         # If there are deps specified but not visited, they're not
         # actually deps of this package.  Raise an error.
         extra = set(spec_deps.keys()).difference(visited)
-
-        # Anything left over is not a valid part of the spec.
         if extra:
             raise InvalidDependencyException(
                 self.name + " does not depend on " + comma_or(extra))
 
         # Mark the spec as normal once done.
         self._normal = True
+        return any_change
 
 
     def normalized(self):
diff --git a/lib/spack/spack/test/optional_deps.py b/lib/spack/spack/test/optional_deps.py
index 265a983f3f..fbee0cfa8f 100644
--- a/lib/spack/spack/test/optional_deps.py
+++ b/lib/spack/spack/test/optional_deps.py
@@ -86,9 +86,24 @@ class ConcretizeTest(MockPackagesTest):
                                        Spec('mpi'))))
 
 
+    def test_default_variant(self):
+        spec = Spec('optional-dep-test-3')
+        spec.concretize()
+        self.assertTrue('a' in spec)
+
+        spec = Spec('optional-dep-test-3~var')
+        spec.concretize()
+        self.assertTrue('a' in spec)
+
+        spec = Spec('optional-dep-test-3+var')
+        spec.concretize()
+        self.assertTrue('b' in spec)
+
+
     def test_transitive_chain(self):
         # Each of these dependencies comes from a conditional
         # dependency on another.  This requires iterating to evaluate
         # the whole chain.
-        self.check_normalize('optional-dep-test+f',
-                             Spec('optional-dep-test+f', Spec('f'), Spec('g'), Spec('mpi')))
+        self.check_normalize(
+            'optional-dep-test+f',
+            Spec('optional-dep-test+f', Spec('f'), Spec('g'), Spec('mpi')))