clean up concreteness detection

- Fixes bugs where concretization would fail due to an erroneously cached
  _concrete attribute.

- Ripped out a bunch of code in spec.py that isn't needed/valid anymore:
  - The various concrete() methods on different types of Specs would
    attempt to statically compute whether the Spec was concrete.
  - This dates back to when DAGs were simpler and there were no optional
    dependencies.  It's actually NOT possible to compute statically
    whether a Spec is concrete now.  The ONLY way you know is if it goes
    through concretization and is marked concrete once that completes.
  - This commit removes all simple concreteness checks and relies only on
    the _concrete attribute.  This should make thinking about
    concreteness simpler.

- Fixed a couple places where Specs need to be marked concrete explicitly.
  - Specs read from files and Specs that are destructively copied from
    concrete Specs now need to be marked concrete explicitly.
  - These spots may previously have "worked", but they were brittle and
    should be explcitly marked anyway.

3 files changed, 27 insertions, 99 deletions

diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
index 9693a75c39..f915e3a0d0 100644
--- a/lib/spack/spack/binary_distribution.py
+++ b/lib/spack/spack/binary_distribution.py
@@ -447,7 +447,12 @@ def get_specs():
                 except fs.FetchError:
                     continue
                 with open(stage.save_filename, 'r') as f:
+                    # read the spec from the build cache file. All specs
+                    # in build caches are concrete (as they aer built) so
+                    # we need to mark this spec concrete on read-in.
                     spec = spack.spec.Spec.from_yaml(f)
+                    spec._mark_concrete()
+
                     specs.add(spec)
                     durls[spec].append(link)
     return specs, durls
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index f2836e8eb8..1b5f41d4c4 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -635,10 +635,6 @@ class FlagMap(HashableMap):
     def valid_compiler_flags():
         return _valid_compiler_flags
 
-    @property
-    def concrete(self):
-        return all(flag in self for flag in _valid_compiler_flags)
-
     def copy(self):
         clone = FlagMap(None)
         for name, value in self.items():
@@ -661,73 +657,6 @@ 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):
-
-        # 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()))
 
@@ -1315,26 +1244,16 @@ class Spec(object):
 
     @property
     def concrete(self):
-        """A spec is concrete if it can describe only ONE build of a package.
-           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
+        """A spec is concrete if it describes a single build of a package.
 
-        # 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
-                              self.variants.concrete and
-                              self.architecture and
-                              self.architecture.concrete and
-                              self.compiler and self.compiler.concrete and
-                              self.compiler_flags.concrete and
-                              self._dependencies.concrete)
+        More formally, a spec is concrete if concretize() has been called
+        on it and it has been marked `_concrete`.
+
+        Concrete specs either can be or have been built. All constraints
+        have been resolved, optional dependencies have been added or
+        removed, a compiler has been chosen, and all variants have
+        values.
+        """
         return self._concrete
 
     def traverse(self, **kwargs):
@@ -1825,8 +1744,8 @@ class Spec(object):
                 if replacement.external:
                     if (spec._dependencies):
                         changed = True
-                        spec._dependencies = DependencyMap(spec)
-                    replacement._dependencies = DependencyMap(replacement)
+                        spec._dependencies = DependencyMap()
+                    replacement._dependencies = DependencyMap()
                     replacement.architecture = self.architecture
 
                 # TODO: could this and the stuff in _dup be cleaned up?
@@ -1986,7 +1905,7 @@ class Spec(object):
     def index(self, deptype=None):
         """Return DependencyMap that points to all the dependencies in this
            spec."""
-        dm = DependencyMap(None)
+        dm = DependencyMap()
         for spec in self.traverse(deptype=deptype):
             dm[spec.name] = spec
         return dm
@@ -2588,8 +2507,8 @@ class Spec(object):
             else None
         self.compiler = other.compiler.copy() if other.compiler else None
         if cleardeps:
-            self._dependents = DependencyMap(self)
-            self._dependencies = DependencyMap(self)
+            self._dependents = DependencyMap()
+            self._dependencies = DependencyMap()
         self.compiler_flags = other.compiler_flags.copy()
         self.compiler_flags.spec = self
         self.variants = other.variants.copy()
@@ -3296,8 +3215,8 @@ class SpecParser(spack.parse.Parser):
         spec.external_path = None
         spec.external_module = None
         spec.compiler_flags = FlagMap(spec)
-        spec._dependents = DependencyMap(spec)
-        spec._dependencies = DependencyMap(spec)
+        spec._dependents = DependencyMap()
+        spec._dependencies = DependencyMap()
         spec.namespace = spec_namespace
         spec._hash = None
         spec._cmp_key_cache = None
diff --git a/lib/spack/spack/test/directory_layout.py b/lib/spack/spack/test/directory_layout.py
index a2609c8162..0cfd796e06 100644
--- a/lib/spack/spack/test/directory_layout.py
+++ b/lib/spack/spack/test/directory_layout.py
@@ -55,8 +55,10 @@ def test_yaml_directory_layout_parameters(
     # Ensure default layout matches expected spec format
     layout_default = YamlDirectoryLayout(str(tmpdir))
     path_default = layout_default.relative_path_for_spec(spec)
-    assert(path_default ==
-           spec.format("${ARCHITECTURE}/${COMPILERNAME}-${COMPILERVER}/${PACKAGE}-${VERSION}-${HASH}"))   # NOQA: ignore=E501
+    assert(path_default == spec.format(
+        "${ARCHITECTURE}/"
+        "${COMPILERNAME}-${COMPILERVER}/"
+        "${PACKAGE}-${VERSION}-${HASH}"))
 
     # Test hash_length parameter works correctly
     layout_10 = YamlDirectoryLayout(str(tmpdir), hash_len=10)
@@ -133,6 +135,8 @@ def test_read_and_write_spec(
         # TODO: increase reuse of build dependencies.
         stored_deptypes = ('link', 'run')
         expected = spec.copy(deps=stored_deptypes)
+        expected._mark_concrete()
+
         assert expected.concrete
         assert expected == spec_from_file
         assert expected.eq_dag(spec_from_file)