Change semantic for providers

alalazo · tgamblin · commit 9bb5cffc73a5 · 2023-08-15T15:54:37.000-07:00
If a possible provider is not used to satisfy a vdep,
then it's not a provider of that vdep.
diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
@@ -455,11 +455,16 @@ error(1, Msg)
 
 % if a package depends on a virtual, it's not external and we have a
 % provider for that virtual then it depends on the provider
-1 { attr("depends_on", PackageNode, ProviderNode, Type) : provider(ProviderNode, node(VirtualID, Virtual)) } 1
+node_depends_on_virtual(PackageNode, Virtual, Type)
   :- dependency_holds(PackageNode, Virtual, Type),
      virtual(Virtual),
      not external(PackageNode).
 
+node_depends_on_virtual(PackageNode, Virtual) :- node_depends_on_virtual(PackageNode, Virtual, Type).
+
+1 { attr("depends_on", PackageNode, ProviderNode, Type) : provider(ProviderNode, node(VirtualID, Virtual)) } 1
+  :- node_depends_on_virtual(PackageNode, Virtual, Type).
+
 attr("virtual_on_edge", PackageNode, ProviderNode, Virtual)
   :- dependency_holds(PackageNode, Virtual, Type),
      attr("depends_on", PackageNode, ProviderNode, Type),
@@ -468,9 +473,7 @@ attr("virtual_on_edge", PackageNode, ProviderNode, Virtual)
 
 % dependencies on virtuals also imply that the virtual is a virtual node
 1 { attr("virtual_node", node(0..X-1, Virtual)) : max_dupes(Virtual, X) }
-  :- dependency_holds(PackageNode, Virtual, Type),
-     virtual(Virtual),
-     not external(PackageNode).
+  :- node_depends_on_virtual(PackageNode, Virtual).
 
 % If there's a virtual node, we must select one and only one provider.
 % The provider must be selected among the possible providers.
@@ -490,13 +493,24 @@ attr("virtual_node", VirtualNode) :- attr("virtual_root", VirtualNode).
 % then the provider is the root package.
 attr("root", PackageNode) :- attr("virtual_root", VirtualNode), provider(PackageNode, VirtualNode).
 
-% If we asked for a root package and that root provides a virtual,
-% the root is a provider for that virtual. This rule is mostly relevant
-% for environments that are concretized together (e.g. where we
-% asks to install "mpich" and "hdf5+mpi" and we want "mpich" to
-% be the mpi provider)
-1 { provider(PackageNode, node(0..X-1, Virtual)) : max_dupes(Virtual, X) } 1 :- attr("node", PackageNode), virtual_condition_holds(PackageNode, Virtual).
-:- 2 { provider(PackageNode, VirtualNode) }, attr("virtual_node", VirtualNode).
+% The provider is selected among the nodes for which the virtual condition holds
+1 { provider(PackageNode, node(X, Virtual)) :
+    attr("node", PackageNode), virtual_condition_holds(PackageNode, Virtual) } 1
+  :- attr("virtual_node", node(X, Virtual)).
+
+% If a spec is selected as a provider, it is for all the virtual it could provide
+:- provider(PackageNode, node(X, Virtual1)),
+   virtual_condition_holds(PackageNode, Virtual2),
+   Virtual2 != Virtual1,
+   unification_set(SetID, PackageNode),
+   unification_set(SetID, node(X, Virtual2)),
+   not provider(PackageNode, node(X, Virtual2)).
+
+% If a spec is a dependency, and could provide a needed virtual, it must be a provider
+:- node_depends_on_virtual(PackageNode, Virtual),
+   depends_on(PackageNode, PossibleProviderNode),
+   virtual_condition_holds(PossibleProviderNode, Virtual),
+   not attr("virtual_on_edge", PackageNode, PossibleProviderNode, Virtual).
 
 % The provider provides the virtual if some provider condition holds.
 virtual_condition_holds(node(ProviderID, Provider), Virtual) :- virtual_condition_holds(ID, node(ProviderID, Provider), Virtual).
diff --git a/lib/spack/spack/test/env.py b/lib/spack/spack/test/env.py
@@ -588,3 +588,35 @@ def test_conflicts_with_packages_that_are_not_dependencies(
         else:
             e.concretize()
             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):
+    """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(
+        """\
+    spack:
+      specs:
+      - mpich
+      - mpich2
+      - mpileaks
+      packages:
+        mpi:
+          require: mpich2
+    """
+    )
+    with ev.Environment(manifest.parent) as e:
+        e.concretize()
+        assert e.matching_spec("mpich")
+        assert e.matching_spec("mpich2")
+
+        mpileaks = e.matching_spec("mpileaks")
+        assert mpileaks.satisfies("^mpich2")
+        assert mpileaks["mpi"].satisfies("mpich2")
+        assert not mpileaks.satisfies("^mpich")
diff --git a/share/spack/gitlab/cloud_pipelines/stacks/e4s-oneapi/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/e4s-oneapi/spack.yaml
@@ -148,7 +148,7 @@ spack:
   - nco
   - netlib-scalapack
   - omega-h
-  # - openmpi
+  - openmpi
   - openpmd-api
   - papi
   - papyrus
