concretizer: modified weights for providers and matching for externals

This commit address the case of concretizing a root spec with a
transitive conditional dependency on a virtual package, provided
by an external. Before these modifications default variant values
for the dependency bringing in the virtual package were not
respected, and the external package providing the virtual was added
to the DAG.

The issue stems from two facts:
- Selecting a provider has higher precedence than selecting default variants
- To ensure that an external is preferred, we used a negative weight

To solve it we shift all the providers weight so that:
- External providers have a weight of 0
- Non external provider have a weight of 10 or more

Using a weight of zero for external providers is such that having
an external provider, if present, or not having a provider at all
has the same effect on the higher priority minimization.

Also fixed a few minor bugs in concretize.lp, that were causing
spurious entries in the final answer set.

Cleaned concretize.lp from leftover rules.

6 files changed, 56 insertions, 35 deletions

diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index f0f2b410e8..da24486c5c 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -634,7 +634,7 @@ class PyclingoDriver(object):
 
         # With a grounded program, we can run the solve.
         result = Result()
-        models = []  # stable moodels if things go well
+        models = []  # stable models if things go well
         cores = []   # unsatisfiable cores if they do not
 
         def on_model(model):
@@ -973,7 +973,7 @@ class SpackSolverSetup(object):
                 continue
 
             for i, provider in enumerate(providers):
-                func(vspec, provider, i)
+                func(vspec, provider, i + 10)
 
     def provider_defaults(self):
         self.gen.h2("Default virtual providers")
diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index baf1c5239f..b27b80dc6e 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -57,10 +57,11 @@ provider(Package, Virtual)
   :- node(Package), provides_virtual(Package, Virtual).
 
 % for any virtual, there can be at most one provider in the DAG
-0 { provider(Package, Virtual) : node(Package) } 1 :- virtual(Virtual).
+0 { provider(Package, Virtual) :
+    node(Package), provides_virtual(Package, Virtual) } 1 :- virtual(Virtual).
 
 % give dependents the virtuals they want
-provider_weight(Dependency, -10)
+provider_weight(Dependency, 0)
  :- virtual(Virtual), depends_on(Package, Dependency),
     provider(Dependency, Virtual),
     external(Dependency).
@@ -84,6 +85,7 @@ provider_weight(Dependency, 100)
  :- virtual(Virtual),
     provider(Dependency, Virtual),
     depends_on(Package, Dependency),
+    not external(Dependency),
     not pkg_provider_preference(Package, Virtual, Dependency, _),
     not default_provider_preference(Virtual, Dependency, _).
 
@@ -355,37 +357,27 @@ node_compiler_version(Package, Compiler, Version) :- node_compiler_version_hard(
    not compiler_supports_os(Compiler, Version, OS),
    not allow_compiler(Compiler, Version).
 
-% dependencies imply we should try to match hard compiler constraints
-% todo: look at what to do about intersecting constraints here. we'd
-% ideally go with the "lowest" pref in the DAG
-node_compiler_match_pref(Package, Compiler)
- :- node_compiler_hard(Package, Compiler).
-node_compiler_match_pref(Dependency, Compiler)
- :- depends_on(Package, Dependency),
-    node_compiler_match_pref(Package, Compiler),
-    not node_compiler_hard(Dependency, _).
-compiler_match(Package, 1)
- :- node_compiler(Package, Compiler),
-    node_compiler_match_pref(Package, Compiler).
-
 % If the compiler is what was prescribed from command line etc.
 % or is the same as a root node, there is a version match
 
-% Compiler prescribed from command line
+% Compiler prescribed in the root spec
 node_compiler_version_match_pref(Package, Compiler, V)
  :- node_compiler_hard(Package, Compiler),
-    node_compiler_version(Package, Compiler, V).
+    node_compiler_version(Package, Compiler, V),
+    not external(Package).
 
-% Compiler inherited from a root node
+% Compiler inherited from a parent node
 node_compiler_version_match_pref(Dependency, Compiler, V)
  :- depends_on(Package, Dependency),
     node_compiler_version_match_pref(Package, Compiler, V),
+    node_compiler_version(Dependency, Compiler, V),
     not node_compiler_hard(Dependency, Compiler).
 
 % Compiler inherited from the root package
 node_compiler_version_match_pref(Dependency, Compiler, V)
  :- depends_on(Package, Dependency),
     node_compiler_version(Package, Compiler, V), root(Package),
+    node_compiler_version(Dependency, Compiler, V),
     not node_compiler_hard(Dependency, Compiler).
 
 compiler_version_match(Package, 1)
@@ -516,25 +508,18 @@ root(Dependency, 1) :- not root(Dependency), node(Dependency).
     : provider_weight(Provider, Weight), root(Provider)
 }.
 
-% Next, we want to minimize:
-% 1. The weights of the providers
-% 2. The version weight of the providers
-% i.e. use as much as possible the most preferred
-% providers at latest versions
+% Next, we want to minimize the weights of the providers
+% i.e. use as much as possible the most preferred providers
 #minimize{
     Weight@11,Provider
     : provider_weight(Provider, Weight), not root(Provider)
 }.
-#minimize{
-    Weight@10,Provider
-    : provider_weight(Provider, _), version_weight(Provider, Weight)
-}.
 
 % For external packages it's more important than for others
-% to match the compiler
-#minimize{
+% to match the compiler with their parent node
+#maximize{
     Weight@10,Package
-    : compiler_weight(Package, Weight), external(Package)
+    : compiler_version_match(Package, Weight), external(Package)
 }.
 
 % Then try to use as much as possible:
diff --git a/lib/spack/spack/solver/display.lp b/lib/spack/spack/solver/display.lp
index 40f6acdb05..22642e2602 100644
--- a/lib/spack/spack/solver/display.lp
+++ b/lib/spack/spack/solver/display.lp
@@ -17,12 +17,10 @@
 #show node_flag_source/2.
 #show no_flags/2.
 
-
-
 #show variant_not_default/4.
 #show provider_weight/2.
 #show version_weight/2.
-#show compiler_match/2.
+#show compiler_version_match/2.
 #show compiler_weight/2.
 #show node_target_match/2.
 #show node_target_weight/2.
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index 496153aab6..761641035f 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -872,3 +872,14 @@ class TestConcretize(object):
 
         assert s.external
         assert 'stuff' not in s
+
+    def test_transitive_conditional_virtual_dependency(self):
+        s = Spec('transitive-conditional-virtual-dependency').concretized()
+
+        # The default for conditional-virtual-dependency is to have
+        # +stuff~mpi, so check that these defaults are respected
+        assert '+stuff' in s['conditional-virtual-dependency']
+        assert '~mpi' in s['conditional-virtual-dependency']
+
+        # 'stuff' is provided by an external package, so check it's present
+        assert 'externalvirtual' in s
diff --git a/var/spack/repos/builtin.mock/packages/conditional-virtual-dependency/package.py b/var/spack/repos/builtin.mock/packages/conditional-virtual-dependency/package.py
new file mode 100644
index 0000000000..8cfbfa0c1a
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/conditional-virtual-dependency/package.py
@@ -0,0 +1,15 @@
+# Copyright 2013-2020 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+class ConditionalVirtualDependency(Package):
+    """Brings in a virtual dependency if certain conditions are met."""
+    homepage = "https://dev.null"
+
+    version('1.0')
+
+    variant('stuff', default=True, description='nope')
+    variant('mpi', default=False, description='nope')
+
+    depends_on('stuff', when='+stuff')
+    depends_on('mpi', when='+mpi')
diff --git a/var/spack/repos/builtin.mock/packages/transitive-conditional-virtual-dependency/package.py b/var/spack/repos/builtin.mock/packages/transitive-conditional-virtual-dependency/package.py
new file mode 100644
index 0000000000..9b1b66df7f
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/transitive-conditional-virtual-dependency/package.py
@@ -0,0 +1,12 @@
+# Copyright 2013-2020 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+class TransitiveConditionalVirtualDependency(Package):
+    """Depends on a package with a conditional virtual dependency."""
+    homepage = "https://dev.null"
+    has_code = False
+    phases = []
+
+    version('1.0')
+    depends_on('conditional-virtual-dependency')