From 1d6662abfbfad0cf3cd43c1c2808fcc49a97d25e Mon Sep 17 00:00:00 2001 From: Philip Sakievich Date: Tue, 1 Apr 2025 13:29:28 -0600 Subject: [PATCH] New test on version satisfaction --- lib/spack/spack/git_ref_operators.py | 85 ------------------- lib/spack/spack/package_base.py | 9 +- lib/spack/spack/solver/asp.py | 13 +-- lib/spack/spack/solver/concretize.lp | 7 +- lib/spack/spack/test/concretization/core.py | 2 +- lib/spack/spack/test/conftest.py | 5 ++ lib/spack/spack/test/packages.py | 5 ++ .../packages/git-ref-package/package.py | 1 + 8 files changed, 30 insertions(+), 97 deletions(-) delete mode 100644 lib/spack/spack/git_ref_operators.py diff --git a/lib/spack/spack/git_ref_operators.py b/lib/spack/spack/git_ref_operators.py deleted file mode 100644 index 689037dfc5c..00000000000 --- a/lib/spack/spack/git_ref_operators.py +++ /dev/null @@ -1,85 +0,0 @@ -# Copyright Spack Project Developers. See COPYRIGHT file for details. -# -# SPDX-License-Identifier: (Apache-2.0 OR MIT) -import os - -import spack.error -import spack.repo -import spack.util.git -from spack.util.executable import ProcessError -from spack.version import GitVersion, StandardVersion, Version - - -class GitRefFetchError(spack.error.SpackError): - pass - - -class GitBranch: - def __init__(self, git_address, name): - self.address = git_address - self.name = name - self.flag = "-h" - - -class GitTag: - def __init__(self, git_address, name): - self.address = git_address - self.name = name - self.flag = "-t" - - -def associated_git_ref(version, package_class): - """ - Retrieve the branch or tag associated with the version - and return the relevant git information for querying the git repo/remote - """ - version_dict = package_class.versions.get(version, {}) - - git_address = version_dict.get("git", None) or getattr(package_class, "git", None) - - if git_address: - branch = version_dict.get("branch", None) - tag = version_dict.get("tag", None) - if branch: - return GitBranch(git_address, branch) - elif tag: - return GitTag(git_address, tag) - return None - - -def retrieve_latest_git_hash(git_ref): - """Get the git hash associated with a tag or branch""" - # remote git operations can sometimes have banners so we must parse the output for a sha - query = spack.util.git.git(required=True)( - "ls-remote", git_ref.flag, git_ref.address, git_ref.name, output=str, error=os.devnull - ) - sha, ref = query.strip().split() - return sha - - -def convert_standard_to_git_version(version, package_class_name): - """ - Converts a StandardVersion to a GitVersion - - This function will assign the Git commit sha to a version if it has a branch or tag - """ - pkg_class = spack.repo.PATH.get_pkg_class(package_class_name) - git_ref = associated_git_ref(version, pkg_class) - if git_ref: - try: - hash = retrieve_latest_git_hash(git_ref) - except (ProcessError, ValueError, AssertionError): - raise GitRefFetchError( - ( - "Failure to fetch git sha when running" - f" `git ls-remote {git_ref.address} {git_ref.name}`\n" - "Confirm network connectivty by running this command followed by:\n" - f"\t`spack fetch {git_ref.address}@{str(version)}`" - "Post a bug report if both of these operations succeed." - ) - ) - - new_version_str = f"git.{hash}={str(version)}" - return GitVersion(new_version_str) - else: - return None diff --git a/lib/spack/spack/package_base.py b/lib/spack/spack/package_base.py index 9305e5a5029..a02c73f7cc6 100644 --- a/lib/spack/spack/package_base.py +++ b/lib/spack/spack/package_base.py @@ -989,16 +989,17 @@ def detect_dev_src_change(self) -> bool: assert dev_path_var and record, "dev_path variant and record must be present" return fsys.recursive_mtime_greater_than(dev_path_var.value, record.installation_time) - def version_or_package_attr(self, attr, version, default=None): + @classmethod + def version_or_package_attr(cls, attr, version, default=None): """ Get an attribute that could be on the version or package with preference to the version """ - version_attrs = self.versions.get(version) + version_attrs = cls.versions.get(version) if version_attrs and attr in version_attrs: return version_attrs.get(attr) - value = getattr(self, attr, default) + value = getattr(cls, attr, default) if value is None: - raise PackageError(f"{attr} attribute not defined on {self.name}") + raise PackageError(f"{attr} attribute not defined on {cls.name}") return value @classmethod diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index 45d21dfa70b..75301e3b60d 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -1522,8 +1522,8 @@ def __init__(self, tests: bool = False): self.assumptions: List[Tuple["clingo.Symbol", bool]] = [] # type: ignore[name-defined] self.declared_versions: Dict[str, List[DeclaredVersion]] = collections.defaultdict(list) self.possible_versions: Dict[str, Set[GitOrStandardVersion]] = collections.defaultdict(set) - self.git_commit_versions: Dict[str, Set[GitOrStandardVersion]] = collections.defaultdict( - set + self.git_commit_versions: Dict[str, Dict[GitOrStandardVersion, str]] = ( + collections.defaultdict(dict) ) self.deprecated_versions: Dict[str, Set[GitOrStandardVersion]] = collections.defaultdict( set @@ -1597,7 +1597,8 @@ def key_fn(version): ) ) if pkg.needs_commit(declared_version.version): - self.git_commit_versions[pkg.name].add(declared_version.version) + commit = pkg.version_or_package_attr("commit", declared_version.version, "") + self.git_commit_versions[pkg.name][declared_version.version] = commit # Declare deprecated versions for this package, if any deprecated = self.deprecated_versions[pkg.name] @@ -2903,11 +2904,13 @@ def virtual_providers(self): def define_version_constraints(self): """Define what version_satisfies(...) means in ASP logic.""" - # TODO(psakiev) could probably consolidate with loop below for pkg_name, versions in sorted(self.possible_versions.items()): for v in versions: if v in self.git_commit_versions[pkg_name]: + sha = self.git_commit_versions[pkg_name].get(v) self.gen.fact(fn.pkg_fact(pkg_name, fn.version_needs_commit(v))) + if sha: + self.gen.fact(fn.pkg_fact(pkg_name, fn.version_has_commit(v, sha))) self.gen.newline() for pkg_name, versions in sorted(self.version_constraints): @@ -4226,7 +4229,7 @@ def _specs_with_commits(spec): if not spec.version.commit_sha: # TODO(psakiev) this will be a failure when commit look up is automated return - if not "commit" in spec.variants: + if "commit" not in spec.variants: spec.variants["commit"] = vt.SingleValuedVariant("commit", spec.version.commit_sha) diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp index 775df26bc5e..0906a7e4cc4 100644 --- a/lib/spack/spack/solver/concretize.lp +++ b/lib/spack/spack/solver/concretize.lp @@ -257,8 +257,6 @@ pkg_fact(Package, version_declared(Version)) :- pkg_fact(Package, version_declar { attr("version", node(ID, Package), Version) : pkg_fact(Package, version_declared(Version)) } :- attr("node", node(ID, Package)). -% TODO(psakiev) need rule if spec has commit version must need commit - % A virtual package may or may not have a version, but never has more than one error(100, "Cannot select a single version for virtual '{0}'", Virtual) :- attr("virtual_node", node(ID, Virtual)), @@ -341,6 +339,11 @@ error(10, "Cannot use commit variant with '{0}@{1}'", Package, Version) not pkg_fact(Package, version_needs_commit(Version)), attr("variant_value", node(ID, Package), "commit", _). +% Versions with commits require a matching commit variant + :- attr("version", node(ID, Packate), Version), + pkg_fact(Package, version_has_commit(Version, Sha)), + not attr("variant_value", node(ID, Package), "commit", Sha). + #defined version_satisfies/3. #defined deprecated_versions_not_allowed/0. #defined deprecated_version/2. diff --git a/lib/spack/spack/test/concretization/core.py b/lib/spack/spack/test/concretization/core.py index 4f695ab307a..dab7a332162 100644 --- a/lib/spack/spack/test/concretization/core.py +++ b/lib/spack/spack/test/concretization/core.py @@ -2732,7 +2732,7 @@ def test_correct_external_is_selected_from_packages_yaml(self, mutable_config): def test_git_based_version_must_exist_to_use_ref(self): # gmake should fail, only has sha256 with pytest.raises(spack.error.UnsatisfiableSpecError) as e: - s = spack.concretize.concretize_one(f"gmake commit={'a' * 40}") + spack.concretize.concretize_one(f"gmake commit={'a' * 40}") assert "Cannot use commit variant with" in e.value.message def test_commit_variant_in_absence_of_version_selects_max_infinity_version(self): diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py index 863cb1c5249..6c80be5f754 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -197,6 +197,11 @@ def latest_commit(): # Get name of default branch (differs by git version) main = git("rev-parse", "--abbrev-ref", "HEAD", output=str, error=str).strip() + if main != "main": + # assure the default branch name is consistent for tests + git("branch", "-m", "main") + main = git("rev-parse", "--abbrev-ref", "HEAD", output=str, error=str).strip() + assert "main" == main # Tag second commit as v1.0 write_file(filename, "[1, 0]") diff --git a/lib/spack/spack/test/packages.py b/lib/spack/spack/test/packages.py index 336b281f30d..019cfd8e349 100644 --- a/lib/spack/spack/test/packages.py +++ b/lib/spack/spack/test/packages.py @@ -347,3 +347,8 @@ def test_phil_package_condtional_variants_may_depend_on_commit(mock_packages, co conditional_variant = spec["git-ref-package"].variants.get("surgical", None) assert conditional_variant assert conditional_variant.value + + +def test_phil_commit_variant_finds_matches_for_commit_versions(mock_packages, config): + spec = spack.concretize.concretize_one(Spec(f"git-ref-commit-dep commit={'c' * 40}")) + assert spec.satisfies("^git-ref-package@stable") diff --git a/var/spack/repos/builtin.mock/packages/git-ref-package/package.py b/var/spack/repos/builtin.mock/packages/git-ref-package/package.py index c922f917c22..6f33571d0b3 100644 --- a/var/spack/repos/builtin.mock/packages/git-ref-package/package.py +++ b/var/spack/repos/builtin.mock/packages/git-ref-package/package.py @@ -16,6 +16,7 @@ class GitRefPackage(AutotoolsPackage): version("develop", branch="develop") version("main", branch="main") + version("stable", tag="stable", commit="c" * 40) version("3.0.1", tag="v3.0.1") version("2.1.6", sha256="a5d504c0d52e2e2721e7e7d86988dec2e290d723ced2307145dedd06aeb6fef2") version("2.1.5", sha256="3f6576971397b379d4205ae5451ff5a68edf6c103b2f03c4188ed7075fbb5f04")