ASP-based solver: don't declare deprecated versions unless required (#38181)

Currently, the concretizer emits facts for all versions known to Spack, including deprecated versions, and has a specific optimization objective to minimize their use.

This commit simplifies how deprecated versions are handled by considering possible versions for a spec only if they appear in a spec literal, or if the `config:deprecated:true` is set directly or through the `--deprecated` flag. The optimization objective has also been removed, in favor of just ordering versions and having deprecated ones last.

This results in:

a) no delayed errors on install, but concretization errors when deprecated versions would be the only option. This is in particular relevant for CI where it's better to get errors early
b) a slight concretization speed-up due to fewer facts
c) a simplification of the logic program.

Co-authored-by: Harmen Stoppels <me@harmenstoppels.nl>

8 files changed, 91 insertions, 42 deletions

diff --git a/lib/spack/spack/cmd/solve.py b/lib/spack/spack/cmd/solve.py
index e27687dc21..9b6d843848 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,
+            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,
+                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 5a46d1f999..758e618c1a 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), 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 bf61444f9a..cc31f50e89 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -1395,7 +1395,10 @@ class Environment:
 
         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, 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 6ddca741ce..a2f4018f31 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -13,7 +13,7 @@ import pprint
 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
 
@@ -137,7 +137,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
@@ -784,7 +792,7 @@ class PyclingoDriver:
         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, deprecated=False):
         """Set up the input and solve for dependencies of ``specs``.
 
         Arguments:
@@ -795,6 +803,7 @@ class PyclingoDriver:
                 the output of this solve.
             control (clingo.Control): configuration for the solver. If None,
                 default values will be used
+            deprecated: if True, allow deprecated versions in the solve
 
         Return:
             A tuple of the solve result, the timer for the different phases of the
@@ -814,7 +823,7 @@ class PyclingoDriver:
         timer.start("setup")
         with self.control.backend() as backend:
             self.backend = backend
-            setup.setup(self, specs, reuse=reuse)
+            setup.setup(self, specs, reuse=reuse, deprecated=deprecated)
         timer.stop("setup")
 
         timer.start("load")
@@ -1894,7 +1903,7 @@ class SpackSolverSetup:
         return clauses
 
     def define_package_versions_and_validate_preferences(
-        self, possible_pkgs, require_checksum: bool
+        self, possible_pkgs, *, require_checksum: bool, deprecated: bool
     ):
         """Declare any versions in specs not declared in packages."""
         packages_yaml = spack.config.get("packages")
@@ -1914,13 +1923,15 @@ class SpackSolverSetup:
                 ]
 
             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 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
@@ -1949,7 +1960,7 @@ class SpackSolverSetup:
                 )
                 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, *, 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
@@ -2329,7 +2340,14 @@ class SpackSolverSetup:
                 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,
+        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
@@ -2337,9 +2355,10 @@ class SpackSolverSetup:
         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
+            deprecated: if True adds deprecated versions into the solve
         """
         self._condition_id_counter = itertools.count()
 
@@ -2368,7 +2387,7 @@ class SpackSolverSetup:
         # 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(
@@ -2414,11 +2433,17 @@ class SpackSolverSetup:
         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, deprecated=deprecated, require_checksum=checksummed
+        )
+        self.define_ad_hoc_versions_from_specs(
+            specs, Provenance.SPEC, require_checksum=checksummed
+        )
+        self.define_ad_hoc_versions_from_specs(
+            dev_specs, Provenance.DEV_SPEC, require_checksum=checksummed
+        )
+        self.validate_and_define_versions_from_requirements(require_checksum=checksummed)
 
         self.gen.h1("Package Constraints")
         for pkg in sorted(self.pkgs):
@@ -2467,7 +2492,7 @@ class SpackSolverSetup:
             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, *, 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
@@ -2932,7 +2957,16 @@ class Solver:
 
         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,
+        deprecated=False,
+    ):
         """
         Arguments:
           specs (list): List of ``Spec`` objects to solve for.
@@ -2943,6 +2977,7 @@ class Solver:
             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).
+          deprecated (bool): allow deprecated version in the solve
         """
         # Check upfront that the variants are admissible
         specs = [s.lookup_hash() for s in specs]
@@ -2950,10 +2985,14 @@ class Solver:
         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, deprecated=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, deprecated=False
+    ):
         """Solve for a stable model of specs in multiple rounds.
 
         This relaxes the assumption of solve that everything must be consistent and
@@ -2968,6 +3007,7 @@ class Solver:
             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
+            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)
@@ -2981,7 +3021,7 @@ class Solver:
         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, deprecated=deprecated
             )
             yield result
 
diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index 9d1b48b379..a4d1dd10a3 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -1361,16 +1361,6 @@ opt_criterion(75, "requirement weight").
       build_priority(PackageNode, Priority)
 }.
 
-% Minimize the number of deprecated versions being used
-opt_criterion(73, "deprecated versions used").
-#minimize{ 0@273: #true }.
-#minimize{ 0@73: #true }.
-#minimize{
-    1@73+Priority,PackageNode
-    : attr("deprecated", PackageNode, _),
-      build_priority(PackageNode, Priority)
-}.
-
 % Minimize the:
 % 1. Version weight
 % 2. Number of variants with a non default value, if not set
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 9a0d4592b8..a79d7172ca 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -2963,8 +2963,9 @@ class Spec:
         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, 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 45aadf6fa7..8bf7da12ed 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -1356,7 +1356,7 @@ class TestConcretize:
             # 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@=1.1.0", ["deprecated-versions@1.1.0"]),
         ],
     )
     @pytest.mark.only_clingo("Use case not supported by the original concretizer")
diff --git a/lib/spack/spack/test/concretize_requirements.py b/lib/spack/spack/test/concretize_requirements.py
index d7c1c88359..7464282d25 100644
--- a/lib/spack/spack/test/concretize_requirements.py
+++ b/lib/spack/spack/test/concretize_requirements.py
@@ -545,13 +545,13 @@ packages:
 
 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,
+    # 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)