issue 4492: DependencyMap.concrete checks for unconditional dependencies (#4499)

* issue 4492: added xfailing test, added owner to DependencyMap

* DependencyMap.concrete checks if we have unconditional dependencies

This fixes #4492 and #4107 using some heuristics to avoid an infinite
recursion among Spec.satisfies, Spec.concrete and DependencyMap.concrete

The idea, as suggested by @becker33, is to check just for unconditional
dependencies. This is not covering the whole space and a package with
just conditional dependencies can still fail in the same way. It should
cover though all the **real** packages we have in our repo so far.

lib/spack/spack/spec.py

@@ -678,10 +678,72 @@ class DependencyMap(HashableMap):
     """Each spec has a DependencyMap containing specs for its dependencies.
        The DependencyMap is keyed by name. """
 
+    def __init__(self, owner):
+        super(DependencyMap, self).__init__()
+
+        # Owner Spec for the current map
+        self.owner = owner
+
     @property
     def concrete(self):
-        return all((d.spec.concrete and d.deptypes)
+
-                   for d in self.values())
+        # Check if the dependencies are concrete and of the correct type
+        dependencies_are_concrete = all(
+            (d.spec.concrete and d.deptypes) for d in self.values()
+        )
+
+        if not dependencies_are_concrete:
+            return False
+
+        # If we are dealing with an external spec, it clearly has no
+        # dependencies managed by spack
+        if self.owner.external:
+            return True
+
+        # Now check we have every dependency we need
+        pkg = self.owner.package
+        for dependency in pkg.dependencies.values():
+            # Check that for every condition we satisfy we satisfy also
+            # the associated constraint
+            for condition, constraint in dependency.items():
+                # WARNING: This part relies on the fact that dependencies are
+                # the last item to be checked when computing if a spec is
+                # concrete. This means that we are ensured that all variants,
+                # versions, compilers, etc. are there with their final value
+                # when we call 'Spec.satisfies(..., strict=True)'
+
+                # FIXME: at the moment check only for non conditional
+                # FIXME: dependencies, to avoid infinite recursion
+
+                # satisfy_condition = self.owner.satisfies(
+                #     condition, strict=True
+                # )
+
+                satisfy_condition = str(condition) == self.owner.name
+
+                if not satisfy_condition:
+                    continue
+
+                dependency_name = constraint.name
+
+                # We don't want to check build dependencies
+                if pkg.dependency_types[dependency_name] == set(['build']):
+                    continue
+
+                try:
+                    dependency = self.owner[dependency_name]
+                    satisfy_constraint = dependency.satisfies(
+                        constraint, strict=True
+                    )
+                except KeyError:
+                    # If the dependency is not there we don't
+                    # satisfy the constraint
+                    satisfy_constraint = False
+
+                if satisfy_condition and not satisfy_constraint:
+                    return False
+
+        return True
 
     def __str__(self):
         return "{deps: %s}" % ', '.join(str(d) for d in sorted(self.values()))
@@ -1144,9 +1206,13 @@ def concrete(self):
            If any of the name, version, architecture, compiler,
            variants, or depdenencies are ambiguous,then it is not concrete.
         """
+        # If we have cached that the spec is concrete, then it's concrete
         if self._concrete:
             return True
 
+        # Otherwise check if the various parts of the spec are concrete.
+        # It's crucial to have the check on dependencies last, as it relies
+        # on all the other parts to be already checked for concreteness.
         self._concrete = bool(not self.virtual and
                               self.namespace is not None and
                               self.versions.concrete and
@@ -1647,8 +1713,8 @@ def _expand_virtual_packages(self):
                 if replacement.external:
                     if (spec._dependencies):
                         changed = True
-                        spec._dependencies = DependencyMap()
+                        spec._dependencies = DependencyMap(spec)
-                    replacement._dependencies = DependencyMap()
+                    replacement._dependencies = DependencyMap(replacement)
                     replacement.architecture = self.architecture
 
                 # TODO: could this and the stuff in _dup be cleaned up?
@@ -1676,6 +1742,7 @@ def feq(cfield, sfield):
                 if spec._dup(replacement, deps=False, cleardeps=False):
                     changed = True
 
+                spec._dependencies.owner = spec
                 self_index.update(spec)
                 done = False
                 break
@@ -1807,7 +1874,7 @@ def flat_dependencies(self, **kwargs):
     def index(self, deptype=None):
         """Return DependencyMap that points to all the dependencies in this
            spec."""
-        dm = DependencyMap()
+        dm = DependencyMap(None)
         for spec in self.traverse(deptype=deptype):
             dm[spec.name] = spec
         return dm
@@ -2416,8 +2483,8 @@ def _dup(self, other, deps=True, cleardeps=True):
             else None
         self.compiler = other.compiler.copy() if other.compiler else None
         if cleardeps:
-            self._dependents = DependencyMap()
+            self._dependents = DependencyMap(self)
-            self._dependencies = DependencyMap()
+            self._dependencies = DependencyMap(self)
         self.compiler_flags = other.compiler_flags.copy()
         self.variants = other.variants.copy()
         self.variants.spec = self
@@ -3078,8 +3145,8 @@ def spec(self, name):
         spec.external_path = None
         spec.external_module = None
         spec.compiler_flags = FlagMap(spec)
-        spec._dependents = DependencyMap()
+        spec._dependents = DependencyMap(spec)
-        spec._dependencies = DependencyMap()
+        spec._dependencies = DependencyMap(spec)
         spec.namespace = spec_namespace
         spec._hash = None
         spec._cmp_key_cache = None

lib/spack/spack/test/concretize.py

@@ -26,7 +26,8 @@
 import spack
 import spack.architecture
 from spack.concretize import find_spec
-from spack.spec import Spec, CompilerSpec, ConflictsInSpecError, SpecError
+from spack.spec import Spec, CompilerSpec
+from spack.spec import ConflictsInSpecError, SpecError
 from spack.version import ver
 
 
@@ -397,3 +398,21 @@ def test_conflicts_in_spec(self, conflict_spec):
             s = Spec(conflict_spec)
             with pytest.raises(exc_type):
                 s.concretize()
+
+    def test_regression_issue_4492(self):
+        # Constructing a spec which has no dependencies, but is otherwise
+        # concrete is kind of difficult. What we will do is to concretize
+        # a spec, and then modify it to have no dependency and reset the
+        # cache values.
+
+        s = Spec('mpileaks')
+        s.concretize()
+
+        # Check that now the Spec is concrete, store the hash
+        assert s.concrete
+
+        # Remove the dependencies and reset caches
+        s._dependencies.clear()
+        s._concrete = False
+
+        assert not s.concrete