Fix spack package inheritance for module variables (#10097)

* we weren't properly setting module variables for the root package in a DAG -- just for transitive dependencies.

3 files changed, 37 insertions, 25 deletions

diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
index 9ef64a6f7e..4427d1d812 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -370,10 +370,8 @@ def set_build_environment_variables(pkg, env, dirty):
     return env
 
 
-def set_module_variables_for_package(pkg, module):
-    """Populate the module scope of install() with some useful functions.
-       This makes things easier for package writers.
-    """
+def _set_variables_for_single_module(pkg, module):
+    """Helper function to set module variables for single module."""
     # number of jobs spack will build with.
     jobs = spack.config.get('config:build_jobs') or multiprocessing.cpu_count()
     if not pkg.parallel:
@@ -444,6 +442,20 @@ def set_module_variables_for_package(pkg, module):
     m.static_to_shared_library = static_to_shared_library
 
 
+def set_module_variables_for_package(pkg):
+    """Populate the module scope of install() with some useful functions.
+       This makes things easier for package writers.
+    """
+    # If a user makes their own package repo, e.g.
+    # spack.pkg.mystuff.libelf.Libelf, and they inherit from an existing class
+    # like spack.pkg.original.libelf.Libelf, then set the module variables
+    # for both classes so the parent class can still use them if it gets
+    # called. parent_class_modules includes pkg.module.
+    modules = parent_class_modules(pkg.__class__)
+    for mod in modules:
+        _set_variables_for_single_module(pkg, mod)
+
+
 def _static_to_shared_library(arch, compiler, static_lib, shared_lib=None,
                               **kwargs):
     """
@@ -591,6 +603,8 @@ def get_std_meson_args(pkg):
 def parent_class_modules(cls):
     """
     Get list of superclass modules that descend from spack.package.PackageBase
+
+    Includes cls.__module__
     """
     if (not issubclass(cls, spack.package.PackageBase) or
         issubclass(spack.package.PackageBase, cls)):
@@ -634,23 +648,15 @@ def setup_package(pkg, dirty):
     spec = pkg.spec
     for dspec in pkg.spec.traverse(order='post', root=False,
                                    deptype=('build', 'test')):
-        # If a user makes their own package repo, e.g.
-        # spack.pkg.mystuff.libelf.Libelf, and they inherit from
-        # an existing class like spack.pkg.original.libelf.Libelf,
-        # then set the module variables for both classes so the
-        # parent class can still use them if it gets called.
         spkg = dspec.package
-        modules = parent_class_modules(spkg.__class__)
-        for mod in modules:
-            set_module_variables_for_package(spkg, mod)
-        set_module_variables_for_package(spkg, spkg.module)
+        set_module_variables_for_package(spkg)
 
         # Allow dependencies to modify the module
         dpkg = dspec.package
         dpkg.setup_dependent_package(pkg.module, spec)
         dpkg.setup_dependent_environment(spack_env, run_env, spec)
 
-    set_module_variables_for_package(pkg, pkg.module)
+    set_module_variables_for_package(pkg)
     pkg.setup_environment(spack_env, run_env)
 
     # Loading modules, in particular if they are meant to be used outside
diff --git a/lib/spack/spack/modules/common.py b/lib/spack/spack/modules/common.py
index 87ad261121..facffa5d73 100644
--- a/lib/spack/spack/modules/common.py
+++ b/lib/spack/spack/modules/common.py
@@ -515,22 +515,13 @@ class BaseContext(tengine.Context):
         # before asking for package-specific modifications
         for item in dependencies(self.spec, 'all'):
             package = self.spec[item.name].package
-            modules = build_environment.parent_class_modules(package.__class__)
-            for mod in modules:
-                build_environment.set_module_variables_for_package(
-                    package, mod
-                )
-            build_environment.set_module_variables_for_package(
-                package, package.module
-            )
+            build_environment.set_module_variables_for_package(package)
             package.setup_dependent_package(
                 self.spec.package.module, self.spec
             )
             package.setup_dependent_environment(_, env, self.spec)
         # Package specific modifications
-        build_environment.set_module_variables_for_package(
-            self.spec.package, self.spec.package.module
-        )
+        build_environment.set_module_variables_for_package(self.spec.package)
         self.spec.package.setup_environment(_, env)
 
         # Modifications required from modules.yaml
diff --git a/lib/spack/spack/test/build_environment.py b/lib/spack/spack/test/build_environment.py
index e055bbf99c..b831f676d3 100644
--- a/lib/spack/spack/test/build_environment.py
+++ b/lib/spack/spack/test/build_environment.py
@@ -215,3 +215,18 @@ def test_spack_paths_before_module_paths(config, mock_packages, monkeypatch):
     paths = os.environ['PATH'].split(':')
 
     assert paths.index(spack_path) < paths.index(module_path)
+
+
+def test_package_inheritance_module_setup(config, mock_packages):
+    s = spack.spec.Spec('multimodule-inheritance')
+    s.concretize()
+    pkg = s.package
+
+    spack.build_environment.setup_package(pkg, False)
+
+    os.environ['TEST_MODULE_VAR'] = 'failed'
+
+    assert pkg.use_module_variable() == 'test_module_variable'
+    assert os.environ['TEST_MODULE_VAR'] == 'test_module_variable'
+
+    os.environ.pop('TEST_MODULE_VAR')