concretize.lp: drop 0 weight of external providers (#45025)

If an external happens to be a provider of anything, the solver would
set its weight to 0, meaning that it is most preferred, even if
packages.yaml config disagrees.

That was done so that `spack external find mpich` would be sufficent to
pick it up as mpi provider.

That may have made sense for mpi specifically, but doesn't make sense
for other virtuals. For example `glibc` provides iconv, and is an
external by design, but it's better to use libiconv as a separate
package as a provider.

Therefore, drop this rule, and instead let users add config:

```
mpi:
  require: [mpich]
```

or

```
mpi:
  buildable: false
```

which is well-documented.

lib/spack/spack/solver/concretize.lp

@@ -611,25 +611,18 @@ do_not_impose(EffectID, node(X, Package))
 % Virtual dependency weights
 %-----------------------------------------------------------------------------
 
-% A provider may have different possible weights depending on whether it's an external
+% A provider has different possible weights depending on its preference. This rule ensures that
-% or not, or on preferences expressed in packages.yaml etc. This rule ensures that
 % we select the weight, among the possible ones, that minimizes the overall objective function.
 1 { provider_weight(DependencyNode, VirtualNode, Weight) :
     possible_provider_weight(DependencyNode, VirtualNode, Weight, _) } 1
  :- provider(DependencyNode, VirtualNode), internal_error("Package provider weights must be unique").
 
-% A provider that is an external can use a weight of 0
+% Any configured provider has a weight based on index in the preference list
-possible_provider_weight(DependencyNode, VirtualNode, 0, "external")
-  :- provider(DependencyNode, VirtualNode),
-     external(DependencyNode).
-
-% A provider mentioned in the default configuration can use a weight
-% according to its priority in the list of providers
 possible_provider_weight(node(ProviderID, Provider), node(VirtualID, Virtual), Weight, "default")
   :- provider(node(ProviderID, Provider), node(VirtualID, Virtual)),
      default_provider_preference(Virtual, Provider, Weight).
 
-% Any provider can use 100 as a weight, which is very high and discourage its use
+% Any non-configured provider has a default weight of 100
 possible_provider_weight(node(ProviderID, Provider), VirtualNode, 100, "fallback")
   :- provider(node(ProviderID, Provider), VirtualNode).
 

lib/spack/spack/test/concretize.py

@@ -688,7 +688,8 @@ def test_nobuild_package(self):
         with pytest.raises(spack.error.SpecError):
             spec.concretize()
 
-    def test_external_and_virtual(self):
+    def test_external_and_virtual(self, mutable_config):
+        mutable_config.set("packages:stuff", {"buildable": False})
         spec = Spec("externaltest")
         spec.concretize()
         assert spec["externaltool"].external_path == os.path.sep + os.path.join(
@@ -1170,16 +1171,14 @@ def test_external_that_would_require_a_virtual_dependency(self):
         assert s.external
         assert "stuff" not in s
 
-    def test_transitive_conditional_virtual_dependency(self):
+    def test_transitive_conditional_virtual_dependency(self, mutable_config):
+        """Test that an external is used as provider if the virtual is non-buildable"""
+        mutable_config.set("packages:stuff", {"buildable": False})
         s = Spec("transitive-conditional-virtual-dependency").concretized()
 
-        # The default for conditional-virtual-dependency is to have
+        # Test that the default +stuff~mpi is maintained, and the right provider is selected
-        # +stuff~mpi, so check that these defaults are respected
+        assert s.satisfies("^conditional-virtual-dependency +stuff~mpi")
-        assert "+stuff" in s["conditional-virtual-dependency"]
+        assert s.satisfies("^[virtuals=stuff] externalvirtual")
-        assert "~mpi" in s["conditional-virtual-dependency"]
-
-        # 'stuff' is provided by an external package, so check it's present
-        assert "externalvirtual" in s
 
     @pytest.mark.regression("20040")
     @pytest.mark.only_clingo("Use case not supported by the original concretizer")
@@ -1785,7 +1784,7 @@ def test_reuse_with_unknown_package_dont_raise(self, tmpdir, temporary_store, mo
         [
             (["libelf", "libelf@0.8.10"], 1, 1),
             (["libdwarf%gcc", "libelf%clang"], 2, 1),
-            (["libdwarf%gcc", "libdwarf%clang"], 3, 2),
+            (["libdwarf%gcc", "libdwarf%clang"], 3, 1),
             (["libdwarf^libelf@0.8.12", "libdwarf^libelf@0.8.13"], 4, 1),
             (["hdf5", "zmpi"], 3, 1),
             (["hdf5", "mpich"], 2, 1),

lib/spack/spack/test/conftest.py

@@ -857,7 +857,7 @@ def _install(spec):
     _install("mpileaks ^mpich")
     _install("mpileaks ^mpich2")
     _install("mpileaks ^zmpi")
-    _install("externaltest")
+    _install("externaltest ^externalvirtual")
     _install("trivial-smoke-test")