Flag propagation: restrict to link/run (#44925)

In practice people don't care about having their build dependencies reinstalled with say cflags=-O3 if that is what is set at the input spec, so restrict propagation to link/run deps only.

Also simplify the encoding in asp.

lib/spack/spack/solver/asp.py

@@ -1914,9 +1914,12 @@ def _spec_clauses(
         for flag_type, flags in spec.compiler_flags.items():
             for flag in flags:
                 clauses.append(f.node_flag(spec.name, flag_type, flag))
-                clauses.append(f.node_flag_source(spec.name, flag_type, spec.name))
                 if not spec.concrete and flag.propagate is True:
-                    clauses.append(f.node_flag_propagate(spec.name, flag_type))
+                    clauses.append(
+                        f.propagate(
+                            spec.name, fn.node_flag(flag_type, flag), fn.edge_types("link", "run")
+                        )
+                    )
 
         # dependencies
         if spec.concrete:
@@ -2741,8 +2744,6 @@ class _Head:
     node_compiler = fn.attr("node_compiler_set")
     node_compiler_version = fn.attr("node_compiler_version_set")
     node_flag = fn.attr("node_flag_set")
-    node_flag_source = fn.attr("node_flag_source")
-    node_flag_propagate = fn.attr("node_flag_propagate")
     propagate = fn.attr("propagate")
 
 
@@ -2758,8 +2759,6 @@ class _Body:
     node_compiler = fn.attr("node_compiler")
     node_compiler_version = fn.attr("node_compiler_version")
     node_flag = fn.attr("node_flag")
-    node_flag_source = fn.attr("node_flag_source")
-    node_flag_propagate = fn.attr("node_flag_propagate")
     propagate = fn.attr("propagate")
 
 
@@ -3346,6 +3345,8 @@ def hash(self, node, h):
     def node(self, node):
         if node not in self._specs:
             self._specs[node] = spack.spec.Spec(node.pkg)
+            for flag_type in spack.spec.FlagMap.valid_compiler_flags():
+                self._specs[node].compiler_flags[flag_type] = []
 
     def _arch(self, node):
         arch = self._specs[node].architecture
@@ -3398,9 +3399,6 @@ def node_flag(self, node, flag_type, flag):
     def node_flag_source(self, node, flag_type, source):
         self._flag_sources[(node, flag_type)].add(source)
 
-    def no_flags(self, node, flag_type):
-        self._specs[node].compiler_flags[flag_type] = []
-
     def external_spec_selected(self, node, idx):
         """This means that the external spec and index idx has been selected for this package."""
         packages_yaml = _external_config_with_implicit_externals(spack.config.CONFIG)
@@ -3493,7 +3491,7 @@ def reorder_flags(self):
                 ordered_compiler_flags = list(llnl.util.lang.dedupe(from_compiler + from_sources))
                 compiler_flags = spec.compiler_flags.get(flag_type, [])
 
-                msg = "%s does not equal %s" % (set(compiler_flags), set(ordered_compiler_flags))
+                msg = f"{set(compiler_flags)} does not equal {set(ordered_compiler_flags)}"
                 assert set(compiler_flags) == set(ordered_compiler_flags), msg
 
                 spec.compiler_flags.update({flag_type: ordered_compiler_flags})
@@ -3563,9 +3561,8 @@ def build_specs(self, function_tuples):
                 # do not bother calling actions on it except for node_flag_source,
                 # since node_flag_source is tracking information not in the spec itself
                 spec = self._specs.get(args[0])
-                if spec and spec.concrete:
+                if spec and spec.concrete and name != "node_flag_source":
-                    if name != "node_flag_source":
+                    continue
-                        continue
 
             action(*args)
 

lib/spack/spack/solver/concretize.lp

@@ -29,7 +29,6 @@
 :- attr("variant_value", PackageNode, _, _), not attr("node", PackageNode).
 :- attr("node_flag_compiler_default", PackageNode), not attr("node", PackageNode).
 :- attr("node_flag", PackageNode, _, _), not attr("node", PackageNode).
-:- attr("no_flags", PackageNode, _), not attr("node", PackageNode).
 :- attr("external_spec_selected", PackageNode, _), not attr("node", PackageNode).
 :- attr("depends_on", ParentNode, _, _), not attr("node", ParentNode).
 :- attr("depends_on", _, ChildNode, _), not attr("node", ChildNode).
@@ -965,12 +964,19 @@ pkg_fact(Package, variant_single_value("dev_path"))
 
 % Propagation roots have a corresponding attr("propagate", ...)
 propagate(RootNode, PropagatedAttribute) :- attr("propagate", RootNode, PropagatedAttribute).
+propagate(RootNode, PropagatedAttribute, EdgeTypes) :- attr("propagate", RootNode, PropagatedAttribute, EdgeTypes).
+
 
 % Propagate an attribute along edges to child nodes
 propagate(ChildNode, PropagatedAttribute) :-
   propagate(ParentNode, PropagatedAttribute),
   depends_on(ParentNode, ChildNode).
 
+propagate(ChildNode, PropagatedAttribute, edge_types(DepType1, DepType2)) :-
+  propagate(ParentNode, PropagatedAttribute, edge_types(DepType1, DepType2)),
+  depends_on(ParentNode, ChildNode),
+  1 { attr("depends_on", ParentNode, ChildNode, DepType1);  attr("depends_on", ParentNode, ChildNode, DepType2) }.
+
 %-----------------------------------------------------------------------------
 % Activation of propagated values
 %-----------------------------------------------------------------------------
@@ -996,6 +1002,33 @@ variant_is_propagated(PackageNode, Variant) :-
    attr("variant_value", PackageNode, Variant, Value),
    not propagate(PackageNode, variant_value(Variant, Value)).
 
+%----
+% Flags
+%----
+
+% A propagated flag implies:
+% 1. The same flag type is not set on this node
+% 2. This node has the same compiler as the propagation source
+
+propagated_flag(node(PackageID, Package), node_flag(FlagType, Flag), SourceNode) :-
+  propagate(node(PackageID, Package), node_flag(FlagType, Flag), _),
+  not attr("node_flag_set", node(PackageID, Package), FlagType, _),
+  % Same compiler as propagation source
+  node_compiler(node(PackageID, Package), CompilerID),
+  node_compiler(SourceNode, CompilerID),
+  attr("propagate", SourceNode, node_flag(FlagType, Flag), _),
+  node(PackageID, Package) != SourceNode,
+  not runtime(Package).
+
+attr("node_flag", PackageNode, FlagType, Flag) :- propagated_flag(PackageNode, node_flag(FlagType, Flag), _).
+attr("node_flag_source", PackageNode, FlagType, SourceNode) :- propagated_flag(PackageNode, node_flag(FlagType, _), SourceNode).
+
+% Cannot propagate the same flag from two distinct sources
+error(100, "{0} and {1} cannot both propagate compiler flags '{2}' to {3}", Source1, Source2, Package, FlagType) :-
+  propagated_flag(node(ID, Package), node_flag(FlagType, _), node(_, Source1)),
+  propagated_flag(node(ID, Package), node_flag(FlagType, _), node(_, Source2)),
+  Source1 < Source2.
+
 %----
 % Compiler constraints
 %----
@@ -1277,45 +1310,9 @@ error(100, "Compiler {1}@{2} requested for {0} cannot be found. Set install_miss
 % Compiler flags
 %-----------------------------------------------------------------------------
 
-% propagate flags when compiler match
-can_inherit_flags(PackageNode, DependencyNode, FlagType)
-  :- same_compiler(PackageNode, DependencyNode),
-     not attr("node_flag_set", DependencyNode, FlagType, _),
-     flag_type(FlagType).
-
-same_compiler(PackageNode, DependencyNode)
-  :- depends_on(PackageNode, DependencyNode),
-     node_compiler(PackageNode, CompilerID),
-     node_compiler(DependencyNode, CompilerID),
-     compiler_id(CompilerID).
-
-node_flag_inherited(DependencyNode, FlagType, Flag)
-  :- attr("node_flag_set", PackageNode, FlagType, Flag),
-     can_inherit_flags(PackageNode, DependencyNode, FlagType),
-     attr("node_flag_propagate", PackageNode, FlagType).
-
-% Ensure propagation
-:- node_flag_inherited(PackageNode, FlagType, Flag),
-   can_inherit_flags(PackageNode, DependencyNode, FlagType),
-   attr("node_flag_propagate", PackageNode, FlagType).
-
-error(100, "{0} and {1} cannot both propagate compiler flags '{2}' to {3}", Source1, Source2, Package, FlagType) :-
-  depends_on(Source1, Package),
-  depends_on(Source2, Package),
-  attr("node_flag_propagate", Source1, FlagType),
-  attr("node_flag_propagate", Source2, FlagType),
-  can_inherit_flags(Source1, Package, FlagType),
-  can_inherit_flags(Source2, Package, FlagType),
-  Source1 < Source2.
-
 % remember where flags came from
-attr("node_flag_source", PackageNode, FlagType, PackageNode)
+attr("node_flag_source", PackageNode, FlagType, PackageNode) :- attr("node_flag_set", PackageNode, FlagType, _).
-  :- attr("node_flag_set", PackageNode, FlagType, _).
+attr("node_flag_source", PackageNode, FlagType, PackageNode) :- attr("node_flag", PackageNode, FlagType, _), attr("hash", PackageNode, _).
-
-attr("node_flag_source", DependencyNode, FlagType, Q)
-  :- attr("node_flag_source", PackageNode, FlagType, Q),
-     node_flag_inherited(DependencyNode, FlagType, _),
-     attr("node_flag_propagate", PackageNode, FlagType).
 
 % compiler flags from compilers.yaml are put on nodes if compiler matches
 attr("node_flag", PackageNode, FlagType, Flag)
@@ -1335,15 +1332,8 @@ attr("node_flag_compiler_default", PackageNode)
      compiler_name(CompilerID, CompilerName),
      compiler_version(CompilerID, Version).
 
-% if a flag is set to something or inherited, it's included
+% Flag set to something
 attr("node_flag", PackageNode, FlagType, Flag) :- attr("node_flag_set", PackageNode, FlagType, Flag).
-attr("node_flag", PackageNode, FlagType, Flag) :- node_flag_inherited(PackageNode, FlagType, Flag).
-
-% if no node flags are set for a type, there are no flags.
-attr("no_flags", PackageNode, FlagType)
-  :- not attr("node_flag", PackageNode, FlagType, _),
-     attr("node", PackageNode),
-     flag_type(FlagType).
 
 #defined compiler_flag/3.
 

lib/spack/spack/test/concretize.py

@@ -421,30 +421,38 @@ def test_compiler_flags_differ_identical_compilers(self, mutable_config, clang12
     @pytest.mark.only_clingo(
         "Optional compiler propagation isn't deprecated for original concretizer"
     )
-    def test_concretize_compiler_flag_propagate(self):
+    @pytest.mark.parametrize(
-        spec = Spec("hypre cflags=='-g' ^openblas")
+        "spec_str,expected,not_expected",
-        spec.concretize()
+        [
-
+            # Simple flag propagation from the root
-        assert spec.satisfies("^openblas cflags='-g'")
+            ("hypre cflags=='-g' ^openblas", ["hypre cflags='-g'", "^openblas cflags='-g'"], []),
-
+            (
-    @pytest.mark.only_clingo(
+                "hypre cflags='-g' ^openblas",
-        "Optional compiler propagation isn't deprecated for original concretizer"
+                ["hypre cflags='-g'", "^openblas"],
+                ["^openblas cflags='-g'"],
+            ),
+            # Setting a flag overrides propagation
+            (
+                "hypre cflags=='-g' ^openblas cflags='-O3'",
+                ["hypre cflags='-g'", "^openblas cflags='-O3'"],
+                ["^openblas cflags='-g'"],
+            ),
+            # Propagation doesn't go across build dependencies
+            (
+                "cmake-client cflags=='-O2 -g'",
+                ["cmake-client cflags=='-O2 -g'", "^cmake"],
+                ["cmake cflags=='-O2 -g'"],
+            ),
+        ],
     )
-    def test_concretize_compiler_flag_does_not_propagate(self):
+    def test_compiler_flag_propagation(self, spec_str, expected, not_expected):
-        spec = Spec("hypre cflags='-g' ^openblas")
+        root = Spec(spec_str).concretized()
-        spec.concretize()
 
-        assert not spec.satisfies("^openblas cflags='-g'")
+        for constraint in expected:
+            assert root.satisfies(constraint)
 
-    @pytest.mark.only_clingo(
+        for constraint in not_expected:
-        "Optional compiler propagation isn't deprecated for original concretizer"
+            assert not root.satisfies(constraint)
-    )
-    def test_concretize_propagate_compiler_flag_not_passed_to_dependent(self):
-        spec = Spec("hypre cflags=='-g' ^openblas cflags='-O3'")
-        spec.concretize()
-
-        assert set(spec.compiler_flags["cflags"]) == set(["-g"])
-        assert spec.satisfies("^openblas cflags='-O3'")
 
     def test_mixing_compilers_only_affects_subdag(self):
         spack.config.set("packages:all:compiler", ["clang", "gcc"])