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 <tgamblin@llnl.gov>

2 files changed, 48 insertions, 33 deletions

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