diff options
| -rw-r--r-- | lib/spack/spack/package.py | 5 | ||||
| -rw-r--r-- | lib/spack/spack/solver/asp.py | 5 | ||||
| -rw-r--r-- | lib/spack/spack/spec.py | 115 | ||||
| -rw-r--r-- | lib/spack/spack/test/spec_semantics.py | 20 |
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() |