modules: append trailing delimiter to MANPATH when set (#36678)

Update modulefile templates to append a trailing delimiter to MANPATH
environment variable, if the modulefile sets it.

With a trailing delimiter at ends of MANPATH's value, man will search
the system man pages after searching the specific paths set.

Using append-path/append_path to add this element, the module tool
ensures it is appended only once. When modulefile is unloaded, the
number of append attempt is decreased, thus the trailing delimiter is
removed only if this number equals 0.

Disclaimer: no path element should be appended to MANPATH by generated
modulefiles. It should always be prepended to ensure this variable's
value ends with the trailing delimiter.

Fixes #11355.

8 files changed, 143 insertions, 1 deletions

diff --git a/lib/spack/spack/modules/common.py b/lib/spack/spack/modules/common.py
index ef3f751471..6384069c42 100644
--- a/lib/spack/spack/modules/common.py
+++ b/lib/spack/spack/modules/common.py
@@ -40,7 +40,7 @@ from typing import Optional
 
 import llnl.util.filesystem
 import llnl.util.tty as tty
-from llnl.util.lang import dedupe
+from llnl.util.lang import dedupe, memoized
 
 import spack.build_environment
 import spack.config
@@ -672,6 +672,7 @@ class BaseContext(tengine.Context):
         return None
 
     @tengine.context_property
+    @memoized
     def environment_modifications(self):
         """List of environment modifications to be processed."""
         # Modifications guessed by inspecting the spec prefix
@@ -743,6 +744,19 @@ class BaseContext(tengine.Context):
         return [(type(x).__name__, x) for x in env if x.name not in exclude]
 
     @tengine.context_property
+    def has_manpath_modifications(self):
+        """True if MANPATH environment variable is modified."""
+        for modification_type, cmd in self.environment_modifications:
+            if not isinstance(
+                cmd, (spack.util.environment.PrependPath, spack.util.environment.AppendPath)
+            ):
+                continue
+            if cmd.name == "MANPATH":
+                return True
+        else:
+            return False
+
+    @tengine.context_property
     def autoload(self):
         """List of modules that needs to be loaded automatically."""
         # From 'autoload' configuration option
diff --git a/lib/spack/spack/test/modules/lmod.py b/lib/spack/spack/test/modules/lmod.py
index d8c34908aa..5cfb6896c3 100644
--- a/lib/spack/spack/test/modules/lmod.py
+++ b/lib/spack/spack/test/modules/lmod.py
@@ -167,6 +167,37 @@ class TestLmod(object):
         assert len([x for x in content if 'append_path("SPACE", "qux", " ")' in x]) == 1
         assert len([x for x in content if 'remove_path("SPACE", "qux", " ")' in x]) == 1
 
+    @pytest.mark.regression("11355")
+    def test_manpath_setup(self, modulefile_content, module_configuration):
+        """Tests specific setup of MANPATH environment variable."""
+
+        module_configuration("autoload_direct")
+
+        # no manpath set by module
+        content = modulefile_content("mpileaks")
+        assert len([x for x in content if 'append_path("MANPATH", "", ":")' in x]) == 0
+
+        # manpath set by module with prepend_path
+        content = modulefile_content("module-manpath-prepend")
+        assert (
+            len([x for x in content if 'prepend_path("MANPATH", "/path/to/man", ":")' in x]) == 1
+        )
+        assert (
+            len([x for x in content if 'prepend_path("MANPATH", "/path/to/share/man", ":")' in x])
+            == 1
+        )
+        assert len([x for x in content if 'append_path("MANPATH", "", ":")' in x]) == 1
+
+        # manpath set by module with append_path
+        content = modulefile_content("module-manpath-append")
+        assert len([x for x in content if 'append_path("MANPATH", "/path/to/man", ":")' in x]) == 1
+        assert len([x for x in content if 'append_path("MANPATH", "", ":")' in x]) == 1
+
+        # manpath set by module with setenv
+        content = modulefile_content("module-manpath-setenv")
+        assert len([x for x in content if 'setenv("MANPATH", "/path/to/man")' in x]) == 1
+        assert len([x for x in content if 'append_path("MANPATH", "", ":")' in x]) == 0
+
     def test_help_message(self, modulefile_content, module_configuration):
         """Tests the generation of module help message."""
 
diff --git a/lib/spack/spack/test/modules/tcl.py b/lib/spack/spack/test/modules/tcl.py
index 57f7c2ba36..5b0f9f789d 100644
--- a/lib/spack/spack/test/modules/tcl.py
+++ b/lib/spack/spack/test/modules/tcl.py
@@ -121,6 +121,46 @@ class TestTcl(object):
         assert len([x for x in content if 'append-path --delim " " SPACE "qux"' in x]) == 1
         assert len([x for x in content if 'remove-path --delim " " SPACE "qux"' in x]) == 1
 
+    @pytest.mark.regression("11355")
+    def test_manpath_setup(self, modulefile_content, module_configuration):
+        """Tests specific setup of MANPATH environment variable."""
+
+        module_configuration("autoload_direct")
+
+        # no manpath set by module
+        content = modulefile_content("mpileaks")
+        assert len([x for x in content if 'append-path --delim ":" MANPATH ""' in x]) == 0
+
+        # manpath set by module with prepend-path
+        content = modulefile_content("module-manpath-prepend")
+        assert (
+            len([x for x in content if 'prepend-path --delim ":" MANPATH "/path/to/man"' in x])
+            == 1
+        )
+        assert (
+            len(
+                [
+                    x
+                    for x in content
+                    if 'prepend-path --delim ":" MANPATH "/path/to/share/man"' in x
+                ]
+            )
+            == 1
+        )
+        assert len([x for x in content if 'append-path --delim ":" MANPATH ""' in x]) == 1
+
+        # manpath set by module with append-path
+        content = modulefile_content("module-manpath-append")
+        assert (
+            len([x for x in content if 'append-path --delim ":" MANPATH "/path/to/man"' in x]) == 1
+        )
+        assert len([x for x in content if 'append-path --delim ":" MANPATH ""' in x]) == 1
+
+        # manpath set by module with setenv
+        content = modulefile_content("module-manpath-setenv")
+        assert len([x for x in content if 'setenv MANPATH "/path/to/man"' in x]) == 1
+        assert len([x for x in content if 'append-path --delim ":" MANPATH ""' in x]) == 0
+
     def test_help_message(self, modulefile_content, module_configuration):
         """Tests the generation of module help message."""
 
diff --git a/share/spack/templates/modules/modulefile.lua b/share/spack/templates/modules/modulefile.lua
index 42ef9e5fd1..f86e76cfe6 100644
--- a/share/spack/templates/modules/modulefile.lua
+++ b/share/spack/templates/modules/modulefile.lua
@@ -84,6 +84,10 @@ setenv("{{ cmd.name }}", "{{ cmd.value }}")
 unsetenv("{{ cmd.name }}")
 {% endif %}
 {% endfor %}
+{# Make sure system man pages are enabled by appending trailing delimiter to MANPATH #}
+{% if has_manpath_modifications %}
+append_path("MANPATH", "", ":")
+{% endif %}
 {% endblock %}
 
 {% block footer %}
diff --git a/share/spack/templates/modules/modulefile.tcl b/share/spack/templates/modules/modulefile.tcl
index 935d1df72d..577c40e47c 100644
--- a/share/spack/templates/modules/modulefile.tcl
+++ b/share/spack/templates/modules/modulefile.tcl
@@ -58,6 +58,10 @@ unsetenv {{ cmd.name }}
 {% endif %}
 {#  #}
 {% endfor %}
+{# Make sure system man pages are enabled by appending trailing delimiter to MANPATH #}
+{% if has_manpath_modifications %}
+append-path --delim ":" MANPATH ""
+{% endif %}
 {% endblock %}
 
 {% block footer %}
diff --git a/var/spack/repos/builtin.mock/packages/module-manpath-append/package.py b/var/spack/repos/builtin.mock/packages/module-manpath-append/package.py
new file mode 100644
index 0000000000..5f31d17a8f
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/module-manpath-append/package.py
@@ -0,0 +1,16 @@
+# Copyright 2013-2023 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack.package import *
+
+
+class ModuleManpathAppend(Package):
+    homepage = "http://www.llnl.gov"
+    url = "http://www.llnl.gov/module-manpath-append-1.0.tar.gz"
+
+    version("1.0", "0123456789abcdef0123456789abcdef")
+
+    def setup_run_environment(self, env):
+        env.append_path("MANPATH", "/path/to/man")
diff --git a/var/spack/repos/builtin.mock/packages/module-manpath-prepend/package.py b/var/spack/repos/builtin.mock/packages/module-manpath-prepend/package.py
new file mode 100644
index 0000000000..f72a280a8e
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/module-manpath-prepend/package.py
@@ -0,0 +1,17 @@
+# Copyright 2013-2023 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack.package import *
+
+
+class ModuleManpathPrepend(Package):
+    homepage = "http://www.llnl.gov"
+    url = "http://www.llnl.gov/module-manpath-prepend-1.0.tar.gz"
+
+    version("1.0", "0123456789abcdef0123456789abcdef")
+
+    def setup_run_environment(self, env):
+        env.prepend_path("MANPATH", "/path/to/man")
+        env.prepend_path("MANPATH", "/path/to/share/man")
diff --git a/var/spack/repos/builtin.mock/packages/module-manpath-setenv/package.py b/var/spack/repos/builtin.mock/packages/module-manpath-setenv/package.py
new file mode 100644
index 0000000000..a3ffe1c02e
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/module-manpath-setenv/package.py
@@ -0,0 +1,16 @@
+# Copyright 2013-2023 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack.package import *
+
+
+class ModuleManpathSetenv(Package):
+    homepage = "http://www.llnl.gov"
+    url = "http://www.llnl.gov/module-manpath-setenv-1.0.tar.gz"
+
+    version("1.0", "0123456789abcdef0123456789abcdef")
+
+    def setup_run_environment(self, env):
+        env.set("MANPATH", "/path/to/man")