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)

lib/spack/docs/conf.py

@@ -215,6 +215,7 @@ def setup(sphinx):
     ("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.

lib/spack/spack/environment/environment.py

@@ -611,39 +611,33 @@ def content_hash(self, specs):
         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
+    def view(self, new: Optional[str] = None) -> SimpleFilesystemView:
-        projection. This gives the permanent projection relative to the root
-        symlink.
         """
-        view = self.view()
+        Returns a view object for the *underlying* view directory. This means that the
-        view_path = view.get_projection_for_spec(spec)
+        self.root symlink is followed, and that the view has to exist on the filesystem
-        rel_path = os.path.relpath(view_path, self._current_root)
+        (unless ``new``). This function is useful when writing to the view.
-        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)
 
         Raise if new is None and there is no current view
 
         Arguments:
-            new (str or None): If a string, create a FilesystemView
+            new: If a string, create a FilesystemView rooted at that path. Default None. This
-                rooted at that path. Default None. This should only be used to
+                should only be used to regenerate the view, and cannot be used to access specs.
-                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 @@ def __init__(self, manifest_dir: Union[str, pathlib.Path]) -> None:
 
         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 @@ def clear(self, re_read=False):
         """Clear the contents of the environment
 
         Arguments:
-            re_read (bool): If ``True``, do not clear ``new_specs`` nor
+            re_read: If ``True``, do not clear ``new_specs``. This value cannot be read from yaml,
-                ``new_installs`` values. These values cannot be read from
+                and needs to be maintained when re-reading an existing environment.
-                yaml, and need 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 @@ def clear(self, re_read=False):
         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 @@ def install_specs(self, specs: Optional[List[Spec]] = None, **install_args):
 
         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)
 
     def all_specs_generator(self) -> Iterable[Spec]:
         """Returns a generator for all concrete specs"""
@@ -2077,11 +2064,7 @@ def write(self, regenerate: bool = True) -> None:
             self.regenerate_views()
             spack.hooks.post_env_write(self)
 
-        self._reset_new_specs_and_installs()
+        self.new_specs.clear()
-
-    def _reset_new_specs_and_installs(self) -> None:
-        self.new_specs = []
-        self.new_installs = []
 
     def update_lockfile(self) -> None:
         with fs.write_tmp_and_move(self.lock_path) as f:

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")

lib/spack/spack/modules/common.py

@@ -43,6 +43,7 @@
 
 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.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 @@ def environment_modifications(self):
         )
         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)
+
+        assert isinstance(use_view, (bool, str))
 
-        spec = self.spec.copy()  # defensive copy before setting prefix
         if use_view:
-            if use_view is True:
-                use_view = spack.environment.default_view_name
-
             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 @@ def environment_modifications(self):
         # 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")):
+        for dep in self.spec.dependencies(deptype=("link", "run")):
-            dep.package.setup_dependent_run_environment(env, spec)
+            dep.package.setup_dependent_run_environment(env, self.spec)
-        spec.package.setup_run_environment(env)
+        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 @@ def environment_modifications(self):
             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("-", "_")

lib/spack/spack/test/cmd/env.py

@@ -2970,51 +2970,51 @@ def test_modules_relative_to_views(environment_from_manifest, install_mockery, m
     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()
+        specs = e.all_specs()
 
-        spec = e.specs_by_hash[e.concretized_order[0]]
+        for module_set in ("uses_view", "without_view"):
-        view_prefix = e.default_view.get_projection_for_spec(spec)
+            modules = glob.glob(f"{e.path}/{module_set}/**/*/*")
-        modules_glob = "%s/modules/**/*/*" % e.path
+            assert len(modules) == len(specs), "Not all modules were generated"
-        modules = glob.glob(modules_glob)
+            for spec in specs:
-        assert len(modules) == 1
+                module = next((m for m in modules if os.path.dirname(m).endswith(spec.name)), None)
-        module = modules[0]
+                assert module, f"Module for {spec} not found"
-
-        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]
 
+                # 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()
 
-    with open(full_module, "r") as f:
+                if module_set == "uses_view":
-        full_contents = f.read()
+                    assert e.default_view.get_projection_for_spec(spec) in contents
-
-    assert view_prefix in contents
                     assert spec.prefix not in contents
-
+                else:
-    assert view_prefix not in full_contents
+                    assert e.default_view.get_projection_for_spec(spec) not in contents
-    assert spec.prefix in full_contents
+                    assert spec.prefix in contents
 
 
 @pytest.mark.regression("24148")

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 = {
+        project_env_mods(*topo_ordered, view=view, env=env)
-            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)
 
     return env