From bfe05789d3d5d6427fae57b34ff53313bb1ac37c Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Thu, 27 May 2021 10:58:26 +0200 Subject: [PATCH 01/10] Fix normalization when checking path in curr_module_paths --- easybuild/tools/modules.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/easybuild/tools/modules.py b/easybuild/tools/modules.py index 78825bde6c..f4e131fe54 100644 --- a/easybuild/tools/modules.py +++ b/easybuild/tools/modules.py @@ -821,7 +821,7 @@ def add_module_path(self, path, set_mod_paths=True): :param set_mod_paths: (re)set self.mod_paths """ path = normalize_path(path) - if path not in curr_module_paths(normalize=True): + if path not in curr_module_paths(clean=False, normalize=True): # add module path via 'module use' and make sure self.mod_paths is synced self.use(path) if set_mod_paths: From a08a28f3a8d0454e58e1dd953de1dd8fb4f79c35 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 6 Apr 2021 17:43:08 +0200 Subject: [PATCH 02/10] Use mk_module_path --- easybuild/tools/modules.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/easybuild/tools/modules.py b/easybuild/tools/modules.py index f4e131fe54..50e4a3adaa 100644 --- a/easybuild/tools/modules.py +++ b/easybuild/tools/modules.py @@ -2052,7 +2052,7 @@ def use(self, path, priority=None): new_mod_path = path else: new_mod_path = [path] + [p for p in cur_mod_path.split(':') if normalize_path(p) != path] - new_mod_path = ':'.join(new_mod_path) + new_mod_path = mk_module_path(new_mod_path) self.log.debug('Changing MODULEPATH from %s to %s' % ('' if cur_mod_path is None else cur_mod_path, new_mod_path)) os.environ['MODULEPATH'] = new_mod_path @@ -2068,7 +2068,7 @@ def unuse(self, path): del os.environ['MODULEPATH'] else: path = normalize_path(path) - new_mod_path = ':'.join(p for p in cur_mod_path.split(':') if normalize_path(p) != path) + new_mod_path = mk_module_path(p for p in cur_mod_path.split(':') if normalize_path(p) != path) if new_mod_path != cur_mod_path: self.log.debug('Changing MODULEPATH from %s to %s' % (cur_mod_path, new_mod_path)) os.environ['MODULEPATH'] = new_mod_path From 2722d929f3b8028d461bc0ea69d9b37f97ea36f5 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 6 Apr 2021 17:50:38 +0200 Subject: [PATCH 03/10] Add clean flag to curr_module_paths to not strip any paths Lmod ignores if a path exists so we should be able to do the same --- easybuild/tools/modules.py | 11 ++++++++--- test/framework/modules.py | 8 ++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/easybuild/tools/modules.py b/easybuild/tools/modules.py index 50e4a3adaa..baf6ee69ee 100644 --- a/easybuild/tools/modules.py +++ b/easybuild/tools/modules.py @@ -2227,14 +2227,19 @@ def get_software_version(name): return version -def curr_module_paths(normalize=False): +def curr_module_paths(normalize=False, clean=True): """ Return a list of current module paths. :param normalize: Normalize the paths + :param clean: If True remove empty and non-existing paths """ - # avoid empty or nonexistent paths, which don't make any sense - module_paths = (p for p in os.environ.get('MODULEPATH', '').split(':') if p and os.path.exists(p)) + if clean: + # avoid empty or nonexistent paths, which don't make any sense + module_paths = (p for p in os.environ.get('MODULEPATH', '').split(':') if p and os.path.exists(p)) + else: + modulepath = os.environ.get('MODULEPATH') + module_paths = [] if modulepath is None else modulepath.split(':') if normalize: module_paths = (normalize_path(p) for p in module_paths) return list(module_paths) diff --git a/test/framework/modules.py b/test/framework/modules.py index bfd21778ba..40fced70e6 100644 --- a/test/framework/modules.py +++ b/test/framework/modules.py @@ -525,15 +525,23 @@ def test_curr_module_paths(self): test3 = os.path.join(self.test_prefix, 'test3') mkdir(test3) + del os.environ['MODULEPATH'] + self.assertEqual(curr_module_paths(), []) + self.assertEqual(curr_module_paths(clean=False), []) + os.environ['MODULEPATH'] = '' self.assertEqual(curr_module_paths(), []) + self.assertEqual(curr_module_paths(clean=False), ['']) os.environ['MODULEPATH'] = '%s:%s:%s' % (test1, test2, test3) self.assertEqual(curr_module_paths(), [test1, test2, test3]) + self.assertEqual(curr_module_paths(clean=False), [test1, test2, test3]) # empty entries and non-existing directories are filtered out os.environ['MODULEPATH'] = '/doesnotexist:%s::%s:' % (test2, test1) self.assertEqual(curr_module_paths(), [test2, test1]) + # Disabling the clean returns them + self.assertEqual(curr_module_paths(clean=False), ['/doesnotexist', test2, '', test1, '']) def test_check_module_path(self): """Test ModulesTool.check_module_path() method""" From 62d2f787b09ab74c87da9fa285bf4ee7da2053c6 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 6 Apr 2021 18:17:34 +0200 Subject: [PATCH 04/10] Unify handling of setting the MODULEPATH in Lmod Introduce _set_module_path which correctly handles empty paths and empty lists with proper logging --- easybuild/tools/modules.py | 46 +++++++++++++++++++++----------------- test/framework/modules.py | 26 +++++++++++++++++++++ 2 files changed, 51 insertions(+), 21 deletions(-) diff --git a/easybuild/tools/modules.py b/easybuild/tools/modules.py index baf6ee69ee..9f343ac221 100644 --- a/easybuild/tools/modules.py +++ b/easybuild/tools/modules.py @@ -2024,6 +2024,28 @@ def update(self): mkdir(cache_dir, parents=True) write_file(cache_fp, stdout) + def _set_module_path(self, new_mod_path): + """ + Set $MODULEPATH to the specified paths and logs the change. + new_mod_path can be None or an iterable of paths + """ + if new_mod_path is not None: + if not isinstance(new_mod_path, list): + assert not isinstance(new_mod_path, str) # Just to be sure + new_mod_path = list(new_mod_path) # Expand generators + new_mod_path = mk_module_path(new_mod_path) if new_mod_path else None + cur_mod_path = os.environ.get('MODULEPATH') + if new_mod_path != cur_mod_path: + self.log.debug( + 'Changing MODULEPATH from %s to %s', + '' if cur_mod_path is None else cur_mod_path, + '' if new_mod_path is None else new_mod_path, + ) + if new_mod_path is None: + del os.environ['MODULEPATH'] + else: + os.environ['MODULEPATH'] = new_mod_path + def use(self, path, priority=None): """ Add path to $MODULEPATH via 'module use'. @@ -2047,31 +2069,13 @@ def use(self, path, priority=None): self.run_module(['use', path]) else: path = normalize_path(path) - cur_mod_path = os.environ.get('MODULEPATH') - if cur_mod_path is None: - new_mod_path = path - else: - new_mod_path = [path] + [p for p in cur_mod_path.split(':') if normalize_path(p) != path] - new_mod_path = mk_module_path(new_mod_path) - self.log.debug('Changing MODULEPATH from %s to %s' % - ('' if cur_mod_path is None else cur_mod_path, new_mod_path)) - os.environ['MODULEPATH'] = new_mod_path + self._set_module_path([path] + [p for p in curr_module_paths(clean=False) if normalize_path(p) != path]) def unuse(self, path): """Remove a module path""" # We can simply remove the path from MODULEPATH to avoid the costly module call - cur_mod_path = os.environ.get('MODULEPATH') - if cur_mod_path is not None: - # Removing the last entry unsets the variable - if cur_mod_path == path: - self.log.debug('Changing MODULEPATH from %s to ' % cur_mod_path) - del os.environ['MODULEPATH'] - else: - path = normalize_path(path) - new_mod_path = mk_module_path(p for p in cur_mod_path.split(':') if normalize_path(p) != path) - if new_mod_path != cur_mod_path: - self.log.debug('Changing MODULEPATH from %s to %s' % (cur_mod_path, new_mod_path)) - os.environ['MODULEPATH'] = new_mod_path + path = normalize_path(path) + self._set_module_path(p for p in curr_module_paths(clean=False) if normalize_path(p) != path) def prepend_module_path(self, path, set_mod_paths=True, priority=None): """ diff --git a/test/framework/modules.py b/test/framework/modules.py index 40fced70e6..2e31a3d13e 100644 --- a/test/framework/modules.py +++ b/test/framework/modules.py @@ -1270,6 +1270,29 @@ def test_module_use_unuse(self): # Tests for Lmod only if isinstance(self.modtool, Lmod): + # Check the helper function + old_module_path = os.environ['MODULEPATH'] + self.modtool._set_module_path(['/foo']) + self.assertEqual(os.environ['MODULEPATH'], '/foo') + self.modtool._set_module_path(['/foo', '/bar']) + self.assertEqual(os.environ['MODULEPATH'], '/foo:/bar') + self.modtool._set_module_path(['']) + self.assertEqual(os.environ['MODULEPATH'], '') + self.modtool._set_module_path([]) + self.assertFalse('MODULEPATH' in os.environ) + self.modtool._set_module_path(None) + self.assertFalse('MODULEPATH' in os.environ) + # Same for generators + self.modtool._set_module_path(i for i in ['/foo']) + self.assertEqual(os.environ['MODULEPATH'], '/foo') + self.modtool._set_module_path(i for i in ['/foo', '/bar']) + self.assertEqual(os.environ['MODULEPATH'], '/foo:/bar') + self.modtool._set_module_path(i for i in ['']) + self.assertEqual(os.environ['MODULEPATH'], '') + self.modtool._set_module_path(i for i in []) + self.assertFalse('MODULEPATH' in os.environ) + os.environ['MODULEPATH'] = old_module_path # Restore + # check whether prepend with priority actually works (priority is specific to Lmod) self.modtool.use(test_dir1, priority=100) self.modtool.use(test_dir3) @@ -1295,6 +1318,9 @@ def test_module_use_unuse(self): self.modtool.use(test_dir1) self.assertEqual(os.environ['MODULEPATH'], test_dir1) self.modtool.unuse(test_dir1) + self.assertFalse('MODULEPATH' in os.environ) + # Unuse when the MODULEPATH is already empty + self.modtool.unuse(test_dir1) self.assertNotIn('MODULEPATH', os.environ) os.environ['MODULEPATH'] = old_module_path # Restore From 2d66114a0767ca1f6dc9ccac9ed13dcfe7dd89bf Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Wed, 7 Apr 2021 12:27:52 +0200 Subject: [PATCH 05/10] Use dir not prefix for mkdtemp --- easybuild/toolchains/mpi/openmpi.py | 2 +- test/framework/easyconfig.py | 6 +++--- test/framework/modules.py | 16 ++++++++-------- test/framework/toolchain.py | 2 +- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/easybuild/toolchains/mpi/openmpi.py b/easybuild/toolchains/mpi/openmpi.py index 436944fb71..57df781499 100644 --- a/easybuild/toolchains/mpi/openmpi.py +++ b/easybuild/toolchains/mpi/openmpi.py @@ -83,7 +83,7 @@ def prepare(self, *args, **kwargs): ompi_ver = self.get_software_version(self.MPI_MODULE_NAME)[0] if LooseVersion(ompi_ver) >= LooseVersion('2.0') and LooseVersion(ompi_ver) < LooseVersion('3.0'): if len(self.orig_tmpdir) > 40: - tmpdir = tempfile.mkdtemp(prefix='/tmp/') + tmpdir = tempfile.mkdtemp(dir='/tmp', prefix='') env.setvar('TMPDIR', tmpdir) warn_msg = "Long $TMPDIR path may cause problems with OpenMPI 2.x, using shorter path: %s" % tmpdir self.log.warning(warn_msg) diff --git a/test/framework/easyconfig.py b/test/framework/easyconfig.py index 7a07eab449..31dce0e3e6 100644 --- a/test/framework/easyconfig.py +++ b/test/framework/easyconfig.py @@ -2781,7 +2781,7 @@ def test_dump_extra(self): '', ]) - handle, testec = tempfile.mkstemp(prefix=self.test_prefix, suffix='.eb') + handle, testec = tempfile.mkstemp(suffix='.eb') os.close(handle) ec = EasyConfig(None, rawtxt=rawtxt) @@ -2844,7 +2844,7 @@ def test_dump_template(self): 'moduleclass = "tools"', ]) - handle, testec = tempfile.mkstemp(prefix=self.test_prefix, suffix='.eb') + handle, testec = tempfile.mkstemp(suffix='.eb') os.close(handle) ec = EasyConfig(None, rawtxt=rawtxt) @@ -2920,7 +2920,7 @@ def test_dump_comments(self): "# trailing comment", ]) - handle, testec = tempfile.mkstemp(prefix=self.test_prefix, suffix='.eb') + handle, testec = tempfile.mkstemp(suffix='.eb') os.close(handle) ec = EasyConfig(None, rawtxt=rawtxt) diff --git a/test/framework/modules.py b/test/framework/modules.py index 2e31a3d13e..3bae1d8b7f 100644 --- a/test/framework/modules.py +++ b/test/framework/modules.py @@ -625,7 +625,7 @@ def broken_prepend_module_path(*args, **kwargs): def test_prepend_module_path(self): """Test prepend_module_path method.""" - test_path = tempfile.mkdtemp(prefix=self.test_prefix) + test_path = tempfile.mkdtemp() self.modtool.prepend_module_path(test_path) self.assertTrue(os.path.samefile(curr_module_paths()[0], test_path)) @@ -648,17 +648,17 @@ def test_prepend_module_path(self): self.assertEqual(modulepath, curr_module_paths()) # test prepending with high priority - test_path_bis = tempfile.mkdtemp(prefix=self.test_prefix) - test_path_tris = tempfile.mkdtemp(prefix=self.test_prefix) - self.modtool.prepend_module_path(test_path_bis, priority=10000) - self.assertEqual(test_path_bis, curr_module_paths()[0]) + test_path_0 = tempfile.mkdtemp(suffix='path_0') + test_path_1 = tempfile.mkdtemp(suffix='path_1') + self.modtool.prepend_module_path(test_path_0, priority=1000) + self.assertEqual(test_path_0, curr_module_paths()[0]) # check whether prepend with priority actually works (only for Lmod) if isinstance(self.modtool, Lmod): - self.modtool.prepend_module_path(test_path_tris) + self.modtool.prepend_module_path(test_path_1) modulepath = curr_module_paths() - self.assertEqual(test_path_bis, modulepath[0]) - self.assertEqual(test_path_tris, modulepath[1]) + self.assertEqual(test_path_0, modulepath[0]) + self.assertEqual(test_path_1, modulepath[1]) def test_ld_library_path(self): """Make sure LD_LIBRARY_PATH is what it should be when loaded multiple modules.""" diff --git a/test/framework/toolchain.py b/test/framework/toolchain.py index 11f5b5b0e3..006413631c 100644 --- a/test/framework/toolchain.py +++ b/test/framework/toolchain.py @@ -3284,7 +3284,7 @@ def prep(): orig_tmpdir = os.environ.get('TMPDIR') if len(orig_tmpdir) > 40: # we need to make sure we have a short $TMPDIR for this test... - orig_tmpdir = tempfile.mkdtemp(prefix='/tmp/') + orig_tmpdir = tempfile.mkdtemp(dir='/tmp') mkdir(orig_tmpdir) os.environ['TMPDIR'] = orig_tmpdir From 9e4c9e3df5cdb0a37df31829bc188f199d50120d Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Wed, 7 Apr 2021 14:54:26 +0200 Subject: [PATCH 06/10] Factor out use of `os.environ.get('__LMOD_Priority_MODULEPATH')` --- easybuild/tools/modules.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/easybuild/tools/modules.py b/easybuild/tools/modules.py index 9f343ac221..380fc33755 100644 --- a/easybuild/tools/modules.py +++ b/easybuild/tools/modules.py @@ -2046,6 +2046,12 @@ def _set_module_path(self, new_mod_path): else: os.environ['MODULEPATH'] = new_mod_path + def _has_module_paths_with_priority(self): + """Return True if there are priorities attached to $MODULEPATH""" + # We simply check, if the Lmod variable is set and non-empty + # See https://github.com/TACC/Lmod/issues/509 + return bool(os.environ.get('__LMOD_Priority_MODULEPATH')) + def use(self, path, priority=None): """ Add path to $MODULEPATH via 'module use'. @@ -2065,7 +2071,7 @@ def use(self, path, priority=None): else: # LMod allows modifying MODULEPATH directly. So do that to avoid the costly module use # unless priorities are in use already - if os.environ.get('__LMOD_Priority_MODULEPATH'): + if self._has_module_paths_with_priority(): self.run_module(['use', path]) else: path = normalize_path(path) From 942819a33cb86ee2599d4f0eea6515d8a0ec38f7 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Wed, 7 Apr 2021 12:31:20 +0200 Subject: [PATCH 07/10] Show warning if prepending module path unexpectatly failed --- easybuild/tools/modules.py | 13 +++++++++++++ test/framework/modules.py | 18 ++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/easybuild/tools/modules.py b/easybuild/tools/modules.py index 380fc33755..6859da8358 100644 --- a/easybuild/tools/modules.py +++ b/easybuild/tools/modules.py @@ -2095,6 +2095,19 @@ def prepend_module_path(self, path, set_mod_paths=True, priority=None): modulepath = curr_module_paths() if not modulepath or os.path.realpath(modulepath[0]) != os.path.realpath(path): self.use(path, priority=priority) + if (priority is None) == (not self._has_module_paths_with_priority()): + modulepath = curr_module_paths(normalize=True, clean=False) + path_idx = modulepath.index(normalize_path(path)) + if path_idx != 0: + prio_msg = "This can happen if paths were added via `module use` with a priority" + if priority is not None: + prio_msg += f" higher than {priority}." + else: + prio_msg += "." + print_warning("Path '%s' could not be prepended to $MODULEPATH. " + "The following paths are still in front of it: %s\n" + "%s", + path, "; ".join(modulepath[:path_idx]), prio_msg, log=self.log) if set_mod_paths: self.set_mod_paths() diff --git a/test/framework/modules.py b/test/framework/modules.py index 3bae1d8b7f..fb00b57a41 100644 --- a/test/framework/modules.py +++ b/test/framework/modules.py @@ -659,6 +659,24 @@ def test_prepend_module_path(self): modulepath = curr_module_paths() self.assertEqual(test_path_0, modulepath[0]) self.assertEqual(test_path_1, modulepath[1]) + test_path_2 = tempfile.mkdtemp(suffix='path_2') + self.modtool.prepend_module_path(test_path_2) + modulepath = curr_module_paths() + self.assertEqual(test_path_0, modulepath[0]) + self.assertEqual(test_path_2, modulepath[1]) + self.assertEqual(test_path_1, modulepath[2]) + + # When prepend fails due to a higher priority path, a warning is shown + self.modtool.unuse(test_path_2) + self.modtool.prepend_module_path(test_path_0, priority=999999) + with self.mocked_stdout_stderr(): + self.modtool.prepend_module_path(test_path_2, priority=1000) + stderr = self.get_stderr() + self.assertIn('could not be prepended', stderr) + modulepath = curr_module_paths() + self.assertEqual(test_path_0, modulepath[0]) + self.assertEqual(test_path_2, modulepath[1]) + self.assertEqual(test_path_1, modulepath[2]) def test_ld_library_path(self): """Make sure LD_LIBRARY_PATH is what it should be when loaded multiple modules.""" From dac2b3a196358869368800284f9ce264d02755ae Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Wed, 7 Apr 2021 15:23:58 +0200 Subject: [PATCH 08/10] Add force_module_command parameter --- easybuild/tools/modules.py | 39 ++++++++++++++++++++++++++++---------- test/framework/modules.py | 13 +++++++++++++ 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/easybuild/tools/modules.py b/easybuild/tools/modules.py index 6859da8358..f9b12d1d6b 100644 --- a/easybuild/tools/modules.py +++ b/easybuild/tools/modules.py @@ -790,12 +790,14 @@ def set_mod_paths(self, mod_paths=None): self.log.debug("$MODULEPATH after set_mod_paths: %s" % os.environ.get('MODULEPATH', '')) - def use(self, path, priority=None): + def use(self, path, priority=None, force_module_command=False): """ Add path to $MODULEPATH via 'module use'. :param path: path to add to $MODULEPATH :param priority: priority for this path in $MODULEPATH (Lmod-specific) + :param force_module_command: If False running the module command may be skipped which is faster + but does not reload modules """ if priority: self.log.info("Ignoring specified priority '%s' when running 'module use %s' (Lmod-specific)", @@ -809,8 +811,14 @@ def use(self, path, priority=None): mkdir(path, parents=True) self.run_module(['use', path]) - def unuse(self, path): - """Remove module path via 'module unuse'.""" + def unuse(self, path, force_module_command=False): + """ + Remove a module path (usually) via 'module unuse'. + + :param path: path to remove from $MODULEPATH + :param force_module_command: If False running the module command may be skipped which is faster + but does not reload modules + """ self.run_module(['unuse', path]) def add_module_path(self, path, set_mod_paths=True): @@ -2052,12 +2060,14 @@ def _has_module_paths_with_priority(self): # See https://github.com/TACC/Lmod/issues/509 return bool(os.environ.get('__LMOD_Priority_MODULEPATH')) - def use(self, path, priority=None): + def use(self, path, priority=None, force_module_command=False): """ Add path to $MODULEPATH via 'module use'. :param path: path to add to $MODULEPATH :param priority: priority for this path in $MODULEPATH (Lmod-specific) + :param force_module_command: If False running the module command may be skipped which is faster + but does not reload modules """ if not path: raise EasyBuildError("Cannot add empty path to $MODULEPATH") @@ -2071,17 +2081,26 @@ def use(self, path, priority=None): else: # LMod allows modifying MODULEPATH directly. So do that to avoid the costly module use # unless priorities are in use already - if self._has_module_paths_with_priority(): + if force_module_command or self._has_module_paths_with_priority(): self.run_module(['use', path]) else: path = normalize_path(path) self._set_module_path([path] + [p for p in curr_module_paths(clean=False) if normalize_path(p) != path]) - def unuse(self, path): - """Remove a module path""" - # We can simply remove the path from MODULEPATH to avoid the costly module call - path = normalize_path(path) - self._set_module_path(p for p in curr_module_paths(clean=False) if normalize_path(p) != path) + def unuse(self, path, force_module_command=False): + """ + Remove a module path + + :param path: path to remove from $MODULEPATH + :param force_module_command: If False running the module command may be skipped which is faster + but does not reload modules + """ + if force_module_command: + super(Lmod, self).unuse(path) + else: + # We can simply remove the path from MODULEPATH to avoid the costly module call + path = normalize_path(path) + self._set_module_path(p for p in curr_module_paths(clean=False) if normalize_path(p) != path) def prepend_module_path(self, path, set_mod_paths=True, priority=None): """ diff --git a/test/framework/modules.py b/test/framework/modules.py index fb00b57a41..169ea03dfd 100644 --- a/test/framework/modules.py +++ b/test/framework/modules.py @@ -1342,6 +1342,19 @@ def test_module_use_unuse(self): self.assertNotIn('MODULEPATH', os.environ) os.environ['MODULEPATH'] = old_module_path # Restore + # Forcing to use the module command reloads modules (Only Lmod does this) + old_module_path = os.environ['MODULEPATH'] + os.environ['MODULEPATH'] = test_dir1 + self.assertFalse('TEST123' in os.environ) + self.modtool.load(['test']) + self.assertEqual(os.getenv('TEST123'), 'one') + self.modtool.use(test_dir2, force_module_command=True) + self.assertEqual(os.getenv('TEST123'), 'two') # Module reloaded + self.modtool.unuse(test_dir2, force_module_command=True) + self.assertEqual(os.getenv('TEST123'), 'one') # Module reloaded + self.modtool.unload(['test']) + os.environ['MODULEPATH'] = old_module_path # Restore + def test_add_and_remove_module_path(self): """Test add_module_path and whether remove_module_path undoes changes of add_module_path""" test_dir1 = tempfile.mkdtemp(suffix="_dir1") From 52d1c7b317c3969e0ec2075c591f06507f085571 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Thu, 27 May 2021 14:27:55 +0200 Subject: [PATCH 09/10] Use os.pathsep consistently --- easybuild/tools/modules.py | 6 ++--- test/framework/modules.py | 46 ++++++++++++++++++++++---------------- 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/easybuild/tools/modules.py b/easybuild/tools/modules.py index f9b12d1d6b..f42e63ccbd 100644 --- a/easybuild/tools/modules.py +++ b/easybuild/tools/modules.py @@ -2278,10 +2278,10 @@ def curr_module_paths(normalize=False, clean=True): """ if clean: # avoid empty or nonexistent paths, which don't make any sense - module_paths = (p for p in os.environ.get('MODULEPATH', '').split(':') if p and os.path.exists(p)) + module_paths = (p for p in os.environ.get('MODULEPATH', '').split(os.pathsep) if p and os.path.exists(p)) else: modulepath = os.environ.get('MODULEPATH') - module_paths = [] if modulepath is None else modulepath.split(':') + module_paths = [] if modulepath is None else modulepath.split(os.pathsep) if normalize: module_paths = (normalize_path(p) for p in module_paths) return list(module_paths) @@ -2291,7 +2291,7 @@ def mk_module_path(paths): """ Create a string representing the list of module paths. """ - return ':'.join(paths) + return os.pathsep.join(paths) def avail_modules_tools(): diff --git a/test/framework/modules.py b/test/framework/modules.py index 169ea03dfd..a7509030f3 100644 --- a/test/framework/modules.py +++ b/test/framework/modules.py @@ -533,12 +533,12 @@ def test_curr_module_paths(self): self.assertEqual(curr_module_paths(), []) self.assertEqual(curr_module_paths(clean=False), ['']) - os.environ['MODULEPATH'] = '%s:%s:%s' % (test1, test2, test3) + os.environ['MODULEPATH'] = os.pathsep.join([test1, test2, test3]) self.assertEqual(curr_module_paths(), [test1, test2, test3]) self.assertEqual(curr_module_paths(clean=False), [test1, test2, test3]) # empty entries and non-existing directories are filtered out - os.environ['MODULEPATH'] = '/doesnotexist:%s::%s:' % (test2, test1) + os.environ['MODULEPATH'] = os.pathsep.join(['/doesnotexist', test2, '', test1, '']) self.assertEqual(curr_module_paths(), [test2, test1]) # Disabling the clean returns them self.assertEqual(curr_module_paths(clean=False), ['/doesnotexist', test2, '', test1, '']) @@ -578,7 +578,7 @@ def test_check_module_path(self): self.assertEqual(os.environ['MODULEPATH'], os.pathsep.join([mod_install_dir, test1, test2])) # check behaviour if non-existing directories are included in $MODULEPATH - os.environ['MODULEPATH'] = '%s:/does/not/exist:%s' % (test3, test2) + os.environ['MODULEPATH'] = os.pathsep.join([test3, '/does/not/exist', test2]) modtool.check_module_path() # non-existing dir is filtered from mod_paths, but stays in $MODULEPATH self.assertEqual(modtool.mod_paths, [mod_install_dir, test1, test3, test2]) @@ -603,11 +603,11 @@ def test_check_module_path_hmns(self): doesnotexist = os.path.join(self.test_prefix, 'doesnotexist') self.assertNotExists(doesnotexist) - os.environ['MODULEPATH'] = '%s:%s' % (core_mod_dir, doesnotexist) + os.environ['MODULEPATH'] = os.pathsep.join([core_mod_dir, doesnotexist]) modtool = modules_tool() self.assertEqual(modtool.mod_paths, [os.path.dirname(core_mod_dir), core_mod_dir]) - self.assertEqual(os.environ['MODULEPATH'], '%s:%s:%s' % (top_mod_dir, core_mod_dir, doesnotexist)) + self.assertEqual(os.environ['MODULEPATH'], os.pathsep.join([top_mod_dir, core_mod_dir, doesnotexist])) # hack prepend_module_path to make sure it's not called again if check_module_path is called again; # prepend_module_path is fairly expensive, so should be avoided, @@ -621,7 +621,7 @@ def broken_prepend_module_path(*args, **kwargs): modtool.check_module_path() self.assertEqual(modtool.mod_paths, [os.path.dirname(core_mod_dir), core_mod_dir]) - self.assertEqual(os.environ['MODULEPATH'], '%s:%s:%s' % (top_mod_dir, core_mod_dir, doesnotexist)) + self.assertEqual(os.environ['MODULEPATH'], os.pathsep.join([top_mod_dir, core_mod_dir, doesnotexist])) def test_prepend_module_path(self): """Test prepend_module_path method.""" @@ -800,7 +800,11 @@ def test_wrong_modulepath(self): """Test whether modules tool can deal with a broken $MODULEPATH.""" test_modules_path = os.path.realpath(os.path.join(os.path.dirname(os.path.abspath(__file__)), 'modules')) modules_test_installpath = os.path.join(self.test_installpath, 'modules', 'all') - os.environ['MODULEPATH'] = '/some/non-existing/path:/this/doesnt/exists/anywhere:%s' % test_modules_path + os.environ['MODULEPATH'] = os.pathsep.join([ + '/some/non-existing/path', + '/this/doesnt/exists/anywhere', + test_modules_path + ]) init_config() # purposely *not* using self.modtool here; # need to check whether creating new ModulesTool instance doesn't break when $MODULEPATH contains faulty paths @@ -1163,10 +1167,11 @@ def test_modules_tool_stateless(self): def test_mk_module_cache_key(self): """Test mk_module_cache_key method.""" - os.environ['MODULEPATH'] = '%s:/tmp/test' % self.test_prefix + module_path = os.pathsep.join([self.test_prefix, '/tmp/test']) + os.environ['MODULEPATH'] = module_path res = self.modtool.mk_module_cache_key('thisisapartialkey') self.assertIsInstance(res, tuple) - self.assertEqual(res, ('MODULEPATH=%s:/tmp/test' % self.test_prefix, self.modtool.COMMAND, 'thisisapartialkey')) + self.assertEqual(res, ('MODULEPATH=%s' % module_path, self.modtool.COMMAND, 'thisisapartialkey')) del os.environ['MODULEPATH'] res = self.modtool.mk_module_cache_key('thisisapartialkey') @@ -1245,11 +1250,11 @@ def test_module_use_unuse(self): self.assertNotIn(test_dir1, os.environ.get('MODULEPATH', '')) self.modtool.use(test_dir1) - self.assertTrue(os.environ['MODULEPATH'].startswith('%s:' % test_dir1)) + self.assertTrue(os.environ['MODULEPATH'].startswith(test_dir1 + os.pathsep)) self.modtool.use(test_dir2) - self.assertTrue(os.environ['MODULEPATH'].startswith('%s:' % test_dir2)) + self.assertTrue(os.environ['MODULEPATH'].startswith(test_dir2 + os.pathsep)) self.modtool.use(test_dir3) - self.assertTrue(os.environ['MODULEPATH'].startswith('%s:' % test_dir3)) + self.assertTrue(os.environ['MODULEPATH'].startswith(test_dir3 + os.pathsep)) # Adding an empty modulepath is not possible modulepath = os.environ.get('MODULEPATH', '') @@ -1280,7 +1285,7 @@ def test_module_use_unuse(self): # also test use with high priority self.modtool.use(test_dir2, priority=10000) - self.assertTrue(os.environ['MODULEPATH'].startswith('%s:' % test_dir2)) + self.assertTrue(os.environ['MODULEPATH'].startswith(test_dir2 + os.pathsep)) self.modtool.load(['test']) self.assertEqual(os.getenv('TEST123'), 'two') @@ -1292,8 +1297,9 @@ def test_module_use_unuse(self): old_module_path = os.environ['MODULEPATH'] self.modtool._set_module_path(['/foo']) self.assertEqual(os.environ['MODULEPATH'], '/foo') - self.modtool._set_module_path(['/foo', '/bar']) - self.assertEqual(os.environ['MODULEPATH'], '/foo:/bar') + foo_and_bar_paths = ['/foo', '/bar'] + self.modtool._set_module_path(foo_and_bar_paths) + self.assertEqual(os.environ['MODULEPATH'], os.pathsep.join(foo_and_bar_paths)) self.modtool._set_module_path(['']) self.assertEqual(os.environ['MODULEPATH'], '') self.modtool._set_module_path([]) @@ -1303,8 +1309,8 @@ def test_module_use_unuse(self): # Same for generators self.modtool._set_module_path(i for i in ['/foo']) self.assertEqual(os.environ['MODULEPATH'], '/foo') - self.modtool._set_module_path(i for i in ['/foo', '/bar']) - self.assertEqual(os.environ['MODULEPATH'], '/foo:/bar') + self.modtool._set_module_path(i for i in foo_and_bar_paths) + self.assertEqual(os.environ['MODULEPATH'], os.pathsep.join(foo_and_bar_paths)) self.modtool._set_module_path(i for i in ['']) self.assertEqual(os.environ['MODULEPATH'], '') self.modtool._set_module_path(i for i in []) @@ -1314,7 +1320,9 @@ def test_module_use_unuse(self): # check whether prepend with priority actually works (priority is specific to Lmod) self.modtool.use(test_dir1, priority=100) self.modtool.use(test_dir3) - self.assertTrue(os.environ['MODULEPATH'].startswith('%s:%s:%s:' % (test_dir2, test_dir1, test_dir3))) + self.assertTrue(os.environ['MODULEPATH'].startswith( + os.pathsep.join([test_dir2, test_dir1, test_dir3]) + )) self.modtool.load(['test']) self.assertEqual(os.getenv('TEST123'), 'two') self.modtool.unload(['test']) @@ -1644,7 +1652,7 @@ def test_modulecmd_strip_source(self): write_file(modulecmd, modulecmd_txt) adjust_permissions(modulecmd, stat.S_IXUSR, add=True) - os.environ['PATH'] = '%s:%s' % (self.test_prefix, os.getenv('PATH')) + os.environ['PATH'] = os.pathsep.join([self.test_prefix, os.getenv('PATH')]) self.allow_deprecated_behaviour() with self.mocked_stdout_stderr(): From 15a429adb727827509e1b6e81bb8a695f69f7769 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Thu, 23 Oct 2025 13:48:49 +0200 Subject: [PATCH 10/10] Fix adding module paths with different priorities When adding a path with a priority which is already at the front we need to run the module command anyway to apply the priority or it can be overriden by subsequent additions. --- easybuild/tools/modules.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/easybuild/tools/modules.py b/easybuild/tools/modules.py index f42e63ccbd..8df5488ba0 100644 --- a/easybuild/tools/modules.py +++ b/easybuild/tools/modules.py @@ -2110,8 +2110,8 @@ def prepend_module_path(self, path, set_mod_paths=True, priority=None): :param set_mod_paths: (re)set self.mod_paths :param priority: priority for this path in $MODULEPATH (Lmod-specific) """ - # Lmod pushes a path to the front on 'module use', no need for (costly) 'module unuse' - modulepath = curr_module_paths() + # If the path is already first in MODULEPATH, do nothing if we don't want to add it with a priority + modulepath = None if priority is not None else curr_module_paths() if not modulepath or os.path.realpath(modulepath[0]) != os.path.realpath(path): self.use(path, priority=priority) if (priority is None) == (not self._has_module_paths_with_priority()):