Merge pull request #3637 from Flamefire/deprecateCreatingDirInUse

Deprecate adding a non-existing path to $MODULEPATH

Author: akesandgren

easybuild/framework/easyblock.py

@@ -2170,7 +2170,7 @@ def prepare_step(self, start_dir=True, load_tc_deps_modules=True):
         curr_modpaths = curr_module_paths()
         for init_modpath in init_modpaths:
             full_mod_path = os.path.join(self.installdir_mod, init_modpath)
-            if full_mod_path not in curr_modpaths:
+            if os.path.exists(full_mod_path) and full_mod_path not in curr_modpaths:
                 self.modules_tool.prepend_module_path(full_mod_path)
 
         # prepare toolchain: load toolchain module and dependencies, set up build environment

easybuild/tools/filetools.py

@@ -537,6 +537,24 @@ def det_common_path_prefix(paths):
         return None
 
 
+def normalize_path(path):
+    """Normalize path removing empty and dot components.
+
+    Similar to os.path.normpath but does not resolve '..' which may return a wrong path when symlinks are used
+    """
+    # In POSIX 3 or more leading slashes are equivalent to 1
+    if path.startswith(os.path.sep):
+        if path.startswith(os.path.sep * 2) and not path.startswith(os.path.sep * 3):
+            start_slashes = os.path.sep * 2
+        else:
+            start_slashes = os.path.sep
+    else:
+        start_slashes = ''
+
+    filtered_comps = (comp for comp in path.split(os.path.sep) if comp and comp != '.')
+    return start_slashes + os.path.sep.join(filtered_comps)
+
+
 def is_alt_pypi_url(url):
     """Determine whether specified URL is already an alternate PyPI URL, i.e. whether it contains a hash."""
     # example: .../packages/5b/03/e135b19fadeb9b1ccb45eac9f60ca2dc3afe72d099f6bd84e03cb131f9bf/easybuild-2.7.0.tar.gz

easybuild/tools/modules.py

@@ -46,7 +46,7 @@
 from easybuild.tools.config import EBROOT_ENV_VAR_ACTIONS, LOADED_MODULES_ACTIONS
 from easybuild.tools.config import build_option, get_modules_tool, install_path
 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.filetools import convert_name, mkdir, normalize_path, 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.run import run_cmd
@@ -362,8 +362,12 @@ def use(self, path, priority=None):
             self.log.info("Ignoring specified priority '%s' when running 'module use %s' (Lmod-specific)",
                           priority, path)
 
-        # make sure path exists before we add it
-        mkdir(path, parents=True)
+        if not path:
+            raise EasyBuildError("Cannot add empty path to $MODULEPATH")
+        if not os.path.exists(path):
+            self.log.deprecated("Path '%s' for module.use should exist" % path, '5.0')
+            # make sure path exists before we add it
+            mkdir(path, parents=True)
         self.run_module(['use', path])
 
     def unuse(self, path):
@@ -377,7 +381,8 @@ def add_module_path(self, path, set_mod_paths=True):
         :param path: path to add to $MODULEPATH via 'use'
         :param set_mod_paths: (re)set self.mod_paths
         """
-        if path not in curr_module_paths():
+        path = normalize_path(path)
+        if path not in curr_module_paths(normalize=True):
             # add module path via 'module use' and make sure self.mod_paths is synced
             self.use(path)
             if set_mod_paths:
@@ -391,8 +396,14 @@ def remove_module_path(self, path, set_mod_paths=True):
         :param set_mod_paths: (re)set self.mod_paths
         """
         # remove module path via 'module unuse' and make sure self.mod_paths is synced
-        if path in curr_module_paths():
-            self.unuse(path)
+        path = normalize_path(path)
+        try:
+            # Unuse the path that is actually present in the environment
+            module_path = next(p for p in curr_module_paths() if normalize_path(p) == path)
+        except StopIteration:
+            pass
+        else:
+            self.unuse(module_path)
 
             if set_mod_paths:
                 self.set_mod_paths()
@@ -431,6 +442,7 @@ def check_module_path(self):
             eb_modpath = os.path.join(install_path(typ='modules'), build_option('suffix_modules_path'))
 
             # make sure EasyBuild module path is in 1st place
+            mkdir(eb_modpath, parents=True)
             self.prepend_module_path(eb_modpath)
             self.log.info("Prepended list of module paths with path used by EasyBuild: %s" % eb_modpath)
 
@@ -665,7 +677,8 @@ def load(self, modules, mod_paths=None, purge=False, init_env=None, allow_reload
         # extend $MODULEPATH if needed
         for mod_path in mod_paths:
             full_mod_path = os.path.join(install_path('mod'), build_option('suffix_modules_path'), mod_path)
-            self.prepend_module_path(full_mod_path)
+            if os.path.exists(full_mod_path):
+                self.prepend_module_path(full_mod_path)
 
         loaded_modules = self.loaded_modules()
         for mod in modules:
@@ -1286,8 +1299,14 @@ def remove_module_path(self, path, set_mod_paths=True):
         # remove module path via 'module use' and make sure self.mod_paths is synced
         # modulecmd.tcl keeps track of how often a path was added via 'module use',
         # so we need to check to make sure it's really removed
-        while path in curr_module_paths():
-            self.unuse(path)
+        path = normalize_path(path)
+        while True:
+            try:
+                # Unuse the path that is actually present in the environment
+                module_path = next(p for p in curr_module_paths() if normalize_path(p) == path)
+            except StopIteration:
+                break
+            self.unuse(module_path)
         if set_mod_paths:
             self.set_mod_paths()
 
@@ -1422,8 +1441,12 @@ def use(self, path, priority=None):
         :param path: path to add to $MODULEPATH
         :param priority: priority for this path in $MODULEPATH (Lmod-specific)
         """
-        # make sure path exists before we add it
-        mkdir(path, parents=True)
+        if not path:
+            raise EasyBuildError("Cannot add empty path to $MODULEPATH")
+        if not os.path.exists(path):
+            self.log.deprecated("Path '%s' for module.use should exist" % path, '5.0')
+            # make sure path exists before we add it
+            mkdir(path, parents=True)
 
         if priority:
             self.run_module(['use', '--priority', str(priority), path])
@@ -1433,11 +1456,12 @@ def use(self, path, priority=None):
             if os.environ.get('__LMOD_Priority_MODULEPATH'):
                 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 p != path]
+                    new_mod_path = [path] + [p for p in cur_mod_path.split(':') if normalize_path(p) != path]
                     new_mod_path = ':'.join(new_mod_path)
                 self.log.debug('Changing MODULEPATH from %s to %s' %
                                ('<unset>' if cur_mod_path is None else cur_mod_path, new_mod_path))
@@ -1453,7 +1477,8 @@ def unuse(self, path):
                 self.log.debug('Changing MODULEPATH from %s to <unset>' % cur_mod_path)
                 del os.environ['MODULEPATH']
             else:
-                new_mod_path = ':'.join(p for p in cur_mod_path.split(':') if p != path)
+                path = normalize_path(path)
+                new_mod_path = ':'.join(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
@@ -1603,12 +1628,17 @@ def get_software_version(name):
     return version
 
 
-def curr_module_paths():
+def curr_module_paths(normalize=False):
     """
     Return a list of current module paths.
+
+    :param normalize: Normalize the 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)]
+    module_paths = (p for p in os.environ.get('MODULEPATH', '').split(':') if p and os.path.exists(p))
+    if normalize:
+        module_paths = (normalize_path(p) for p in module_paths)
+    return list(module_paths)
 
 
 def mk_module_path(paths):

easybuild/tools/toolchain/toolchain.py

@@ -636,7 +636,9 @@ def _load_toolchain_module(self, silent=False):
             if self.init_modpaths:
                 mod_path_suffix = build_option('suffix_modules_path')
                 for modpath in self.init_modpaths:
-                    self.modules_tool.prepend_module_path(os.path.join(install_path('mod'), mod_path_suffix, modpath))
+                    modpath = os.path.join(install_path('mod'), mod_path_suffix, modpath)
+                    if os.path.exists(modpath):
+                        self.modules_tool.prepend_module_path(modpath)
 
             # load modules for all dependencies
             self.log.debug("Loading module for toolchain: %s", tc_mod)

test/framework/easyconfig.py

@@ -1666,7 +1666,6 @@ def test_external_dependencies(self):
         # by adding a couple of matching module files with some useful data in them
         # (use Tcl syntax, so it works with all varieties of module tools)
         mod_dir = os.path.join(self.test_prefix, 'modules')
-        self.modtool.use(mod_dir)
 
         pi_mod_txt = '\n'.join([
             "#%Module",
@@ -1691,6 +1690,8 @@ def test_external_dependencies(self):
         ])
         write_file(os.path.join(mod_dir, 'foobar/2.3.4'), foobar_mod_txt)
 
+        self.modtool.use(mod_dir)
+
         ec = EasyConfig(toy_ec)
         deps = ec.dependencies()
 

test/framework/filetools.py

@@ -357,6 +357,21 @@ def test_common_path_prefix(self):
         self.assertEqual(ft.det_common_path_prefix(['foo']), None)
         self.assertEqual(ft.det_common_path_prefix([]), None)
 
+    def test_normalize_path(self):
+        """Test normalize_path"""
+        self.assertEqual(ft.normalize_path(''), '')
+        self.assertEqual(ft.normalize_path('/'), '/')
+        self.assertEqual(ft.normalize_path('//'), '//')
+        self.assertEqual(ft.normalize_path('///'), '/')
+        self.assertEqual(ft.normalize_path('/foo/bar/baz'), '/foo/bar/baz')
+        self.assertEqual(ft.normalize_path('/foo//bar/././baz/'), '/foo/bar/baz')
+        self.assertEqual(ft.normalize_path('foo//bar/././baz/'), 'foo/bar/baz')
+        self.assertEqual(ft.normalize_path('//foo//bar/././baz/'), '//foo/bar/baz')
+        self.assertEqual(ft.normalize_path('///foo//bar/././baz/'), '/foo/bar/baz')
+        self.assertEqual(ft.normalize_path('////foo//bar/././baz/'), '/foo/bar/baz')
+        self.assertEqual(ft.normalize_path('/././foo//bar/././baz/'), '/foo/bar/baz')
+        self.assertEqual(ft.normalize_path('//././foo//bar/././baz/'), '//foo/bar/baz')
+
     def test_download_file(self):
         """Test download_file function."""
         fn = 'toy-0.0.tar.gz'

test/framework/modules.py

@@ -531,14 +531,15 @@ def test_check_module_path(self):
         os.environ['MODULEPATH'] = test2
         modtool.check_module_path()
         self.assertEqual(modtool.mod_paths, [mod_install_dir, test1, test2])
-        self.assertEqual(os.environ['MODULEPATH'], mod_install_dir + ':' + test1 + ':' + test2)
+        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)
         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])
-        self.assertEqual(os.environ['MODULEPATH'], ':'.join([mod_install_dir, test1, test3, '/does/not/exist', test2]))
+        self.assertEqual(os.environ['MODULEPATH'],
+                         os.pathsep.join([mod_install_dir, test1, test3, '/does/not/exist', test2]))
 
     def test_check_module_path_hmns(self):
         """Test behaviour of check_module_path with HierarchicalMNS."""
@@ -1090,8 +1091,9 @@ def test_module_caches(self):
         """Test module caches and invalidate_module_caches_for function."""
         self.assertEqual(mod.MODULE_AVAIL_CACHE, {})
 
-        # purposely extending $MODULEPATH with non-existing path, should be handled fine
+        # purposely extending $MODULEPATH with an empty path, should be handled fine
         nonpath = os.path.join(self.test_prefix, 'nosuchfileordirectory')
+        mkdir(nonpath)
         self.modtool.use(nonpath)
         modulepaths = [p for p in os.environ.get('MODULEPATH', '').split(os.pathsep) if p]
         self.assertTrue(any([os.path.samefile(nonpath, mp) for mp in modulepaths]))
@@ -1164,6 +1166,11 @@ def test_module_use_unuse(self):
         self.modtool.use(test_dir3)
         self.assertTrue(os.environ['MODULEPATH'].startswith('%s:' % test_dir3))
 
+        # Adding an empty modulepath is not possible
+        modulepath = os.environ.get('MODULEPATH', '')
+        self.assertErrorRegex(EasyBuildError, "Cannot add empty path", self.modtool.use, '')
+        self.assertEqual(os.environ.get('MODULEPATH', ''), modulepath)
+
         # make sure the right test module is loaded
         self.modtool.load(['test'])
         self.assertEqual(os.getenv('TEST123'), 'three')
@@ -1224,17 +1231,67 @@ def test_module_use_unuse(self):
             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)
-            old_module_path = os.environ['MODULEPATH']
-            self.modtool.use('')
-            self.assertEqual(os.environ['MODULEPATH'], ':' + old_module_path)
-            self.modtool.unuse('')
-            self.assertEqual(os.environ['MODULEPATH'], old_module_path)
-            # Even works when the whole path is empty
-            os.environ['MODULEPATH'] = ''
-            self.modtool.unuse('')
-            self.assertFalse('MODULEPATH' in os.environ)
-            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")
+        test_dir2 = tempfile.mkdtemp(suffix="_dir2")
+        old_module_path = os.environ.get('MODULEPATH')
+        del os.environ['MODULEPATH']
+        self.modtool.add_module_path(test_dir1)
+        self.assertEqual(os.environ['MODULEPATH'], test_dir1)
+        self.modtool.add_module_path(test_dir2)
+        test_dir_2_and_1 = os.pathsep.join([test_dir2, test_dir1])
+        self.assertEqual(os.environ['MODULEPATH'], test_dir_2_and_1)
+        # Adding the same path does not change the path
+        self.modtool.add_module_path(test_dir1)
+        self.assertEqual(os.environ['MODULEPATH'], test_dir_2_and_1)
+        self.modtool.add_module_path(test_dir2)
+        self.assertEqual(os.environ['MODULEPATH'], test_dir_2_and_1)
+        # Even when a (meaningless) slash is added
+        # This occurs when using an empty modules directory name
+        self.modtool.add_module_path(os.path.join(test_dir1, ''))
+        self.assertEqual(os.environ['MODULEPATH'], test_dir_2_and_1)
+
+        # Similar tests for remove_module_path
+        self.modtool.remove_module_path(test_dir2)
+        self.assertEqual(os.environ['MODULEPATH'], test_dir1)
+        # Same again -> no-op
+        self.modtool.remove_module_path(test_dir2)
+        self.assertEqual(os.environ['MODULEPATH'], test_dir1)
+        # And with empty last part
+        self.modtool.remove_module_path(os.path.join(test_dir1, ''))
+        self.assertEqual(os.environ.get('MODULEPATH', ''), '')
+
+        # And with some more trickery
+        # Lmod seems to remove empty paths: /foo//bar/. -> /foo/bar
+        # Environment-Modules 4.x seems to resolve relative paths: /foo/../foo -> /foo
+        # Hence we can only check the real paths
+        def get_resolved_module_path():
+            return os.pathsep.join(os.path.realpath(p) for p in os.environ['MODULEPATH'].split(os.pathsep))
+
+        test_dir1_relative = os.path.join(test_dir1, '..', os.path.basename(test_dir1))
+        test_dir2_dot = os.path.join(os.path.dirname(test_dir2), '.', os.path.basename(test_dir2))
+        self.modtool.add_module_path(test_dir1_relative)
+        self.assertEqual(get_resolved_module_path(), test_dir1)
+        # Adding the same path, but in a different form may be possible, but may also be ignored, e.g. in EnvModules
+        self.modtool.add_module_path(test_dir1)
+        if get_resolved_module_path() != test_dir1:
+            self.assertEqual(get_resolved_module_path(), os.pathsep.join([test_dir1, test_dir1]))
+            self.modtool.remove_module_path(test_dir1)
+            self.assertEqual(get_resolved_module_path(), test_dir1)
+        self.modtool.add_module_path(test_dir2_dot)
+        self.assertEqual(get_resolved_module_path(), test_dir_2_and_1)
+        self.modtool.remove_module_path(test_dir2_dot)
+        self.assertEqual(get_resolved_module_path(), test_dir1)
+        # Force adding such a dot path which can be removed with either variant
+        os.environ['MODULEPATH'] = os.pathsep.join([test_dir2_dot, test_dir1_relative])
+        self.modtool.remove_module_path(test_dir2_dot)
+        self.assertEqual(get_resolved_module_path(), test_dir1)
+        os.environ['MODULEPATH'] = os.pathsep.join([test_dir2_dot, test_dir1_relative])
+        self.modtool.remove_module_path(test_dir2)
+        self.assertEqual(get_resolved_module_path(), test_dir1)
+
+        os.environ['MODULEPATH'] = old_module_path  # Restore
 
     def test_module_use_bash(self):
         """Test whether effect of 'module use' is preserved when a new bash session is started."""