From a3bc9dbfe8fa6cdb7bec32063f264a7d799689d4 Mon Sep 17 00:00:00 2001
From: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Date: Fri, 24 May 2024 19:06:28 +0200
Subject: [PATCH] Make strong preferences even stronger (#44373)

Before this PR, if Spack could see a possibility to reuse a spec that
doesn't match a strong preference, it would do so. After the PR, a
strong preference would take precedence.
---
 lib/spack/spack/solver/concretize.lp          | 28 ++++++------
 .../spack/test/concretize_requirements.py     | 43 +++++++++++++++++++
 2 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index 4d8a202ce3..20bcd46ad6 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -1345,8 +1345,10 @@ build(PackageNode) :- not attr("hash", PackageNode, _), attr("node", PackageNode
 % topmost-priority criterion to reuse what is installed.
 %
 % The priority ranges are:
-%   200+        Shifted priorities for build nodes; correspond to priorities 0 - 99.
-%   100 - 199   Unshifted priorities. Currently only includes minimizing #builds.
+%   1000+       Optimizations for concretization errors
+%   300 - 1000  Highest priority optimizations for valid solutions
+%   200 - 299   Shifted priorities for build nodes; correspond to priorities 0 - 99.
+%   100 - 199   Unshifted priorities. Currently only includes minimizing #builds and minimizing dupes.
 %   0   -  99   Priorities for non-built nodes.
 build_priority(PackageNode, 200) :- build(PackageNode), attr("node", PackageNode).
 build_priority(PackageNode, 0)   :- not build(PackageNode), attr("node", PackageNode).
@@ -1394,6 +1396,16 @@ build_priority(PackageNode, 0)   :- not build(PackageNode), attr("node", Package
 %   2. a `#minimize{ 0@2 : #true }.` statement that ensures the criterion
 %      is displayed (clingo doesn't display sums over empty sets by default)
 
+% A condition group specifies one or more specs that must be satisfied.
+% Specs declared first are preferred, so we assign increasing weights and
+% minimize the weights.
+opt_criterion(310, "requirement weight").
+#minimize{ 0@310: #true }.
+#minimize {
+    Weight@310,PackageNode,Group
+    : requirement_weight(PackageNode, Group, Weight)
+}.
+
 % Try hard to reuse installed packages (i.e., minimize the number built)
 opt_criterion(110, "number of packages to build (vs. reuse)").
 #minimize { 0@110: #true }.
@@ -1405,18 +1417,6 @@ opt_criterion(100, "number of nodes from the same package").
 #minimize { ID@100,Package : attr("virtual_node", node(ID, Package)) }.
 #defined optimize_for_reuse/0.
 
-% A condition group specifies one or more specs that must be satisfied.
-% Specs declared first are preferred, so we assign increasing weights and
-% minimize the weights.
-opt_criterion(75, "requirement weight").
-#minimize{ 0@275: #true }.
-#minimize{ 0@75: #true }.
-#minimize {
-    Weight@75+Priority,PackageNode,Group
-    : requirement_weight(PackageNode, Group, Weight),
-      build_priority(PackageNode, Priority)
-}.
-
 % Minimize the number of deprecated versions being used
 opt_criterion(73, "deprecated versions used").
 #minimize{ 0@273: #true }.
diff --git a/lib/spack/spack/test/concretize_requirements.py b/lib/spack/spack/test/concretize_requirements.py
index ea1dc526df..960ef90a3a 100644
--- a/lib/spack/spack/test/concretize_requirements.py
+++ b/lib/spack/spack/test/concretize_requirements.py
@@ -1176,3 +1176,46 @@ def test_forward_multi_valued_variant_using_requires(
 
     for constraint in not_expected:
         assert not s.satisfies(constraint)
+
+
+def test_strong_preferences_higher_priority_than_reuse(concretize_scope, mock_packages):
+    """Tests that strong preferences have a higher priority than reusing specs."""
+    reused_spec = Spec("adios2~bzip2").concretized()
+    reuse_nodes = list(reused_spec.traverse())
+    root_specs = [Spec("ascent+adios2")]
+
+    # Check that without further configuration adios2 is reused
+    with spack.config.override("concretizer:reuse", True):
+        solver = spack.solver.asp.Solver()
+        setup = spack.solver.asp.SpackSolverSetup()
+        result, _, _ = solver.driver.solve(setup, root_specs, reuse=reuse_nodes)
+        ascent = result.specs[0]
+    assert ascent["adios2"].dag_hash() == reused_spec.dag_hash(), ascent
+
+    # If we stick a preference, adios2 is not reused
+    update_packages_config(
+        """
+    packages:
+      adios2:
+        prefer:
+        - "+bzip2"
+"""
+    )
+    with spack.config.override("concretizer:reuse", True):
+        solver = spack.solver.asp.Solver()
+        setup = spack.solver.asp.SpackSolverSetup()
+        result, _, _ = solver.driver.solve(setup, root_specs, reuse=reuse_nodes)
+        ascent = result.specs[0]
+
+    assert ascent["adios2"].dag_hash() != reused_spec.dag_hash()
+    assert ascent["adios2"].satisfies("+bzip2")
+
+    # A preference is still preference, so we can override from input
+    with spack.config.override("concretizer:reuse", True):
+        solver = spack.solver.asp.Solver()
+        setup = spack.solver.asp.SpackSolverSetup()
+        result, _, _ = solver.driver.solve(
+            setup, [Spec("ascent+adios2 ^adios2~bzip2")], reuse=reuse_nodes
+        )
+        ascent = result.specs[0]
+    assert ascent["adios2"].dag_hash() == reused_spec.dag_hash(), ascent