Fix callbacks accumulation when using mixins with builders (#43100)

fixes #43097

Before this PR the behavior of mixins used together with
builders was to mask completely the callbacks defined from
the class coming later in the MRO.

Here we fix the behavior by accumulating all callbacks,
and de-duplicating them later.

lib/spack/spack/builder.py

@@ -9,6 +9,8 @@
 import inspect
 from typing import List, Optional, Tuple
 
+from llnl.util import lang
+
 import spack.build_environment
 
 #: Builder classes, as registered by the "builder" decorator
@@ -231,24 +233,27 @@ def __new__(mcs, name, bases, attr_dict):
         for temporary_stage in (_RUN_BEFORE, _RUN_AFTER):
             staged_callbacks = temporary_stage.callbacks
 
-            # We don't have callbacks in this class, move on
-            if not staged_callbacks:
+            # Here we have an adapter from an old-style package. This means there is no
+            # hierarchy of builders, and every callback that had to be combined between
+            # *Package and *Builder has been combined already by _PackageAdapterMeta
+            if name == "Adapter":
                 continue
 
-            # If we are here we have callbacks. To get a complete list, get first what
-            # was attached to parent classes, then prepend what we have registered here.
+            # If we are here we have callbacks. To get a complete list, we accumulate all the
+            # callbacks from base classes, we deduplicate them, then prepend what we have
+            # registered here.
             #
             # The order should be:
             # 1. Callbacks are registered in order within the same class
             # 2. Callbacks defined in derived classes precede those defined in base
             #    classes
+            callbacks_from_base = []
             for base in bases:
-                callbacks_from_base = getattr(base, temporary_stage.attribute_name, None)
-                if callbacks_from_base:
-                    break
-            else:
-                callbacks_from_base = []
-
+                current_callbacks = getattr(base, temporary_stage.attribute_name, None)
+                if not current_callbacks:
+                    continue
+                callbacks_from_base.extend(current_callbacks)
+            callbacks_from_base = list(lang.dedupe(callbacks_from_base))
             # Set the callbacks in this class and flush the temporary stage
             attr_dict[temporary_stage.attribute_name] = staged_callbacks[:] + callbacks_from_base
             del temporary_stage.callbacks[:]

lib/spack/spack/test/builder.py

@@ -164,3 +164,20 @@ def test_install_time_test_callback(tmpdir, config, mock_packages, mock_stage):
     with open(s.package.tester.test_log_file, "r") as f:
         results = f.read().replace("\n", " ")
         assert "PyTestCallback test" in results
+
+
+@pytest.mark.regression("43097")
+@pytest.mark.usefixtures("builder_test_repository", "config")
+def test_mixins_with_builders(working_env):
+    """Tests that run_after and run_before callbacks are accumulated correctly,
+    when mixins are used with builders.
+    """
+    s = spack.spec.Spec("builder-and-mixins").concretized()
+    builder = spack.builder.create(s.package)
+
+    # Check that callbacks added by the mixin are in the list
+    assert any(fn.__name__ == "before_install" for _, fn in builder.run_before_callbacks)
+    assert any(fn.__name__ == "after_install" for _, fn in builder.run_after_callbacks)
+
+    # Check that callback from the GenericBuilder are in the list too
+    assert any(fn.__name__ == "sanity_check_prefix" for _, fn in builder.run_after_callbacks)

lib/spack/spack/test/cmd/list.py

@@ -144,12 +144,9 @@ def test_list_repos():
         os.path.join(spack.paths.repos_path, "builder.test"),
     ):
         total_pkgs = len(list().strip().split())
-
         mock_pkgs = len(list("-r", "builtin.mock").strip().split())
         builder_pkgs = len(list("-r", "builder.test").strip().split())
-
-        assert builder_pkgs == 8
-        assert total_pkgs > mock_pkgs > builder_pkgs
-
         both_repos = len(list("-r", "builtin.mock", "-r", "builder.test").strip().split())
+
+        assert total_pkgs > mock_pkgs > builder_pkgs
         assert both_repos == total_pkgs

var/spack/repos/builder.test/packages/builder-and-mixins/package.py

@@ -0,0 +1,32 @@
+# Copyright 2013-2024 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)
+import spack.builder
+from spack.build_systems import generic
+from spack.package import *
+
+
+class BuilderAndMixins(Package):
+    """This package defines a mixin for its builder"""
+
+    homepage = "http://www.example.com"
+    url = "http://www.example.com/a-1.0.tar.gz"
+
+    version("2.0", md5="abcdef0123456789abcdef0123456789")
+    version("1.0", md5="0123456789abcdef0123456789abcdef")
+
+
+class BuilderMixin(metaclass=spack.builder.PhaseCallbacksMeta):
+    @run_before("install")
+    def before_install(self):
+        pass
+
+    @run_after("install")
+    def after_install(self):
+        pass
+
+
+class GenericBuilder(BuilderMixin, generic.GenericBuilder):
+    def install(self, pkg, spec, prefix):
+        pass