set_packge_py_globals: only set pure build related globals on the root in build context (#42215)

Previously `std_args` was called on non-roots in a build context, which is redundant, and also leads to issues when `std_args` expects build deps of the `pkg` to be installed.

3 files changed, 66 insertions, 41 deletions

diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
index 1ef3c658dc..68369cbe05 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -555,58 +555,55 @@ def set_package_py_globals(pkg, context: Context = Context.BUILD):
     """
     module = ModuleChangePropagator(pkg)
 
-    m = module
-
     if context == Context.BUILD:
-        jobs = determine_number_of_jobs(parallel=pkg.parallel)
-        m.make_jobs = jobs
-
-        # TODO: make these build deps that can be installed if not found.
-        m.make = MakeExecutable("make", jobs)
-        m.gmake = MakeExecutable("gmake", jobs)
-        m.ninja = MakeExecutable("ninja", jobs, supports_jobserver=False)
-        # TODO: johnwparent: add package or builder support to define these build tools
-        # for now there is no entrypoint for builders to define these on their
-        # own
-        if sys.platform == "win32":
-            m.nmake = Executable("nmake")
-            m.msbuild = Executable("msbuild")
-            # analog to configure for win32
-            m.cscript = Executable("cscript")
-
-        # Find the configure script in the archive path
-        # Don't use which for this; we want to find it in the current dir.
-        m.configure = Executable("./configure")
-
-        # Standard CMake arguments
-        m.std_cmake_args = spack.build_systems.cmake.CMakeBuilder.std_args(pkg)
-        m.std_meson_args = spack.build_systems.meson.MesonBuilder.std_args(pkg)
-        m.std_pip_args = spack.build_systems.python.PythonPipBuilder.std_args(pkg)
+        module.std_cmake_args = spack.build_systems.cmake.CMakeBuilder.std_args(pkg)
+        module.std_meson_args = spack.build_systems.meson.MesonBuilder.std_args(pkg)
+        module.std_pip_args = spack.build_systems.python.PythonPipBuilder.std_args(pkg)
+
+    jobs = determine_number_of_jobs(parallel=pkg.parallel)
+    module.make_jobs = jobs
+
+    # TODO: make these build deps that can be installed if not found.
+    module.make = MakeExecutable("make", jobs)
+    module.gmake = MakeExecutable("gmake", jobs)
+    module.ninja = MakeExecutable("ninja", jobs, supports_jobserver=False)
+    # TODO: johnwparent: add package or builder support to define these build tools
+    # for now there is no entrypoint for builders to define these on their
+    # own
+    if sys.platform == "win32":
+        module.nmake = Executable("nmake")
+        module.msbuild = Executable("msbuild")
+        # analog to configure for win32
+        module.cscript = Executable("cscript")
+
+    # Find the configure script in the archive path
+    # Don't use which for this; we want to find it in the current dir.
+    module.configure = Executable("./configure")
 
     # Put spack compiler paths in module scope. (Some packages use it
     # in setup_run_environment etc, so don't put it context == build)
     link_dir = spack.paths.build_env_path
-    m.spack_cc = os.path.join(link_dir, pkg.compiler.link_paths["cc"])
-    m.spack_cxx = os.path.join(link_dir, pkg.compiler.link_paths["cxx"])
-    m.spack_f77 = os.path.join(link_dir, pkg.compiler.link_paths["f77"])
-    m.spack_fc = os.path.join(link_dir, pkg.compiler.link_paths["fc"])
+    module.spack_cc = os.path.join(link_dir, pkg.compiler.link_paths["cc"])
+    module.spack_cxx = os.path.join(link_dir, pkg.compiler.link_paths["cxx"])
+    module.spack_f77 = os.path.join(link_dir, pkg.compiler.link_paths["f77"])
+    module.spack_fc = os.path.join(link_dir, pkg.compiler.link_paths["fc"])
 
     # Useful directories within the prefix are encapsulated in
     # a Prefix object.
-    m.prefix = pkg.prefix
+    module.prefix = pkg.prefix
 
     # Platform-specific library suffix.
-    m.dso_suffix = dso_suffix
+    module.dso_suffix = dso_suffix
 
     def static_to_shared_library(static_lib, shared_lib=None, **kwargs):
-        compiler_path = kwargs.get("compiler", m.spack_cc)
+        compiler_path = kwargs.get("compiler", module.spack_cc)
         compiler = Executable(compiler_path)
 
         return _static_to_shared_library(
             pkg.spec.architecture, compiler, static_lib, shared_lib, **kwargs
         )
 
-    m.static_to_shared_library = static_to_shared_library
+    module.static_to_shared_library = static_to_shared_library
 
     module.propagate_changes_to_mro()
 
@@ -975,8 +972,8 @@ class SetupContext:
         self.should_set_package_py_globals = (
             self.should_setup_dependent_build_env | self.should_setup_run_env | UseMode.ROOT
         )
-        # In a build context, the root and direct build deps need build-specific globals set.
-        self.needs_build_context = UseMode.ROOT | UseMode.BUILDTIME_DIRECT
+        # In a build context, the root needs build-specific globals set.
+        self.needs_build_context = UseMode.ROOT
 
     def set_all_package_py_globals(self):
         """Set the globals in modules of package.py files."""
diff --git a/lib/spack/spack/build_systems/meson.py b/lib/spack/spack/build_systems/meson.py
index 860fa4cf1b..2c2bc0d1a0 100644
--- a/lib/spack/spack/build_systems/meson.py
+++ b/lib/spack/spack/build_systems/meson.py
@@ -149,7 +149,7 @@ class MesonBuilder(BaseBuilder):
         else:
             default_library = "shared"
 
-        args = [
+        return [
             "-Dprefix={0}".format(pkg.prefix),
             # If we do not specify libdir explicitly, Meson chooses something
             # like lib/x86_64-linux-gnu, which causes problems when trying to
@@ -163,8 +163,6 @@ class MesonBuilder(BaseBuilder):
             "-Dwrap_mode=nodownload",
         ]
 
-        return args
-
     @property
     def build_dirname(self):
         """Returns the directory name to use when building the package."""
diff --git a/lib/spack/spack/test/build_environment.py b/lib/spack/spack/test/build_environment.py
index fb9874daeb..1f2d9e904f 100644
--- a/lib/spack/spack/test/build_environment.py
+++ b/lib/spack/spack/test/build_environment.py
@@ -457,10 +457,10 @@ def test_parallel_false_is_not_propagating(default_mock_concretization):
     # b (parallel =True)
     s = default_mock_concretization("a foobar=bar")
 
-    spack.build_environment.set_package_py_globals(s.package)
+    spack.build_environment.set_package_py_globals(s.package, context=Context.BUILD)
     assert s["a"].package.module.make_jobs == 1
 
-    spack.build_environment.set_package_py_globals(s["b"].package)
+    spack.build_environment.set_package_py_globals(s["b"].package, context=Context.BUILD)
     assert s["b"].package.module.make_jobs == spack.build_environment.determine_number_of_jobs(
         parallel=s["b"].package.parallel
     )
@@ -685,3 +685,33 @@ def test_clear_compiler_related_runtime_variables_of_build_deps(default_mock_con
     assert "FC" not in result
     assert "F77" not in result
     assert result["ANOTHER_VAR"] == "this-should-be-present"
+
+
+@pytest.mark.parametrize("context", [Context.BUILD, Context.RUN])
+def test_build_system_globals_only_set_on_root_during_build(default_mock_concretization, context):
+    """Test whether when setting up a build environment, the build related globals are set only
+    in the top level spec.
+
+    TODO: Since module instances are globals themselves, and Spack defines properties on them, they
+    persist across tests. In principle this is not terrible, cause the variables are mostly static.
+    But obviously it can lead to very hard to find bugs... We should get rid of those globals and
+    define them instead as a property on the package instance.
+    """
+    root = spack.spec.Spec("mpileaks").concretized()
+    build_variables = ("std_cmake_args", "std_meson_args", "std_pip_args")
+
+    # See todo above, we clear out any properties that may have been set by the previous test.
+    # Commenting this loop will make the test fail. I'm leaving it here as a reminder that those
+    # globals were always a bad idea, and we should pass them to the package instance.
+    for spec in root.traverse():
+        for variable in build_variables:
+            spec.package.module.__dict__.pop(variable, None)
+
+    spack.build_environment.SetupContext(root, context=context).set_all_package_py_globals()
+
+    # Excpect the globals to be set at the root in a build context only.
+    should_be_set = lambda depth: context == Context.BUILD and depth == 0
+
+    for depth, spec in root.traverse(depth=True, root=True):
+        for variable in build_variables:
+            assert hasattr(spec.package.module, variable) == should_be_set(depth)