Review fixes

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

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]}")

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"""

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:
+    if std_version:
-        vstring = f"git.{pre_equal}={post_equal}"
+        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:
+    if std_version:
-        assert v.std_version == Version(post_equal)
+        assert v.std_version == Version(std_version)
 
     if v.is_commit:
         assert v.ref == v.commit_sha