From 128bf5c52dd18bef02eac5971954d25f136a9f9e Mon Sep 17 00:00:00 2001 From: Philip Sakievich Date: Wed, 26 Mar 2025 06:12:39 -0600 Subject: [PATCH] Review fixes --- lib/spack/spack/solver/asp.py | 10 +++-- lib/spack/spack/test/cmd/find.py | 7 ++-- lib/spack/spack/test/concretization/core.py | 42 ++++++++++++++++++++- lib/spack/spack/test/versions.py | 16 ++++---- 4 files changed, 59 insertions(+), 16 deletions(-) diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index 39cce4c2d2c..44254be15af 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -1522,7 +1522,9 @@ 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, Set[GitOrStandardVersion]] = collections.defaultdict( + set + ) self.deprecated_versions: Dict[str, Set[GitOrStandardVersion]] = collections.defaultdict( set ) @@ -3125,7 +3127,7 @@ def setup( specs = tuple(specs) # ensure compatible types to add self.gen.h1("Reusable concrete specs") - #TODO(psakiev) need fact spec has commit + # TODO(psakiev) need fact spec has commit self.define_concrete_input_specs(specs, self.pkgs) if reuse: self.gen.fact(fn.optimize_for_reuse()) @@ -4208,7 +4210,9 @@ def _specs_with_commits(spec): if not (has_commit_var or has_git_version): return # StandardVersions paired to git branches or tags and GitVersions - assert spec.package.needs_commit(spec.version), f"{spec.name}@{spec.version} can not have a commit variant" + assert spec.package.needs_commit( + spec.version + ), f"{spec.name}@{spec.version} can not have a commit variant" # Specs with commit variants # - variant value satsifies commit regex diff --git a/lib/spack/spack/test/cmd/find.py b/lib/spack/spack/test/cmd/find.py index 128f6ca9e56..502e7647e6f 100644 --- a/lib/spack/spack/test/cmd/find.py +++ b/lib/spack/spack/test/cmd/find.py @@ -5,16 +5,17 @@ import argparse import json import os +import pathlib import sys from textwrap import dedent -import pathlib import pytest import spack.cmd as cmd import spack.cmd.find import spack.concretize import spack.environment as ev +import spack.package_base import spack.repo import spack.store import spack.user_environment as uenv @@ -621,9 +622,9 @@ def test_phil_add_find_based_on_commit_sha(mock_git_version_info, monkeypatch): file_url = pathlib.Path(repo_path).as_uri() monkeypatch.setattr(spack.package_base.PackageBase, "git", file_url, raising=False) - + env("create", "test") - with ev.read("test") as e: + with ev.read("test"): install("--fake", "--add", f"git-test-commit commit={commits[0]}") output = find(f"commit={commits[0]}") diff --git a/lib/spack/spack/test/concretization/core.py b/lib/spack/spack/test/concretization/core.py index 6592fb1a46d..541f07f824c 100644 --- a/lib/spack/spack/test/concretization/core.py +++ b/lib/spack/spack/test/concretization/core.py @@ -3,10 +3,10 @@ # SPDX-License-Identifier: (Apache-2.0 OR MIT) import copy import os +import pathlib import sys import jinja2 -import pathlib import pytest import archspec.cpu @@ -23,6 +23,7 @@ import spack.detection import spack.error import spack.hash_types as ht +import spack.package_base import spack.paths import spack.platforms import spack.platforms.test @@ -2732,7 +2733,7 @@ def test_phil_add_git_based_version_must_exist_to_use_ref(self): s = spack.concretize.concretize_one(f"git-ref-package commit={'a' * 40}") assert s.satisfies("@main") - #gmake should fail, only has sha256 + # gmake should fail, only has sha256 with pytest.raises(spack.error.UnsatisfiableSpecError) as e: s = spack.concretize.concretize_one(f"gmake commit={'a' * 40}") assert "Cannot use commit variant with" in e.value.message @@ -3284,6 +3285,43 @@ def test_spec_unification(unify, mutable_config, mock_packages): _ = spack.cmd.parse_specs([a_restricted, b], concretize=True) +@pytest.mark.usefixtures("mutable_config", "mock_packages", "do_not_check_runtimes_on_reuse") +@pytest.mark.parametrize( + "spec_str, error_type", + [ + # TODO write actual Exceptions for these to give good error messages + (f"git-ref-package@main commit={'a' * 40}", None), + (f"git-ref-package@main commit={'a' * 39}", AssertionError), + (f"git-ref-package@2.1.6 commit={'a' * 40}", spack.error.UnsatisfiableSpecError), + (f"git-ref-package@git.2.1.6=2.1.6 commit={'a' * 40}", None), + # TODO(psakiev): need to monkeypatch git so this case doesn't query the web + # (f"git-ref-package@git.2.1.6 commit={'a' * 40}", None), + ], +) +def test_spec_containing_commit_variant(spec_str, error_type): + spec = spack.spec.Spec(spec_str) + if error_type is None: + spack.concretize.concretize_one(spec) + else: + with pytest.raises(error_type): + spack.concretize.concretize_one(spec) + + +@pytest.mark.usefixtures("mutable_config", "mock_packages", "do_not_check_runtimes_on_reuse") +@pytest.mark.parametrize("version_str", [f"git.{'a' * 40}=main", "git.2.1.5=main"]) +def test_relationship_git_versions_and_commit_variant(version_str): + """ + Confirm that GitVersions auto assign and poopulate the commit variant correctly + """ + # This should be a short lived test and can be deleted when we remove GitVersions + spec = spack.spec.Spec(f"git-ref-package@{version_str}") + spec = spack.concretize.concretize_one(spec) + if spec.version.commit_sha: + assert spec.version.commit_sha == spec.variants["commit"].value + else: + assert "commit" not in spec.variants + + def test_concretization_cache_roundtrip(use_concretization_cache, monkeypatch, mutable_config): """Tests whether we can write the results of a clingo solve to the cache and load the same spec request from the cache to produce identical specs""" diff --git a/lib/spack/spack/test/versions.py b/lib/spack/spack/test/versions.py index 2d8c56dc6e2..8689763f5b9 100644 --- a/lib/spack/spack/test/versions.py +++ b/lib/spack/spack/test/versions.py @@ -812,27 +812,27 @@ def test_version_list_with_range_and_concrete_version_is_not_concrete(): @pytest.mark.parametrize( - "pre_equal, post_equal", + "git_ref, std_version", (("foo", "develop"), ("a" * 40, "develop"), ("a" * 40, None), ("v1.2.0", "1.2.0")), ) -def test_git_versions_store_ref_requests(pre_equal, post_equal): +def test_git_versions_store_ref_requests(git_ref, std_version): """ User requested ref's should be known on creation Commit and standard version may not be known until concretization To be concrete a GitVersion must have a commit and standard version """ - if post_equal: - vstring = f"git.{pre_equal}={post_equal}" + if std_version: + vstring = f"git.{git_ref}={std_version}" else: - vstring = pre_equal + vstring = git_ref v = Version(vstring) assert isinstance(v, GitVersion) - assert v.ref == pre_equal + assert v.ref == git_ref - if post_equal: - assert v.std_version == Version(post_equal) + if std_version: + assert v.std_version == Version(std_version) if v.is_commit: assert v.ref == v.commit_sha