From 8880a00862026700a82de1004e24ff06e54a165a Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Thu, 23 Dec 2021 00:41:45 -0800 Subject: package_hash: remove all unassigned strings, not just docstrings Some packages use top-level unassigned strings instead of comments, either just after a docstring on in the body somewhere else. Ignore those strings becasue they have no effect on package behavior. - [x] adjust RemoveDocstrings to remove all free-standing strings. - [x] move tests for util/package_hash.py to test/util/package_hash.py Co-authored-by: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> --- lib/spack/spack/test/package_hash.py | 65 ---------------- lib/spack/spack/test/util/package_hash.py | 125 ++++++++++++++++++++++++++++++ lib/spack/spack/util/package_hash.py | 16 +++- 3 files changed, 137 insertions(+), 69 deletions(-) delete mode 100644 lib/spack/spack/test/package_hash.py create mode 100644 lib/spack/spack/test/util/package_hash.py diff --git a/lib/spack/spack/test/package_hash.py b/lib/spack/spack/test/package_hash.py deleted file mode 100644 index 6e50fee5ce..0000000000 --- a/lib/spack/spack/test/package_hash.py +++ /dev/null @@ -1,65 +0,0 @@ -# Copyright 2013-2021 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.spec import Spec -from spack.util.package_hash import package_content, package_hash - - -def test_hash(tmpdir, mock_packages, config): - package_hash("hash-test1@1.2") - - -def test_different_variants(tmpdir, mock_packages, config): - spec1 = Spec("hash-test1@1.2 +variantx") - spec2 = Spec("hash-test1@1.2 +varianty") - assert package_hash(spec1) == package_hash(spec2) - - -def test_all_same_but_name(tmpdir, mock_packages, config): - spec1 = Spec("hash-test1@1.2") - spec2 = Spec("hash-test2@1.2") - compare_sans_name(True, spec1, spec2) - - spec1 = Spec("hash-test1@1.2 +varianty") - spec2 = Spec("hash-test2@1.2 +varianty") - compare_sans_name(True, spec1, spec2) - - -def test_all_same_but_archive_hash(tmpdir, mock_packages, config): - """ - Archive hash is not intended to be reflected in Package hash. - """ - spec1 = Spec("hash-test1@1.3") - spec2 = Spec("hash-test2@1.3") - compare_sans_name(True, spec1, spec2) - - -def test_all_same_but_patch_contents(tmpdir, mock_packages, config): - spec1 = Spec("hash-test1@1.1") - spec2 = Spec("hash-test2@1.1") - compare_sans_name(True, spec1, spec2) - - -def test_all_same_but_patches_to_apply(tmpdir, mock_packages, config): - spec1 = Spec("hash-test1@1.4") - spec2 = Spec("hash-test2@1.4") - compare_sans_name(True, spec1, spec2) - - -def test_all_same_but_install(tmpdir, mock_packages, config): - spec1 = Spec("hash-test1@1.5") - spec2 = Spec("hash-test2@1.5") - compare_sans_name(False, spec1, spec2) - - -def compare_sans_name(eq, spec1, spec2): - content1 = package_content(spec1) - content1 = content1.replace(spec1.package.__class__.__name__, '') - content2 = package_content(spec2) - content2 = content2.replace(spec2.package.__class__.__name__, '') - if eq: - assert content1 == content2 - else: - assert content1 != content2 diff --git a/lib/spack/spack/test/util/package_hash.py b/lib/spack/spack/test/util/package_hash.py new file mode 100644 index 0000000000..16829586e4 --- /dev/null +++ b/lib/spack/spack/test/util/package_hash.py @@ -0,0 +1,125 @@ +# Copyright 2013-2021 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) + +import ast + +import spack.paths +import spack.util.package_hash as ph +from spack.spec import Spec +from spack.util.unparse import unparse + + +def test_hash(tmpdir, mock_packages, config): + ph.package_hash("hash-test1@1.2") + + +def test_different_variants(tmpdir, mock_packages, config): + spec1 = Spec("hash-test1@1.2 +variantx") + spec2 = Spec("hash-test1@1.2 +varianty") + assert ph.package_hash(spec1) == ph.package_hash(spec2) + + +def test_all_same_but_name(tmpdir, mock_packages, config): + spec1 = Spec("hash-test1@1.2") + spec2 = Spec("hash-test2@1.2") + compare_sans_name(True, spec1, spec2) + + spec1 = Spec("hash-test1@1.2 +varianty") + spec2 = Spec("hash-test2@1.2 +varianty") + compare_sans_name(True, spec1, spec2) + + +def test_all_same_but_archive_hash(tmpdir, mock_packages, config): + """ + Archive hash is not intended to be reflected in Package hash. + """ + spec1 = Spec("hash-test1@1.3") + spec2 = Spec("hash-test2@1.3") + compare_sans_name(True, spec1, spec2) + + +def test_all_same_but_patch_contents(tmpdir, mock_packages, config): + spec1 = Spec("hash-test1@1.1") + spec2 = Spec("hash-test2@1.1") + compare_sans_name(True, spec1, spec2) + + +def test_all_same_but_patches_to_apply(tmpdir, mock_packages, config): + spec1 = Spec("hash-test1@1.4") + spec2 = Spec("hash-test2@1.4") + compare_sans_name(True, spec1, spec2) + + +def test_all_same_but_install(tmpdir, mock_packages, config): + spec1 = Spec("hash-test1@1.5") + spec2 = Spec("hash-test2@1.5") + compare_sans_name(False, spec1, spec2) + + +def compare_sans_name(eq, spec1, spec2): + content1 = ph.package_content(spec1) + content1 = content1.replace(spec1.package.__class__.__name__, '') + content2 = ph.package_content(spec2) + content2 = content2.replace(spec2.package.__class__.__name__, '') + if eq: + assert content1 == content2 + else: + assert content1 != content2 + + +many_strings = '''\ +"""ONE""" +"""TWO""" + +var = "THREE" # make sure this is not removed + +"FOUR" + +class ManyDocstrings: + """FIVE""" + """SIX""" + + x = "SEVEN" + + def method1(): + """EIGHT""" + + print("NINE") + + "TEN" + for i in range(10): + print(i) + + def method2(): + """ELEVEN""" + return "TWELVE" +''' + + +def test_remove_docstrings(): + tree = ast.parse(many_strings) + tree = ph.RemoveDocstrings().visit(tree) + + unparsed = unparse(tree) + + # make sure the methods are preserved + assert "method1" in unparsed + assert "method2" in unparsed + + # all of these are unassigned and should be removed + assert "ONE" not in unparsed + assert "TWO" not in unparsed + assert "FOUR" not in unparsed + assert "FIVE" not in unparsed + assert "SIX" not in unparsed + assert "EIGHT" not in unparsed + assert "TEN" not in unparsed + assert "ELEVEN" not in unparsed + + # these are used in legitimate expressions + assert "THREE" in unparsed + assert "SEVEN" in unparsed + assert "NINE" in unparsed + assert "TWELVE" in unparsed diff --git a/lib/spack/spack/util/package_hash.py b/lib/spack/spack/util/package_hash.py index bb9ad40e45..d1412e16fd 100644 --- a/lib/spack/spack/util/package_hash.py +++ b/lib/spack/spack/util/package_hash.py @@ -15,12 +15,20 @@ import spack.util.naming class RemoveDocstrings(ast.NodeTransformer): - """Transformer that removes docstrings from a Python AST.""" + """Transformer that removes docstrings from a Python AST. + + This removes *all* strings that aren't on the RHS of an assignment statement from + the body of functions, classes, and modules -- even if they're not directly after + the declaration. + + """ def remove_docstring(self, node): + def unused_string(node): + """Criteria for unassigned body strings.""" + return isinstance(node, ast.Expr) and isinstance(node.value, ast.Str) + if node.body: - if isinstance(node.body[0], ast.Expr) and \ - isinstance(node.body[0].value, ast.Str): - node.body.pop(0) + node.body = [child for child in node.body if not unused_string(child)] self.generic_visit(node) return node -- cgit v1.2.3-70-g09d2