From d76a8b7de7a6007555b8d60fd88f4c1fe65e41f7 Mon Sep 17 00:00:00 2001
From: Todd Gamblin <tgamblin@llnl.gov>
Date: Tue, 28 Mar 2023 11:18:54 -0700
Subject: retry: bugfix: package requirements with git commits (#35057)
(#36347)
- [x] Specs that define 'new' versions in the require: section need to generate associated facts to indicate that those versions are valid.
- [x] add test to verify success with unknown versions.
- [x] remove unneeded check that was leading to extra complexity and test
failures (at this point, all `hash=version` does not require listing out that version
in `packages.yaml`)
- [x] unique index for origin (dont reuse 0)
Co-authored-by: Peter Josef Scheibel <scheibel1@llnl.gov>
---
lib/spack/spack/solver/asp.py | 97 +++++++++++-----
lib/spack/spack/test/concretize.py | 10 +-
lib/spack/spack/test/concretize_requirements.py | 146 ++++++++++++++++++++++++
lib/spack/spack/version.py | 15 ++-
4 files changed, 234 insertions(+), 34 deletions(-)
(limited to 'lib')
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index ad3b4afbd9..4d409bb8c8 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -103,6 +103,7 @@ version_origin_fields = [
"dev_spec",
"external",
"packages_yaml",
+ "package_requirements",
"package_py",
"installed",
]
@@ -115,9 +116,10 @@ version_provenance = collections.namedtuple( # type: ignore
"VersionProvenance", version_origin_fields
)(**{name: i for i, name in enumerate(version_origin_fields)})
-#: Named tuple to contain information on declared versions
+
DeclaredVersion = collections.namedtuple("DeclaredVersion", ["version", "idx", "origin"])
+
# Below numbers are used to map names of criteria to the order
# they appear in the solution. See concretize.lp
@@ -872,30 +874,6 @@ class SpackSolverSetup(object):
)
)
- 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):
@@ -1651,6 +1629,15 @@ class SpackSolverSetup(object):
DeclaredVersion(version=dep.version, idx=0, origin=origin)
)
self.possible_versions[dep.name].add(dep.version)
+ if (
+ isinstance(dep.version, spack.version.GitVersion)
+ and dep.version.user_supplied_reference
+ ):
+ defined_version = spack.version.Version(dep.version.ref_version_str)
+ self.declared_versions[dep.name].append(
+ DeclaredVersion(version=defined_version, idx=1, origin=origin)
+ )
+ self.possible_versions[dep.name].add(defined_version)
def _supported_targets(self, compiler_name, compiler_version, targets):
"""Get a list of which targets are supported by the compiler.
@@ -1887,7 +1874,11 @@ class SpackSolverSetup(object):
# This is needed to account for a variable number of
# numbers e.g. if both 1.0 and 1.0.2 are possible versions
- exact_match = [v for v in allowed_versions if v == versions]
+ exact_match = [
+ v
+ for v in allowed_versions
+ if v == versions and not isinstance(v, spack.version.GitVersion)
+ ]
if exact_match:
allowed_versions = exact_match
@@ -2089,6 +2080,11 @@ class SpackSolverSetup(object):
self.add_concrete_versions_from_specs(specs, version_provenance.spec)
self.add_concrete_versions_from_specs(dev_specs, version_provenance.dev_spec)
+ req_version_specs = _get_versioned_specs_from_pkg_requirements()
+ self.add_concrete_versions_from_specs(
+ req_version_specs, version_provenance.package_requirements
+ )
+
self.gen.h1("Concrete input spec definitions")
self.define_concrete_input_specs(specs, possible)
@@ -2163,6 +2159,55 @@ class SpackSolverSetup(object):
self.gen.fact(fn.concretize_everything())
+def _get_versioned_specs_from_pkg_requirements():
+ """If package requirements mention versions that are not mentioned
+ elsewhere, then we need to collect those to mark them as possible
+ versions.
+ """
+ req_version_specs = list()
+ config = spack.config.get("packages")
+ for pkg_name, d in config.items():
+ if pkg_name == "all":
+ continue
+ if "require" in d:
+ req_version_specs.extend(_specs_from_requires(pkg_name, d["require"]))
+ return req_version_specs
+
+
+def _specs_from_requires(pkg_name, section):
+ if isinstance(section, str):
+ spec = spack.spec.Spec(section)
+ if not spec.name:
+ spec.name = pkg_name
+ extracted_specs = [spec]
+ else:
+ spec_strs = []
+ for spec_group in section:
+ if isinstance(spec_group, str):
+ spec_strs.append(spec_group)
+ else:
+ # Otherwise it is a one_of or any_of: get the values
+ (x,) = spec_group.values()
+ spec_strs.extend(x)
+
+ extracted_specs = []
+ for spec_str in spec_strs:
+ spec = spack.spec.Spec(spec_str)
+ if not spec.name:
+ spec.name = pkg_name
+ extracted_specs.append(spec)
+
+ version_specs = []
+ for spec in extracted_specs:
+ try:
+ spec.version
+ version_specs.append(spec)
+ except spack.error.SpecError:
+ pass
+
+ return version_specs
+
+
class SpecBuilder(object):
"""Class with actions to rebuild a spec from ASP results."""
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index 80ad390e53..9754b47f6a 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -22,7 +22,6 @@ import spack.platforms
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
@@ -1845,16 +1844,13 @@ class TestConcretize(object):
assert s.satisfies("@0.1:")
@pytest.mark.parametrize("git_ref", ("a" * 40, "0.2.15", "fbranch"))
- def test_git_ref_version_errors_if_unknown_version(self, git_ref):
+ def test_git_ref_version_succeeds_with_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()
+ s.concretize()
+ assert s.satisfies("develop-branch-version@main")
@pytest.mark.regression("31484")
def test_installed_externals_are_reused(self, mutable_database, repo_with_changing_recipe):
diff --git a/lib/spack/spack/test/concretize_requirements.py b/lib/spack/spack/test/concretize_requirements.py
index f58cd87694..ecb33c6527 100644
--- a/lib/spack/spack/test/concretize_requirements.py
+++ b/lib/spack/spack/test/concretize_requirements.py
@@ -13,6 +13,7 @@ import spack.repo
import spack.util.spack_yaml as syaml
from spack.solver.asp import UnsatisfiableSpecError
from spack.spec import Spec
+from spack.util.url import path_to_file_url
pytestmark = pytest.mark.skipif(sys.platform == "win32", reason="Windows uses old concretizer")
@@ -140,6 +141,151 @@ packages:
Spec("x@1.1").concretize()
+def test_git_user_supplied_reference_satisfaction(
+ concretize_scope, test_repo, mock_git_version_info, monkeypatch
+):
+ repo_path, filename, commits = mock_git_version_info
+
+ monkeypatch.setattr(
+ spack.package_base.PackageBase, "git", path_to_file_url(repo_path), raising=False
+ )
+
+ specs = ["v@{commit0}=2.2", "v@{commit0}", "v@2.2", "v@{commit0}=2.3"]
+
+ format_info = {"commit0": commits[0]}
+
+ hash_eq_ver, just_hash, just_ver, hash_eq_other_ver = [
+ Spec(x.format(**format_info)) for x in specs
+ ]
+
+ assert hash_eq_ver.satisfies(just_hash)
+ assert not just_hash.satisfies(hash_eq_ver)
+ assert hash_eq_ver.satisfies(just_ver)
+ assert not just_ver.satisfies(hash_eq_ver)
+ assert not hash_eq_ver.satisfies(hash_eq_other_ver)
+ assert not hash_eq_other_ver.satisfies(hash_eq_ver)
+
+
+def test_requirement_adds_new_version(
+ concretize_scope, test_repo, mock_git_version_info, monkeypatch
+):
+ if spack.config.get("config:concretizer") == "original":
+ pytest.skip("Original concretizer does not support configuration" " requirements")
+
+ repo_path, filename, commits = mock_git_version_info
+ monkeypatch.setattr(
+ spack.package_base.PackageBase, "git", path_to_file_url(repo_path), raising=False
+ )
+
+ a_commit_hash = commits[0]
+ conf_str = """\
+packages:
+ v:
+ require: "@{0}=2.2"
+""".format(
+ a_commit_hash
+ )
+ update_packages_config(conf_str)
+
+ s1 = Spec("v").concretized()
+ assert s1.satisfies("@2.2")
+ assert s1.satisfies("@{0}".format(a_commit_hash))
+ # Make sure the git commit info is retained
+ assert isinstance(s1.version, spack.version.GitVersion)
+ assert s1.version.ref == a_commit_hash
+
+
+def test_requirement_adds_git_hash_version(
+ concretize_scope, test_repo, mock_git_version_info, monkeypatch
+):
+ if spack.config.get("config:concretizer") == "original":
+ pytest.skip("Original concretizer does not support configuration" " requirements")
+
+ repo_path, filename, commits = mock_git_version_info
+ monkeypatch.setattr(
+ spack.package_base.PackageBase, "git", path_to_file_url(repo_path), raising=False
+ )
+
+ a_commit_hash = commits[0]
+ conf_str = """\
+packages:
+ v:
+ require: "@{0}"
+""".format(
+ a_commit_hash
+ )
+ update_packages_config(conf_str)
+
+ s1 = Spec("v").concretized()
+ assert s1.satisfies("@{0}".format(a_commit_hash))
+
+
+def test_requirement_adds_multiple_new_versions(
+ concretize_scope, test_repo, mock_git_version_info, monkeypatch
+):
+ if spack.config.get("config:concretizer") == "original":
+ pytest.skip("Original concretizer does not support configuration" " requirements")
+
+ repo_path, filename, commits = mock_git_version_info
+ monkeypatch.setattr(
+ spack.package_base.PackageBase, "git", path_to_file_url(repo_path), raising=False
+ )
+
+ conf_str = """\
+packages:
+ v:
+ require:
+ - one_of: ["@{0}=2.2", "@{1}=2.3"]
+""".format(
+ commits[0], commits[1]
+ )
+ update_packages_config(conf_str)
+
+ s1 = Spec("v").concretized()
+ assert s1.satisfies("@2.2")
+
+ s2 = Spec("v@{0}".format(commits[1])).concretized()
+ assert s2.satisfies("@{0}".format(commits[1]))
+ assert s2.satisfies("@2.3")
+
+
+# TODO: this belongs in the concretize_preferences test module but uses
+# fixtures defined only here
+def test_preference_adds_new_version(
+ concretize_scope, test_repo, mock_git_version_info, monkeypatch
+):
+ if spack.config.get("config:concretizer") == "original":
+ pytest.skip("Original concretizer does not support configuration" " requirements")
+
+ repo_path, filename, commits = mock_git_version_info
+ monkeypatch.setattr(
+ spack.package_base.PackageBase, "git", path_to_file_url(repo_path), raising=False
+ )
+
+ conf_str = """\
+packages:
+ v:
+ version: ["{0}=2.2", "{1}=2.3"]
+""".format(
+ commits[0], commits[1]
+ )
+ update_packages_config(conf_str)
+
+ s1 = Spec("v").concretized()
+ assert s1.satisfies("@2.2")
+ assert s1.satisfies("@{0}".format(commits[0]))
+
+ s2 = Spec("v@2.3").concretized()
+ # Note: this check will fail: the command-line spec version is preferred
+ # assert s2.satisfies("@{0}".format(commits[1]))
+ assert s2.satisfies("@2.3")
+
+ s3 = Spec("v@{0}".format(commits[1])).concretized()
+ assert s3.satisfies("@{0}".format(commits[1]))
+ # Note: this check will fail: the command-line spec version is preferred
+ # assert s3.satisfies("@2.3")
+
+
def test_requirement_is_successfully_applied(concretize_scope, test_repo):
"""If a simple requirement can be satisfied, make sure the
concretization succeeds and the requirement spec is applied.
diff --git a/lib/spack/spack/version.py b/lib/spack/spack/version.py
index 53093d9459..d744e223c2 100644
--- a/lib/spack/spack/version.py
+++ b/lib/spack/spack/version.py
@@ -627,7 +627,20 @@ class GitVersion(VersionBase):
self_cmp = self._cmp(other.ref_lookup)
other_cmp = other._cmp(self.ref_lookup)
- if other.is_ref:
+ if self.is_ref and other.is_ref:
+ if self.ref != other.ref:
+ return False
+ elif self.user_supplied_reference and other.user_supplied_reference:
+ return self.ref_version == other.ref_version
+ elif other.user_supplied_reference:
+ return False
+ else:
+ # In this case, 'other' does not supply a version equivalence
+ # with "=" and the commit strings are equal. 'self' may specify
+ # a version equivalence, but that is extra info and will
+ # satisfy no matter what it is.
+ return True
+ elif 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:
--
cgit v1.2.3-70-g09d2