GitRef Versions: Bug Fixes (#32437)

* Fix a version cmp bug in asp.py

* Fix submodule bug for git refs

* Add branch in logic for submodules

* Fix git version comparisons

main does not satisfy git.foo=main
git.foo=main does satisfy main

4 files changed, 47 insertions, 3 deletions

diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py
index 319aed95ca..ae23bacf14 100644
--- a/lib/spack/spack/fetch_strategy.py
+++ b/lib/spack/spack/fetch_strategy.py
@@ -1543,7 +1543,19 @@ def for_package_version(pkg, version):
             ref_type: version.ref,
             "no_cache": True,
         }
+
         kwargs["submodules"] = getattr(pkg, "submodules", False)
+
+        # if we have a ref_version already, and it is a version from the package
+        # we can use that version's submodule specifications
+        if pkg.version.ref_version:
+            ref_version = spack.version.Version(pkg.version.ref_version[0])
+            ref_version_attributes = pkg.versions.get(ref_version)
+            if ref_version_attributes:
+                kwargs["submodules"] = ref_version_attributes.get(
+                    "submodules", kwargs["submodules"]
+                )
+
         fetcher = GitFetchStrategy(**kwargs)
         return fetcher
 
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index aeef4044d5..8e2e8bfd95 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1516,8 +1516,10 @@ class SpackSolverSetup(object):
             # specs will be computed later
             version_preferences = packages_yaml.get(pkg_name, {}).get("version", [])
             for idx, v in enumerate(version_preferences):
+                # v can be a string so force it into an actual version for comparisons
+                ver = spack.version.Version(v)
                 self.declared_versions[pkg_name].append(
-                    DeclaredVersion(version=v, idx=idx, origin=version_provenance.packages_yaml)
+                    DeclaredVersion(version=ver, idx=idx, origin=version_provenance.packages_yaml)
                 )
 
         for spec in specs:
diff --git a/lib/spack/spack/test/spec_syntax.py b/lib/spack/spack/test/spec_syntax.py
index e58cf4214d..ec92cc877e 100644
--- a/lib/spack/spack/test/spec_syntax.py
+++ b/lib/spack/spack/test/spec_syntax.py
@@ -837,3 +837,15 @@ class TestSpecSyntax(object):
         for a, b in itertools.product(specs, repeat=2):
             # Check that we can compare without raising an error
             assert a <= b or b < a
+
+    def test_git_ref_spec_equivalences(self, mock_packages, mock_stage):
+        s1 = sp.Spec("develop-branch-version@git.{hash}=develop".format(hash="a" * 40))
+        s2 = sp.Spec("develop-branch-version@git.{hash}=develop".format(hash="b" * 40))
+        s3 = sp.Spec("develop-branch-version@git.0.2.15=develop")
+        s_no_git = sp.Spec("develop-branch-version@develop")
+
+        assert s1.satisfies(s_no_git)
+        assert s2.satisfies(s_no_git)
+        assert not s_no_git.satisfies(s1)
+        assert not s2.satisfies(s1)
+        assert not s3.satisfies(s1)
diff --git a/lib/spack/spack/version.py b/lib/spack/spack/version.py
index e67ab260ba..624d9faeae 100644
--- a/lib/spack/spack/version.py
+++ b/lib/spack/spack/version.py
@@ -599,15 +599,33 @@ class GitVersion(VersionBase):
         """A Version 'satisfies' another if it is at least as specific and has
         a common prefix.  e.g., we want gcc@4.7.3 to satisfy a request for
         gcc@4.7 so that when a user asks to build with gcc@4.7, we can find
-        a suitable compiler.
+        a suitable compiler. In the case of two GitVersions we require the ref_versions
+        to satisify one another and the versions to be an exact match.
         """
+
         self_cmp = self._cmp(other.ref_lookup)
         other_cmp = other._cmp(self.ref_lookup)
 
+        if other.is_ref:
+            # if other is a ref then satisfaction requires an exact version match
+            # i.e. the GitRef must match this.version for satisfaction
+            # this creates an asymmetric comparison:
+            #  - 'foo@main'.satisfies('foo@git.hash=main') == False
+            #  - 'foo@git.hash=main'.satisfies('foo@main') == True
+            version_match = self.version == other.version
+        elif self.is_ref:
+            # other is not a ref then it is a version base and we need to compare
+            # this.ref
+            version_match = self.ref_version == other.version
+        else:
+            # neither is a git ref.  We shouldn't ever be here, but if we are this variable
+            # is not meaningful and defaults to true
+            version_match = True
+
         # Do the final comparison
         nself = len(self_cmp)
         nother = len(other_cmp)
-        return nother <= nself and self_cmp[:nother] == other_cmp
+        return nother <= nself and self_cmp[:nother] == other_cmp and version_match
 
     def __repr__(self):
         return "GitVersion(" + repr(self.string) + ")"