ci: Skip removed packages during automatic checksum verification (#49681)

* Skip packages removed for automatic checksum verification

* Unify finding modified or added packages with spack.repo logic

* Remove unused imports

* Fix unit-tests using shared modified function

* Update last remaining unit test to new format

lib/spack/spack/cmd/ci.py

@@ -4,7 +4,6 @@
 
 import json
 import os
-import re
 import shutil
 import sys
 from typing import Dict
@@ -26,12 +25,10 @@
 import spack.hash_types as ht
 import spack.mirrors.mirror
 import spack.package_base
-import spack.paths
 import spack.repo
 import spack.spec
 import spack.stage
 import spack.util.executable
-import spack.util.git
 import spack.util.gpg as gpg_util
 import spack.util.timer as timer
 import spack.util.url as url_util
@@ -45,7 +42,6 @@
 SPACK_COMMAND = "spack"
 INSTALL_FAIL_CODE = 1
 FAILED_CREATE_BUILDCACHE_CODE = 100
-BUILTIN = re.compile(r"var\/spack\/repos\/builtin\/packages\/([^\/]+)\/package\.py")
 
 
 def deindent(desc):
@@ -783,18 +779,15 @@ def ci_verify_versions(args):
     then parses the git diff between the two to determine which packages
     have been modified verifies the new checksums inside of them.
     """
-    with fs.working_dir(spack.paths.prefix):
+    # Get a list of all packages that have been changed or added
-        # We use HEAD^1 explicitly on the merge commit created by
+    # between from_ref and to_ref
-        # GitHub Actions. However HEAD~1 is a safer default for the helper function.
+    pkgs = spack.repo.get_all_package_diffs("AC", args.from_ref, args.to_ref)
-        files = spack.util.git.get_modified_files(from_ref=args.from_ref, to_ref=args.to_ref)
-
-    # Get a list of package names from the modified files.
-    pkgs = [(m.group(1), p) for p in files for m in [BUILTIN.search(p)] if m]
 
     failed_version = False
-    for pkg_name, path in pkgs:
+    for pkg_name in pkgs:
         spec = spack.spec.Spec(pkg_name)
         pkg = spack.repo.PATH.get_pkg_class(spec.name)(spec)
+        path = spack.repo.PATH.package_path(pkg_name)
 
         # Skip checking manual download packages and trust the maintainers
         if pkg.manual_download:
@@ -818,7 +811,7 @@ def ci_verify_versions(args):
             # TODO: enforce every version have a commit or a sha256 defined if not
             # an infinite version (there are a lot of package's where this doesn't work yet.)
 
-        with fs.working_dir(spack.paths.prefix):
+        with fs.working_dir(os.path.dirname(path)):
             added_checksums = spack_ci.get_added_versions(
                 checksums_version_dict, path, from_ref=args.from_ref, to_ref=args.to_ref
             )

lib/spack/spack/test/ci.py

@@ -32,7 +32,7 @@ def repro_dir(tmp_path):
 
 
 def test_get_added_versions_new_checksum(mock_git_package_changes):
-    repo_path, filename, commits = mock_git_package_changes
+    repo, filename, commits = mock_git_package_changes
 
     checksum_versions = {
         "3f6576971397b379d4205ae5451ff5a68edf6c103b2f03c4188ed7075fbb5f04": Version("2.1.5"),
@@ -41,7 +41,7 @@ def test_get_added_versions_new_checksum(mock_git_package_changes):
         "86993903527d9b12fc543335c19c1d33a93797b3d4d37648b5addae83679ecd8": Version("2.0.0"),
     }
 
-    with fs.working_dir(str(repo_path)):
+    with fs.working_dir(repo.packages_path):
         added_versions = ci.get_added_versions(
             checksum_versions, filename, from_ref=commits[-1], to_ref=commits[-2]
         )
@@ -50,7 +50,7 @@ def test_get_added_versions_new_checksum(mock_git_package_changes):
 
 
 def test_get_added_versions_new_commit(mock_git_package_changes):
-    repo_path, filename, commits = mock_git_package_changes
+    repo, filename, commits = mock_git_package_changes
 
     checksum_versions = {
         "74253725f884e2424a0dd8ae3f69896d5377f325": Version("2.1.6"),
@@ -60,9 +60,9 @@ def test_get_added_versions_new_commit(mock_git_package_changes):
         "86993903527d9b12fc543335c19c1d33a93797b3d4d37648b5addae83679ecd8": Version("2.0.0"),
     }
 
-    with fs.working_dir(str(repo_path)):
+    with fs.working_dir(repo.packages_path):
         added_versions = ci.get_added_versions(
-            checksum_versions, filename, from_ref=commits[2], to_ref=commits[1]
+            checksum_versions, filename, from_ref=commits[-2], to_ref=commits[-3]
         )
         assert len(added_versions) == 1
         assert added_versions[0] == Version("2.1.6")

lib/spack/spack/test/cmd/ci.py

@@ -1978,6 +1978,13 @@ def test_ci_validate_git_versions_invalid(
         assert f"Invalid commit for diff-test@{version}" in err
 
 
+def mock_packages_path(path):
+    def packages_path():
+        return path
+
+    return packages_path
+
+
 @pytest.fixture
 def verify_standard_versions_valid(monkeypatch):
     def validate_standard_versions(pkg, versions):
@@ -2024,9 +2031,12 @@ def test_ci_verify_versions_valid(
     mock_git_package_changes,
     verify_standard_versions_valid,
     verify_git_versions_valid,
+    tmpdir,
 ):
-    repo_path, _, commits = mock_git_package_changes
+    repo, _, commits = mock_git_package_changes
-    monkeypatch.setattr(spack.paths, "prefix", repo_path)
+    spack.repo.PATH.put_first(repo)
+
+    monkeypatch.setattr(spack.repo, "packages_path", mock_packages_path(repo.packages_path))
 
     out = ci_cmd("verify-versions", commits[-1], commits[-3])
     assert "Validated diff-test@2.1.5" in out
@@ -2040,9 +2050,10 @@ def test_ci_verify_versions_standard_invalid(
     verify_standard_versions_invalid,
     verify_git_versions_invalid,
 ):
-    repo_path, _, commits = mock_git_package_changes
+    repo, _, commits = mock_git_package_changes
+    spack.repo.PATH.put_first(repo)
 
-    monkeypatch.setattr(spack.paths, "prefix", repo_path)
+    monkeypatch.setattr(spack.repo, "packages_path", mock_packages_path(repo.packages_path))
 
     out = ci_cmd("verify-versions", commits[-1], commits[-3], fail_on_error=False)
     assert "Invalid checksum found diff-test@2.1.5" in out
@@ -2050,8 +2061,10 @@ def test_ci_verify_versions_standard_invalid(
 
 
 def test_ci_verify_versions_manual_package(monkeypatch, mock_packages, mock_git_package_changes):
-    repo_path, _, commits = mock_git_package_changes
+    repo, _, commits = mock_git_package_changes
-    monkeypatch.setattr(spack.paths, "prefix", repo_path)
+    spack.repo.PATH.put_first(repo)
+
+    monkeypatch.setattr(spack.repo, "packages_path", mock_packages_path(repo.packages_path))
 
     pkg_class = spack.spec.Spec("diff-test").package_class
     monkeypatch.setattr(pkg_class, "manual_download", True)

lib/spack/spack/test/conftest.py

@@ -243,13 +243,11 @@ def latest_commit():
 
 
 @pytest.fixture
-def mock_git_package_changes(git, tmpdir, override_git_repos_cache_path):
+def mock_git_package_changes(git, tmpdir, override_git_repos_cache_path, monkeypatch):
     """Create a mock git repo with known structure of package edits
 
     The structure of commits in this repo is as follows::
 
-       o diff-test: modification to make manual download package
-       |
        o diff-test: add v1.2 (from a git ref)
        |
        o diff-test: add v1.1 (from source tarball)
@@ -261,8 +259,12 @@ def mock_git_package_changes(git, tmpdir, override_git_repos_cache_path):
     Important attributes of the repo for test coverage are: multiple package
     versions are added with some coming from a tarball and some from git refs.
     """
-    repo_path = str(tmpdir.mkdir("git_package_changes_repo"))
+    filename = "diff-test/package.py"
-    filename = "var/spack/repos/builtin/packages/diff-test/package.py"
+
+    repo_path, _ = spack.repo.create_repo(str(tmpdir.mkdir("myrepo")))
+    repo_cache = spack.util.file_cache.FileCache(str(tmpdir.mkdir("cache")))
+
+    repo = spack.repo.Repo(repo_path, cache=repo_cache)
 
     def commit(message):
         global commit_counter
@@ -276,7 +278,7 @@ def commit(message):
         )
         commit_counter += 1
 
-    with working_dir(repo_path):
+    with working_dir(repo.packages_path):
         git("init")
 
         git("config", "user.name", "Spack")
@@ -307,17 +309,11 @@ def latest_commit():
         commit("diff-test: add v2.1.6")
         commits.append(latest_commit())
 
-        # convert pkg-a to a manual download package
-        shutil.copy2(f"{spack.paths.test_path}/data/conftest/diff-test/package-3.txt", filename)
-        git("add", filename)
-        commit("diff-test: modification to make manual download package")
-        commits.append(latest_commit())
-
         # The commits are ordered with the last commit first in the list
         commits = list(reversed(commits))
 
     # Return the git directory to install, the filename used, and the commits
-    yield repo_path, filename, commits
+    yield repo, filename, commits
 
 
 @pytest.fixture(autouse=True)

lib/spack/spack/test/data/conftest/diff-test/package-3.txt

@@ -1,23 +0,0 @@
-# Copyright Spack Project Developers. See COPYRIGHT file for details.
-#
-# SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
-from spack.package import *
-
-
-class DiffTest(AutotoolsPackage):
-    """zlib replacement with optimizations for next generation systems."""
-
-    homepage = "https://github.com/zlib-ng/zlib-ng"
-    url = "https://github.com/zlib-ng/zlib-ng/archive/2.0.0.tar.gz"
-    git = "https://github.com/zlib-ng/zlib-ng.git"
-
-    license("Zlib")
-
-    manual_download = True
-
-    version("2.1.6", tag="2.1.6", commit="74253725f884e2424a0dd8ae3f69896d5377f325")
-    version("2.1.5", sha256="3f6576971397b379d4205ae5451ff5a68edf6c103b2f03c4188ed7075fbb5f04")
-    version("2.1.4", sha256="a0293475e6a44a3f6c045229fe50f69dc0eebc62a42405a51f19d46a5541e77a")
-    version("2.0.7", sha256="6c0853bb27738b811f2b4d4af095323c3d5ce36ceed6b50e5f773204fb8f7200")
-    version("2.0.0", sha256="86993903527d9b12fc543335c19c1d33a93797b3d4d37648b5addae83679ecd8")

lib/spack/spack/test/util/git.py

@@ -7,9 +7,9 @@
 
 
 def test_modified_files(mock_git_package_changes):
-    repo_path, filename, commits = mock_git_package_changes
+    repo, filename, commits = mock_git_package_changes
 
-    with working_dir(repo_path):
+    with working_dir(repo.packages_path):
         files = get_modified_files(from_ref="HEAD~1", to_ref="HEAD")
         assert len(files) == 1
         assert files[0] == filename