concretizer: swap priority of selecting provider and default variant (#20182)

refers #20040

Before this PR optimization rules would have selected default
providers at a higher priority than default variants. Here we
swap this priority and we consider variants that are forced by
any means (root spec or spec in depends_on clause) the same as
if they were with a default value.

This prevents the solver from avoiding expected configurations
just because they contain directives like:

depends_on('pkg+foo')

and `+foo` is not the default variant value for pkg.

5 files changed, 72 insertions, 13 deletions

diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index d012bc3cf3..d8b7c125be 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -194,6 +194,7 @@ variant_value(Package, Variant, Value)
 variant_not_default(Package, Variant, Value, 1)
  :- variant_value(Package, Variant, Value),
     not variant_default_value(Package, Variant, Value),
+    not variant_set(Package, Variant, Value),
     node(Package).
 
 variant_not_default(Package, Variant, Value, 0)
@@ -201,6 +202,12 @@ variant_not_default(Package, Variant, Value, 0)
     variant_default_value(Package, Variant, Value),
     node(Package).
 
+variant_not_default(Package, Variant, Value, 0)
+ :- variant_value(Package, Variant, Value),
+    variant_set(Package, Variant, Value),
+    node(Package).
+
+
 % The default value for a variant in a package is what is written
 % in the package.py file, unless some preference is set in packages.yaml
 variant_default_value(Package, Variant, Value)
@@ -508,28 +515,24 @@ root(Dependency, 1) :- not root(Dependency), node(Dependency).
     : provider_weight(Provider, Weight), root(Provider)
 }.
 
-% 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)
-}.
-
 % For external packages it's more important than for others
 % to match the compiler with their parent node
 #maximize{
-    Weight@10,Package
+    Weight@12,Package
     : compiler_version_match(Package, Weight), external(Package)
 }.
 
-% Then try to use as much as possible:
-% 1. Default variants
-% 2. Latest versions
-% of all the other nodes in the DAG
+% Try to use default variants or variants that have been set
 #minimize {
-    Weight@9,Package,Variant,Value
+    Weight@11,Package,Variant,Value
     : variant_not_default(Package, Variant, Value, Weight), not root(Package)
 }.
+% Minimize the weights of the providers, i.e. use as much as
+% possible the most preferred providers
+#minimize{
+    Weight@9,Provider
+    : provider_weight(Provider, Weight), not root(Provider)
+}.
 % If the value is a multivalued variant there could be multiple
 % values set as default. Since a default value has a weight of 0 we
 % need to maximize their number below to ensure they're all set
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index 39799fc1bc..fa08eb8097 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -943,3 +943,14 @@ class TestConcretize(object):
     def test_target_ranges_in_conflicts(self):
         with pytest.raises(spack.error.SpackError):
             Spec('impossible-concretization').concretized()
+
+    @pytest.mark.regression('20040')
+    def test_variant_not_default(self):
+        s = Spec('ecp-viz-sdk').concretized()
+
+        # Check default variant value for the package
+        assert '+dep' in s['conditional-constrained-dependencies']
+
+        # Check that non-default variant values are forced on the dependency
+        d = s['dep-with-variants']
+        assert '+foo+bar+baz' in d
diff --git a/var/spack/repos/builtin.mock/packages/conditional-constrained-dependencies/package.py b/var/spack/repos/builtin.mock/packages/conditional-constrained-dependencies/package.py
new file mode 100644
index 0000000000..68fee3e9c7
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/conditional-constrained-dependencies/package.py
@@ -0,0 +1,16 @@
+# 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 ConditionalConstrainedDependencies(Package):
+    """Package that has a variant which adds a dependency forced to
+    use non default values.
+    """
+    homepage = "https://dev.null"
+
+    version('1.0')
+
+    # This variant is on by default and attaches a dependency
+    # with a lot of variants set at their non-default values
+    variant('dep', default=True, description='nope')
+    depends_on('dep-with-variants+foo+bar+baz', when='+dep')
diff --git a/var/spack/repos/builtin.mock/packages/dep-with-variants/package.py b/var/spack/repos/builtin.mock/packages/dep-with-variants/package.py
new file mode 100644
index 0000000000..d1b08cd5df
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/dep-with-variants/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 DepWithVariants(Package):
+    """Package that has a variant which adds a dependency forced to
+    use non default values.
+    """
+    homepage = "https://dev.null"
+
+    version('1.0')
+
+    variant('foo', default=False, description='nope')
+    variant('bar', default=False, description='nope')
+    variant('baz', default=False, description='nope')
diff --git a/var/spack/repos/builtin.mock/packages/ecp-viz-sdk/package.py b/var/spack/repos/builtin.mock/packages/ecp-viz-sdk/package.py
new file mode 100644
index 0000000000..76e2718c6f
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/ecp-viz-sdk/package.py
@@ -0,0 +1,14 @@
+# 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 EcpVizSdk(Package):
+    """Package that has a dependency with a variant which
+    adds a transitive dependency forced to use non default
+    values.
+    """
+    homepage = "https://dev.null"
+
+    version('1.0')
+
+    depends_on('conditional-constrained-dependencies')