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>

8 files changed, 175 insertions, 18 deletions

diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 0cca744359..4514bd0e96 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1922,7 +1922,7 @@ class SpackSolverSetup:
             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 SpackSolverSetup:
             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 SpackSolverSetup:
                 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
diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index 5e98e5cf11..d5f24ddc3b 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/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)
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index 0af689ddd5..eba86d14fc 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -349,6 +349,9 @@ class TestConcretize:
             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 @@ class TestConcretize:
     @pytest.mark.only_clingo(
         "Optional compiler propagation isn't deprecated for original concretizer"
     )
-    def test_concretize_propagate_disabled_variant(self):
+    @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")
+        spec = Spec("ascent~~shared +adios2 ^adios2+shared")
         spec.concretize()
 
-        assert spec.satisfies("^openblas~shared")
+        assert spec.satisfies("^adios2+shared")
+        assert spec.satisfies("^bzip2~shared")
 
-    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")
+    @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("^openblas+shared")
+        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):
diff --git a/var/spack/repos/builtin.mock/packages/adios2/package.py b/var/spack/repos/builtin.mock/packages/adios2/package.py
new file mode 100644
index 0000000000..fb2f43ea0e
--- /dev/null
+++ b/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")
diff --git a/var/spack/repos/builtin.mock/packages/ascent/package.py b/var/spack/repos/builtin.mock/packages/ascent/package.py
new file mode 100644
index 0000000000..9a8db472dc
--- /dev/null
+++ b/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")
diff --git a/var/spack/repos/builtin.mock/packages/bzip2/package.py b/var/spack/repos/builtin.mock/packages/bzip2/package.py
new file mode 100644
index 0000000000..326533ac5e
--- /dev/null
+++ b/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.")
diff --git a/var/spack/repos/builtin.mock/packages/dependency-foo-bar/package.py b/var/spack/repos/builtin.mock/packages/dependency-foo-bar/package.py
new file mode 100644
index 0000000000..21e67f8a61
--- /dev/null
+++ b/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="")
diff --git a/var/spack/repos/builtin.mock/packages/parent-foo-bar/package.py b/var/spack/repos/builtin.mock/packages/parent-foo-bar/package.py
new file mode 100644
index 0000000000..14516566a9
--- /dev/null
+++ b/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")