From 5df10c04cd4ffe223f346b8bd9e5cd80dedbfe34 Mon Sep 17 00:00:00 2001 From: "Adam J. Stewart" Date: Wed, 23 Mar 2022 02:50:00 -0500 Subject: Use stable URLs and `?full_index=1` for all github patches (#29239) The number of commit characters in patch files fetched from GitHub can change, so we should use `full_index=1` to enforce full commit hashes (and a stable patch `sha256`). Similarly, URLs for branches like `master` don't give us stable patch files, because branches are moving targets. Use specific tags or commits for those. - [x] update all github patch URLs to use `full_index=1` - [x] don't use `master` or other branches for patches - [x] add an audit check and a test for `?full_index=1` Co-authored-by: Todd Gamblin --- lib/spack/spack/audit.py | 61 +++++++++++++++++++++++++++---------------- lib/spack/spack/test/audit.py | 20 +++++++------- 2 files changed, 48 insertions(+), 33 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/audit.py b/lib/spack/spack/audit.py index 8787f520c2..893c7f6aa3 100644 --- a/lib/spack/spack/audit.py +++ b/lib/spack/spack/audit.py @@ -41,11 +41,14 @@ import re from six.moves.urllib.request import urlopen -try: - from collections.abc import Sequence # novm -except ImportError: - from collections import Sequence +import llnl.util.lang +from llnl.util.compat import Sequence +import spack.config +import spack.patch +import spack.repo +import spack.spec +import spack.variant #: Map an audit tag to a list of callables implementing checks CALLBACKS = {} @@ -180,7 +183,6 @@ config_compiler = AuditClass( @config_compiler def _search_duplicate_compilers(error_cls): """Report compilers with the same spec and two different definitions""" - import spack.config errors = [] compilers = list(sorted( @@ -217,8 +219,6 @@ config_packages = AuditClass( @config_packages def _search_duplicate_specs_in_externals(error_cls): """Search for duplicate specs declared as externals""" - import spack.config - errors, externals = [], collections.defaultdict(list) packages_yaml = spack.config.get('packages') @@ -265,6 +265,7 @@ package_directives = AuditClass( kwargs=('pkgs',) ) + #: Sanity checks on linting # This can take some time, so it's run separately from packages package_https_directives = AuditClass( @@ -275,15 +276,40 @@ package_https_directives = AuditClass( ) +@package_directives +def _check_patch_urls(pkgs, error_cls): + """Ensure that patches fetched from GitHub have stable sha256 hashes.""" + github_patch_url_re = ( + r"^https?://github\.com/.+/.+/(?:commit|pull)/[a-fA-F0-9]*.(?:patch|diff)" + ) + + errors = [] + for pkg_name in pkgs: + pkg = spack.repo.get(pkg_name) + for condition, patches in pkg.patches.items(): + for patch in patches: + if not isinstance(patch, spack.patch.UrlPatch): + continue + + if not re.match(github_patch_url_re, patch.url): + continue + + full_index_arg = "?full_index=1" + if not patch.url.endswith(full_index_arg): + errors.append(error_cls( + "patch URL in package {0} must end with {1}".format( + pkg.name, full_index_arg, + ), + [patch.url], + )) + + return errors + + @package_https_directives def _linting_package_file(pkgs, error_cls): """Check for correctness of links """ - import llnl.util.lang - - import spack.repo - import spack.spec - errors = [] for pkg_name in pkgs: pkg = spack.repo.get(pkg_name) @@ -308,11 +334,6 @@ def _linting_package_file(pkgs, error_cls): @package_directives def _unknown_variants_in_directives(pkgs, error_cls): """Report unknown or wrong variants in directives for this package""" - import llnl.util.lang - - import spack.repo - import spack.spec - errors = [] for pkg_name in pkgs: pkg = spack.repo.get(pkg_name) @@ -367,9 +388,6 @@ def _unknown_variants_in_directives(pkgs, error_cls): @package_directives def _unknown_variants_in_dependencies(pkgs, error_cls): """Report unknown dependencies and wrong variants for dependencies""" - import spack.repo - import spack.spec - errors = [] for pkg_name in pkgs: pkg = spack.repo.get(pkg_name) @@ -417,8 +435,6 @@ def _unknown_variants_in_dependencies(pkgs, error_cls): @package_directives def _version_constraints_are_satisfiable_by_some_version_in_repo(pkgs, error_cls): """Report if version constraints used in directives are not satisfiable""" - import spack.repo - errors = [] for pkg_name in pkgs: pkg = spack.repo.get(pkg_name) @@ -455,7 +471,6 @@ def _version_constraints_are_satisfiable_by_some_version_in_repo(pkgs, error_cls def _analyze_variants_in_directive(pkg, constraint, directive, error_cls): - import spack.variant variant_exceptions = ( spack.variant.InconsistentValidationError, spack.variant.MultipleValuesInExclusiveVariantError, diff --git a/lib/spack/spack/test/audit.py b/lib/spack/spack/test/audit.py index ffc0d5b8d9..6ad986643e 100644 --- a/lib/spack/spack/test/audit.py +++ b/lib/spack/spack/test/audit.py @@ -8,30 +8,30 @@ import spack.audit import spack.config -@pytest.mark.parametrize('packages,failing_check', [ +@pytest.mark.parametrize('packages,expected_error', [ # A non existing variant is used in a conflict directive (['wrong-variant-in-conflicts'], 'PKG-DIRECTIVES'), # The package declares a non-existing dependency (['missing-dependency'], 'PKG-DIRECTIVES'), # The package use a non existing variant in a depends_on directive (['wrong-variant-in-depends-on'], 'PKG-DIRECTIVES'), + # This package has a GitHub patch URL without full_index=1 + (['invalid-github-patch-url'], 'PKG-DIRECTIVES'), # This package has no issues (['mpileaks'], None), # This package has a conflict with a trigger which cannot constrain the constraint # Should not raise an error (['unconstrainable-conflict'], None), ]) -def test_package_audits(packages, failing_check, mock_packages): +def test_package_audits(packages, expected_error, mock_packages): reports = spack.audit.run_group('packages', pkgs=packages) - for check, errors in reports: - # Check that we have errors only if there is an expected failure - # and that the tag matches our expectations - if bool(failing_check): - assert check == failing_check - assert errors - else: - assert not errors + # Check that errors were reported only for the expected failure + actual_errors = [check for check, errors in reports if errors] + if expected_error: + assert [expected_error] == actual_errors + else: + assert not actual_errors # Data used in the test below to audit the double definition of a compiler -- cgit v1.2.3-60-g2f50