package hash: gracefully handle @when with non-string args (#14153)

* when constructing package hash, default to including a method in the content hash if we can't determine whether it would be included by examining the AST
* add a test for updated content-hash calculations
* refactor content hash tests to eliminate repeated lines

2 files changed, 45 insertions, 26 deletions

diff --git a/lib/spack/spack/test/packages.py b/lib/spack/spack/test/packages.py
index 186b0d0007..bd4ba95053 100644
--- a/lib/spack/spack/test/packages.py
+++ b/lib/spack/spack/test/packages.py
@@ -16,6 +16,11 @@ from spack.version import VersionChecksumError
 import spack.directives
 
 
+def _generate_content_strip_name(spec):
+    content = package_content(spec)
+    return content.replace(spec.package.__class__.__name__, '')
+
+
 @pytest.mark.usefixtures('config', 'mock_packages')
 class TestPackage(object):
     def test_load_package(self):
@@ -53,38 +58,43 @@ class TestPackage(object):
         assert '_3db' == mod_to_class('3db')
 
     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)
-        content2 = content2.replace(spec2.package.__class__.__name__, '')
+        spec1 = Spec("hash-test1@1.1").concretized()
+        spec2 = Spec("hash-test2@1.1").concretized()
+        content1 = _generate_content_strip_name(spec1)
+        content2 = _generate_content_strip_name(spec2)
         assert spec1.package.content_hash(content=content1) != \
             spec2.package.content_hash(content=content2)
 
     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)
-        content2 = content2.replace(spec2.package.__class__.__name__, '')
+        spec1 = Spec("hash-test1@1.2 +variantx").concretized()
+        spec2 = Spec("hash-test2@1.2 ~variantx").concretized()
+        content1 = _generate_content_strip_name(spec1)
+        content2 = _generate_content_strip_name(spec2)
         assert spec1.package.content_hash(content=content1) == \
             spec2.package.content_hash(content=content2)
 
+    def test_content_hash_cannot_get_details_from_ast(self):
+        """Packages hash-test1 and hash-test3 would be considered the same
+           except that hash-test3 conditionally executes a phase based on
+           a "when" directive that Spack cannot evaluate by examining the
+           AST. This test ensures that Spack can compute a content hash
+           for hash-test3. If Spack cannot determine when a phase applies,
+           it adds it by default, so the test also ensures that the hashes
+           differ where Spack includes a phase on account of AST-examination
+           failure.
+        """
+        spec3 = Spec("hash-test1@1.7").concretized()
+        spec4 = Spec("hash-test3@1.7").concretized()
+        content3 = _generate_content_strip_name(spec3)
+        content4 = _generate_content_strip_name(spec4)
+        assert(spec3.package.content_hash(content=content3) !=
+               spec4.package.content_hash(content=content4))
+
     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)
-        content2 = content2.replace(spec2.package.__class__.__name__, '')
+        spec1 = Spec("hash-test1@1.3").concretized()
+        spec2 = Spec("hash-test2@1.3").concretized()
+        content1 = _generate_content_strip_name(spec1)
+        content2 = _generate_content_strip_name(spec2)
         assert spec1.package.content_hash(content=content1) != \
             spec2.package.content_hash(content=content2)
 
diff --git a/lib/spack/spack/util/package_hash.py b/lib/spack/spack/util/package_hash.py
index 2a3ee80fd5..18b126486c 100644
--- a/lib/spack/spack/util/package_hash.py
+++ b/lib/spack/spack/util/package_hash.py
@@ -69,8 +69,17 @@ class TagMultiMethods(ast.NodeVisitor):
         if node.decorator_list:
             dec = node.decorator_list[0]
             if isinstance(dec, ast.Call) and dec.func.id == 'when':
-                cond = dec.args[0].s
-                nodes.append((node, self.spec.satisfies(cond, strict=True)))
+                try:
+                    cond = dec.args[0].s
+                    nodes.append(
+                        (node, self.spec.satisfies(cond, strict=True)))
+                except AttributeError:
+                    # In this case the condition for the 'when' decorator is
+                    # not a string literal (for example it may be a Python
+                    # variable name). Therefore the function is added
+                    # unconditionally since we don't know whether the
+                    # constraint applies or not.
+                    nodes.append((node, None))
         else:
             nodes.append((node, None))