bugfix: handle new `dag_hash()` on old concrete specs gracefully. (#30678)

Trying to compute `dag_hash()` or `package_hash()` on a concrete spec that doesn't have
a `_package_hash` attribute would attempt to recompute the package hash.

This most commonly manifests as a failed lookup of a namespace if you attempt to uninstall
or compute the hashes of packages in exsternal repositories that aren't registered, e.g.:

```console
> spack spec --json c/htno
==> Error: Unknown namespace: myrepo
```

While it wouldn't change the already-assigned `dag_hash` value, this behavior is
incorrect, since the package file for a previously concrete spec:
  1. might have changed since concretization,
  2. might not exist anymore, or
  3. might just not be findable by Spack.

This PR ensures that the package hash can't be computed on older concrete specs. Instead
of calling `package_hash()` from within `to_node_dict()`, we now check for the `_package_hash`
attribute and only add the package_hash to the spec record if it's there.

This PR also handles the tricky semantics of computing `package_hash()` at concretization
time. We have to compute it *before* marking the spec concrete so that `to_node_dict` can
use it. But this means that the logic for `package_hash()` can't rely on `spec.concrete`,
as it is called *during* concretization. Instead of checking for concreteness, `package_hash()`
now checks `_patches_assigned()` to determine whether it should add them to the package
hash.

- [x] Add an assert to `package_hash()` so it can't be called on specs for which it
      would be wrong.
- [x] Add an `_assign_hash()` method to handle tricky semantics of `package_hash`
      and `dag_hash`.
- [x] Rework concretization to call `_assign_hash()` before and after marking specs
      concrete.
- [x] Rework content hash part of package hash to check for `_patches_assigned()`
      instead of `spec.concrete`.
- [x] regression test

4 files changed, 124 insertions, 21 deletions

diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index ce8f2add09..1ca619b974 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -1714,7 +1714,10 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)):
                 hash_content.append(source_id.encode('utf-8'))
 
         # patch sha256's
-        if self.spec.concrete:
+        # Only include these if they've been assigned by the concretizer.
+        # We check spec._patches_assigned instead of spec.concrete because
+        # we have to call package_hash *before* marking specs concrete
+        if self.spec._patches_assigned():
             hash_content.extend(
                 ':'.join((p.sha256, str(p.level))).encode('utf-8')
                 for p in self.spec.patches
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 21578b2dc2..b1efb84368 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -2107,8 +2107,9 @@ class SpecBuilder(object):
         for s in self._specs.values():
             _develop_specs_from_env(s, ev.active_environment())
 
-        for s in self._specs.values():
-            s._mark_concrete()
+        # mark concrete and assign hashes to all specs in the solve
+        for root in roots.values():
+            root._finalize_concretization()
 
         for s in self._specs.values():
             spack.spec.Spec.ensure_no_deprecated(s)
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 10afd16004..db6eafaf37 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -1776,7 +1776,7 @@ class Spec(object):
         json_text = sjson.dump(node_dict)
         return spack.util.hash.b32_hash(json_text)
 
-    def _cached_hash(self, hash, length=None):
+    def _cached_hash(self, hash, length=None, force=False):
         """Helper function for storing a cached hash on the spec.
 
         This will run spec_hash() with the deptype and package_hash
@@ -1785,6 +1785,8 @@ class Spec(object):
 
         Arguments:
             hash (spack.hash_types.SpecHashDescriptor): type of hash to generate.
+            length (int): length of hash prefix to return (default is full hash string)
+            force (bool): cache the hash even if spec is not concrete (default False)
         """
         if not hash.attr:
             return self.spec_hash(hash)[:length]
@@ -1794,13 +1796,20 @@ class Spec(object):
             return hash_string[:length]
         else:
             hash_string = self.spec_hash(hash)
-            if self.concrete:
+            if force or self.concrete:
                 setattr(self, hash.attr, hash_string)
 
             return hash_string[:length]
 
     def package_hash(self):
         """Compute the hash of the contents of the package for this node"""
+        # Concrete specs with the old DAG hash did not have the package hash, so we do
+        # not know what the package looked like at concretization time
+        if self.concrete and not self._package_hash:
+            raise ValueError(
+                "Cannot call package_hash() on concrete specs with the old dag_hash()"
+            )
+
         return self._cached_hash(ht.package_hash)
 
     def dag_hash(self, length=None):
@@ -1923,8 +1932,13 @@ class Spec(object):
             if hasattr(variant, '_patches_in_order_of_appearance'):
                 d['patches'] = variant._patches_in_order_of_appearance
 
-        if self._concrete and hash.package_hash:
-            package_hash = self.package_hash()
+        if self._concrete and hash.package_hash and self._package_hash:
+            # We use the attribute here instead of `self.package_hash()` because this
+            # should *always* be assignhed at concretization time. We don't want to try
+            # to compute a package hash for concrete spec where a) the package might not
+            # exist, or b) the `dag_hash` didn't include the package hash when the spec
+            # was concretized.
+            package_hash = self._package_hash
 
             # Full hashes are in bytes
             if (not isinstance(package_hash, six.text_type)
@@ -2694,8 +2708,8 @@ class Spec(object):
             # TODO: or turn external_path into a lazy property
             Spec.ensure_external_path_if_external(s)
 
-        # Mark everything in the spec as concrete, as well.
-        self._mark_concrete()
+        # assign hashes and mark concrete
+        self._finalize_concretization()
 
         # If any spec in the DAG is deprecated, throw an error
         Spec.ensure_no_deprecated(self)
@@ -2725,6 +2739,21 @@ class Spec(object):
         # there are declared inconsistencies)
         self.architecture.target.optimization_flags(self.compiler)
 
+    def _patches_assigned(self):
+        """Whether patches have been assigned to this spec by the concretizer."""
+        # FIXME: _patches_in_order_of_appearance is attached after concretization
+        # FIXME: to store the order of patches.
+        # FIXME: Probably needs to be refactored in a cleaner way.
+        if "patches" not in self.variants:
+            return False
+
+        # ensure that patch state is consistent
+        patch_variant = self.variants["patches"]
+        assert hasattr(patch_variant, "_patches_in_order_of_appearance"), \
+            "patches should always be assigned with a patch variant."
+
+        return True
+
     @staticmethod
     def inject_patches_variant(root):
         # This dictionary will store object IDs rather than Specs as keys
@@ -2859,7 +2888,6 @@ class Spec(object):
 
         concretized = answer[name]
         self._dup(concretized)
-        self._mark_concrete()
 
     def concretize(self, tests=False):
         """Concretize the current spec.
@@ -2896,6 +2924,64 @@ class Spec(object):
                 s.clear_cached_hashes()
             s._mark_root_concrete(value)
 
+    def _assign_hash(self, hash):
+        """Compute and cache the provided hash type for this spec and its dependencies.
+
+        Arguments:
+            hash (spack.hash_types.SpecHashDescriptor): the hash to assign to nodes
+                in the spec.
+
+        There are special semantics to consider for `package_hash`.
+
+        This should be called:
+          1. for `package_hash`, immediately after concretization, but *before* marking
+             concrete, and
+          2. for `dag_hash`, immediately after marking concrete.
+
+        `package_hash` is tricky, because we can't call it on *already* concrete specs,
+        but we need to assign it *at concretization time* to just-concretized specs. So,
+        the concretizer must assign the package hash *before* marking their specs
+        concrete (so that the only concrete specs are the ones already marked concrete).
+
+        `dag_hash` is also tricky, since it cannot compute `package_hash()` lazily for
+        the same reason. `package_hash` needs to be assigned *at concretization time*,
+        so, `to_node_dict()` can't just assume that it can compute `package_hash` itself
+        -- it needs to either see or not see a `_package_hash` attribute.
+
+        Rules of thumb for `package_hash`:
+          1. Old-style concrete specs from *before* `dag_hash` included `package_hash`
+             will not have a `_package_hash` attribute at all.
+          2. New-style concrete specs will have a `_package_hash` assigned at
+             concretization time.
+          3. Abstract specs will not have a `_package_hash` attribute at all.
+
+        """
+        for spec in self.traverse():
+            # Already concrete specs either already have a package hash (new dag_hash())
+            # or they never will b/c we can't know it (old dag_hash()). Skip them.
+            if hash is ht.package_hash and not spec.concrete:
+                spec._cached_hash(hash, force=True)
+
+                # keep this check here to ensure package hash is saved
+                assert getattr(spec, hash.attr)
+            else:
+                spec._cached_hash(hash)
+
+    def _finalize_concretization(self):
+        """Assign hashes to this spec, and mark it concrete.
+
+        This is called at the end of concretization.
+        """
+        # See docs for in _assign_hash for why package_hash needs to happen here.
+        self._assign_hash(ht.package_hash)
+
+        # Mark everything in the spec as concrete
+        self._mark_concrete()
+
+        # Assign dag_hash (this *could* be done lazily, but it's assigned anyway in
+        # ensure_no_deprecated, and it's clearer to see explicitly where it happens)
+        self._assign_hash(ht.dag_hash)
+
     def concretized(self, tests=False):
         """This is a non-destructive version of concretize().
 
@@ -3656,19 +3742,12 @@ class Spec(object):
         TODO: this only checks in the package; it doesn't resurrect old
         patches from install directories, but it probably should.
         """
-        if not self.concrete:
-            raise spack.error.SpecError("Spec is not concrete: " + str(self))
-
         if not hasattr(self, "_patches"):
             self._patches = []
-            if 'patches' in self.variants:
-                # FIXME: _patches_in_order_of_appearance is attached after
-                # FIXME: concretization to store the order of patches somewhere.
-                # FIXME: Needs to be refactored in a cleaner way.
-
-                # translate patch sha256sums to patch objects by consulting the index
-                self._patches = []
-                for sha256 in self.variants['patches']._patches_in_order_of_appearance:
+
+            # translate patch sha256sums to patch objects by consulting the index
+            if self._patches_assigned():
+                for sha256 in self.variants["patches"]._patches_in_order_of_appearance:
                     index = spack.repo.path.patch_index
                     patch = index.patch_for_package(sha256, self.package)
                     self._patches.append(patch)
diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py
index 58db63d0d1..4d3d5037fc 100644
--- a/lib/spack/spack/test/spec_semantics.py
+++ b/lib/spack/spack/test/spec_semantics.py
@@ -1273,3 +1273,23 @@ def test_spec_installed(install_mockery, database):
     # 'a' is not in the mock DB and is not installed
     spec = Spec("a").concretized()
     assert not spec.installed
+
+
+@pytest.mark.regression('30678')
+def test_call_dag_hash_on_old_dag_hash_spec(mock_packages, config):
+    # create a concrete spec
+    a = Spec("a").concretized()
+    dag_hashes = {
+        spec.name: spec.dag_hash() for spec in a.traverse()
+    }
+
+    # make it look like an old DAG hash spec with no package hash on the spec.
+    for spec in a.traverse():
+        assert spec.concrete
+        spec._package_hash = None
+
+    for spec in a.traverse():
+        assert dag_hashes[spec.name] == spec.dag_hash()
+
+        with pytest.raises(ValueError, match='Cannot call package_hash()'):
+            spec.package_hash()