From 746eaaf01ac824caa9649b368249b5a6ebdb2311 Mon Sep 17 00:00:00 2001 From: Xavier Delaruelle Date: Tue, 13 Jun 2023 10:29:11 +0200 Subject: 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. --- lib/spack/spack/modules/common.py | 16 ++++++++- lib/spack/spack/test/modules/lmod.py | 31 +++++++++++++++++ lib/spack/spack/test/modules/tcl.py | 40 ++++++++++++++++++++++ share/spack/templates/modules/modulefile.lua | 4 +++ share/spack/templates/modules/modulefile.tcl | 4 +++ .../packages/module-manpath-append/package.py | 16 +++++++++ .../packages/module-manpath-prepend/package.py | 17 +++++++++ .../packages/module-manpath-setenv/package.py | 16 +++++++++ 8 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 var/spack/repos/builtin.mock/packages/module-manpath-append/package.py create mode 100644 var/spack/repos/builtin.mock/packages/module-manpath-prepend/package.py create mode 100644 var/spack/repos/builtin.mock/packages/module-manpath-setenv/package.py 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 @@ -742,6 +743,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.""" 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") -- cgit v1.2.3-70-g09d2