From d0bd39f97e9bfe1746d1dec3bc37c6deb528d4ef Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 6 Apr 2021 17:43:08 +0200 Subject: [PATCH 1/3] 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 3e3f004dc5..29c816198b 100644 --- a/easybuild/tools/modules.py +++ b/easybuild/tools/modules.py @@ -1438,7 +1438,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 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 @@ -1453,7 +1453,7 @@ def unuse(self, path): self.log.debug('Changing MODULEPATH from %s to ' % cur_mod_path) del os.environ['MODULEPATH'] else: - new_mod_path = ':'.join(p for p in cur_mod_path.split(':') if p != path) + new_mod_path = mk_module_path(p for p in cur_mod_path.split(':') if 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 1e1b7477a4a8011f091e924ba5d6dadeaba9bda7 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 6 Apr 2021 17:50:38 +0200 Subject: [PATCH 2/3] 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 | 12 +++++++++--- test/framework/modules.py | 8 ++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/easybuild/tools/modules.py b/easybuild/tools/modules.py index 29c816198b..78a28c2af5 100644 --- a/easybuild/tools/modules.py +++ b/easybuild/tools/modules.py @@ -1603,12 +1603,18 @@ def get_software_version(name): return version -def curr_module_paths(): +def curr_module_paths(clean=True): """ Return a list of current module paths. + clean: If True remove empty and non-existing paths """ - # avoid empty or nonexistent paths, which don't make any sense - return [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 + paths = [p for p in os.environ.get('MODULEPATH', '').split(':') if p and os.path.exists(p)] + else: + modulepath = os.environ.get('MODULEPATH') + paths = [] if modulepath is None else modulepath.split(':') + return paths def mk_module_path(paths): diff --git a/test/framework/modules.py b/test/framework/modules.py index a2f4633787..e0fd7c90a6 100644 --- a/test/framework/modules.py +++ b/test/framework/modules.py @@ -489,15 +489,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 2b53995f6f129b3965f34df7a58f6a427763317d Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 6 Apr 2021 18:17:34 +0200 Subject: [PATCH 3/3] 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 78a28c2af5..31b44efcca 100644 --- a/easybuild/tools/modules.py +++ b/easybuild/tools/modules.py @@ -48,7 +48,7 @@ from easybuild.tools.environment import ORIG_OS_ENVIRON, restore_env, setvar, unset_env_vars from easybuild.tools.filetools import convert_name, mkdir, path_matches, read_file, which, write_file from easybuild.tools.module_naming_scheme.mns import DEVEL_MODULE_SUFFIX -from easybuild.tools.py2vs3 import subprocess_popen_text +from easybuild.tools.py2vs3 import subprocess_popen_text, string_type from easybuild.tools.run import run_cmd from easybuild.tools.utilities import get_subclasses, nub @@ -1415,6 +1415,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, string_type) # 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'. @@ -1433,30 +1455,12 @@ def use(self, path, priority=None): if os.environ.get('__LMOD_Priority_MODULEPATH'): self.run_module(['use', path]) else: - 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 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 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: - new_mod_path = mk_module_path(p for p in cur_mod_path.split(':') if 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 + self._set_module_path(p for p in curr_module_paths(clean=False) if 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 e0fd7c90a6..3fdf182d2b 100644 --- a/test/framework/modules.py +++ b/test/framework/modules.py @@ -1203,6 +1203,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) @@ -1229,6 +1252,9 @@ def test_module_use_unuse(self): 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.assertFalse('MODULEPATH' in os.environ) os.environ['MODULEPATH'] = old_module_path # Restore # Using an empty path still works (technically) (Lmod only, ignored by Tcl)