summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorTodd Gamblin <gamblin2@llnl.gov>2022-09-15 17:57:10 -0700
committerGitHub <noreply@github.com>2022-09-16 00:57:10 +0000
commita0c7209dc1480b16531fcb992642986f4796bc28 (patch)
tree6a5c5366fa88367c16643de8d29b1051a075a4ec
parent46d7ba9f789b1129e6879e1e58d1a09fe8f4ad10 (diff)
downloadspack-a0c7209dc1480b16531fcb992642986f4796bc28.tar.gz
spack-a0c7209dc1480b16531fcb992642986f4796bc28.tar.bz2
spack-a0c7209dc1480b16531fcb992642986f4796bc28.tar.xz
spack-a0c7209dc1480b16531fcb992642986f4796bc28.zip
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.
-rw-r--r--lib/spack/spack/hash_types.py2
-rw-r--r--lib/spack/spack/solver/asp.py3
-rw-r--r--lib/spack/spack/spec.py13
-rw-r--r--lib/spack/spack/test/cmd/diff.py1
-rw-r--r--lib/spack/spack/test/spec_semantics.py22
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()