spack solve: respect unify:false config

Signed-off-by: Gregory Becker <becker33@llnl.gov>

lib/spack/spack/ci/common.py

@@ -616,7 +616,7 @@ def generate_ir(self):
             # Reindex script
             {
                 "reindex-job": {
-                    "script:": ["spack buildcache update-index --keys {index_target_mirror}"]
+                    "script:": ["spack -d buildcache update-index --keys {index_target_mirror}"]
                 }
             },
             # Cleanup script

lib/spack/spack/cmd/solve.py

@@ -136,20 +136,7 @@ def solve(parser, args):
     setup_only = set(show) == {"asp"}
     unify = spack.config.get("concretizer:unify")
     allow_deprecated = spack.config.get("config:deprecated", False)
-    if unify != "when_possible":
-        # set up solver parameters
-        # Note: reuse and other concretizer prefs are passed as configuration
-        result = solver.solve(
-            specs,
-            out=output,
-            timers=args.timers,
-            stats=args.stats,
-            setup_only=setup_only,
-            allow_deprecated=allow_deprecated,
-        )
-        if not setup_only:
-            _process_result(result, show, required_format, kwargs)
-    else:
+    if unify == "when_possible":
         for idx, result in enumerate(
             solver.solve_in_rounds(
                 specs,
@@ -166,3 +153,29 @@ def solve(parser, args):
                 print("% END ROUND {0}\n".format(idx))
             if not setup_only:
                 _process_result(result, show, required_format, kwargs)
+    elif unify:
+        # set up solver parameters
+        # Note: reuse and other concretizer prefs are passed as configuration
+        result = solver.solve(
+            specs,
+            out=output,
+            timers=args.timers,
+            stats=args.stats,
+            setup_only=setup_only,
+            allow_deprecated=allow_deprecated,
+        )
+        if not setup_only:
+            _process_result(result, show, required_format, kwargs)
+    else:
+        for spec in specs:
+            print("SOLVING SPEC:", spec)
+            result = solver.solve(
+                [spec],
+                out=output,
+                timers=args.timers,
+                stats=args.stats,
+                setup_only=setup_only,
+                allow_deprecated=allow_deprecated,
+            )
+            if not setup_only:
+                _process_result(result, show, required_format, kwargs)

lib/spack/spack/fetch_strategy.py

@@ -1591,15 +1591,13 @@ def for_package_version(pkg, version=None):
         version = pkg.version
 
     # if it's a commit, we must use a GitFetchStrategy
-    commit_sha = pkg.spec.variants.get("commit", None)
-    if isinstance(version, spack.version.GitVersion) or commit_sha:
+    if isinstance(version, spack.version.GitVersion):
         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
-        if isinstance(version, spack.version.GitVersion):
-            version.attach_lookup(spack.version.git_ref_lookup.GitRefLookup(pkg.name))
+        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
@@ -1607,27 +1605,16 @@ 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}
 
-        # 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)
+        kwargs["submodules"] = getattr(pkg, "submodules", False)
 
         # if the ref_version is a known version from the package, use that version's
-        # 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)
+        # 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"])
 
         fetcher = GitFetchStrategy(**kwargs)
         return fetcher

lib/spack/spack/package_base.py

@@ -989,42 +989,6 @@ 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

@@ -1522,9 +1522,6 @@ 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
         )
@@ -1597,11 +1594,6 @@ 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):
@@ -2696,8 +2688,6 @@ 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"]:
@@ -2905,22 +2895,12 @@ 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):
@@ -3184,10 +3164,6 @@ 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)
@@ -3197,7 +3173,6 @@ 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")
@@ -4165,10 +4140,6 @@ 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
 
@@ -4209,29 +4180,6 @@ 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

@@ -333,37 +333,9 @@ 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.
-
-% rule that says if a spec has a commit choose the max version that can accept a commit
-
-% rule that says if a constraint is on a commit and a version matches that commit then the constraint is on the version
-
 #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

@@ -2866,7 +2866,7 @@ def _validate_version(self):
             v.ref_version
         except vn.VersionLookupError:
             before = self.cformat("{name}{@version}{/hash:7}")
-            v.std_version = vn.StandardVersion.from_string("develop")
+            v._ref_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}')}."
@@ -4428,7 +4428,7 @@ def attach_git_version_lookup(self):
         if not self.name:
             return
         for v in self.versions:
-            if isinstance(v, vn.GitVersion) and v.std_version is None:
+            if isinstance(v, vn.GitVersion) and v._ref_version is None:
                 v.attach_lookup(spack.version.git_ref_lookup.GitRefLookup(self.fullname))
 
 
@@ -4615,7 +4615,7 @@ def substitute_abstract_variants(spec: Spec):
     # in $spack/lib/spack/spack/spec_list.py
     unknown = []
     for name, v in spec.variants.items():
-        if name in ("dev_path", "commit"):
+        if name == "dev_path":
             spec.variants.substitute(vt.SingleValuedVariant(name, v._original_value))
             continue
         elif name in vt.reserved_names:

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

@@ -5,7 +5,6 @@
 import argparse
 import json
 import os
-import pathlib
 import sys
 from textwrap import dedent
 
@@ -15,7 +14,6 @@
 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
@@ -614,18 +612,3 @@ 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_phil_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

@@ -3,7 +3,6 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 import copy
 import os
-import pathlib
 import sys
 
 import jinja2
@@ -23,7 +22,6 @@
 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
@@ -2729,17 +2727,6 @@ def test_correct_external_is_selected_from_packages_yaml(self, mutable_config):
         assert s.satisfies("%clang")
         assert s.prefix == "/tmp/prefix2"
 
-    def test_phil_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.mark.skip("not supporting on this branch")
-    def test_phil_commit_variant_in_absence_of_version_selects_max_infinity_version(self):
-        spec = spack.concretize.concretize_one(f"git-ref-package commit={'a' * 40}")
-        assert spec.satisfies("@develop")
-
 
 @pytest.fixture()
 def duplicates_test_repository():
@@ -3269,68 +3256,6 @@ 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),
-        (f"git-ref-package@git.{'a' * 40}=2.1.6 commit={'a' * 40}", None),
-    ],
-)
-def test_phil_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_phil_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_phil_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/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.std_version representation
+    The repo consists of a single file, in which the GitVersion._ref_version representation
     of each commit is expressed as a string.
 
     Important attributes of the repo for test coverage are: multiple branches,
@@ -197,11 +197,6 @@ 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,7 +4,6 @@
 
 import copy
 import os
-import pathlib
 import shutil
 
 import pytest
@@ -20,7 +19,6 @@
 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"
@@ -431,19 +429,3 @@ 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

@@ -333,28 +333,3 @@ def test_package_can_have_sparse_checkout_properties(mock_packages, mock_fetch,
     assert isinstance(fetcher, spack.fetch_strategy.GitFetchStrategy)
     assert hasattr(fetcher, "git_sparse_paths")
     assert fetcher.git_sparse_paths == pkg_cls.git_sparse_paths
-
-
-def test_phil_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_phil_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
-
-
-@pytest.mark.skip("not supporting on this branch")
-def test_phil_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("^git-ref-package@stable")

lib/spack/spack/test/versions.py

@@ -811,33 +811,6 @@ 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_phil_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

@@ -24,16 +24,12 @@
 reserved_names = [
     "arch",
     "architecture",
-    "branch",
-    "commit",
     "dev_path",
     "namespace",
     "operating_system",
     "os",
     "patches",
     "platform",
-    "ref",
-    "tag",
     "target",
 ]
 

lib/spack/spack/version/__init__.py

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

lib/spack/spack/version/common.py

@@ -23,12 +23,13 @@
 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 is_git_commit_sha(string) or "=" in string[1:]
+    return (
+        string.startswith("git.")
+        or len(string) == 40
+        and bool(COMMIT_VERSION.match(string))
+        or "=" in string[1:]
+    )
 
 
 class VersionError(spack.error.SpackError):

lib/spack/spack/version/version_types.py

@@ -548,19 +548,15 @@ class GitVersion(ConcreteVersion):
     sufficient.
     """
 
-    __slots__ = ["has_git_prefix", "commit_sha", "ref", "std_version", "_ref_lookup"]
+    __slots__ = ["ref", "has_git_prefix", "is_commit", "_ref_lookup", "_ref_version"]
 
     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
@@ -569,27 +565,23 @@ 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.std_version = StandardVersion(
+            self._ref_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.std_version = None
+            self._ref_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.std_version is not None:
-            return self.std_version
+        if self._ref_version is not None:
+            return self._ref_version
 
         if self.ref_lookup is None:
             raise VersionLookupError(
@@ -602,10 +594,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.std_version = StandardVersion(
+        self._ref_version = StandardVersion(
             version_string, *parse_string_components(version_string)
         )
-        return self.std_version
+        return self._ref_version
 
     def intersects(self, other: VersionType) -> bool:
         # For concrete things intersects = satisfies = equality
@@ -637,9 +629,7 @@ def satisfies(self, other: VersionType) -> bool:
         raise TypeError(f"'satisfies()' not supported for instances of {type(other)}")
 
     def __str__(self) -> str:
-        s = ""
-        if self.ref:
-            s += f"git.{self.ref}" if self.has_git_prefix else 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
@@ -661,7 +651,6 @@ 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/repos/builtin.mock/packages/git-ref-commit-dep/package.py

@@ -1,24 +0,0 @@
-# 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("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/repos/builtin.mock/packages/git-ref-package/package.py

@@ -14,10 +14,6 @@ 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")
@@ -38,12 +34,6 @@ 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/repos/builtin.mock/packages/git-test-commit/package.py

@@ -11,7 +11,6 @@ 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