Propagate variant across nodes that don't have that variant (#38512)

Before this PR, variant were not propagated to leaf nodes that could accept 
the propagated value, if some intermediate node couldn't accept it.

This PR fixes that issue by marking nodes as "candidate" for propagation
and by setting the variant only if it can be accepted by the node.

Co-authored-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>

lib/spack/spack/solver/asp.py

@@ -1922,7 +1922,7 @@ class Head:
             node_flag = fn.attr("node_flag_set")
             node_flag_source = fn.attr("node_flag_source")
             node_flag_propagate = fn.attr("node_flag_propagate")
-            variant_propagate = fn.attr("variant_propagate")
+            variant_propagation_candidate = fn.attr("variant_propagation_candidate")
 
         class Body:
             node = fn.attr("node")
@@ -1936,7 +1936,7 @@ class Body:
             node_flag = fn.attr("node_flag")
             node_flag_source = fn.attr("node_flag_source")
             node_flag_propagate = fn.attr("node_flag_propagate")
-            variant_propagate = fn.attr("variant_propagate")
+            variant_propagation_candidate = fn.attr("variant_propagation_candidate")
 
         f = Body if body else Head
 
@@ -1985,7 +1985,9 @@ class Body:
                 clauses.append(f.variant_value(spec.name, vname, value))
 
                 if variant.propagate:
-                    clauses.append(f.variant_propagate(spec.name, vname, value, spec.name))
+                    clauses.append(
+                        f.variant_propagation_candidate(spec.name, vname, value, spec.name)
+                    )
 
                 # Tell the concretizer that this is a possible value for the
                 # variant, to account for things like int/str values where we

lib/spack/spack/solver/concretize.lp

@@ -757,23 +757,36 @@ node_has_variant(node(ID, Package), Variant) :-
   pkg_fact(Package, variant(Variant)),
   attr("node", node(ID, Package)).
 
-attr("variant_propagate", PackageNode, Variant, Value, Source) :-
+% Variant propagation is forwarded to dependencies
+attr("variant_propagation_candidate", PackageNode, Variant, Value, Source) :-
   attr("node", PackageNode),
   depends_on(ParentNode, PackageNode),
-  attr("variant_propagate", ParentNode, Variant, Value, Source),
-  not attr("variant_set", PackageNode, Variant).
+  attr("variant_value", node(_, Source), Variant, Value),
+  attr("variant_propagation_candidate", ParentNode, Variant, _, Source).
 
-attr("variant_value", node(ID, Package), Variant, Value) :-
-  attr("node", node(ID, Package)),
+% If the node is a candidate, and it has the variant and value,
+% then those variant and value should be propagated
+attr("variant_propagate", node(ID, Package), Variant, Value, Source) :-
+  attr("variant_propagation_candidate", node(ID, Package), Variant, Value, Source),
   node_has_variant(node(ID, Package), Variant),
-  attr("variant_propagate", node(ID, Package), Variant, Value, _),
-  pkg_fact(Package, variant_possible_value(Variant, Value)).
+  pkg_fact(Package, variant_possible_value(Variant, Value)),
+  not attr("variant_set", node(ID, Package), Variant).
 
+% Propagate the value, if there is the corresponding attribute
+attr("variant_value", PackageNode, Variant, Value) :- attr("variant_propagate", PackageNode, Variant, Value, _).
+
+% If a variant is propagated, we cannot have extraneous values (this is for multi valued variants)
+variant_is_propagated(PackageNode, Variant) :- attr("variant_propagate", PackageNode, Variant, _, _).
+:- variant_is_propagated(PackageNode, Variant),
+   attr("variant_value", PackageNode, Variant, Value),
+   not attr("variant_propagate", PackageNode, Variant, Value, _).
+
+% Cannot receive different values from different sources on the same variant
 error(100, "{0} and {1} cannot both propagate variant '{2}' to package {3} with values '{4}' and '{5}'", Source1, Source2, Variant, Package, Value1, Value2) :-
   attr("variant_propagate", node(X, Package), Variant, Value1, Source1),
   attr("variant_propagate", node(X, Package), Variant, Value2, Source2),
   node_has_variant(node(X, Package), Variant),
-  Value1 < Value2.
+  Value1 < Value2, Source1 < Source2.
 
 % a variant cannot be set if it is not a variant on the package
 error(100, "Cannot set variant '{0}' for package '{1}' because the variant condition cannot be satisfied for the given spec", Variant, Package)

lib/spack/spack/test/concretize.py

@@ -349,6 +349,9 @@ def test_compiler_flags_differ_identical_compilers(self):
             spec.concretize()
             assert spec.satisfies("cflags=-O2")
 
+    @pytest.mark.only_clingo(
+        "Optional compiler propagation isn't deprecated for original concretizer"
+    )
     def test_concretize_compiler_flag_propagate(self):
         spec = Spec("hypre cflags=='-g' ^openblas")
         spec.concretize()
@@ -458,19 +461,54 @@ def test_concretize_two_virtuals_with_dual_provider_and_a_conflict(self):
     @pytest.mark.only_clingo(
         "Optional compiler propagation isn't deprecated for original concretizer"
     )
-    def test_concretize_propagate_disabled_variant(self):
-        """Test a package variant value was passed from its parent."""
-        spec = Spec("hypre~~shared ^openblas")
-        spec.concretize()
-
-        assert spec.satisfies("^openblas~shared")
+    @pytest.mark.parametrize(
+        "spec_str,expected_propagation",
+        [
+            ("hypre~~shared ^openblas+shared", [("hypre", "~shared"), ("openblas", "+shared")]),
+            # Propagates past a node that doesn't have the variant
+            ("hypre~~shared ^openblas", [("hypre", "~shared"), ("openblas", "~shared")]),
+            (
+                "ascent~~shared +adios2",
+                [("ascent", "~shared"), ("adios2", "~shared"), ("bzip2", "~shared")],
+            ),
+            # Propagates below a node that uses the other value explicitly
+            (
+                "ascent~~shared +adios2 ^adios2+shared",
+                [("ascent", "~shared"), ("adios2", "+shared"), ("bzip2", "~shared")],
+            ),
+            (
+                "ascent++shared +adios2 ^adios2~shared",
+                [("ascent", "+shared"), ("adios2", "~shared"), ("bzip2", "+shared")],
+            ),
+        ],
+    )
+    def test_concretize_propagate_disabled_variant(self, spec_str, expected_propagation):
+        """Tests various patterns of boolean variant propagation"""
+        spec = Spec(spec_str).concretized()
+        for key, expected_satisfies in expected_propagation:
+            spec[key].satisfies(expected_satisfies)
 
+    @pytest.mark.only_clingo(
+        "Optional compiler propagation isn't deprecated for original concretizer"
+    )
     def test_concretize_propagated_variant_is_not_passed_to_dependent(self):
         """Test a package variant value was passed from its parent."""
-        spec = Spec("hypre~~shared ^openblas+shared")
+        spec = Spec("ascent~~shared +adios2 ^adios2+shared")
         spec.concretize()
 
-        assert spec.satisfies("^openblas+shared")
+        assert spec.satisfies("^adios2+shared")
+        assert spec.satisfies("^bzip2~shared")
+
+    @pytest.mark.only_clingo(
+        "Optional compiler propagation isn't deprecated for original concretizer"
+    )
+    def test_concretize_propagate_specified_variant(self):
+        """Test that only the specified variant is propagated to the dependencies"""
+        spec = Spec("parent-foo-bar ~~foo")
+        spec.concretize()
+
+        assert spec.satisfies("~foo") and spec.satisfies("^dependency-foo-bar~foo")
+        assert spec.satisfies("+bar") and not spec.satisfies("^dependency-foo-bar+bar")
 
     @pytest.mark.only_clingo("Original concretizer is allowed to forego variant propagation")
     def test_concretize_propagate_multivalue_variant(self):

var/spack/repos/builtin.mock/packages/adios2/package.py

@@ -0,0 +1,22 @@
+# Copyright 2013-2023 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)
+
+
+from spack.package import *
+
+
+class Adios2(Package):
+    """This packagae has the variants shared and
+    bzip2, both defaulted to True"""
+
+    homepage = "https://example.com"
+    url = "https://example.com/adios2.tar.gz"
+
+    version("2.9.1", sha256="ddfa32c14494250ee8a48ef1c97a1bf6442c15484bbbd4669228a0f90242f4f9")
+
+    variant("shared", default=True, description="Build shared libraries")
+    variant("bzip2", default=True, description="Enable BZip2 compression")
+
+    depends_on("bzip2")

var/spack/repos/builtin.mock/packages/ascent/package.py

@@ -0,0 +1,21 @@
+# Copyright 2013-2023 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)
+
+from spack.package import *
+
+
+class Ascent(Package):
+    """This packagae has the variants shared, defaulted
+    to True and adios2 defaulted to False"""
+
+    homepage = "https://github.com/Alpine-DAV/ascent"
+    url = "http://www.example.com/ascent-1.0.tar.gz"
+
+    version("0.9.2", sha256="44cd954aa5db478ab40042cd54fd6fcedf25000c3bb510ca23fcff8090531b91")
+
+    variant("adios2", default=False, description="Build Adios2 filter support")
+    variant("shared", default=True, description="Build Ascent as shared libs")
+
+    depends_on("adios2", when="+adios2")

var/spack/repos/builtin.mock/packages/bzip2/package.py

@@ -0,0 +1,19 @@
+# Copyright 2013-2023 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)
+
+
+from spack.package import *
+
+
+class Bzip2(Package):
+    """This packagae has the variants shared
+    defaulted to True"""
+
+    homepage = "https://example.com"
+    url = "https://example.com/bzip2-1.0.8tar.gz"
+
+    version("1.0.8", sha256="ab5a03176ee106d3f0fa90e381da478ddae405918153cca248e682cd0c4a2269")
+
+    variant("shared", default=True, description="Enables the build of shared libraries.")

var/spack/repos/builtin.mock/packages/dependency-foo-bar/package.py

@@ -0,0 +1,20 @@
+# Copyright 2013-2023 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)
+
+from spack.package import *
+
+
+class DependencyFooBar(Package):
+    """This package has a variant "bar", which is False by default, and
+    variant "foo" which is True by default.
+    """
+
+    homepage = "http://www.example.com"
+    url = "http://www.example.com/dependency-foo-bar-1.0.tar.gz"
+
+    version("1.0", md5="1234567890abcdefg1234567890098765")
+
+    variant("foo", default=True, description="")
+    variant("bar", default=False, description="")

var/spack/repos/builtin.mock/packages/parent-foo-bar/package.py

@@ -0,0 +1,22 @@
+# Copyright 2013-2023 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)
+
+from spack.package import *
+
+
+class ParentFooBar(Package):
+    """This package has a variant "bar", which is True by default, and depends on another
+    package which has the same variant defaulting to False.
+    """
+
+    homepage = "http://www.example.com"
+    url = "http://www.example.com/parent-foo-bar-1.0.tar.gz"
+
+    version("1.0", md5="abcdefg0123456789abcdefghfedcba0")
+
+    variant("foo", default=True, description="")
+    variant("bar", default=True, description="")
+
+    depends_on("dependency-foo-bar")