concretizer: handle conflicts with compiler ranges correctly

As reported, conflicts with compiler ranges were not treated
correctly. This commit adds tests to verify the expected behavior
for the new concretizer.

The new rules to enforce a correct behavior involve:
- Adding a rule to prefer the compiler selected for
  the root package, if no other preference is set
- Give a strong negative weight to compiler preferences
  expressed in packages.yaml
- Maximize on compiler AND compiler version match

5 files changed, 70 insertions, 21 deletions

diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index a79f3c7695..c1e483577f 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -771,9 +771,7 @@ class SpackSolverSetup(object):
                 # TODO: find a better way to generate clauses for integrity
                 # TODO: constraints, instead of generating them for the body
                 # TODO: of a rule and filter unwanted functions.
-                to_be_filtered = [
-                    'node_compiler_hard', 'node_compiler_version_satisfies'
-                ]
+                to_be_filtered = ['node_compiler_hard']
                 clauses = [x for x in clauses if x.name not in to_be_filtered]
                 external = fn.external(pkg.name)
 
@@ -835,9 +833,10 @@ class SpackSolverSetup(object):
         ppk = spack.package_prefs.PackagePrefs(pkg.name, 'compiler', all=False)
         matches = sorted(compiler_list, key=ppk)
 
-        for i, cspec in enumerate(matches):
+        for i, cspec in enumerate(reversed(matches)):
             self.gen.fact(fn.node_compiler_preference(
-                pkg.name, cspec.name, cspec.version, i))
+                pkg.name, cspec.name, cspec.version, -i * 100
+            ))
 
     def pkg_rules(self, pkg, tests):
         pkg = packagize(pkg)
@@ -1438,7 +1437,7 @@ class SpackSolverSetup(object):
         possible = spack.package.possible_dependencies(
             *specs,
             virtuals=self.possible_virtuals,
-            deptype=("build", "link", "run", "test")
+            deptype=spack.dependency.all_deptypes
         )
         pkgs = set(possible)
 
diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index 1cbccce74d..d6e63f3606 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -61,15 +61,22 @@ provider(Package, Virtual)
 0 { provider(Package, Virtual) : node(Package) } 1 :- virtual(Virtual).
 
 % give dependents the virtuals they want
+provider_weight(Dependency, -10)
+ :- virtual(Virtual), depends_on(Package, Dependency),
+    provider(Dependency, Virtual),
+    external(Dependency).
+
 provider_weight(Dependency, Weight)
  :- virtual(Virtual), depends_on(Package, Dependency),
     provider(Dependency, Virtual),
-    pkg_provider_preference(Package, Virtual, Dependency, Weight).
+    pkg_provider_preference(Package, Virtual, Dependency, Weight),
+    not external(Dependency).
 
 provider_weight(Dependency, Weight)
  :- virtual(Virtual), depends_on(Package, Dependency),
     provider(Dependency, Virtual),
     not pkg_provider_preference(Package, Virtual, Dependency, _),
+    not external(Dependency),
     default_provider_preference(Virtual, Dependency, Weight).
 
 % if there's no preference for something, it costs 100 to discourage its
@@ -347,13 +354,26 @@ 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
 node_compiler_version_match_pref(Package, Compiler, V)
  :- node_compiler_hard(Package, Compiler),
     node_compiler_version(Package, Compiler, V).
+
+% Compiler inherited from a root node
 node_compiler_version_match_pref(Dependency, Compiler, V)
  :- depends_on(Package, Dependency),
     node_compiler_version_match_pref(Package, 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),
+    not node_compiler_hard(Dependency, Compiler).
+
 compiler_version_match(Package, 1)
  :- node_compiler_version(Package, Compiler, V),
     node_compiler_version_match_pref(Package, Compiler, V).
@@ -371,13 +391,13 @@ compiler_weight(Package, Weight)
 compiler_weight(Package, Weight)
  :- node_compiler(Package, Compiler),
     node_compiler_version(Package, Compiler, V),
-    not node_compiler_preference(Package, Compiler, _, _),
+    not node_compiler_preference(Package, Compiler, V, _),
     default_compiler_preference(Compiler, V, Weight).
 compiler_weight(Package, 100)
  :- node_compiler(Package, Compiler),
     node_compiler_version(Package, Compiler, Version),
-    not node_compiler_preference(Package, Compiler, _, _),
-    not default_compiler_preference(Compiler, _, _).
+    not node_compiler_preference(Package, Compiler, Version, _),
+    not default_compiler_preference(Compiler, Version, _).
 
 #defined node_compiler_preference/4.
 #defined default_compiler_preference/3.
@@ -464,7 +484,7 @@ root(Dependency, 1) :- not root(Dependency), node(Dependency).
 }.
 
 % compiler preferences
-#maximize{ Weight@8,Package : compiler_match(Package, Weight) }.
+#maximize{ Weight@8,Package : compiler_version_match(Package, Weight) }.
 #minimize{ Weight@7,Package : compiler_weight(Package, Weight) }.
 
 % prefer more recent versions.
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index 6072d8957e..f80eb41981 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -14,7 +14,7 @@ import spack.error
 import spack.repo
 
 from spack.concretize import find_spec
-from spack.spec import Spec, CompilerSpec
+from spack.spec import Spec
 from spack.version import ver
 from spack.util.mock_package import MockPackageMultiRepo
 import spack.compilers
@@ -351,14 +351,14 @@ class TestConcretize(object):
         spec.normalize()
         spec.concretize()
 
-    def test_compiler_inheritance(self):
-        spec = Spec('mpileaks')
-        spec.normalize()
-        spec['dyninst'].compiler = CompilerSpec('clang')
-        spec.concretize()
-        # TODO: not exactly the syntax I would like.
-        assert spec['libdwarf'].compiler.satisfies('clang')
-        assert spec['libelf'].compiler.satisfies('clang')
+    @pytest.mark.parametrize('compiler_str', [
+        'clang', 'gcc', 'gcc@4.5.0', 'clang@:3.3.0'
+    ])
+    def test_compiler_inheritance(self, compiler_str):
+        spec_str = 'mpileaks %{0}'.format(compiler_str)
+        spec = Spec(spec_str).concretized()
+        assert spec['libdwarf'].compiler.satisfies(compiler_str)
+        assert spec['libelf'].compiler.satisfies(compiler_str)
 
     def test_external_package(self):
         spec = Spec('externaltool%gcc')
@@ -660,3 +660,18 @@ class TestConcretize(object):
         with pytest.raises(spack.error.SpackError):
             s = Spec(spec_str)
             s.concretize()
+
+    @pytest.mark.parametrize('spec_str,expected_str', [
+        # Unconstrained versions select default compiler (gcc@4.5.0)
+        ('bowtie@1.3.0', '%gcc@4.5.0'),
+        # Version with conflicts and no valid gcc select another compiler
+        ('bowtie@1.2.2', '%clang@3.3'),
+        # If a higher gcc is available still prefer that
+        ('bowtie@1.2.2 os=redhat6', '%gcc@4.7.2'),
+    ])
+    def test_compiler_conflicts_in_package_py(self, spec_str, expected_str):
+        if spack.config.get('config:concretizer') == 'original':
+            pytest.skip('Original concretizer cannot work around conflicts')
+
+        s = Spec(spec_str).concretized()
+        assert s.satisfies(expected_str)
diff --git a/var/spack/repos/builtin.mock/packages/bowtie/package.py b/var/spack/repos/builtin.mock/packages/bowtie/package.py
new file mode 100644
index 0000000000..3f8363c2a8
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/bowtie/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 Bowtie(Package):
+    """Mock package to test conflicts on compiler ranges"""
+
+    homepage = "http://www.example.org"
+    url = "http://bowtie-1.2.2.tar.bz2"
+
+    version('1.3.0', '1c837ecd990bb022d07e7aab32b09847')
+    version('1.2.2', '1c837ecd990bb022d07e7aab32b09847')
+    version('1.2.0', '1c837ecd990bb022d07e7aab32b09847')
+
+    conflicts('%gcc@:4.5.0', when='@1.2.2')
diff --git a/var/spack/repos/builtin/packages/akantu/package.py b/var/spack/repos/builtin/packages/akantu/package.py
index c58225261f..939c14483b 100644
--- a/var/spack/repos/builtin/packages/akantu/package.py
+++ b/var/spack/repos/builtin/packages/akantu/package.py
@@ -49,7 +49,7 @@ class Akantu(CMakePackage):
 
     extends('python', when='+python')
 
-    conflicts('gcc@:5.3.99')
+    conflicts('%gcc@:5.3.99')
     conflicts('@:3.0.99 external_solvers=petsc')
     conflicts('@:3.0.99 +python')