From d76a8b7de7a6007555b8d60fd88f4c1fe65e41f7 Mon Sep 17 00:00:00 2001 From: Todd Gamblin 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 --- 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-60-g2f50