From 512645ff2efbf522aefc55e9384a9f5025c98d91 Mon Sep 17 00:00:00 2001 From: Scott Wittenburg Date: Mon, 24 Jan 2022 21:42:45 -0700 Subject: environment: key by dag_hash instead of build_hash --- lib/spack/spack/ci.py | 2 +- lib/spack/spack/environment/environment.py | 80 +++++++++++++++++------------- lib/spack/spack/spec.py | 15 +++++- lib/spack/spack/test/ci.py | 6 +-- lib/spack/spack/test/cmd/env.py | 10 ++-- 5 files changed, 68 insertions(+), 45 deletions(-) diff --git a/lib/spack/spack/ci.py b/lib/spack/spack/ci.py index 2ad8fd5924..9419869232 100644 --- a/lib/spack/spack/ci.py +++ b/lib/spack/spack/ci.py @@ -202,7 +202,7 @@ def format_root_spec(spec, main_phase, strip_compiler): return '{0}@{1} arch={2}'.format( spec.name, spec.version, spec.architecture) else: - return spec.build_hash() + return spec.dag_hash() def spec_deps_key(s): diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index 5ef53f5b72..7359bdd591 100644 --- a/lib/spack/spack/environment/environment.py +++ b/lib/spack/spack/environment/environment.py @@ -94,7 +94,7 @@ spack: valid_environment_name_re = r'^\w[\w-]*$' #: version of the lockfile format. Must increase monotonically. -lockfile_format_version = 3 +lockfile_format_version = 4 # Magic names # The name of the standalone spec list in the manifest yaml @@ -439,7 +439,7 @@ class ViewDescriptor(object): def content_hash(self, specs): d = syaml.syaml_dict([ ('descriptor', self.to_dict()), - ('specs', [(spec.full_hash(), spec.prefix) for spec in sorted(specs)]) + ('specs', [(spec.dag_hash(), spec.prefix) for spec in sorted(specs)]) ]) contents = sjson.dump(d) return spack.util.hash.b32_hash(contents) @@ -1010,14 +1010,9 @@ class Environment(object): if not matches: # concrete specs match against concrete specs in the env - # by *dag hash*, not build hash. - dag_hashes_in_order = [ - self.specs_by_hash[build_hash].dag_hash() - for build_hash in self.concretized_order - ] - + # by dag hash. specs_hashes = zip( - self.concretized_user_specs, dag_hashes_in_order + self.concretized_user_specs, self.concretized_order ) matches = [ @@ -1331,7 +1326,7 @@ class Environment(object): spec = next( s for s in self.user_specs if s.satisfies(user_spec) ) - concrete = self.specs_by_hash.get(spec.build_hash()) + concrete = self.specs_by_hash.get(spec.dag_hash()) if not concrete: concrete = spec.concretized(tests=tests) self._add_concrete_spec(spec, concrete) @@ -1499,7 +1494,7 @@ class Environment(object): # update internal lists of specs self.concretized_user_specs.append(spec) - h = concrete.build_hash() + h = concrete.dag_hash() self.concretized_order.append(h) self.specs_by_hash[h] = concrete @@ -1621,9 +1616,7 @@ class Environment(object): return sorted(all_specs) def all_hashes(self): - """Return hashes of all specs. - - Note these hashes exclude build dependencies.""" + """Return hashes of all specs.""" return list(set(s.dag_hash() for s in self.all_specs())) def roots(self): @@ -1695,11 +1688,10 @@ class Environment(object): for user_spec, concretized_user_spec in self.concretized_specs(): # Deal with concrete specs differently if spec.concrete: - # Matching a concrete spec is more restrictive - # than just matching the dag hash + # TODO: do we still need the extra check comparing dag hashes? is_match = ( spec in concretized_user_spec and - concretized_user_spec[spec.name].build_hash() == spec.build_hash() + concretized_user_spec[spec.name].dag_hash() == spec.dag_hash() ) if is_match: matches[spec] = spec @@ -1781,12 +1773,12 @@ class Environment(object): concrete_specs = {} for spec in self.specs_by_hash.values(): for s in spec.traverse(): - build_hash = s.build_hash() - if build_hash not in concrete_specs: - spec_dict = s.to_node_dict(hash=ht.build_hash) + dag_hash = s.dag_hash() + if dag_hash not in concrete_specs: + spec_dict = s.node_dict_with_hashes(hash=ht.dag_hash) # Assumes no legacy formats, since this was just created. spec_dict[ht.dag_hash.name] = s.dag_hash() - concrete_specs[build_hash] = spec_dict + concrete_specs[dag_hash] = spec_dict hash_spec_list = zip( self.concretized_order, self.concretized_user_specs) @@ -1828,33 +1820,53 @@ class Environment(object): root_hashes = set(self.concretized_order) specs_by_hash = {} - for build_hash, node_dict in json_specs_by_hash.items(): + for dag_hash, node_dict in json_specs_by_hash.items(): spec = Spec.from_node_dict(node_dict) if d['_meta']['lockfile-version'] > 1: # Build hash is stored as a key, but not as part of the node dict # To ensure build hashes are not recomputed, we reattach here - setattr(spec, ht.build_hash.attr, build_hash) - specs_by_hash[build_hash] = spec + setattr(spec, ht.dag_hash.attr, dag_hash) + specs_by_hash[dag_hash] = spec - for build_hash, node_dict in json_specs_by_hash.items(): + for dag_hash, node_dict in json_specs_by_hash.items(): for _, dep_hash, deptypes, _ in ( Spec.dependencies_from_node_dict(node_dict)): - specs_by_hash[build_hash]._add_dependency( + specs_by_hash[dag_hash]._add_dependency( specs_by_hash[dep_hash], deptypes) - # If we are reading an older lockfile format (which uses dag hashes - # that exclude build deps), we use this to convert the old - # concretized_order to the full hashes (preserving the order) + # Current lockfile key: dag_hash() (dag_hash() == full_hash()) + # Previous lockfile keys from most recent to least: + # 1. build_hash + # 2. dag_hash (computed *without* build deps) + + # If we are reading an older lockfile format, the key may have been computed + # using a different hash type than the one spack uses currently (which + # includes build deps as well as the package hash). If this is the case + # the following code updates the keys in in 'concretized_order' to be computed + # using the hash type spack currently uses, while maintaining the order of the + # list. old_hash_to_new = {} self.specs_by_hash = {} for _, spec in specs_by_hash.items(): - dag_hash = spec.dag_hash() + # - to get old dag_hash() (w/out build deps) use runtime_hash() now + # - dag_hash() now includes build deps and package hash + # - i.e. dag_hash() == full_hash() + # - regardless of what hash type keyed the lockfile we're reading, + # the dag_hash we read from the file may appear appear in install + # trees and binary mirrors, and as such, must be considered the + # permanent id of the spec. + dag_hash = spec.dag_hash() # == full_hash() build_hash = spec.build_hash() - if dag_hash in root_hashes: - old_hash_to_new[dag_hash] = build_hash + runtime_hash = spec.runtime_hash() # == old dag_hash() + + if runtime_hash in root_hashes: + old_hash_to_new[runtime_hash] = dag_hash + elif build_hash in root_hashes: + old_hash_to_new[build_hash] = dag_hash - if (dag_hash in root_hashes or build_hash in root_hashes): - self.specs_by_hash[build_hash] = spec + if (runtime_hash in root_hashes or + build_hash in root_hashes or dag_hash in root_hashes): + self.specs_by_hash[dag_hash] = spec if old_hash_to_new: # Replace any older hashes in concretized_order with hashes diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index ef75eaff2c..46ee04ae90 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -2103,6 +2103,8 @@ class Spec(object): node[hash.name] = self.build_hash() elif hash.name == 'process_hash': node[hash.name] = self.process_hash() + elif hash.name == 'runtime_hash': + node[hash.name] = self.runtime_hash() return node @@ -2244,11 +2246,17 @@ class Spec(object): dep_hash, deptypes = elt elif isinstance(elt, dict): # new format: elements of dependency spec are keyed. - for key in (ht.full_hash.name, + for key in (ht.dag_hash.name, + ht.full_hash.name, ht.build_hash.name, - ht.dag_hash.name, + ht.runtime_hash.name, ht.process_hash.name): if key in elt: + # FIXME: if the key is 'hash' it could mean the old + # dag hash without build deps, or the new dag hash which + # is equivalent to the full hash. If this was the old + # dag hash, we need to keep the hash value but set the + # key hash type to "runtime_hash". dep_hash, deptypes = elt[key], elt['type'] hash_type = key break @@ -3793,6 +3801,7 @@ class Spec(object): self._dunder_hash = other._dunder_hash self._normal = True self._full_hash = other._full_hash + self._runtime_hash = other._runtime_hash self._package_hash = other._package_hash else: self._hash = None @@ -3802,6 +3811,7 @@ class Spec(object): # always set it False here to avoid the complexity of checking self._normal = False self._full_hash = None + self._runtime_hash = None self._package_hash = None return changed @@ -4741,6 +4751,7 @@ class Spec(object): for dep in ret.traverse(root=True, order='post'): opposite = other_nodes if dep.name in self_nodes else self_nodes if any(name in dep for name in opposite.keys()): + # package hash cannot be affected by splice dep.clear_cached_hashes(ignore=['package_hash']) diff --git a/lib/spack/spack/test/ci.py b/lib/spack/spack/test/ci.py index 5dc094ae7a..8bd59124bf 100644 --- a/lib/spack/spack/test/ci.py +++ b/lib/spack/spack/test/ci.py @@ -96,9 +96,9 @@ def test_get_concrete_specs(config, mutable_mock_env_path, mock_packages): with e as active_env: for s in active_env.all_specs(): - hash_dict[s.name] = s.build_hash() + hash_dict[s.name] = s.dag_hash() if s.name == 'dyninst': - dyninst_hash = s.build_hash() + dyninst_hash = s.dag_hash() assert(dyninst_hash) @@ -107,7 +107,7 @@ def test_get_concrete_specs(config, mutable_mock_env_path, mock_packages): assert 'root' in spec_map concrete_root = spec_map['root'] - assert(concrete_root.build_hash() == dyninst_hash) + assert(concrete_root.dag_hash() == dyninst_hash) s = spec.Spec('dyninst') print('nonconc spec name: {0}'.format(s.name)) diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py index 35276054aa..f698cd2715 100644 --- a/lib/spack/spack/test/cmd/env.py +++ b/lib/spack/spack/test/cmd/env.py @@ -975,14 +975,14 @@ def create_v1_lockfile_dict(roots, all_specs): }, "roots": list( { - "hash": s.dag_hash(), + "hash": s.runtime_hash(), "spec": s.name } for s in roots ), # Version one lockfiles use the dag hash without build deps as keys, # but they write out the full node dict (including build deps) "concrete_specs": dict( - (s.dag_hash(), s.to_node_dict(hash=ht.build_hash)) + (s.runtime_hash(), s.to_node_dict(hash=ht.build_hash)) for s in all_specs ) } @@ -1016,8 +1016,8 @@ def test_read_old_lock_and_write_new(tmpdir): # When the lockfile is rewritten, it should adopt the new hash scheme # which accounts for all dependencies, including build dependencies assert hashes == set([ - x.build_hash(), - y.build_hash()]) + x.dag_hash(), + y.dag_hash()]) @pytest.mark.usefixtures('config') @@ -1118,7 +1118,7 @@ def test_store_different_build_deps(): # according to the DAG hash (since build deps are excluded from # comparison by default). Although the dag hashes are equal, the specs # are not considered equal because they compare build deps. - assert x_concretized['y'].dag_hash() == y_concretized.dag_hash() + assert x_concretized['y'].runtime_hash() == y_concretized.runtime_hash() _env_create('test', with_view=False) e = ev.read('test') -- cgit v1.2.3-60-g2f50