Fix bug where patches specified by dependents were not applied (#8272)

Fixes #7885

#7193 added the patches_to_apply function to collect patches which are then
applied in Package.do_patch. However this only collects patches that are
associated with the Package object and does not include Spec-related patches
(which are applied by dependents, added in #5476).

Spec.patches already collects patches from the package as well as those applied
by dependents, so the Package.patches_to_apply function isn't necessary. All
uses of Package.patches_to_apply are replaced with Package.spec.patches.

This also updates Package.content_hash to require the associated spec to be
concrete: Spec.patches is only set after concretization. Before this PR, it was
possible for Package.content_hash to be valid before concretizing the associated
Spec if all patches were associated with the Package (vs. being applied by
dependents). This behavior was unreliable though so the change is unlikely to
be disruptive.

2 files changed, 13 insertions, 12 deletions

diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 2b5299197f..73a872545c 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -40,7 +40,6 @@ import functools
 import glob
 import hashlib
 import inspect
-import itertools
 import os
 import re
 import shutil
@@ -1129,7 +1128,7 @@ class PackageBase(with_metaclass(PackageMeta, object)):
 
         # Apply all the patches for specs that match this one
         patched = False
-        for patch in self.patches_to_apply():
+        for patch in patches:
             try:
                 with working_dir(self.stage.source_path):
                     patch.apply(self.stage)
@@ -1173,19 +1172,15 @@ class PackageBase(with_metaclass(PackageMeta, object)):
         else:
             touch(no_patches_file)
 
-    def patches_to_apply(self):
-        """If the patch set does not change between two invocations of spack,
-           then all patches in the set will be applied in the same order"""
-        patchesToApply = itertools.chain.from_iterable(
-            patch_list
-            for spec, patch_list in self.patches.items()
-            if self.spec.satisfies(spec))
-        return list(patchesToApply)
-
     def content_hash(self, content=None):
         """Create a hash based on the sources and logic used to build the
         package. This includes the contents of all applied patches and the
         contents of applicable functions in the package subclass."""
+        if not self.spec.concrete:
+            err_msg = ("Cannot invoke content_hash on a package"
+                       " if the associated spec is not concrete")
+            raise spack.error.SpackError(err_msg)
+
         hashContent = list()
         source_id = fs.for_package_version(self, self.version).source_id()
         if not source_id:
@@ -1199,7 +1194,7 @@ class PackageBase(with_metaclass(PackageMeta, object)):
         else:
             hashContent.append(source_id.encode('utf-8'))
         hashContent.extend(':'.join((p.sha256, str(p.level))).encode('utf-8')
-                           for p in self.patches_to_apply())
+                           for p in self.spec.patches)
         hashContent.append(package_hash(self.spec, content))
         return base64.b32encode(
             hashlib.sha256(bytes().join(sorted(hashContent))).digest()).lower()
diff --git a/lib/spack/spack/test/packages.py b/lib/spack/spack/test/packages.py
index 2d9878ed96..9fae9decfb 100644
--- a/lib/spack/spack/test/packages.py
+++ b/lib/spack/spack/test/packages.py
@@ -71,6 +71,8 @@ class TestPackage(object):
     def test_content_hash_all_same_but_patch_contents(self):
         spec1 = Spec("hash-test1@1.1")
         spec2 = Spec("hash-test2@1.1")
+        spec1.concretize()
+        spec2.concretize()
         content1 = package_content(spec1)
         content1 = content1.replace(spec1.package.__class__.__name__, '')
         content2 = package_content(spec2)
@@ -81,6 +83,8 @@ class TestPackage(object):
     def test_content_hash_different_variants(self):
         spec1 = Spec("hash-test1@1.2 +variantx")
         spec2 = Spec("hash-test2@1.2 ~variantx")
+        spec1.concretize()
+        spec2.concretize()
         content1 = package_content(spec1)
         content1 = content1.replace(spec1.package.__class__.__name__, '')
         content2 = package_content(spec2)
@@ -91,6 +95,8 @@ class TestPackage(object):
     def test_all_same_but_archive_hash(self):
         spec1 = Spec("hash-test1@1.3")
         spec2 = Spec("hash-test2@1.3")
+        spec1.concretize()
+        spec2.concretize()
         content1 = package_content(spec1)
         content1 = content1.replace(spec1.package.__class__.__name__, '')
         content2 = package_content(spec2)