From 4f14db19c4884d480cd102e965287bf29667f0c3 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Tue, 19 Sep 2023 11:44:49 +0200 Subject: [PATCH] ASP-based solver: declare deprecated versions iff config:deprecated:true (#40011) By default, do not let deprecated versions enter the solve. Previously you could concretize to something deprecated, only to get errors on install. With this commit, we get errors on concretization, so the issue is caught earlier. --- lib/spack/spack/cmd/solve.py | 16 ++- lib/spack/spack/concretize.py | 5 +- lib/spack/spack/environment/environment.py | 5 +- lib/spack/spack/solver/asp.py | 110 ++++++++++++++---- lib/spack/spack/solver/concretize.lp | 8 ++ lib/spack/spack/spec.py | 3 +- lib/spack/spack/test/concretize.py | 10 +- .../spack/test/concretize_requirements.py | 30 +++-- 8 files changed, 146 insertions(+), 41 deletions(-) diff --git a/lib/spack/spack/cmd/solve.py b/lib/spack/spack/cmd/solve.py index e27687dc216..4a30b1694b9 100644 --- a/lib/spack/spack/cmd/solve.py +++ b/lib/spack/spack/cmd/solve.py @@ -176,17 +176,29 @@ def solve(parser, args): output = sys.stdout if "asp" in show else None 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 + 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 idx, result in enumerate( - solver.solve_in_rounds(specs, out=output, timers=args.timers, stats=args.stats) + solver.solve_in_rounds( + specs, + out=output, + timers=args.timers, + stats=args.stats, + allow_deprecated=allow_deprecated, + ) ): if "solutions" in show: tty.msg("ROUND {0}".format(idx)) diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py index 5a46d1f999c..e4acef401e4 100644 --- a/lib/spack/spack/concretize.py +++ b/lib/spack/spack/concretize.py @@ -744,8 +744,11 @@ def concretize_specs_together(*abstract_specs, **kwargs): def _concretize_specs_together_new(*abstract_specs, **kwargs): import spack.solver.asp + allow_deprecated = spack.config.get("config:deprecated", False) solver = spack.solver.asp.Solver() - result = solver.solve(abstract_specs, tests=kwargs.get("tests", False)) + result = solver.solve( + abstract_specs, tests=kwargs.get("tests", False), allow_deprecated=allow_deprecated + ) result.raise_if_unsat() return [s.copy() for s in result.specs] diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index c7a90a9b784..d8f04c7d4bc 100644 --- a/lib/spack/spack/environment/environment.py +++ b/lib/spack/spack/environment/environment.py @@ -1396,7 +1396,10 @@ def _concretize_together_where_possible( result_by_user_spec = {} solver = spack.solver.asp.Solver() - for result in solver.solve_in_rounds(specs_to_concretize, tests=tests): + allow_deprecated = spack.config.get("config:deprecated", False) + for result in solver.solve_in_rounds( + specs_to_concretize, tests=tests, allow_deprecated=allow_deprecated + ): result_by_user_spec.update(result.specs_by_input) result = [] diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index ddc05ec58f7..810e82e96d0 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -13,7 +13,7 @@ import re import types import warnings -from typing import List, NamedTuple, Tuple, Union +from typing import List, NamedTuple, Optional, Sequence, Tuple, Union import archspec.cpu @@ -138,7 +138,15 @@ class RequirementKind(enum.Enum): PACKAGE = enum.auto() -DeclaredVersion = collections.namedtuple("DeclaredVersion", ["version", "idx", "origin"]) +class DeclaredVersion(NamedTuple): + """Data class to contain information on declared versions used in the solve""" + + #: String representation of the version + version: str + #: Unique index assigned to this version + idx: int + #: Provenance of the version + origin: Provenance # Below numbers are used to map names of criteria to the order @@ -785,7 +793,7 @@ def fact(self, head): if choice: self.assumptions.append(atom) - def solve(self, setup, specs, reuse=None, output=None, control=None): + def solve(self, setup, specs, reuse=None, output=None, control=None, allow_deprecated=False): """Set up the input and solve for dependencies of ``specs``. Arguments: @@ -796,6 +804,7 @@ def solve(self, setup, specs, reuse=None, output=None, control=None): the output of this solve. control (clingo.Control): configuration for the solver. If None, default values will be used + allow_deprecated: if True, allow deprecated versions in the solve Return: A tuple of the solve result, the timer for the different phases of the @@ -815,7 +824,7 @@ def solve(self, setup, specs, reuse=None, output=None, control=None): timer.start("setup") with self.control.backend() as backend: self.backend = backend - setup.setup(self, specs, reuse=reuse) + setup.setup(self, specs, reuse=reuse, allow_deprecated=allow_deprecated) timer.stop("setup") timer.start("load") @@ -1898,7 +1907,7 @@ class Body: return clauses def define_package_versions_and_validate_preferences( - self, possible_pkgs, require_checksum: bool + self, possible_pkgs, *, require_checksum: bool, allow_deprecated: bool ): """Declare any versions in specs not declared in packages.""" packages_yaml = spack.config.get("packages") @@ -1918,13 +1927,15 @@ def define_package_versions_and_validate_preferences( ] for idx, (v, version_info) in enumerate(package_py_versions): + if version_info.get("deprecated", False): + self.deprecated_versions[pkg_name].add(v) + if not allow_deprecated: + continue + self.possible_versions[pkg_name].add(v) self.declared_versions[pkg_name].append( DeclaredVersion(version=v, idx=idx, origin=Provenance.PACKAGE_PY) ) - deprecated = version_info.get("deprecated", False) - if deprecated: - self.deprecated_versions[pkg_name].add(v) if pkg_name not in packages_yaml or "version" not in packages_yaml[pkg_name]: continue @@ -1953,7 +1964,9 @@ def define_package_versions_and_validate_preferences( ) self.possible_versions[pkg_name].add(vdef) - def define_ad_hoc_versions_from_specs(self, specs, origin, require_checksum: bool): + def define_ad_hoc_versions_from_specs( + self, specs, origin, *, allow_deprecated: bool, require_checksum: bool + ): """Add concrete versions to possible versions from lists of CLI/dev specs.""" for s in traverse.traverse_nodes(specs): # If there is a concrete version on the CLI *that we know nothing @@ -1969,6 +1982,9 @@ def define_ad_hoc_versions_from_specs(self, specs, origin, require_checksum: boo s.format("No matching version for constraint {name}{@versions}") ) + if not allow_deprecated and version in self.deprecated_versions[s.name]: + continue + declared = DeclaredVersion(version=version, idx=0, origin=origin) self.declared_versions[s.name].append(declared) self.possible_versions[s.name].add(version) @@ -2333,7 +2349,14 @@ def define_concrete_input_specs(self, specs, possible): if spec.concrete: self._facts_from_concrete_spec(spec, possible) - def setup(self, driver, specs, reuse=None): + def setup( + self, + driver: PyclingoDriver, + specs: Sequence[spack.spec.Spec], + *, + reuse: Optional[List[spack.spec.Spec]] = None, + allow_deprecated: bool = False, + ): """Generate an ASP program with relevant constraints for specs. This calls methods on the solve driver to set up the problem with @@ -2341,9 +2364,10 @@ def setup(self, driver, specs, reuse=None): specs, as well as constraints from the specs themselves. Arguments: - driver (PyclingoDriver): driver instance of this solve - specs (list): list of Specs to solve - reuse (None or list): list of concrete specs that can be reused + driver: driver instance of this solve + specs: list of Specs to solve + reuse: list of concrete specs that can be reused + allow_deprecated: if True adds deprecated versions into the solve """ self._condition_id_counter = itertools.count() @@ -2369,10 +2393,13 @@ def setup(self, driver, specs, reuse=None): # rules to generate an ASP program. self.gen = driver + if not allow_deprecated: + self.gen.fact(fn.deprecated_versions_not_allowed()) + # Calculate develop specs # they will be used in addition to command line specs # in determining known versions/targets/os - dev_specs = () + dev_specs: Tuple[spack.spec.Spec, ...] = () env = ev.active_environment() if env: dev_specs = tuple( @@ -2418,11 +2445,22 @@ def setup(self, driver, specs, reuse=None): self.external_packages() # TODO: make a config option for this undocumented feature - require_checksum = "SPACK_CONCRETIZER_REQUIRE_CHECKSUM" in os.environ - self.define_package_versions_and_validate_preferences(self.pkgs, require_checksum) - self.define_ad_hoc_versions_from_specs(specs, Provenance.SPEC, require_checksum) - self.define_ad_hoc_versions_from_specs(dev_specs, Provenance.DEV_SPEC, require_checksum) - self.validate_and_define_versions_from_requirements(require_checksum) + checksummed = "SPACK_CONCRETIZER_REQUIRE_CHECKSUM" in os.environ + self.define_package_versions_and_validate_preferences( + self.pkgs, allow_deprecated=allow_deprecated, require_checksum=checksummed + ) + self.define_ad_hoc_versions_from_specs( + specs, Provenance.SPEC, allow_deprecated=allow_deprecated, require_checksum=checksummed + ) + self.define_ad_hoc_versions_from_specs( + dev_specs, + Provenance.DEV_SPEC, + allow_deprecated=allow_deprecated, + require_checksum=checksummed, + ) + self.validate_and_define_versions_from_requirements( + allow_deprecated=allow_deprecated, require_checksum=checksummed + ) self.gen.h1("Package Constraints") for pkg in sorted(self.pkgs): @@ -2471,7 +2509,9 @@ def literal_specs(self, specs): if self.concretize_everything: self.gen.fact(fn.solve_literal(idx)) - def validate_and_define_versions_from_requirements(self, require_checksum: bool): + def validate_and_define_versions_from_requirements( + self, *, allow_deprecated: bool, require_checksum: bool + ): """If package requirements mention concrete versions that are not mentioned elsewhere, then we need to collect those to mark them as possible versions. If they are abstract and statically have no match, then we @@ -2504,6 +2544,9 @@ def validate_and_define_versions_from_requirements(self, require_checksum: bool) if v in self.possible_versions[name]: continue + if not allow_deprecated and v in self.deprecated_versions[name]: + continue + # If concrete an not yet defined, conditionally define it, like we do for specs # from the command line. if not require_checksum or _is_checksummed_git_version(v): @@ -2937,7 +2980,16 @@ def _reusable_specs(self, specs): return reusable_specs - def solve(self, specs, out=None, timers=False, stats=False, tests=False, setup_only=False): + def solve( + self, + specs, + out=None, + timers=False, + stats=False, + tests=False, + setup_only=False, + allow_deprecated=False, + ): """ Arguments: specs (list): List of ``Spec`` objects to solve for. @@ -2948,6 +3000,7 @@ def solve(self, specs, out=None, timers=False, stats=False, tests=False, setup_o If a tuple of package names, concretize test dependencies for named packages (defaults to False: do not concretize test dependencies). setup_only (bool): if True, stop after setup and don't solve (default False). + allow_deprecated (bool): allow deprecated version in the solve """ # Check upfront that the variants are admissible specs = [s.lookup_hash() for s in specs] @@ -2955,10 +3008,14 @@ def solve(self, specs, out=None, timers=False, stats=False, tests=False, setup_o reusable_specs.extend(self._reusable_specs(specs)) setup = SpackSolverSetup(tests=tests) output = OutputConfiguration(timers=timers, stats=stats, out=out, setup_only=setup_only) - result, _, _ = self.driver.solve(setup, specs, reuse=reusable_specs, output=output) + result, _, _ = self.driver.solve( + setup, specs, reuse=reusable_specs, output=output, allow_deprecated=allow_deprecated + ) return result - def solve_in_rounds(self, specs, out=None, timers=False, stats=False, tests=False): + def solve_in_rounds( + self, specs, out=None, timers=False, stats=False, tests=False, allow_deprecated=False + ): """Solve for a stable model of specs in multiple rounds. This relaxes the assumption of solve that everything must be consistent and @@ -2973,6 +3030,7 @@ def solve_in_rounds(self, specs, out=None, timers=False, stats=False, tests=Fals timers (bool): print timing if set to True stats (bool): print internal statistics if set to True tests (bool): add test dependencies to the solve + allow_deprecated (bool): allow deprecated version in the solve """ specs = [s.lookup_hash() for s in specs] reusable_specs = self._check_input_and_extract_concrete_specs(specs) @@ -2986,7 +3044,11 @@ def solve_in_rounds(self, specs, out=None, timers=False, stats=False, tests=Fals output = OutputConfiguration(timers=timers, stats=stats, out=out, setup_only=False) while True: result, _, _ = self.driver.solve( - setup, input_specs, reuse=reusable_specs, output=output + setup, + input_specs, + reuse=reusable_specs, + output=output, + allow_deprecated=allow_deprecated, ) yield result diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp index 9d1b48b3794..7a41a9e8348 100644 --- a/lib/spack/spack/solver/concretize.lp +++ b/lib/spack/spack/solver/concretize.lp @@ -196,6 +196,13 @@ attr("deprecated", node(ID, Package), Version) :- attr("version", node(ID, Package), Version), pkg_fact(Package, deprecated_version(Version)). +error(100, "Package '{0}' needs the deprecated version '{1}', and this is not allowed", Package, Version) + :- deprecated_versions_not_allowed(), + attr("version", node(ID, Package), Version), + not external(node(ID, Package)), + not concrete(node(ID, Package)), + pkg_fact(Package, deprecated_version(Version)). + possible_version_weight(node(ID, Package), Weight) :- attr("version", node(ID, Package), Version), pkg_fact(Package, version_declared(Version, Weight)). @@ -252,6 +259,7 @@ attr("node_version_satisfies", node(ID, Package), Constraint) pkg_fact(Package, version_satisfies(Constraint, Version)). #defined version_satisfies/3. +#defined deprecated_versions_not_allowed/0. #defined deprecated_version/2. %----------------------------------------------------------------------------- diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index 05f10b9aa73..c88998d54ed 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -2924,8 +2924,9 @@ def _new_concretize(self, tests=False): if self._concrete: return + allow_deprecated = spack.config.get("config:deprecated", False) solver = spack.solver.asp.Solver() - result = solver.solve([self], tests=tests) + result = solver.solve([self], tests=tests, allow_deprecated=allow_deprecated) result.raise_if_unsat() # take the best answer diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py index 2197d6b3cbb..63c6699de2b 100644 --- a/lib/spack/spack/test/concretize.py +++ b/lib/spack/spack/test/concretize.py @@ -1356,15 +1356,15 @@ def test_multivalued_variants_from_cli(self, spec_str, expected_dict): [ # Version 1.1.0 is deprecated and should not be selected, unless we # explicitly asked for that - ("deprecated-versions", ["deprecated-versions@1.0.0"]), - ("deprecated-versions@1.1.0", ["deprecated-versions@1.1.0"]), + ("deprecated-versions", "deprecated-versions@1.0.0"), + ("deprecated-versions@=1.1.0", "deprecated-versions@1.1.0"), ], ) @pytest.mark.only_clingo("Use case not supported by the original concretizer") def test_deprecated_versions_not_selected(self, spec_str, expected): - s = Spec(spec_str).concretized() - for abstract_spec in expected: - assert abstract_spec in s + with spack.config.override("config:deprecated", True): + s = Spec(spec_str).concretized() + s.satisfies(expected) @pytest.mark.regression("24196") def test_version_badness_more_important_than_default_mv_variants(self): diff --git a/lib/spack/spack/test/concretize_requirements.py b/lib/spack/spack/test/concretize_requirements.py index d7c1c883597..cd510060884 100644 --- a/lib/spack/spack/test/concretize_requirements.py +++ b/lib/spack/spack/test/concretize_requirements.py @@ -543,21 +543,37 @@ def test_reuse_oneof(concretize_scope, create_test_repo, mutable_database, fake_ assert not s2.satisfies("@2.5 %gcc") -def test_requirements_are_higher_priority_than_deprecation(concretize_scope, test_repo): - """Test that users can override a deprecated version with a requirement.""" - # @2.3 is a deprecated versions. Ensure that any_of picks both constraints, +@pytest.mark.parametrize( + "allow_deprecated,expected,not_expected", + [(True, ["@=2.3", "%gcc"], []), (False, ["%gcc"], ["@=2.3"])], +) +def test_requirements_and_deprecated_versions( + allow_deprecated, expected, not_expected, concretize_scope, test_repo +): + """Tests the expected behavior of requirements and deprecated versions. + + If deprecated versions are not allowed, concretization should just pick + the other requirement. + + If deprecated versions are allowed, both requirements are honored. + """ + # 2.3 is a deprecated versions. Ensure that any_of picks both constraints, # since they are possible conf_str = """\ packages: y: require: - - any_of: ["@2.3", "%gcc"] + - any_of: ["@=2.3", "%gcc"] """ update_packages_config(conf_str) - s1 = Spec("y").concretized() - assert s1.satisfies("@2.3") - assert s1.satisfies("%gcc") + with spack.config.override("config:deprecated", allow_deprecated): + s1 = Spec("y").concretized() + for constrain in expected: + assert s1.satisfies(constrain) + + for constrain in not_expected: + assert not s1.satisfies(constrain) @pytest.mark.parametrize("spec_str,requirement_str", [("x", "%gcc"), ("x", "%clang")])