`SetupContext.get_env_modifications` fixes and documentation (#40683)

Call setup_dependent_run_environment on both link and run edges,
instead of only run edges, which restores old behavior.

Move setup_build_environment into get_env_modifications

Also call setup_run_environment on direct build deps, since their run
environment has to be set up.

3 files changed, 83 insertions, 74 deletions

diff --git a/lib/spack/docs/images/setup_env.png b/lib/spack/docs/images/setup_env.png
new file mode 100644
index 0000000000..4b16cac281
--- /dev/null
+++ b/lib/spack/docs/images/setup_env.png
diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst
index fad913cb0f..89afac75fe 100644
--- a/lib/spack/docs/packaging_guide.rst
+++ b/lib/spack/docs/packaging_guide.rst
@@ -2688,60 +2688,6 @@ appear in the package file (or in this case, in the list).
    right version. If two packages depend on ``binutils`` patched *the
    same* way, they can both use a single installation of ``binutils``.
 
-.. _setup-dependent-environment:
-
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-Influence how dependents are built or run
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-Spack provides a mechanism for dependencies to influence the
-environment of their dependents by overriding  the
-:meth:`setup_dependent_run_environment <spack.package_base.PackageBase.setup_dependent_run_environment>`
-or the
-:meth:`setup_dependent_build_environment <spack.builder.Builder.setup_dependent_build_environment>`
-methods.
-The Qt package, for instance, uses this call:
-
-.. literalinclude:: _spack_root/var/spack/repos/builtin/packages/qt/package.py
-   :pyobject: Qt.setup_dependent_build_environment
-   :linenos:
-
-to set the ``QTDIR`` environment variable so that packages
-that depend on a particular Qt installation will find it.
-Another good example of how a dependency can influence
-the build environment of dependents is the Python package:
-
-.. literalinclude:: _spack_root/var/spack/repos/builtin/packages/python/package.py
-   :pyobject: Python.setup_dependent_build_environment
-   :linenos:
-
-In the method above it is ensured that any package that depends on Python
-will have the ``PYTHONPATH``, ``PYTHONHOME`` and ``PATH`` environment
-variables set appropriately before starting the installation. To make things
-even simpler the ``python setup.py`` command is also inserted into the module
-scope of dependents by overriding a third method called
-:meth:`setup_dependent_package <spack.package_base.PackageBase.setup_dependent_package>`
-:
-
-.. literalinclude:: _spack_root/var/spack/repos/builtin/packages/python/package.py
-   :pyobject: Python.setup_dependent_package
-   :linenos:
-
-This allows most python packages to have a very simple install procedure,
-like the following:
-
-.. code-block:: python
-
-   def install(self, spec, prefix):
-       setup_py("install", "--prefix={0}".format(prefix))
-
-Finally the Python package takes also care of the modifications to ``PYTHONPATH``
-to allow dependencies to run correctly:
-
-.. literalinclude:: _spack_root/var/spack/repos/builtin/packages/python/package.py
-    :pyobject: Python.setup_dependent_run_environment
-    :linenos:
-
 
 .. _packaging_conflicts:
 
@@ -2886,6 +2832,70 @@ variant(s) are selected.  This may be accomplished with conditional
        extends("python", when="+python")
        ...
 
+.. _setup-environment:
+
+--------------------------------------------
+Runtime and build time environment variables
+--------------------------------------------
+
+Spack provides a few methods to help package authors set up the required environment variables for
+their package. Environment variables typically depend on how the package is used: variables that
+make sense during the build phase may not be needed at runtime, and vice versa. Further, sometimes
+it makes sense to let a dependency set the environment variables for its dependents. To allow all
+this, Spack provides four different methods that can be overridden in a package:
+
+1. :meth:`setup_build_environment <spack.builder.Builder.setup_build_environment>`
+2. :meth:`setup_run_environment <spack.package_base.PackageBase.setup_run_environment>`
+3. :meth:`setup_dependent_build_environment <spack.builder.Builder.setup_dependent_build_environment>`
+4. :meth:`setup_dependent_run_environment <spack.package_base.PackageBase.setup_dependent_run_environment>`
+
+The Qt package, for instance, uses this call:
+
+.. literalinclude:: _spack_root/var/spack/repos/builtin/packages/qt/package.py
+   :pyobject: Qt.setup_dependent_build_environment
+   :linenos:
+
+to set the ``QTDIR`` environment variable so that packages that depend on a particular Qt
+installation will find it.
+
+The following diagram will give you an idea when each of these methods is called in a build
+context:
+
+.. image:: images/setup_env.png
+   :align: center
+
+Notice that ``setup_dependent_run_environment`` can be called multiple times, once for each
+dependent package, whereas ``setup_run_environment`` is called only once for the package itself.
+This means that the former should only be used if the environment variables depend on the dependent
+package, whereas the latter should be used if the environment variables depend only on the package
+itself.
+
+--------------------------------
+Setting package module variables
+--------------------------------
+
+Apart from modifying environment variables of the dependent package, you can also define Python
+variables to be used by the dependent. This is done by implementing
+:meth:`setup_dependent_package <spack.package_base.PackageBase.setup_dependent_package>`. An
+example of this can be found in the ``Python`` package:
+
+.. literalinclude:: _spack_root/var/spack/repos/builtin/packages/python/package.py
+   :pyobject: Python.setup_dependent_package
+   :linenos:
+
+This allows Python packages to directly use these variables:
+
+.. code-block:: python
+
+   def install(self, spec, prefix):
+       ...
+       install("script.py", python_platlib)
+
+.. note::
+
+   We recommend using ``setup_dependent_package`` sparingly, as it is not always clear where
+   global variables are coming from when editing a ``package.py`` file.
+
 -----
 Views
 -----
diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
index 96c8cb8a4a..3f6830ad33 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -752,19 +752,13 @@ def setup_package(pkg, dirty, context: Context = Context.BUILD):
     target = platform.target(pkg.spec.architecture.target)
     platform.setup_platform_environment(pkg, env_mods)
 
-    if context == Context.BUILD:
-        tty.debug("setup_package: setup build environment for root")
-        builder = spack.builder.create(pkg)
-        builder.setup_build_environment(env_mods)
-
-        if (not dirty) and (not env_mods.is_unset("CPATH")):
-            tty.debug(
-                "A dependency has updated CPATH, this may lead pkg-"
-                "config to assume that the package is part of the system"
-                " includes and omit it when invoked with '--cflags'."
-            )
-    elif context == Context.TEST:
+    if context == Context.TEST:
         env_mods.prepend_path("PATH", ".")
+    elif context == Context.BUILD and not dirty and not env_mods.is_unset("CPATH"):
+        tty.debug(
+            "A dependency has updated CPATH, this may lead pkg-config to assume that the package "
+            "is part of the system includes and omit it when invoked with '--cflags'."
+        )
 
     # First apply the clean environment changes
     env_base.apply_modifications()
@@ -953,8 +947,11 @@ class SetupContext:
             reversed(specs_with_type), lambda t: t[0].external
         )
         self.should_be_runnable = UseMode.BUILDTIME_DIRECT | UseMode.RUNTIME_EXECUTABLE
-        self.should_setup_run_env = UseMode.RUNTIME | UseMode.RUNTIME_EXECUTABLE
+        self.should_setup_run_env = (
+            UseMode.BUILDTIME_DIRECT | UseMode.RUNTIME | UseMode.RUNTIME_EXECUTABLE
+        )
         self.should_setup_dependent_build_env = UseMode.BUILDTIME | UseMode.BUILDTIME_DIRECT
+        self.should_setup_build_env = UseMode.ROOT if context == Context.BUILD else UseMode(0)
 
         if context == Context.RUN or context == Context.TEST:
             self.should_be_runnable |= UseMode.ROOT
@@ -994,8 +991,9 @@ class SetupContext:
         - Updating PATH for packages that are required at runtime
         - Updating CMAKE_PREFIX_PATH and PKG_CONFIG_PATH so that their respective
         tools can find Spack-built dependencies (when context=build)
-        - Running custom package environment modifications (setup_run_environment,
-        setup_dependent_build_environment, setup_dependent_run_environment)
+        - Running custom package environment modifications: setup_run_environment,
+        setup_dependent_run_environment, setup_build_environment,
+        setup_dependent_build_environment.
 
         The (partial) order imposed on the specs is externals first, then topological
         from leaf to root. That way externals cannot contribute search paths that would shadow
@@ -1008,16 +1006,17 @@ class SetupContext:
             if self.should_setup_dependent_build_env & flag:
                 self._make_buildtime_detectable(dspec, env)
 
-                for spec in self.specs:
-                    builder = spack.builder.create(pkg)
-                    builder.setup_dependent_build_environment(env, spec)
+                for root in self.specs:  # there is only one root in build context
+                    spack.builder.create(pkg).setup_dependent_build_environment(env, root)
+
+            if self.should_setup_build_env & flag:
+                spack.builder.create(pkg).setup_build_environment(env)
 
             if self.should_be_runnable & flag:
                 self._make_runnable(dspec, env)
 
             if self.should_setup_run_env & flag:
-                # TODO: remove setup_dependent_run_environment...
-                for spec in dspec.dependents(deptype=dt.RUN):
+                for spec in dspec.dependents(deptype=dt.LINK | dt.RUN):
                     if id(spec) in self.nodes_in_subdag:
                         pkg.setup_dependent_run_environment(env, spec)
                 pkg.setup_run_environment(env)