From a0c7209dc1480b16531fcb992642986f4796bc28 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Thu, 15 Sep 2022 17:57:10 -0700 Subject: 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. --- lib/spack/spack/hash_types.py | 2 +- lib/spack/spack/solver/asp.py | 3 +++ lib/spack/spack/spec.py | 13 +++++++++++++ lib/spack/spack/test/cmd/diff.py | 1 + lib/spack/spack/test/spec_semantics.py | 22 ++++++++++++++++++++++ 5 files changed, 40 insertions(+), 1 deletion(-) (limited to 'lib') 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() -- cgit v1.2.3-60-g2f50