From 60580f587179f57e182f9ff957dc96c3ac079c0a Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Sat, 14 Dec 2019 14:31:39 -0800 Subject: [PATCH] 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 --- lib/spack/spack/test/packages.py | 58 +++++++++++-------- lib/spack/spack/util/package_hash.py | 13 ++++- .../packages/hash-test1/package.py | 9 ++- .../packages/hash-test3/package.py | 42 ++++++++++++++ 4 files changed, 95 insertions(+), 27 deletions(-) create mode 100644 var/spack/repos/builtin.mock/packages/hash-test3/package.py 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 @@ 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 @@ def test_package_class_names(self): 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 @@ def visit_FunctionDef(self, node): # noqa 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)) diff --git a/var/spack/repos/builtin.mock/packages/hash-test1/package.py b/var/spack/repos/builtin.mock/packages/hash-test1/package.py index 879e08147e..ffcaa89eb9 100644 --- a/var/spack/repos/builtin.mock/packages/hash-test1/package.py +++ b/var/spack/repos/builtin.mock/packages/hash-test1/package.py @@ -19,6 +19,9 @@ class HashTest1(Package): version('1.2', 'b' * 32) version('1.3', 'c' * 32) version('1.4', 'd' * 32) + version('1.5', 'd' * 32) + version('1.6', 'e' * 32) + version('1.7', 'f' * 32) patch('patch1.patch', when="@1.1") patch('patch2.patch', when="@1.4") @@ -34,6 +37,10 @@ def install(self, spec, prefix): print("install 1") os.listdir(os.getcwd()) - @when('@1.5') + @when('@1.5:') def install(self, spec, prefix): os.listdir(os.getcwd()) + + @when('@1.5,1.6') + def extra_phase(self, spec, prefix): + pass diff --git a/var/spack/repos/builtin.mock/packages/hash-test3/package.py b/var/spack/repos/builtin.mock/packages/hash-test3/package.py new file mode 100644 index 0000000000..345309a5eb --- /dev/null +++ b/var/spack/repos/builtin.mock/packages/hash-test3/package.py @@ -0,0 +1,42 @@ +# Copyright 2013-2019 Lawrence Livermore National Security, LLC and other +# Spack Project Developers. See the top-level COPYRIGHT file for details. +# +# SPDX-License-Identifier: (Apache-2.0 OR MIT) + +from spack import * + +import os + + +class HashTest3(Package): + """Used to test package hashing + """ + + homepage = "http://www.hashtest3.org" + url = "http://www.hashtest1.org/downloads/hashtest3-1.1.tar.bz2" + + version('1.2', 'b' * 32) + version('1.3', 'c' * 32) + version('1.5', 'd' * 32) + version('1.6', 'e' * 32) + version('1.7', 'f' * 32) + + variant('variantx', default=False, description='Test variant X') + variant('varianty', default=False, description='Test variant Y') + + def setup_dependent_environment(self, spack_env, run_env, dependent_spec): + pass + + @when('@:1.4') + def install(self, spec, prefix): + print("install 1") + os.listdir(os.getcwd()) + + @when('@1.5:') + def install(self, spec, prefix): + os.listdir(os.getcwd()) + + for _version_constraint in ['@1.5', '@1.6']: + @when(_version_constraint) + def extra_phase(self, spec, prefix): + pass