From ce75adada6cd3564d42d37bef19eb35dbf368e2a Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Mon, 11 Mar 2024 20:48:21 +0100 Subject: [PATCH] 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 | 25 +++++++++------ lib/spack/spack/test/builder.py | 17 ++++++++++ lib/spack/spack/test/cmd/list.py | 7 ++-- .../packages/builder-and-mixins/package.py | 32 +++++++++++++++++++ 4 files changed, 66 insertions(+), 15 deletions(-) create mode 100644 var/spack/repos/builder.test/packages/builder-and-mixins/package.py diff --git a/lib/spack/spack/builder.py b/lib/spack/spack/builder.py index 4119062ab6b..00734d55337 100644 --- a/lib/spack/spack/builder.py +++ b/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[:] diff --git a/lib/spack/spack/test/builder.py b/lib/spack/spack/test/builder.py index 9a99e6ee08c..4bd128c3bfb 100644 --- a/lib/spack/spack/test/builder.py +++ b/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) diff --git a/lib/spack/spack/test/cmd/list.py b/lib/spack/spack/test/cmd/list.py index a46e690cd27..4a925046738 100644 --- a/lib/spack/spack/test/cmd/list.py +++ b/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 diff --git a/var/spack/repos/builder.test/packages/builder-and-mixins/package.py b/var/spack/repos/builder.test/packages/builder-and-mixins/package.py new file mode 100644 index 00000000000..2e12b0e8061 --- /dev/null +++ b/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