Merge branch 'develop' into psakiev/f/git-version-refactor

Ensure that if a commit variant is on the spec fetch operations
use the commit

lib/spack/docs/basic_usage.rst

@@ -1222,6 +1222,23 @@ A version specifier can also be a list of ranges and specific versions,
 separated by commas.  For example, ``@1.0:1.5,=1.7.1`` matches any version
 in the range ``1.0:1.5`` and the specific version ``1.7.1``.
 
+^^^^^^^^^^^^^^^^^
+Binary Provenance
+^^^^^^^^^^^^^^^^^
+
+Spack versions are paired to attributes that determine the source code Spack
+will use to build. Checksummed assets are preferred but there are a few 
+notable exceptions such as git branches and tags i.e ``pkg@develop``.
+These versions do not naturally have source provenance because they refer to a range
+of commits (branches) or can be changed outside the spack packaging infrastructure
+(tags). Without source provenace we can not have binary provenance.
+
+Spack has a reserved variant to allow users to complete source and binary provenance
+for these cases: ``pkg@develop commit=<SHA>``.  The ``commit`` variant must be supplied
+the full 40 character commit SHA. Using a partial commit SHA or assigning
+the ``commit`` variant to a version that is not using a branch or tag reference will
+lead to an error during concretization.
+
 ^^^^^^^^^^^^
 Git versions
 ^^^^^^^^^^^^

lib/spack/docs/packaging_guide.rst

@@ -1267,7 +1267,11 @@ Git fetching supports the following parameters to ``version``:
   If paths provided are directories then all the subdirectories and associated files
   will also be cloned.
 
-Only one of ``tag``, ``branch``, or ``commit`` can be used at a time.
+``tag`` and ``branch`` should not be combined in the version parameters. We strongly
+recommend that all ``tag`` entries be paired with ``commit``. Providing the full 
+``commit`` SHA hash allows for Spack to preserve binary provenance for all binaries. 
+This is due to the fact that git tags and branches are mutable references to commits, 
+but git commits are guaranteed to be unique points in the git history.
 
 The destination directory for the clone is the standard stage source path.
 

lib/spack/spack/fetch_strategy.py

@@ -1710,13 +1710,15 @@ def for_package_version(pkg, version=None):
         version = pkg.version
 
     # if it's a commit, we must use a GitFetchStrategy
-    if isinstance(version, spack.version.GitVersion):
+    commit_sha = pkg.spec.variants.get("commit", None)
+    if isinstance(version, spack.version.GitVersion) or commit_sha:
         if not hasattr(pkg, "git"):
             raise spack.error.FetchError(
                 f"Cannot fetch git version for {pkg.name}. Package has no 'git' attribute"
             )
         # Populate the version with comparisons to other commits
-        version.attach_lookup(spack.version.git_ref_lookup.GitRefLookup(pkg.name))
+        if isinstance(version, spack.version.GitVersion):
+            version.attach_lookup(spack.version.git_ref_lookup.GitRefLookup(pkg.name))
 
         # For GitVersion, we have no way to determine whether a ref is a branch or tag
         # Fortunately, we handle branches and tags identically, except tags are
@@ -1724,16 +1726,27 @@ def for_package_version(pkg, version=None):
         # We call all non-commit refs tags in this context, at the cost of a slight
         # performance hit for branches on older versions of git.
         # Branches cannot be cached, so we tell the fetcher not to cache tags/branches
-        ref_type = "commit" if version.is_commit else "tag"
-        kwargs = {"git": pkg.git, ref_type: version.ref, "no_cache": True}
 
-        kwargs["submodules"] = getattr(pkg, "submodules", False)
+        # TODO(psakiev) eventually we should  only need to clone based on the commit
+        ref_type = None
+        ref_value = None
+        if commit_sha:
+            ref_type = "commit"
+            ref_value = commit_sha.value
+        else:
+            ref_type = "commit" if version.is_commit else "tag"
+            ref_value = version.ref
+
+        kwargs = {ref_type: ref_value, "no_cache": ref_type != "commit"}
+        kwargs["git"] = pkg.version_or_package_attr("git", version)
+        kwargs["submodules"] = pkg.version_or_package_attr("submodules", version, False)
 
         # if the ref_version is a known version from the package, use that version's
-        # submodule specifications
-        ref_version_attributes = pkg.versions.get(pkg.version.ref_version)
-        if ref_version_attributes:
-            kwargs["submodules"] = ref_version_attributes.get("submodules", kwargs["submodules"])
+        # attributes
+        ref_version = getattr(pkg.version, "ref_version", None)
+        if ref_version:
+            kwargs["git"] = pkg.version_or_package_attr("git", ref_version)
+            kwargs["submodules"] = pkg.version_or_package_attr("submodules", ref_version, False)
 
         fetcher = GitFetchStrategy(**kwargs)
         return fetcher

lib/spack/spack/package_base.py

@@ -1001,6 +1001,42 @@ 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)
 
+    @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 = cls.versions.get(version)
+        if version_attrs and attr in version_attrs:
+            return version_attrs.get(attr)
+        value = getattr(cls, attr, default)
+        if value is None:
+            raise PackageError(f"{attr} attribute not defined on {cls.name}")
+        return value
+
+    @classmethod
+    def needs_commit(cls, version) -> bool:
+        """
+        Method for checking if the package instance needs a commit sha to be found
+        """
+        if isinstance(version, GitVersion):
+            return True
+
+        ver_attrs = cls.versions.get(version)
+        if ver_attrs:
+            return bool(ver_attrs.get("commit") or ver_attrs.get("tag") or ver_attrs.get("branch"))
+
+        return False
+
+    def resolve_binary_provenance(self) -> None:
+        """
+        Method to ensure concrete spec has binary provenance.
+        Base implementation will look up git commits when appropriate.
+        Packages may override this implementation for custom implementations
+        """
+        # TODO in follow on PR adding here so SNL team can begin work ahead of spack core
+        pass
+
     def all_urls_for_version(self, version: StandardVersion) -> List[str]:
         """Return all URLs derived from version_urls(), url, urls, and
         list_url (if it contains a version) in a package in that order.

lib/spack/spack/solver/asp.py

@@ -1519,6 +1519,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, Dict[GitOrStandardVersion, str]] = (
+            collections.defaultdict(dict)
+        )
         self.deprecated_versions: Dict[str, Set[GitOrStandardVersion]] = collections.defaultdict(
             set
         )
@@ -1592,6 +1595,11 @@ def key_fn(version):
                 )
             )
 
+        for v in self.possible_versions[pkg.name]:
+            if pkg.needs_commit(v):
+                commit = pkg.version_or_package_attr("commit", v, "")
+                self.git_commit_versions[pkg.name][v] = commit
+
         # Declare deprecated versions for this package, if any
         deprecated = self.deprecated_versions[pkg.name]
         for v in sorted(deprecated):
@@ -2689,6 +2697,8 @@ def define_package_versions_and_validate_preferences(
             if pkg_name not in packages_yaml or "version" not in packages_yaml[pkg_name]:
                 continue
 
+            # TODO(psakiev) Need facts about versions
+            # - requires_commit (associated with tag or branch)
             version_defs: List[GitOrStandardVersion] = []
 
             for vstr in packages_yaml[pkg_name]["version"]:
@@ -2880,12 +2890,22 @@ def virtual_providers(self):
 
     def define_version_constraints(self):
         """Define what version_satisfies(...) means in ASP logic."""
+
+        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)
+                    if sha:
+                        self.gen.fact(fn.pkg_fact(pkg_name, fn.version_has_commit(v, sha)))
+                    else:
+                        self.gen.fact(fn.pkg_fact(pkg_name, fn.version_needs_commit(v)))
+        self.gen.newline()
+
         for pkg_name, versions in sorted(self.version_constraints):
             # generate facts for each package constraint and the version
             # that satisfies it
             for v in sorted(v for v in self.possible_versions[pkg_name] if v.satisfies(versions)):
                 self.gen.fact(fn.pkg_fact(pkg_name, fn.version_satisfies(versions, v)))
-
             self.gen.newline()
 
     def collect_virtual_constraints(self):
@@ -3180,6 +3200,10 @@ def setup(
             allow_deprecated=allow_deprecated, require_checksum=checksummed
         )
 
+        self.gen.h1("Infinity Versions")
+        for i, v in enumerate(spack.version.infinity_versions):
+            self.gen.fact(fn.infinity_version(v, i))
+
         self.gen.h1("Package Constraints")
         for pkg in sorted(self.pkgs):
             self.gen.h2("Package rules: %s" % pkg)
@@ -3188,6 +3212,7 @@ def setup(
 
         self.gen.h1("Special variants")
         self.define_auto_variant("dev_path", multi=False)
+        self.define_auto_variant("commit", multi=False)
         self.define_auto_variant("patches", multi=True)
 
         self.gen.h1("Develop specs")
@@ -4146,6 +4171,10 @@ def build_specs(self, function_tuples):
                         spack.version.git_ref_lookup.GitRefLookup(spec.fullname)
                     )
 
+        # check for commits must happen after all version adaptations are complete
+        for s in self._specs.values():
+            _specs_with_commits(s)
+
         specs = self.execute_explicit_splices()
         return specs
 
@@ -4186,6 +4215,29 @@ def execute_explicit_splices(self):
         return specs
 
 
+def _specs_with_commits(spec):
+    if not spec.package.needs_commit(spec.version):
+        return
+
+    # check integrity of specified commit shas
+    if "commit" in spec.variants:
+        invalid_commit_msg = (
+            f"Internal Error: {spec.name}'s assigned commit {spec.variants['commit'].value}"
+            " does not meet commit syntax requirements."
+        )
+        assert vn.is_git_commit_sha(spec.variants["commit"].value), invalid_commit_msg
+
+    spec.package.resolve_binary_provenance()
+    # TODO(psakiev) assert commit is associated with ref
+
+    if isinstance(spec.version, spack.version.GitVersion):
+        if not spec.version.commit_sha:
+            # TODO(psakiev) this will be a failure when commit look up is automated
+            return
+        if "commit" not in spec.variants:
+            spec.variants["commit"] = vt.SingleValuedVariant("commit", spec.version.commit_sha)
+
+
 def _inject_patches_variant(root: spack.spec.Spec) -> None:
     # This dictionary will store object IDs rather than Specs as keys
     # since the Spec __hash__ will change as patches are added to them

lib/spack/spack/solver/concretize.lp

@@ -343,9 +343,33 @@ attr("node_version_satisfies", node(ID, Package), Constraint)
   :- attr("version", node(ID, Package), Version),
      pkg_fact(Package, version_satisfies(Constraint, Version)).
 
+% if a version needs a commit or has one it can use the commit variant
+can_accept_commit(Package, Version) :- pkg_fact(Package, version_needs_commit(Version)).
+can_accept_commit(Package, Version) :- pkg_fact(Package, version_has_commit(Version, _)).
+
+% Specs with a commit variant can't use versions that don't need commits
+error(10, "Cannot use commit variant with '{0}@={1}'", Package, Version)
+  :- attr("version", node(ID, Package), Version),
+     not can_accept_commit(Package, Version),
+     attr("variant_value", node(ID, Package), "commit", _).
+
+error(10, "Commit '{0}' must match package.py value '{1}' for '{2}@={3}'", Vsha, Psha, Package, Version)
+  :- attr("version", node(ID, Package), Version),
+     attr("variant_value", node(ID, Package), "commit", Vsha),
+     pkg_fact(Package, version_has_commit(Version, Psha)),
+     Vsha != Psha.
+
+% need a rule for above, can't select a version that needs a commit if variant matches a version that has a commit
+  :- attr("version", node(ID, Package), VersionA),
+     attr("variant_value", node(ID, Package), "commit", Vsha),
+     pkg_fact(Package, version_has_commit(VersionB, Psha)),
+     Vsha == Psha,
+     VersionA != VersionB.
+
 #defined version_satisfies/3.
 #defined deprecated_versions_not_allowed/0.
 #defined deprecated_version/2.
+#defined can_accept_commit/2.
 
 %-----------------------------------------------------------------------------
 % Spec conditions and imposed constraints

lib/spack/spack/spec.py

@@ -2884,7 +2884,7 @@ def _validate_version(self):
             v.ref_version
         except vn.VersionLookupError:
             before = self.cformat("{name}{@version}{/hash:7}")
-            v._ref_version = vn.StandardVersion.from_string("develop")
+            v.std_version = vn.StandardVersion.from_string("develop")
             tty.debug(
                 f"the git sha of {before} could not be resolved to spack version; "
                 f"it has been replaced by {self.cformat('{name}{@version}{/hash:7}')}."
@@ -4483,7 +4483,7 @@ def attach_git_version_lookup(self):
         if not self.name:
             return
         for v in self.versions:
-            if isinstance(v, vn.GitVersion) and v._ref_version is None:
+            if isinstance(v, vn.GitVersion) and v.std_version is None:
                 v.attach_lookup(spack.version.git_ref_lookup.GitRefLookup(self.fullname))
 
     def original_spec_format(self) -> int:
@@ -4679,7 +4679,7 @@ def substitute_abstract_variants(spec: Spec):
         if v.concrete and v.type == vt.VariantType.MULTI:
             continue
 
-        if name == "dev_path":
+        if name in ("dev_path", "commit"):
             v.type = vt.VariantType.SINGLE
             v.concrete = True
             continue

lib/spack/spack/test/cmd/find.py

@@ -5,6 +5,7 @@
 import argparse
 import json
 import os
+import pathlib
 import sys
 from textwrap import dedent
 
@@ -14,6 +15,7 @@
 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
@@ -607,3 +609,18 @@ def _nresults(_qresult):
         assert _nresults(_query(e, "--tag=tag0")) == (1, 0)
         assert _nresults(_query(e, "--tag=tag1")) == (1, 1)
         assert _nresults(_query(e, "--tag=tag2")) == (0, 1)
+
+
+@pytest.mark.usefixtures("install_mockery", "mock_fetch", "mutable_mock_env_path")
+def test_find_based_on_commit_sha(mock_git_version_info, monkeypatch):
+    repo_path, filename, commits = mock_git_version_info
+    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"):
+        install("--fake", "--add", f"git-test-commit commit={commits[0]}")
+
+        output = find(f"commit={commits[0]}")
+        assert "git-test-commit" in output

lib/spack/spack/test/concretization/core.py

@@ -2,6 +2,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 import os
+import pathlib
 import platform
 import sys
 
@@ -21,6 +22,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
@@ -2583,6 +2585,12 @@ def test_correct_external_is_selected_from_packages_yaml(self, mutable_config):
         assert s.satisfies("~opt")
         assert s.prefix == "/tmp/prefix2"
 
+    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:
+            spack.concretize.concretize_one(f"gmake commit={'a' * 40}")
+            assert "Cannot use commit variant with" in e.value.message
+
 
 @pytest.fixture()
 def duplicates_test_repository():
@@ -3115,6 +3123,107 @@ 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",
+    [
+        (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),
+        (f"git-ref-package@git.{'a' * 40}=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(
+    "spec_str, error_type",
+    [
+        (f"git-test-commit@git.main commit={'a' * 40}", None),
+        (f"git-test-commit@git.v1.0 commit={'a' * 40}", None),
+        ("git-test-commit@{sha} commit={sha}", None),
+        ("git-test-commit@{sha} commit=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", None),
+    ],
+)
+def test_spec_with_commit_interacts_with_lookup(
+    mock_git_version_info, monkeypatch, spec_str, error_type
+):
+    # This test will be short lived. Technically we could do further checks with a Lookup
+    # but skipping impl since we are going to deprecate
+    repo_path, filename, commits = mock_git_version_info
+    file_url = pathlib.Path(repo_path).as_uri()
+    monkeypatch.setattr(spack.package_base.PackageBase, "git", file_url, raising=False)
+    spec = spack.spec.Spec(spec_str.format(sha=commits[-1]))
+    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 populates 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
+
+
+@pytest.mark.usefixtures("install_mockery", "do_not_check_runtimes_on_reuse")
+def test_abstract_commit_spec_reuse():
+    commit = "abcd" * 10
+    spec_str_1 = f"git-ref-package@develop commit={commit}"
+    spec_str_2 = f"git-ref-package commit={commit}"
+    spec1 = spack.concretize.concretize_one(spack.spec.Spec(spec_str_1))
+    PackageInstaller([spec1.package], fake=True, explicit=True).install()
+
+    with spack.config.override("concretizer:reuse", True):
+        spec2 = spack.spec.Spec(spec_str_2)
+        spec2 = spack.concretize.concretize_one(spec2)
+        assert spec1.dag_hash() == spec2.dag_hash()
+
+
+@pytest.mark.usefixtures("install_mockery", "do_not_check_runtimes_on_reuse")
+@pytest.mark.parametrize(
+    "installed_commit, incoming_commit, reusable",
+    [("a" * 40, "b" * 40, False), (None, "b" * 40, False), ("a" * 40, None, True)],
+)
+def test_commit_variant_can_be_reused(installed_commit, incoming_commit, reusable):
+    # install a non-default variant to test if reuse picks it
+    if installed_commit:
+        spec_str_1 = f"git-ref-package@develop commit={installed_commit} ~opt"
+    else:
+        spec_str_1 = "git-ref-package@develop ~opt"
+
+    if incoming_commit:
+        spec_str_2 = f"git-ref-package@develop commit={incoming_commit}"
+    else:
+        spec_str_2 = "git-ref-package@develop"
+
+    spec1 = spack.concretize.concretize_one(spack.spec.Spec(spec_str_1))
+    PackageInstaller([spec1.package], fake=True, explicit=True).install()
+
+    with spack.config.override("concretizer:reuse", True):
+        spec2 = spack.spec.Spec(spec_str_2)
+        spec2 = spack.concretize.concretize_one(spec2)
+        assert (spec1.dag_hash() == spec2.dag_hash()) == reusable
+
+
 def test_concretization_cache_roundtrip(
     mock_packages, use_concretization_cache, monkeypatch, mutable_config
 ):

lib/spack/spack/test/conftest.py

@@ -154,7 +154,7 @@ def mock_git_version_info(git, tmpdir, override_git_repos_cache_path):
        o second commit (v1.0)
        o first commit
 
-    The repo consists of a single file, in which the GitVersion._ref_version representation
+    The repo consists of a single file, in which the GitVersion.std_version representation
     of each commit is expressed as a string.
 
     Important attributes of the repo for test coverage are: multiple branches,
@@ -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]")

lib/spack/spack/test/git_fetch.py

@@ -4,6 +4,7 @@
 
 import copy
 import os
+import pathlib
 import shutil
 
 import pytest
@@ -19,6 +20,7 @@
 from spack.fetch_strategy import GitFetchStrategy
 from spack.spec import Spec
 from spack.stage import Stage
+from spack.variant import SingleValuedVariant
 from spack.version import Version
 
 _mock_transport_error = "Mock HTTP transport error"
@@ -429,3 +431,19 @@ def test_git_sparse_paths_partial_clone(
 
         # fixture file is in the sparse-path expansion tree
         assert os.path.isfile(t.file)
+
+
+@pytest.mark.disable_clean_stage_check
+def test_commit_variant_clone(
+    git, default_mock_concretization, mutable_mock_repo, mock_git_version_info, monkeypatch
+):
+
+    repo_path, filename, commits = mock_git_version_info
+    test_commit = commits[-1]
+    s = default_mock_concretization("git-test")
+    args = {"git": pathlib.Path(repo_path).as_uri()}
+    monkeypatch.setitem(s.package.versions, Version("git"), args)
+    s.variants["commit"] = SingleValuedVariant("commit", test_commit)
+    s.package.do_stage()
+    with working_dir(s.package.stage.source_path):
+        assert git("rev-parse", "HEAD", output=str, error=str).strip() == test_commit

lib/spack/spack/test/packages.py

@@ -339,6 +339,30 @@ def test_package_can_have_sparse_checkout_properties(mock_packages, mock_fetch,
     assert fetcher.git_sparse_paths == pkg_cls.git_sparse_paths
 
 
+def test_package_can_depend_on_commit_of_dependency(mock_packages, config):
+    spec = spack.concretize.concretize_one(Spec("git-ref-commit-dep@1.0.0"))
+    assert spec.satisfies(f"^git-ref-package commit={'a' * 40}")
+    assert "surgical" not in spec["git-ref-package"].variants
+
+
+def test_package_condtional_variants_may_depend_on_commit(mock_packages, config):
+    spec = spack.concretize.concretize_one(Spec("git-ref-commit-dep@develop"))
+    assert spec.satisfies(f"^git-ref-package commit={'b' * 40}")
+    conditional_variant = spec["git-ref-package"].variants.get("surgical", None)
+    assert conditional_variant
+    assert conditional_variant.value
+
+
+def test_commit_variant_finds_matches_for_commit_versions(mock_packages, config):
+    """
+    test conditional dependence on `when='commit=<sha>'`
+    git-ref-commit-dep variant commit-selector depends on a specific commit of git-ref-package
+    that commit is associated with the stable version of git-ref-package
+    """
+    spec = spack.concretize.concretize_one(Spec("git-ref-commit-dep+commit-selector"))
+    assert spec.satisfies(f"^git-ref-package commit={'c' * 40}")
+
+
 def test_pkg_name_can_only_be_derived_when_package_module():
     """When the module prefix is not spack_repo (or legacy spack.pkg) we cannot derive
     a package name."""

lib/spack/spack/test/versions.py

@@ -811,6 +811,33 @@ def test_version_list_with_range_and_concrete_version_is_not_concrete():
     assert not v.concrete
 
 
+@pytest.mark.parametrize(
+    "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(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 std_version:
+        vstring = f"git.{git_ref}={std_version}"
+    else:
+        vstring = git_ref
+
+    v = Version(vstring)
+
+    assert isinstance(v, GitVersion)
+    assert v.ref == git_ref
+
+    if std_version:
+        assert v.std_version == Version(std_version)
+
+    if v.is_commit:
+        assert v.ref == v.commit_sha
+
+
 @pytest.mark.parametrize(
     "vstring, eq_vstring, is_commit",
     (

lib/spack/spack/variant.py

@@ -23,12 +23,16 @@
 RESERVED_NAMES = {
     "arch",
     "architecture",
+    "branch",
+    "commit",
     "dev_path",
     "namespace",
     "operating_system",
     "os",
     "patches",
     "platform",
+    "ref",
+    "tag",
     "target",
 }
 

lib/spack/spack/version/__init__.py

@@ -20,6 +20,7 @@
     VersionError,
     VersionLookupError,
     infinity_versions,
+    is_git_commit_sha,
     is_git_version,
 )
 from .version_types import (
@@ -58,6 +59,7 @@
     "any_version",
     "from_string",
     "infinity_versions",
+    "is_git_commit_sha",
     "is_git_version",
     "ver",
 ]

lib/spack/spack/version/common.py

@@ -23,13 +23,12 @@
 STRING_TO_PRERELEASE = {"alpha": ALPHA, "beta": BETA, "rc": RC, "final": FINAL}
 
 
+def is_git_commit_sha(string: str) -> bool:
+    return len(string) == 40 and bool(COMMIT_VERSION.match(string))
+
+
 def is_git_version(string: str) -> bool:
-    return (
-        string.startswith("git.")
-        or len(string) == 40
-        and bool(COMMIT_VERSION.match(string))
-        or "=" in string[1:]
-    )
+    return string.startswith("git.") or is_git_commit_sha(string) or "=" in string[1:]
 
 
 class VersionError(spack.error.SpackError):

lib/spack/spack/version/version_types.py

@@ -548,15 +548,19 @@ class GitVersion(ConcreteVersion):
     sufficient.
     """
 
-    __slots__ = ["ref", "has_git_prefix", "is_commit", "_ref_lookup", "_ref_version"]
+    __slots__ = ["has_git_prefix", "commit_sha", "ref", "std_version", "_ref_lookup"]
 
     def __init__(self, string: str):
+        # TODO will be required for concrete specs when commit lookup added
+        self.commit_sha: Optional[str] = None
+        self.std_version: Optional[StandardVersion] = None
+
+        # optional user supplied git ref
+        self.ref: Optional[str] = None
+
         # An object that can lookup git refs to compare them to versions
         self._ref_lookup: Optional[AbstractRefLookup] = None
 
-        # This is the effective version.
-        self._ref_version: Optional[StandardVersion]
-
         self.has_git_prefix = string.startswith("git.")
 
         # Drop `git.` prefix
@@ -565,23 +569,27 @@ def __init__(self, string: str):
         if "=" in normalized_string:
             # Store the git reference, and parse the user provided version.
             self.ref, spack_version = normalized_string.split("=")
-            self._ref_version = StandardVersion(
+            self.std_version = StandardVersion(
                 spack_version, *parse_string_components(spack_version)
             )
         else:
             # The ref_version is lazily attached after parsing, since we don't know what
             # package it applies to here.
-            self._ref_version = None
+            self.std_version = None
             self.ref = normalized_string
 
         # Used by fetcher
         self.is_commit: bool = len(self.ref) == 40 and bool(COMMIT_VERSION.match(self.ref))
 
+        # translations
+        if self.is_commit:
+            self.commit_sha = self.ref
+
     @property
     def ref_version(self) -> StandardVersion:
         # Return cached version if we have it
-        if self._ref_version is not None:
-            return self._ref_version
+        if self.std_version is not None:
+            return self.std_version
 
         if self.ref_lookup is None:
             raise VersionLookupError(
@@ -594,10 +602,10 @@ def ref_version(self) -> StandardVersion:
         # Add a -git.<distance> suffix when we're not exactly on a tag
         if distance > 0:
             version_string += f"-git.{distance}"
-        self._ref_version = StandardVersion(
+        self.std_version = StandardVersion(
             version_string, *parse_string_components(version_string)
         )
-        return self._ref_version
+        return self.std_version
 
     def intersects(self, other: VersionType) -> bool:
         # For concrete things intersects = satisfies = equality
@@ -629,7 +637,9 @@ def satisfies(self, other: VersionType) -> bool:
         raise TypeError(f"'satisfies()' not supported for instances of {type(other)}")
 
     def __str__(self) -> str:
-        s = f"git.{self.ref}" if self.has_git_prefix else self.ref
+        s = ""
+        if self.ref:
+            s += f"git.{self.ref}" if self.has_git_prefix else self.ref
         # Note: the solver actually depends on str(...) to produce the effective version.
         # So when a lookup is attached, we require the resolved version to be printed.
         # But for standalone git versions that don't have a repo attached, it would still
@@ -651,6 +661,7 @@ def __eq__(self, other: object) -> bool:
         return (
             isinstance(other, GitVersion)
             and self.ref == other.ref
+            # TODO(psakiev) this needs to chamge to commits when we turn on lookups
             and self.ref_version == other.ref_version
         )
 

var/spack/test_repos/builtin.mock/packages/git-ref-commit-dep/package.py

@@ -0,0 +1,25 @@
+# Copyright Spack Project Developers. See COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack.package import *
+
+
+class GitRefCommitDep(AutotoolsPackage):
+    """
+    tests dependency using commit
+    """
+
+    homepage = "https://github.com/dummy/dummy"
+    git = "https://github.com/dummy/dummy.git"
+    url = git
+
+    version("develop", branch="develop")
+    version("main", branch="main")
+    version("1.0.0", sha256="a5d504c0d52e2e2721e7e7d86988dec2e290d723ced2307145dedd06aeb6fef2")
+
+    variant("commit-selector", default=False, description="test grabbing a specific commit")
+
+    depends_on(f"git-ref-package commit={'a' * 40}", when="@1.0.0")
+    depends_on(f"git-ref-package commit={'b' * 40}", when="@develop")
+    depends_on(f"git-ref-package commit={'c' * 40}", when="+commit-selector")

var/spack/test_repos/builtin.mock/packages/git-ref-package/package.py

@@ -14,6 +14,10 @@ class GitRefPackage(AutotoolsPackage):
     url = "https://github.com/dummy/dummy/archive/2.0.0.tar.gz"
     git = "https://github.com/dummy/dummy.git"
 
+    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")
     version("2.1.4", sha256="a0293475e6a44a3f6c045229fe50f69dc0eebc62a42405a51f19d46a5541e77a")
@@ -34,6 +38,12 @@ class GitRefPackage(AutotoolsPackage):
     variant("opt", default=True, description="Enable optimizations")
     variant("shared", default=True, description="Build shared library")
     variant("pic", default=True, description="Enable position-independent code (PIC)")
+    variant(
+        "surgical",
+        default=True,
+        when=f"commit={'b' * 40}",
+        description="Testing conditional on commit",
+    )
 
     conflicts("+shared~pic")
 

var/spack/test_repos/builtin.mock/packages/git-test-commit/package.py

@@ -11,6 +11,7 @@ class GitTestCommit(Package):
     homepage = "http://www.git-fetch-example.com"
     # git='to-be-filled-in-by-test'
 
+    version("main", branch="main")
     version("1.0", tag="v1.0")
     version("1.1", tag="v1.1")
     version("1.2", tag="1.2")  # not a typo