generate modules of non-roots during spack install of env (#42147)

Fixes a bug where Spack did not generate module files of non-roots during
spack install with an active environment.

The reason being that Environment.new_installs only contained roots.

This PR:

Drops special casing of automatic module generation in post-install hooks
When `use_view`, compute environment variable modifications like always, and
applies a view projection to them afterwards, like we do for spack env activate.
This ensures we don't have to delay module generation until after the view is
created.

Fixes another bug in use_view where prefixes of dependencies would not be
projected -- previously Spack would only temporarily set the current spec's prefix.
Removes the one and only use of the post_env_write hook (but doesn't drop it to
make this backportable w/o changes)

6 files changed, 91 insertions, 104 deletions

diff --git a/lib/spack/docs/conf.py b/lib/spack/docs/conf.py
index 64a55b3151..8f52edb89c 100644
--- a/lib/spack/docs/conf.py
+++ b/lib/spack/docs/conf.py
@@ -215,6 +215,7 @@ nitpick_ignore = [
     ("py:class", "spack.spec.InstallStatus"),
     ("py:class", "spack.spec.SpecfileReaderBase"),
     ("py:class", "spack.install_test.Pb"),
+    ("py:class", "spack.filesystem_view.SimpleFilesystemView"),
 ]
 
 # The reST default role (used for this markup: `text`) to use for all documents.
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index 25e2360fdc..2a6cc6e774 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -611,39 +611,33 @@ class ViewDescriptor:
         return spack.util.hash.b32_hash(contents)
 
     def get_projection_for_spec(self, spec):
-        """Get projection for spec relative to view root
+        """Get projection for spec. This function does not require the view
+        to exist on the filesystem."""
+        return self._view(self.root).get_projection_for_spec(spec)
 
-        Getting the projection from the underlying root will get the temporary
-        projection. This gives the permanent projection relative to the root
-        symlink.
+    def view(self, new: Optional[str] = None) -> SimpleFilesystemView:
         """
-        view = self.view()
-        view_path = view.get_projection_for_spec(spec)
-        rel_path = os.path.relpath(view_path, self._current_root)
-        return os.path.join(self.root, rel_path)
-
-    def view(self, new=None):
-        """
-        Generate the FilesystemView object for this ViewDescriptor
-
-        By default, this method returns a FilesystemView object rooted at the
-        current underlying root of this ViewDescriptor (self._current_root)
+        Returns a view object for the *underlying* view directory. This means that the
+        self.root symlink is followed, and that the view has to exist on the filesystem
+        (unless ``new``). This function is useful when writing to the view.
 
         Raise if new is None and there is no current view
 
         Arguments:
-            new (str or None): If a string, create a FilesystemView
-                rooted at that path. Default None. This should only be used to
-                regenerate the view, and cannot be used to access specs.
+            new: If a string, create a FilesystemView rooted at that path. Default None. This
+                should only be used to regenerate the view, and cannot be used to access specs.
         """
         root = new if new else self._current_root
         if not root:
             # This can only be hit if we write a future bug
-            msg = (
+            raise SpackEnvironmentViewError(
                 "Attempting to get nonexistent view from environment. "
-                "View root is at %s" % self.root
+                f"View root is at {self.root}"
             )
-            raise SpackEnvironmentViewError(msg)
+        return self._view(root)
+
+    def _view(self, root: str) -> SimpleFilesystemView:
+        """Returns a view object for a given root dir."""
         return SimpleFilesystemView(
             root,
             spack.store.STORE.layout,
@@ -810,7 +804,6 @@ class Environment:
 
         self._unify = None
         self.new_specs: List[Spec] = []
-        self.new_installs: List[Spec] = []
         self.views: Dict[str, ViewDescriptor] = {}
 
         #: Specs from "spack.yaml"
@@ -939,10 +932,8 @@ class Environment:
         """Clear the contents of the environment
 
         Arguments:
-            re_read (bool): If ``True``, do not clear ``new_specs`` nor
-                ``new_installs`` values. These values cannot be read from
-                yaml, and need to be maintained when re-reading an existing
-                environment.
+            re_read: If ``True``, do not clear ``new_specs``. This value cannot be read from yaml,
+                and needs to be maintained when re-reading an existing environment.
         """
         self.spec_lists = collections.OrderedDict()
         self.spec_lists[user_speclist_name] = SpecList()
@@ -956,7 +947,6 @@ class Environment:
         if not re_read:
             # things that cannot be recreated from file
             self.new_specs = []  # write packages for these on write()
-            self.new_installs = []  # write modules for these on write()
 
     @property
     def active(self):
@@ -1751,10 +1741,7 @@ class Environment:
 
         installs = [(spec.package, {**install_args, "explicit": spec in roots}) for spec in specs]
 
-        try:
-            PackageInstaller(installs).install()
-        finally:
-            self.new_installs.extend(s for s in specs if s.installed)
+        PackageInstaller(installs).install()
 
     def all_specs_generator(self) -> Iterable[Spec]:
         """Returns a generator for all concrete specs"""
@@ -2077,11 +2064,7 @@ class Environment:
             self.regenerate_views()
             spack.hooks.post_env_write(self)
 
-        self._reset_new_specs_and_installs()
-
-    def _reset_new_specs_and_installs(self) -> None:
-        self.new_specs = []
-        self.new_installs = []
+        self.new_specs.clear()
 
     def update_lockfile(self) -> None:
         with fs.write_tmp_and_move(self.lock_path) as f:
diff --git a/lib/spack/spack/hooks/module_file_generation.py b/lib/spack/spack/hooks/module_file_generation.py
index cb51a4eb85..485861d27c 100644
--- a/lib/spack/spack/hooks/module_file_generation.py
+++ b/lib/spack/spack/hooks/module_file_generation.py
@@ -34,21 +34,8 @@ def _for_each_enabled(
 
 
 def post_install(spec, explicit: bool):
-    import spack.environment as ev  # break import cycle
-
-    if ev.active_environment():
-        # If the installed through an environment, we skip post_install
-        # module generation and generate the modules on env_write so Spack
-        # can manage interactions between env views and modules
-        return
-
     _for_each_enabled(spec, "write", explicit)
 
 
 def post_uninstall(spec):
     _for_each_enabled(spec, "remove")
-
-
-def post_env_write(env):
-    for spec in env.new_installs:
-        _for_each_enabled(spec, "write")
diff --git a/lib/spack/spack/modules/common.py b/lib/spack/spack/modules/common.py
index 1246403c89..a0b9870f12 100644
--- a/lib/spack/spack/modules/common.py
+++ b/lib/spack/spack/modules/common.py
@@ -43,6 +43,7 @@ from llnl.util.lang import dedupe, memoized
 
 import spack.build_environment
 import spack.config
+import spack.deptypes as dt
 import spack.environment
 import spack.error
 import spack.modules.common
@@ -53,6 +54,7 @@ import spack.schema.environment
 import spack.spec
 import spack.store
 import spack.tengine as tengine
+import spack.user_environment
 import spack.util.environment
 import spack.util.file_permissions as fp
 import spack.util.path
@@ -695,28 +697,33 @@ class BaseContext(tengine.Context):
         )
         spack.config.merge_yaml(
             prefix_inspections,
-            spack.config.get("modules:%s:prefix_inspections" % self.conf.name, {}),
+            spack.config.get(f"modules:{self.conf.name}:prefix_inspections", {}),
         )
 
-        use_view = spack.config.get("modules:%s:use_view" % self.conf.name, False)
+        use_view = spack.config.get(f"modules:{self.conf.name}:use_view", False)
 
-        spec = self.spec.copy()  # defensive copy before setting prefix
-        if use_view:
-            if use_view is True:
-                use_view = spack.environment.default_view_name
+        assert isinstance(use_view, (bool, str))
 
+        if use_view:
             env = spack.environment.active_environment()
             if not env:
                 raise spack.environment.SpackEnvironmentViewError(
                     "Module generation with views requires active environment"
                 )
 
-            view = env.views[use_view]
+            view_name = spack.environment.default_view_name if use_view is True else use_view
 
-            spec.prefix = view.get_projection_for_spec(spec)
+            if not env.has_view(view_name):
+                raise spack.environment.SpackEnvironmentViewError(
+                    f"View {view_name} not found in environment {env.name} when generating modules"
+                )
+
+            view = env.views[view_name]
+        else:
+            view = None
 
         env = spack.util.environment.inspect_path(
-            spec.prefix, prefix_inspections, exclude=spack.util.environment.is_system_path
+            self.spec.prefix, prefix_inspections, exclude=spack.util.environment.is_system_path
         )
 
         # Let the extendee/dependency modify their extensions/dependencies
@@ -726,13 +733,19 @@ class BaseContext(tengine.Context):
         # whole chain of setup_dependent_package has to be followed from leaf to spec.
         # So: just run it here, but don't collect env mods.
         spack.build_environment.SetupContext(
-            spec, context=Context.RUN
+            self.spec, context=Context.RUN
         ).set_all_package_py_globals()
 
         # Then run setup_dependent_run_environment before setup_run_environment.
-        for dep in spec.dependencies(deptype=("link", "run")):
-            dep.package.setup_dependent_run_environment(env, spec)
-        spec.package.setup_run_environment(env)
+        for dep in self.spec.dependencies(deptype=("link", "run")):
+            dep.package.setup_dependent_run_environment(env, self.spec)
+        self.spec.package.setup_run_environment(env)
+
+        # Project the environment variables from prefix to view if needed
+        if view and self.spec in view:
+            spack.user_environment.project_env_mods(
+                *self.spec.traverse(deptype=dt.LINK | dt.RUN), view=view, env=env
+            )
 
         # Modifications required from modules.yaml
         env.extend(self.conf.env)
@@ -754,11 +767,11 @@ class BaseContext(tengine.Context):
             msg = "some tokens cannot be expanded in an environment variable name"
             _check_tokens_are_valid(x.name, message=msg)
             # Transform them
-            x.name = spec.format(x.name, transform=transform)
+            x.name = self.spec.format(x.name, transform=transform)
             if self.modification_needs_formatting(x):
                 try:
                     # Not every command has a value
-                    x.value = spec.format(x.value)
+                    x.value = self.spec.format(x.value)
                 except AttributeError:
                     pass
             x.name = str(x.name).replace("-", "_")
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index 74ce4d31d1..c9137e6aea 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -2970,51 +2970,51 @@ spack:
     assert spec.prefix not in contents
 
 
-def test_multiple_modules_post_env_hook(environment_from_manifest, install_mockery, mock_fetch):
+def test_modules_exist_after_env_install(
+    environment_from_manifest, install_mockery, mock_fetch, monkeypatch
+):
+    # Some caching issue
+    monkeypatch.setattr(spack.modules.tcl, "configuration_registry", {})
     environment_from_manifest(
         """
 spack:
   specs:
-  - trivial-install-test-package
+  - mpileaks
   modules:
     default:
       enable:: [tcl]
       use_view: true
       roots:
-        tcl: modules
+        tcl: uses_view
     full:
       enable:: [tcl]
       roots:
-        tcl: full_modules
+        tcl: without_view
 """
     )
 
     with ev.read("test") as e:
         install()
-
-        spec = e.specs_by_hash[e.concretized_order[0]]
-        view_prefix = e.default_view.get_projection_for_spec(spec)
-        modules_glob = "%s/modules/**/*/*" % e.path
-        modules = glob.glob(modules_glob)
-        assert len(modules) == 1
-        module = modules[0]
-
-        full_modules_glob = "%s/full_modules/**/*/*" % e.path
-        full_modules = glob.glob(full_modules_glob)
-        assert len(full_modules) == 1
-        full_module = full_modules[0]
-
-    with open(module, "r") as f:
-        contents = f.read()
-
-    with open(full_module, "r") as f:
-        full_contents = f.read()
-
-    assert view_prefix in contents
-    assert spec.prefix not in contents
-
-    assert view_prefix not in full_contents
-    assert spec.prefix in full_contents
+        specs = e.all_specs()
+
+        for module_set in ("uses_view", "without_view"):
+            modules = glob.glob(f"{e.path}/{module_set}/**/*/*")
+            assert len(modules) == len(specs), "Not all modules were generated"
+            for spec in specs:
+                module = next((m for m in modules if os.path.dirname(m).endswith(spec.name)), None)
+                assert module, f"Module for {spec} not found"
+
+                # Now verify that modules have paths pointing into the view instead of the package
+                # prefix if and only if they set use_view to true.
+                with open(module, "r") as f:
+                    contents = f.read()
+
+                if module_set == "uses_view":
+                    assert e.default_view.get_projection_for_spec(spec) in contents
+                    assert spec.prefix not in contents
+                else:
+                    assert e.default_view.get_projection_for_spec(spec) not in contents
+                    assert spec.prefix in contents
 
 
 @pytest.mark.regression("24148")
diff --git a/lib/spack/spack/user_environment.py b/lib/spack/spack/user_environment.py
index 32f3c809c1..756b7c09a2 100644
--- a/lib/spack/spack/user_environment.py
+++ b/lib/spack/spack/user_environment.py
@@ -66,6 +66,20 @@ def unconditional_environment_modifications(view):
     return env
 
 
+def project_env_mods(
+    *specs: spack.spec.Spec, view, env: environment.EnvironmentModifications
+) -> None:
+    """Given a list of environment modifications, project paths changes to the view."""
+    prefix_to_prefix = {s.prefix: view.get_projection_for_spec(s) for s in specs if not s.external}
+    # Avoid empty regex if all external
+    if not prefix_to_prefix:
+        return
+    prefix_regex = re.compile("|".join(re.escape(p) for p in prefix_to_prefix.keys()))
+    for mod in env.env_modifications:
+        if isinstance(mod, environment.NameValueModifier):
+            mod.value = prefix_regex.sub(lambda m: prefix_to_prefix[m.group(0)], mod.value)
+
+
 def environment_modifications_for_specs(
     *specs: spack.spec.Spec, view=None, set_package_py_globals: bool = True
 ):
@@ -101,17 +115,6 @@ def environment_modifications_for_specs(
 
     # Apply view projections if any.
     if view:
-        prefix_to_prefix = {
-            s.prefix: view.get_projection_for_spec(s)
-            for s in reversed(topo_ordered)
-            if not s.external
-        }
-        # Avoid empty regex if all external
-        if not prefix_to_prefix:
-            return env
-        prefix_regex = re.compile("|".join(re.escape(p) for p in prefix_to_prefix.keys()))
-        for mod in env.env_modifications:
-            if isinstance(mod, environment.NameValueModifier):
-                mod.value = prefix_regex.sub(lambda m: prefix_to_prefix[m.group(0)], mod.value)
+        project_env_mods(*topo_ordered, view=view, env=env)
 
     return env