Do not impose provider conditions, if the node is not a provider (#39456)

* Do not impose provider conditions, if the node is not a provider

fixes #39455

When a node can be a provider of a spec, but is not selected as
a provider, we should not be imposing provider conditions on the
virtual.

* Adjust the integrity constraint, by using the correct atom

2 files changed, 30 insertions, 8 deletions

diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index 3d35eea3ff..ba3045737d 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -124,6 +124,9 @@ attr(Name, node(min_dupe_id, A1), A2, A3, A4) :- literal(LiteralID, Name, A1, A2
 attr("node_flag_source", node(min_dupe_id, A1), A2, node(min_dupe_id, A3)) :- literal(LiteralID, "node_flag_source", A1, A2, A3), solve_literal(LiteralID).
 attr("depends_on",       node(min_dupe_id, A1), node(min_dupe_id, A2), A3) :- literal(LiteralID, "depends_on",       A1, A2, A3), solve_literal(LiteralID).
 
+% Discriminate between "roots" that have been explicitly requested, and roots that are deduced from "virtual roots"
+explicitly_requested_root(node(min_dupe_id, A1)) :- literal(LiteralID, "root", A1), solve_literal(LiteralID).
+
 #defined concretize_everything/0.
 #defined literal/1.
 #defined literal/3.
@@ -519,6 +522,19 @@ virtual_condition_holds(ID, node(ProviderID, Provider), Virtual) :-
    condition_holds(ID, node(ProviderID, Provider)),
    virtual(Virtual).
 
+% If a "provider" condition holds, but this package is not a provider, do not impose the "provider" condition
+do_not_impose(EffectID, node(X, Package))
+  :- virtual_condition_holds(ID, node(X, Package), Virtual),
+     pkg_fact(Package, condition_effect(ID, EffectID)),
+     not provider(node(X, Package), node(_, Virtual)).
+
+% Choose the provider among root specs, if possible
+:- provider(ProviderNode, node(min_dupe_id, Virtual)),
+   virtual_condition_holds(_, PossibleProvider, Virtual),
+   PossibleProvider != ProviderNode,
+   explicitly_requested_root(PossibleProvider),
+   not explicitly_requested_root(ProviderNode).
+
 % A package cannot be the actual provider for a virtual if it does not
 % fulfill the conditions to provide that virtual
 :- provider(PackageNode, node(VirtualID, Virtual)),
diff --git a/lib/spack/spack/test/env.py b/lib/spack/spack/test/env.py
index 3f4fd3701c..4396557f8a 100644
--- a/lib/spack/spack/test/env.py
+++ b/lib/spack/spack/test/env.py
@@ -587,33 +587,39 @@ spack:
             assert any(s.satisfies(expected_spec) for s in e.concrete_roots())
 
 
-def test_requires_on_virtual_and_potential_providers(tmp_path, mock_packages, config):
+@pytest.mark.regression("39455")
+@pytest.mark.only_clingo("Known failure of the original concretizer")
+@pytest.mark.parametrize(
+    "possible_mpi_spec,unify", [("mpich", False), ("mpich", True), ("zmpi", False), ("zmpi", True)]
+)
+def test_requires_on_virtual_and_potential_providers(
+    possible_mpi_spec, unify, tmp_path, mock_packages, config
+):
     """Tests that in an environment we can add packages explicitly, even though they provide
     a virtual package, and we require the provider of the same virtual to be another package,
     if they are added explicitly by their name.
     """
-    if spack.config.get("config:concretizer") == "original":
-        pytest.xfail("Known failure of the original concretizer")
-
     manifest = tmp_path / "spack.yaml"
     manifest.write_text(
-        """\
+        f"""\
     spack:
       specs:
-      - mpich
+      - {possible_mpi_spec}
       - mpich2
       - mpileaks
       packages:
         mpi:
           require: mpich2
+      concretizer:
+        unify: {unify}
     """
     )
     with ev.Environment(manifest.parent) as e:
         e.concretize()
-        assert e.matching_spec("mpich")
+        assert e.matching_spec(possible_mpi_spec)
         assert e.matching_spec("mpich2")
 
         mpileaks = e.matching_spec("mpileaks")
         assert mpileaks.satisfies("^mpich2")
         assert mpileaks["mpi"].satisfies("mpich2")
-        assert not mpileaks.satisfies("^mpich")
+        assert not mpileaks.satisfies(f"^{possible_mpi_spec}")