bugfix: package hash should affect process, dag, and dunder hashes (#32516)

This fixes a bug where two installations that differ only by package hash will not show
up in `spack find`.

The bug arose because `_cmp_node` on `Spec` didn't include the package hash in its
yielded fields. So, any two `Spec` objects that were only different by package hash
would appear to be equal and would overwrite each other when inserted into the same
`dict`. Note that we could still *install* specs with different package hashes, and they
would appear in the database, but we code that needed to put them into data structures
that use `__hash__` would have issues.

This PR makes `Spec.__hash__` and `Spec.__eq__` include the `process_hash()`, and it
makes `Spec._cmp_node` include the package hash. All of these *should* include all
information in a spec so that we don't end up in a situation where we are blind to
particular field differences.

Eventually, we should unify the `_cmp_*` methods with `to_node_dict` so there aren't two
sources of truth, but this needs some thought, since the `_cmp_*` methods exist for
speed. We should benchmark whether it's really worth having two types of hashing now
that we use `json` instead of `yaml` for spec hashing.

- [x] Add `package_hash` to `Spec._cmp_node`
- [x] Add `package_hash` to `spack.solve.asp.spec_clauses` so that the `package_hash`
      will show up in `spack diff`.
- [x] Add `package_hash` to the `process_hash` (which doesn't affect abstract specs
      but will make concrete specs correct)
- [x] Make `_cmp_iter` report the dag_hash so that no two specs with different
      process hashes will be considered equal.

5 files changed, 40 insertions, 1 deletions

diff --git a/lib/spack/spack/hash_types.py b/lib/spack/spack/hash_types.py
index d903515035..5058bfbd7c 100644
--- a/lib/spack/spack/hash_types.py
+++ b/lib/spack/spack/hash_types.py
@@ -44,7 +44,7 @@ dag_hash = SpecHashDescriptor(deptype=("build", "link", "run"), package_hash=Tru
 
 #: Hash descriptor used only to transfer a DAG, as is, across processes
 process_hash = SpecHashDescriptor(
-    deptype=("build", "link", "run", "test"), package_hash=False, name="process_hash"
+    deptype=("build", "link", "run", "test"), package_hash=True, name="process_hash"
 )
 
 
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 8e2e8bfd95..51782cf670 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1445,6 +1445,9 @@ class SpackSolverSetup(object):
 
         # dependencies
         if spec.concrete:
+            # older specs do not have package hashes, so we have to do this carefully
+            if getattr(spec, "_package_hash", None):
+                clauses.append(fn.package_hash(spec.name, spec._package_hash))
             clauses.append(fn.hash(spec.name, spec.dag_hash()))
 
         # add all clauses from dependencies
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 09769e4860..1194104a53 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -4056,6 +4056,9 @@ class Spec(object):
         yield self.compiler_flags
         yield self.architecture
 
+        # this is not present on older specs
+        yield getattr(self, "_package_hash", None)
+
     def eq_node(self, other):
         """Equality with another spec, not including dependencies."""
         return (other is not None) and lang.lazy_eq(self._cmp_node, other._cmp_node)
@@ -4065,6 +4068,16 @@ class Spec(object):
         for item in self._cmp_node():
             yield item
 
+        # This needs to be in _cmp_iter so that no specs with different process hashes
+        # are considered the same by `__hash__` or `__eq__`.
+        #
+        # TODO: We should eventually unify the `_cmp_*` methods with `to_node_dict` so
+        # TODO: there aren't two sources of truth, but this needs some thought, since
+        # TODO: they exist for speed.  We should benchmark whether it's really worth
+        # TODO: having two types of hashing now that we use `json` instead of `yaml` for
+        # TODO: spec hashing.
+        yield self.process_hash() if self.concrete else None
+
         def deps():
             for dep in sorted(itertools.chain.from_iterable(self._dependencies.values())):
                 yield dep.spec.name
diff --git a/lib/spack/spack/test/cmd/diff.py b/lib/spack/spack/test/cmd/diff.py
index 9189c45ede..dacbbb3c8c 100644
--- a/lib/spack/spack/test/cmd/diff.py
+++ b/lib/spack/spack/test/cmd/diff.py
@@ -86,6 +86,7 @@ def test_load_first(install_mockery, mock_fetch, mock_archive, mock_packages):
             "node_compiler",
             "node_compiler_version",
             "node",
+            "package_hash",
             "hash",
         )
     )
diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py
index b543eae9a8..7100cfdb5f 100644
--- a/lib/spack/spack/test/spec_semantics.py
+++ b/lib/spack/spack/test/spec_semantics.py
@@ -1257,3 +1257,25 @@ def test_concretize_partial_old_dag_hash_spec(mock_packages, config):
 def test_unsupported_compiler():
     with pytest.raises(UnsupportedCompilerError):
         Spec("gcc%fake-compiler").validate_or_raise()
+
+
+def test_package_hash_affects_dunder_and_dag_hash(mock_packages, config):
+    a1 = Spec("a").concretized()
+    a2 = Spec("a").concretized()
+
+    assert hash(a1) == hash(a2)
+    assert a1.dag_hash() == a2.dag_hash()
+    assert a1.process_hash() == a2.process_hash()
+
+    a1.clear_cached_hashes()
+    a2.clear_cached_hashes()
+
+    # tweak the dag hash of one of these specs
+    new_hash = "00000000000000000000000000000000"
+    if new_hash == a1._package_hash:
+        new_hash = "11111111111111111111111111111111"
+    a1._package_hash = new_hash
+
+    assert hash(a1) != hash(a2)
+    assert a1.dag_hash() != a2.dag_hash()
+    assert a1.process_hash() != a2.process_hash()