Stable splice-topo order for resolving splicing (#48605)

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

lib/spack/spack/solver/asp.py

@@ -37,6 +37,7 @@
 import spack.package_prefs
 import spack.platforms
 import spack.repo
+import spack.solver.splicing
 import spack.spec
 import spack.store
 import spack.util.crypto
@@ -1740,7 +1741,7 @@ def package_splice_rules(self, pkg):
                     if any(
                         v in cond.variants or v in spec_to_splice.variants for v in match_variants
                     ):
-                        raise Exception(
+                        raise spack.error.PackageError(
                             "Overlap between match_variants and explicitly set variants"
                         )
                     variant_constraints = self._gen_match_variant_splice_constraints(
@@ -3374,14 +3375,6 @@ def consume_facts(self):
         self._setup.effect_rules()
 
 
-# This should be a dataclass, but dataclasses don't work on Python 3.6
-class Splice:
-    def __init__(self, splice_node: NodeArgument, child_name: str, child_hash: str):
-        self.splice_node = splice_node
-        self.child_name = child_name
-        self.child_hash = child_hash
-
-
 class SpecBuilder:
     """Class with actions to rebuild a spec from ASP results."""
 
@@ -3421,7 +3414,7 @@ def __init__(self, specs, hash_lookup=None):
         self._specs: Dict[NodeArgument, spack.spec.Spec] = {}
 
         # Matches parent nodes to splice node
-        self._splices: Dict[NodeArgument, List[Splice]] = {}
+        self._splices: Dict[spack.spec.Spec, List[spack.solver.splicing.Splice]] = {}
         self._result = None
         self._command_line_specs = specs
         self._flag_sources: Dict[Tuple[NodeArgument, str], Set[str]] = collections.defaultdict(
@@ -3628,50 +3621,12 @@ def splice_at_hash(
         child_name: str,
         child_hash: str,
     ):
-        splice = Splice(splice_node, child_name=child_name, child_hash=child_hash)
+        parent_spec = self._specs[parent_node]
-        self._splices.setdefault(parent_node, []).append(splice)
+        splice_spec = self._specs[splice_node]
-
+        splice = spack.solver.splicing.Splice(
-    def _resolve_automatic_splices(self):
+            splice_spec, child_name=child_name, child_hash=child_hash
-        """After all of the specs have been concretized, apply all immediate splices.
+        )
-
+        self._splices.setdefault(parent_spec, []).append(splice)
-        Use reverse topological order to ensure that all dependencies are resolved
-        before their parents, allowing for maximal sharing and minimal copying.
-
-        """
-        fixed_specs = {}
-
-        # create a mapping from dag hash to an integer representing position in reverse topo order.
-        specs = self._specs.values()
-        topo_order = list(traverse.traverse_nodes(specs, order="topo", key=traverse.by_dag_hash))
-        topo_lookup = {spec.dag_hash(): index for index, spec in enumerate(reversed(topo_order))}
-
-        # iterate over specs, children before parents
-        for node, spec in sorted(self._specs.items(), key=lambda x: topo_lookup[x[1].dag_hash()]):
-            immediate = self._splices.get(node, [])
-            if not immediate and not any(
-                edge.spec in fixed_specs for edge in spec.edges_to_dependencies()
-            ):
-                continue
-            new_spec = spec.copy(deps=False)
-            new_spec.clear_caches(ignore=("package_hash",))
-            new_spec.build_spec = spec
-            for edge in spec.edges_to_dependencies():
-                depflag = edge.depflag & ~dt.BUILD
-                if any(edge.spec.dag_hash() == splice.child_hash for splice in immediate):
-                    splice = [s for s in immediate if s.child_hash == edge.spec.dag_hash()][0]
-                    new_spec.add_dependency_edge(
-                        self._specs[splice.splice_node], depflag=depflag, virtuals=edge.virtuals
-                    )
-                elif edge.spec in fixed_specs:
-                    new_spec.add_dependency_edge(
-                        fixed_specs[edge.spec], depflag=depflag, virtuals=edge.virtuals
-                    )
-                else:
-                    new_spec.add_dependency_edge(
-                        edge.spec, depflag=depflag, virtuals=edge.virtuals
-                    )
-            self._specs[node] = new_spec
-            fixed_specs[spec] = new_spec
 
     @staticmethod
     def sort_fn(function_tuple) -> Tuple[int, int]:
@@ -3764,7 +3719,15 @@ def build_specs(self, function_tuples):
         for root in roots.values():
             root._finalize_concretization()
 
-        self._resolve_automatic_splices()
+        # Only attempt to resolve automatic splices if the solver produced any
+        if self._splices:
+            resolved_splices = spack.solver.splicing._resolve_collected_splices(
+                list(self._specs.values()), self._splices
+            )
+            new_specs = {}
+            for node, spec in self._specs.items():
+                new_specs[node] = resolved_splices.get(spec, spec)
+            self._specs = new_specs
 
         for s in self._specs.values():
             spack.spec.Spec.ensure_no_deprecated(s)

lib/spack/spack/solver/splicing.py

@@ -0,0 +1,73 @@
+# Copyright Spack Project Developers. See COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+from functools import cmp_to_key
+from typing import Dict, List, NamedTuple
+
+import spack.deptypes as dt
+from spack.spec import Spec
+from spack.traverse import by_dag_hash, traverse_nodes
+
+
+class Splice(NamedTuple):
+    #: The spec being spliced into a parent
+    splice_spec: Spec
+    #: The name of the child that splice spec is replacing
+    child_name: str
+    #: The hash of the child that `splice_spec` is replacing
+    child_hash: str
+
+
+def _resolve_collected_splices(
+    specs: List[Spec], splices: Dict[Spec, List[Splice]]
+) -> Dict[Spec, Spec]:
+    """After all of the specs have been concretized, apply all immediate splices.
+    Returns a dict mapping original specs to their resolved counterparts
+    """
+
+    def splice_cmp(s1: Spec, s2: Spec):
+        """This function can be used to sort a list of specs such that that any
+        spec which will be spliced into a parent comes after the parent it will
+        be spliced into. This order ensures that transitive splices will be
+        executed in the correct order.
+        """
+
+        s1_splices = splices.get(s1, [])
+        s2_splices = splices.get(s2, [])
+        if any([s2.dag_hash() == splice.splice_spec.dag_hash() for splice in s1_splices]):
+            return -1
+        elif any([s1.dag_hash() == splice.splice_spec.dag_hash() for splice in s2_splices]):
+            return 1
+        else:
+            return 0
+
+    splice_order = sorted(specs, key=cmp_to_key(splice_cmp))
+    reverse_topo_order = reversed(
+        [x for x in traverse_nodes(splice_order, order="topo", key=by_dag_hash) if x in specs]
+    )
+
+    already_resolved: Dict[Spec, Spec] = {}
+    for spec in reverse_topo_order:
+        immediate = splices.get(spec, [])
+        if not immediate and not any(
+            edge.spec in already_resolved for edge in spec.edges_to_dependencies()
+        ):
+            continue
+        new_spec = spec.copy(deps=False)
+        new_spec.clear_caches(ignore=("package_hash",))
+        new_spec.build_spec = spec
+        for edge in spec.edges_to_dependencies():
+            depflag = edge.depflag & ~dt.BUILD
+            if any(edge.spec.dag_hash() == splice.child_hash for splice in immediate):
+                splice = [s for s in immediate if s.child_hash == edge.spec.dag_hash()][0]
+                # If the spec being splice in is also spliced
+                splice_spec = already_resolved.get(splice.splice_spec, splice.splice_spec)
+                new_spec.add_dependency_edge(splice_spec, depflag=depflag, virtuals=edge.virtuals)
+            elif edge.spec in already_resolved:
+                new_spec.add_dependency_edge(
+                    already_resolved[edge.spec], depflag=depflag, virtuals=edge.virtuals
+                )
+            else:
+                new_spec.add_dependency_edge(edge.spec, depflag=depflag, virtuals=edge.virtuals)
+        already_resolved[spec] = new_spec
+    return already_resolved

lib/spack/spack/test/abi_splicing.py

@@ -103,7 +103,6 @@ def test_splice_build_splice_node(install_specs, mutable_config):
     assert concrete["splice-h"].build_spec.dag_hash() == concrete["splice-h"].dag_hash()
 
 
-@pytest.mark.xfail(reason="the spliced splice-h has sometimes the original splice-h hash")
 def test_double_splice(install_specs, mutable_config):
     """Tests splicing two dependencies of an installed spec, for other installed specs"""
     splice_t, splice_h, splice_z = install_specs(