Git Ref versions can be paired to defined versions in the spec (#30998)

The current use of git ref's as a version requires a search algorithm to pick the right matching version based on the tags in the git history of the package. This is less than ideal for the use case where users already know the specific version they want the git ref to be associated with. This PR makes a new version syntax [package]@[ref]=[version] to allow the users to specify the exact hash they wish to use.
2022-08-04 14:17:34 -06:00 · 2022-08-04 14:17:34 -06:00 · 3b1401f292
commit 3b1401f292
parent 19a8bb53f0
11 changed files with 180 additions and 21 deletions
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@ -101,14 +101,7 @@ def getter(node):
 ast_sym = ast_getter("symbol", "term")

 #: Order of precedence for version origins. Topmost types are preferred.
-version_origin_fields = [
-    "spec",
-    "external",
-    "packages_yaml",
-    "package_py",
-    "installed",
-]
-
+version_origin_fields = ["spec", "external", "packages_yaml", "package_py", "installed"]
 #: Look up version precedence strings by enum id
 version_origin_str = {i: name for i, name in enumerate(version_origin_fields)}

@ -511,7 +504,10 @@ def _normalize_packages_yaml(packages_yaml):
                entry["buildable"] = False

        externals = data.get("externals", [])
-        keyfn = lambda x: spack.spec.Spec(x["spec"]).name
+
+        def keyfn(x):
+            return spack.spec.Spec(x["spec"]).name
+
        for provider, specs in itertools.groupby(externals, key=keyfn):
            entry = normalized_yaml.setdefault(provider, {})
            entry.setdefault("externals", []).extend(specs)
@ -793,6 +789,30 @@ def key_fn(version):
                )
            )

+        for v in most_to_least_preferred:
+            # There are two paths for creating the ref_version in GitVersions.
+            # The first uses a lookup to supply a tag and distance as a version.
+            # The second is user specified and can be resolved as a standard version.
+            # This second option is constrained such that the user version must be known to Spack
+            if (
+                isinstance(v.version, spack.version.GitVersion)
+                and v.version.user_supplied_reference
+            ):
+                ref_version = spack.version.Version(v.version.ref_version_str)
+                self.gen.fact(fn.version_equivalent(pkg.name, v.version, ref_version))
+                # disqualify any git supplied version from user if they weren't already known
+                # versions in spack
+                if not any(ref_version == dv.version for dv in most_to_least_preferred if v != dv):
+                    msg = (
+                        "The reference version '{version}' for package '{package}' is not defined."
+                        " Either choose another reference version or define '{version}' in your"
+                        " version preferences or package.py file for {package}.".format(
+                            package=pkg.name, version=str(ref_version)
+                        )
+                    )
+
+                    raise UnsatisfiableSpecError(msg)
+
        # Declare deprecated versions for this package, if any
        deprecated = self.deprecated_versions[pkg.name]
        for v in sorted(deprecated):
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@ -76,6 +76,11 @@ version_declared(Package, Version, Weight) :- version_declared(Package, Version,
 % versions are declared w/priority -- declared with priority implies declared
 version_declared(Package, Version) :- version_declared(Package, Version, _).

+% a spec with a git hash version is equivalent to one with the same matched version
+version_satisfies(Package, Constraint, HashVersion) :- version_satisfies(Package, Constraint, EquivalentVersion),
+  version_equivalent(Package, HashVersion, EquivalentVersion).
+#defined version_equivalent/3.
+
 % If something is a package, it has only one version and that must be a
 % declared version.
 % We allow clingo to choose any version(s), and infer an error if there
--- a/lib/spack/spack/solver/display.lp
+++ b/lib/spack/spack/solver/display.lp
@ -26,6 +26,7 @@
 #show node_flag_source/2.
 #show no_flags/2.
 #show external_spec_selected/2.
+#show version_equivalent/3.

 #show build/1.

--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@ -5265,6 +5265,12 @@ def version(self):
        end = None
        if self.accept(ID):
            start = self.token.value
+            if self.accept(EQ):
+                # This is for versions that are associated with a hash
+                # i.e. @[40 char hash]=version
+                start += self.token.value
+                self.expect(VAL)
+                start += self.token.value

        if self.accept(COLON):
            if self.accept(ID):
--- a/lib/spack/spack/test/cmd/spec.py
+++ b/lib/spack/spack/test/cmd/spec.py
@ -12,6 +12,7 @@
 import spack.error
 import spack.spec
 import spack.store
+from spack.fetch_strategy import FetchError
 from spack.main import SpackCommand, SpackCommandError

 pytestmark = pytest.mark.usefixtures("config", "mutable_mock_repo")
@ -202,3 +203,21 @@ def test_env_aware_spec(mutable_mock_env_path):
        assert "libdwarf@20130729" in output
        assert "libelf@0.8.1" in output
        assert "mpich@3.0.4" in output
+
+
+@pytest.mark.parametrize(
+    "name, version, error",
+    [
+        ("develop-branch-version", "f3c7206350ac8ee364af687deaae5c574dcfca2c=develop", None),
+        ("develop-branch-version", "git." + "a" * 40 + "=develop", None),
+        ("callpath", "f3c7206350ac8ee364af687deaae5c574dcfca2c=1.0", FetchError),
+        ("develop-branch-version", "git.foo=0.2.15", None),
+    ],
+)
+def test_spec_version_assigned_git_ref_as_version(name, version, error):
+    if error:
+        with pytest.raises(error):
+            output = spec(name + "@" + version)
+    else:
+        output = spec(name + "@" + version)
+        assert version in output
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@ -21,6 +21,7 @@
 import spack.repo
 import spack.variant as vt
 from spack.concretize import find_spec
+from spack.solver.asp import UnsatisfiableSpecError
 from spack.spec import Spec
 from spack.version import ver

@ -1735,3 +1736,34 @@ def test_not_reusing_incompatible_os_or_compiler(self):
        concrete_spec = result.specs[0]
        assert concrete_spec.satisfies("%gcc@4.5.0")
        assert concrete_spec.satisfies("os=debian6")
+
+    def test_git_hash_assigned_version_is_preferred(self):
+        hash = "a" * 40
+        s = Spec("develop-branch-version@%s=develop" % hash)
+        c = s.concretized()
+        assert hash in str(c)
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="Not supported on Windows (yet)")
+    @pytest.mark.parametrize("git_ref", ("a" * 40, "0.2.15", "main"))
+    def test_git_ref_version_is_equivalent_to_specified_version(self, git_ref):
+        if spack.config.get("config:concretizer") == "original":
+            pytest.skip("Original concretizer cannot account for git hashes")
+        s = Spec("develop-branch-version@git.%s=develop" % git_ref)
+        c = s.concretized()
+        assert git_ref in str(c)
+        print(str(c))
+        assert s.satisfies("@develop")
+        assert s.satisfies("@0.1:")
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="Not supported on Windows (yet)")
+    @pytest.mark.parametrize("git_ref", ("a" * 40, "0.2.15", "fbranch"))
+    def test_git_ref_version_errors_if_unknown_version(self, git_ref):
+        if spack.config.get("config:concretizer") == "original":
+            pytest.skip("Original concretizer cannot account for git hashes")
+        # main is not defined in the package.py for this file
+        s = Spec("develop-branch-version@git.%s=main" % git_ref)
+        with pytest.raises(
+            UnsatisfiableSpecError,
+            match="The reference version 'main' for package 'develop-branch-version'",
+        ):
+            s.concretized()
--- a/lib/spack/spack/test/spec_syntax.py
+++ b/lib/spack/spack/test/spec_syntax.py
@ -185,6 +185,10 @@ def test_multiple_specs_long_second(self):
            'mvapich cflags="-O3 -fPIC" emacs^ncurses%intel',
        )

+    def test_spec_with_version_hash_pair(self):
+        hash = "abc12" * 8
+        self.check_parse("develop-branch-version@%s=develop" % hash)
+
    def test_full_specs(self):
        self.check_parse(
            "mvapich_foo" " ^_openmpi@1.2:1.4,1.6%intel@12.1+debug~qt_4" " ^stackwalker@8.1_1e"
--- a/lib/spack/spack/test/versions.py
+++ b/lib/spack/spack/test/versions.py
@ -585,7 +585,7 @@ def test_list_highest():
    assert vl2.lowest() == Version("master")


-@pytest.mark.parametrize("version_str", ["foo 1.2.0", "!", "1!2"])
+@pytest.mark.parametrize("version_str", ["foo 1.2.0", "!", "1!2", "=1.2.0"])
 def test_invalid_versions(version_str):
    """Ensure invalid versions are rejected with a ValueError"""
    with pytest.raises(ValueError):
@ -727,3 +727,22 @@ def test_version_list_with_range_included_in_concrete_version_interpreted_as_ran
 def test_version_list_with_range_and_concrete_version_is_not_concrete():
    v = VersionList([Version("3.1"), VersionRange("3.1.1", "3.1.2")])
    assert v.concrete
+
+
+@pytest.mark.parametrize(
+    "vstring, eq_vstring, is_commit",
+    (
+        ("abc12" * 8 + "=develop", "develop", True),
+        ("git." + "abc12" * 8 + "=main", "main", True),
+        ("a" * 40 + "=develop", "develop", True),
+        ("b" * 40 + "=3.2", "3.2", True),
+        ("git.foo=3.2", "3.2", False),
+    ),
+)
+def test_git_ref_can_be_assigned_a_version(vstring, eq_vstring, is_commit):
+    v = Version(vstring)
+    v_equivalent = Version(eq_vstring)
+    assert v.is_commit == is_commit
+    assert v.is_ref
+    assert not v._ref_lookup
+    assert v_equivalent.version == v.ref_version
--- a/lib/spack/spack/version.py
+++ b/lib/spack/spack/version.py
@ -45,7 +45,7 @@
 __all__ = ["Version", "VersionRange", "VersionList", "ver"]

 # Valid version characters
-VALID_VERSION = re.compile(r"^[A-Za-z0-9_.-]+$")
+VALID_VERSION = re.compile(r"^[A-Za-z0-9_.-][=A-Za-z0-9_.-]*$")

 # regex for a commit version
 COMMIT_VERSION = re.compile(r"^[a-f0-9]{40}$")
@ -178,6 +178,8 @@ def is_git_version(string):
        return True
    elif len(string) == 40 and COMMIT_VERSION.match(string):
        return True
+    elif "=" in string:
+        return True
    return False


@ -211,9 +213,13 @@ def __init__(self, string):
        if string and not VALID_VERSION.match(string):
            raise ValueError("Bad characters in version string: %s" % string)

+        self.separators, self.version = self._generate_seperators_and_components(string)
+
+    def _generate_seperators_and_components(self, string):
        segments = SEGMENT_REGEX.findall(string)
-        self.version = tuple(int(m[0]) if m[0] else VersionStrComponent(m[1]) for m in segments)
-        self.separators = tuple(m[2] for m in segments)
+        components = tuple(int(m[0]) if m[0] else VersionStrComponent(m[1]) for m in segments)
+        separators = tuple(m[2] for m in segments)
+        return separators, components

    @property
    def dotted(self):
@ -464,21 +470,30 @@ def __init__(self, string):
        if not isinstance(string, str):
            string = str(string)  # In case we got a VersionBase or GitVersion object

-        git_prefix = string.startswith("git.")
-        self.ref = string[4:] if git_prefix else string
+        # An object that can lookup git refs to compare them to versions
+        self.user_supplied_reference = False
+        self._ref_lookup = None
+        self.ref_version = None

-        self.is_commit = len(self.ref) == 40 and COMMIT_VERSION.match(self.ref)
+        git_prefix = string.startswith("git.")
+        pruned_string = string[4:] if git_prefix else string
+
+        if "=" in pruned_string:
+            self.ref, self.ref_version_str = pruned_string.split("=")
+            _, self.ref_version = self._generate_seperators_and_components(self.ref_version_str)
+            self.user_supplied_reference = True
+        else:
+            self.ref = pruned_string
+
+        self.is_commit = bool(len(self.ref) == 40 and COMMIT_VERSION.match(self.ref))
        self.is_ref = git_prefix  # is_ref False only for comparing to VersionBase
        self.is_ref |= bool(self.is_commit)

        # ensure git.<hash> and <hash> are treated the same by dropping 'git.'
-        canonical_string = self.ref if self.is_commit else string
+        # unless we are assigning a version with =
+        canonical_string = self.ref if (self.is_commit and not self.ref_version) else string
        super(GitVersion, self).__init__(canonical_string)

-        # An object that can lookup git refs to compare them to versions
-        self._ref_lookup = None
-        self.ref_version = None
-
    def _cmp(self, other_lookups=None):
        # No need to rely on git comparisons for develop-like refs
        if len(self.version) == 2 and self.isdevelop():
@ -603,6 +618,10 @@ def generate_git_lookup(self, pkg_name):
        if not self.is_ref:
            tty.die("%s is not a git version." % self)

+        # don't need a lookup if we already have a version assigned
+        if self.ref_version:
+            return
+
        # Generate a commit looker-upper
        self._ref_lookup = CommitLookup(pkg_name)

--- a/var/spack/repos/builtin.mock/packages/depends-on-develop/package.py
+++ b/var/spack/repos/builtin.mock/packages/depends-on-develop/package.py
@ -0,0 +1,17 @@
+# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+
+from spack.package import *
+
+
+class DependsOnDevelop(Package):
+    homepage = "example.com"
+    url = "fake.com"
+
+    version("main", branch="main")
+    version("0.0.0", sha256="0123456789abcdef0123456789abcdef")
+
+    depends_on("develop-branch-version@develop")
--- a/var/spack/repos/builtin.mock/packages/develop-branch-version/package.py
+++ b/var/spack/repos/builtin.mock/packages/develop-branch-version/package.py
@ -0,0 +1,17 @@
+# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack.package import *
+
+
+class DevelopBranchVersion(Package):
+    """Dummy package with develop version"""
+
+    homepage = "http://www.openblas.net"
+    url = "http://github.com/xianyi/OpenBLAS/archive/v0.2.15.tar.gz"
+    git = "https://github.com/dummy/repo.git"
+
+    version("develop", branch="develop")
+    version("0.2.15", "b1190f3d3471685f17cfd1ec1d252ac9")