Merge pull request #3770 from Flamefire/duplicate_path_module

akesandgren · web-flow · commit 4cde6297a036 · 2021-10-21T10:39:04.000+02:00
Filter out duplicate paths added to module files
diff --git a/easybuild/framework/easyblock.py b/easybuild/framework/easyblock.py
@@ -2451,7 +2451,8 @@ def extensions_step(self, fetch=False, install=True):
             if install:
                 try:
                     ext.prerun()
-                    txt = ext.run()
+                    with self.module_generator.start_module_creation():
+                        txt = ext.run()
                     if txt:
                         self.module_extra_extensions += txt
                     ext.postrun()
@@ -3222,21 +3223,18 @@ def make_module_step(self, fake=False):
         else:
             trace_msg("generating module file @ %s" % self.mod_filepath)
 
-        txt = self.module_generator.MODULE_SHEBANG
-        if txt:
-            txt += '\n'
-
-        if self.modules_header:
-            txt += self.modules_header + '\n'
-
-        txt += self.make_module_description()
-        txt += self.make_module_group_check()
-        txt += self.make_module_deppaths()
-        txt += self.make_module_dep()
-        txt += self.make_module_extend_modpath()
-        txt += self.make_module_req()
-        txt += self.make_module_extra()
-        txt += self.make_module_footer()
+        with self.module_generator.start_module_creation() as txt:
+            if self.modules_header:
+                txt += self.modules_header + '\n'
+
+            txt += self.make_module_description()
+            txt += self.make_module_group_check()
+            txt += self.make_module_deppaths()
+            txt += self.make_module_dep()
+            txt += self.make_module_extend_modpath()
+            txt += self.make_module_req()
+            txt += self.make_module_extra()
+            txt += self.make_module_footer()
 
         hook_txt = run_hook(MODULE_WRITE, self.hooks, args=[self, mod_filepath, txt])
         if hook_txt is not None:
diff --git a/easybuild/tools/module_generator.py b/easybuild/tools/module_generator.py
@@ -37,11 +37,12 @@
 import os
 import re
 import tempfile
+from contextlib import contextmanager
 from distutils.version import LooseVersion
 from textwrap import wrap
 
 from easybuild.base import fancylogger
-from easybuild.tools.build_log import EasyBuildError
+from easybuild.tools.build_log import EasyBuildError, print_warning
 from easybuild.tools.config import build_option, get_module_syntax, install_path
 from easybuild.tools.filetools import convert_name, mkdir, read_file, remove_file, resolve_path, symlink, write_file
 from easybuild.tools.modules import ROOT_ENV_VAR_NAME_PREFIX, EnvironmentModulesC, Lmod, modules_tool
@@ -136,17 +137,29 @@ def __init__(self, application, fake=False):
         self.fake_mod_path = tempfile.mkdtemp()
 
         self.modules_tool = modules_tool()
+        self.added_paths_per_key = None
 
-    def append_paths(self, key, paths, allow_abs=False, expand_relpaths=True):
+    @contextmanager
+    def start_module_creation(self):
         """
-        Generate append-path statements for the given list of paths.
+        Prepares creating a module and returns the file header (shebang) if any including the newline
 
-        :param key: environment variable to append paths to
-        :param paths: list of paths to append
-        :param allow_abs: allow providing of absolute paths
-        :param expand_relpaths: expand relative paths into absolute paths (by prefixing install dir)
+        Meant to be used in a with statement:
+            with generator.start_module_creation() as txt:
+                # Write txt
         """
-        return self.update_paths(key, paths, prepend=False, allow_abs=allow_abs, expand_relpaths=expand_relpaths)
+        if self.added_paths_per_key is not None:
+            raise EasyBuildError('Module creation already in process. '
+                                 'You cannot create multiple modules at the same time!')
+        # Mapping of keys/env vars to paths already added
+        self.added_paths_per_key = dict()
+        txt = self.MODULE_SHEBANG
+        if txt:
+            txt += '\n'
+        try:
+            yield txt
+        finally:
+            self.added_paths_per_key = None
 
     def create_symlinks(self, mod_symlink_paths, fake=False):
         """Create moduleclass symlink(s) to actual module file."""
@@ -191,6 +204,49 @@ def get_modules_path(self, fake=False, mod_path_suffix=None):
 
         return os.path.join(mod_path, mod_path_suffix)
 
+    def _filter_paths(self, key, paths):
+        """Filter out paths already added to key and return the remaining ones"""
+        if self.added_paths_per_key is None:
+            # For compatibility this is only a warning for now and we don't filter any paths
+            print_warning('Module creation has not been started. Call start_module_creation first!')
+            return paths
+
+        added_paths = self.added_paths_per_key.setdefault(key, set())
+        # paths can be a string
+        if isinstance(paths, string_type):
+            if paths in added_paths:
+                filtered_paths = None
+            else:
+                added_paths.add(paths)
+                filtered_paths = paths
+        else:
+            # Coerce any iterable/generator into a list
+            if not isinstance(paths, list):
+                paths = list(paths)
+            filtered_paths = [x for x in paths if x not in added_paths and not added_paths.add(x)]
+        if filtered_paths != paths:
+            removed_paths = paths if filtered_paths is None else [x for x in paths if x not in filtered_paths]
+            print_warning("Supressed adding the following path(s) to $%s of the module as they were already added: %s",
+                          key, removed_paths,
+                          log=self.log)
+            if not filtered_paths:
+                filtered_paths = None
+        return filtered_paths
+
+    def append_paths(self, key, paths, allow_abs=False, expand_relpaths=True):
+        """
+        Generate append-path statements for the given list of paths.
+
+        :param key: environment variable to append paths to
+        :param paths: list of paths to append
+        :param allow_abs: allow providing of absolute paths
+        :param expand_relpaths: expand relative paths into absolute paths (by prefixing install dir)
+        """
+        paths = self._filter_paths(key, paths)
+        if paths is None:
+            return ''
+        return self.update_paths(key, paths, prepend=False, allow_abs=allow_abs, expand_relpaths=expand_relpaths)
+
     def prepend_paths(self, key, paths, allow_abs=False, expand_relpaths=True):
         """
         Generate prepend-path statements for the given list of paths.
@@ -200,6 +256,9 @@ def prepend_paths(self, key, paths, allow_abs=False, expand_relpaths=True):
         :param allow_abs: allow providing of absolute paths
         :param expand_relpaths: expand relative paths into absolute paths (by prefixing install dir)
         """
+        paths = self._filter_paths(key, paths)
+        if paths is None:
+            return ''
         return self.update_paths(key, paths, prepend=True, allow_abs=allow_abs, expand_relpaths=expand_relpaths)
 
     def _modulerc_check_module_version(self, module_version):
diff --git a/test/framework/easyblock.py b/test/framework/easyblock.py
@@ -435,7 +435,8 @@ def test_make_module_req(self):
         # this is not a path that should be picked up
         os.mkdir(os.path.join(eb.installdir, 'CPATH'))
 
-        guess = eb.make_module_req()
+        with eb.module_generator.start_module_creation():
+            guess = eb.make_module_req()
 
         if get_module_syntax() == 'Tcl':
             self.assertTrue(re.search(r"^prepend-path\s+CLASSPATH\s+\$root/bla.jar$", guess, re.M))
@@ -462,7 +463,8 @@ def test_make_module_req(self):
 
         # check that bin is only added to PATH if there are files in there
         write_file(os.path.join(eb.installdir, 'bin', 'test'), 'test')
-        guess = eb.make_module_req()
+        with eb.module_generator.start_module_creation():
+            guess = eb.make_module_req()
         if get_module_syntax() == 'Tcl':
             self.assertTrue(re.search(r"^prepend-path\s+PATH\s+\$root/bin$", guess, re.M))
             self.assertFalse(re.search(r"^prepend-path\s+PATH\s+\$root/sbin$", guess, re.M))
@@ -481,7 +483,8 @@ def test_make_module_req(self):
             self.assertFalse('prepend_path("CMAKE_LIBRARY_PATH", pathJoin(root, "lib64"))' in guess)
         # -- With files
         write_file(os.path.join(eb.installdir, 'lib64', 'libfoo.so'), 'test')
-        guess = eb.make_module_req()
+        with eb.module_generator.start_module_creation():
+            guess = eb.make_module_req()
         if get_module_syntax() == 'Tcl':
             self.assertTrue(re.search(r"^prepend-path\s+CMAKE_LIBRARY_PATH\s+\$root/lib64$", guess, re.M))
         elif get_module_syntax() == 'Lua':
@@ -490,7 +493,8 @@ def test_make_module_req(self):
         write_file(os.path.join(eb.installdir, 'lib', 'libfoo.so'), 'test')
         shutil.rmtree(os.path.join(eb.installdir, 'lib64'))
         os.symlink('lib', os.path.join(eb.installdir, 'lib64'))
-        guess = eb.make_module_req()
+        with eb.module_generator.start_module_creation():
+            guess = eb.make_module_req()
         if get_module_syntax() == 'Tcl':
             self.assertFalse(re.search(r"^prepend-path\s+CMAKE_LIBRARY_PATH\s+\$root/lib64$", guess, re.M))
         elif get_module_syntax() == 'Lua':
@@ -509,7 +513,8 @@ def test_make_module_req(self):
 
         # check for behavior when a string value is used as dict value by make_module_req_guesses
         eb.make_module_req_guess = lambda: {'PATH': 'bin'}
-        txt = eb.make_module_req()
+        with eb.module_generator.start_module_creation():
+            txt = eb.make_module_req()
         if get_module_syntax() == 'Tcl':
             self.assertTrue(re.match(r"^\nprepend-path\s+PATH\s+\$root/bin\n$", txt, re.M))
         elif get_module_syntax() == 'Lua':
@@ -520,7 +525,8 @@ def test_make_module_req(self):
         # check for correct behaviour if empty string is specified as one of the values
         # prepend-path statements should be included for both the 'bin' subdir and the install root
         eb.make_module_req_guess = lambda: {'PATH': ['bin', '']}
-        txt = eb.make_module_req()
+        with eb.module_generator.start_module_creation():
+            txt = eb.make_module_req()
         if get_module_syntax() == 'Tcl':
             self.assertTrue(re.search(r"\nprepend-path\s+PATH\s+\$root/bin\n", txt, re.M))
             self.assertTrue(re.search(r"\nprepend-path\s+PATH\s+\$root\n", txt, re.M))
@@ -535,7 +541,8 @@ def test_make_module_req(self):
         for path in ['pathA', 'pathB', 'pathC']:
             os.mkdir(os.path.join(eb.installdir, 'lib', path))
             write_file(os.path.join(eb.installdir, 'lib', path, 'libfoo.so'), 'test')
-        txt = eb.make_module_req()
+        with eb.module_generator.start_module_creation():
+            txt = eb.make_module_req()
         if get_module_syntax() == 'Tcl':
             self.assertTrue(re.search(r"\nprepend-path\s+LD_LIBRARY_PATH\s+\$root/lib/pathC\n" +
                                       r"prepend-path\s+LD_LIBRARY_PATH\s+\$root/lib/pathA\n" +
@@ -1152,7 +1159,7 @@ def test_make_module_step(self):
         # purposely use a 'nasty' description, that includes (unbalanced) special chars: [, ], {, }
         descr = "This {is a}} [fancy]] [[description]]. {{[[TEST}]"
         modextravars = {'PI': '3.1415', 'FOO': 'bar'}
-        modextrapaths = {'PATH': 'pibin', 'CPATH': 'pi/include'}
+        modextrapaths = {'PATH': ('bin', 'pibin'), 'CPATH': 'pi/include'}
         self.contents = '\n'.join([
             'easyblock = "ConfigureMake"',
             'name = "%s"' % name,
@@ -1177,6 +1184,10 @@ def test_make_module_step(self):
         eb.make_builddir()
         eb.prepare_step()
 
+        # Create a dummy file in bin to test if the duplicate entry of modextrapaths is ignored
+        os.makedirs(os.path.join(eb.installdir, 'bin'))
+        write_file(os.path.join(eb.installdir, 'bin', 'dummy_exe'), 'hello')
+
         modpath = os.path.join(eb.make_module_step(), name, version)
         if get_module_syntax() == 'Lua':
             modpath += '.lua'
@@ -1210,14 +1221,20 @@ def test_make_module_step(self):
                 self.assertTrue(False, "Unknown module syntax: %s" % get_module_syntax())
             self.assertTrue(regex.search(txt), "Pattern %s found in %s" % (regex.pattern, txt))
 
-        for (key, val) in modextrapaths.items():
-            if get_module_syntax() == 'Tcl':
-                regex = re.compile(r'^prepend-path\s+%s\s+\$root/%s$' % (key, val), re.M)
-            elif get_module_syntax() == 'Lua':
-                regex = re.compile(r'^prepend_path\("%s", pathJoin\(root, "%s"\)\)$' % (key, val), re.M)
-            else:
-                self.assertTrue(False, "Unknown module syntax: %s" % get_module_syntax())
-            self.assertTrue(regex.search(txt), "Pattern %s found in %s" % (regex.pattern, txt))
+        for (key, vals) in modextrapaths.items():
+            if isinstance(vals, string_type):
+                vals = [vals]
+            for val in vals:
+                if get_module_syntax() == 'Tcl':
+                    regex = re.compile(r'^prepend-path\s+%s\s+\$root/%s$' % (key, val), re.M)
+                elif get_module_syntax() == 'Lua':
+                    regex = re.compile(r'^prepend_path\("%s", pathJoin\(root, "%s"\)\)$' % (key, val), re.M)
+                else:
+                    self.assertTrue(False, "Unknown module syntax: %s" % get_module_syntax())
+                self.assertTrue(regex.search(txt), "Pattern %s found in %s" % (regex.pattern, txt))
+                # Check for duplicates
+                num_prepends = len(regex.findall(txt))
+                self.assertEqual(num_prepends, 1, "Expected exactly 1 %s command in %s" % (regex.pattern, txt))
 
         for (name, ver) in [('GCC', '6.4.0-2.28')]:
             if get_module_syntax() == 'Tcl':
diff --git a/test/framework/module_generator.py b/test/framework/module_generator.py
