From 22d7937c50fa56e8db3819ae1039c0bb26383bd1 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Tue, 1 Dec 2020 07:45:48 +0100 Subject: 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. --- lib/spack/spack/solver/concretize.lp | 29 ++++++++++++---------- lib/spack/spack/test/concretize.py | 11 ++++++++ .../package.py | 16 ++++++++++++ .../packages/dep-with-variants/package.py | 15 +++++++++++ .../builtin.mock/packages/ecp-viz-sdk/package.py | 14 +++++++++++ 5 files changed, 72 insertions(+), 13 deletions(-) create mode 100644 var/spack/repos/builtin.mock/packages/conditional-constrained-dependencies/package.py create mode 100644 var/spack/repos/builtin.mock/packages/dep-with-variants/package.py create mode 100644 var/spack/repos/builtin.mock/packages/ecp-viz-sdk/package.py 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') -- cgit v1.2.3-70-g09d2