interpret statements that extend $MODULEPATH in modpath_extensions_for method #2104

bartoldeman · 2017-02-02T01:17:14Z

This way lmod can do some parsing of the lua file, when the MODULEPATH
extension uses environment variables.

This way lmod can do some parsing of the lua file, when the MODULEPATH extension uses environment variables.

bartoldeman · 2017-02-02T01:18:10Z

(we expand MODULEPATH based on an "ARCH" environment variable)

…xist.

The "if" logic works correctly now but it really tested for the modules to exist in $HOME during the testcase which is not appropriate. The reason the test worked previously is that the MODULEPATH extension was taken unconditionally -- however that does not work if the syntax is more complex as in e.g. prepend_path("MODULEPATH", pathJoin(os.getenv("HOME"), "Public/easybuild/modules/Compiler/GCCcore/6.3.0")) without parsing this was literally taken as the string 'pathJoin(os.getenv("HOME"), "Public/easybuild/modules/Compiler/GCCcore/6.3.0")'

boegel · 2017-03-14T21:24:00Z

@bartoldeman Still one failing test:

======================================================================
FAIL: Test modpath_extensions_for method.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/211045405/lib/python2.6/site-packages/easybuild_framework-3.2.0.dev0-py2.6.egg/test/framework/modules.py", line 405, in test_modpath_extensions_for
    self.assertErrorRegex(EasyBuildError, error_pattern, self.modtool.modpath_extensions_for, ['nosuchmodule/1.2'])
  File "/home/travis/virtualenv/python2.6.9/lib/python2.6/site-packages/vsc_install-0.10.25-py2.6.egg/vsc/install/testing.py", line 126, in assertErrorRegex
    self.assertTrue(regex.search(msg), "Pattern '%s' is found in '%s'" % (regex.pattern, msg))
AssertionError: Pattern 'Can't get value from a non-existing module' is found in 'Can't get MODULEPATH from a non-existing module nosuchmodule/1.2'

boegel · 2017-03-14T21:24:59Z

easybuild/tools/modules.py

@@ -711,15 +711,6 @@ def loaded_modules(self):

        return loaded_modules

-    def read_module_file(self, mod_name):


@bartoldeman I'm not sure if we should remove this method?

It is dead code now so it could be removed in principle.

boegel · 2017-03-14T21:25:25Z

easybuild/tools/modules.py

@@ -747,7 +738,9 @@ def modpath_extensions_for(self, mod_names):

        modpath_exts = {}
        for mod_name in mod_names:
-            modtxt = self.read_module_file(mod_name)
+            if not self.exist([mod_name], skip_avail=True)[0]:


will this (still) work for hidden modules?

yes: read_module_name() called modulefile_path() which calls get_value_from_modulefile() which also called exist() in the same way.

boegel · 2017-03-14T21:26:32Z

test/framework/modules.py

+            # we cannot use mod_dir = os.environ['HOME'] here since that may not exist in the test
+            # environment
+            "if { [ file isdirectory %s/Compiler/GCC/4.7.2 ] } {" % mod_dir,
+            "    module use %s/Compiler/GCC/4.7.2" % mod_dir,


we can redefine $HOME in the test to os.path.join(self.test_prefix, 'HOME')?

Good idea. Will do that later.

@boegel

We can redefine $HOME in the test to os.path.join(self.test_prefix, 'HOME') as suggested by @boegel.

…extensions Travis reported that modulecmd.tcl does not include the extensions in its module show output. Therefore the module show output is only valid if there is at least one match.

bartoldeman · 2017-03-15T03:00:39Z

I am hitting an issue where "module show" does not show "module use" statements for modulecmd.tcl (it does show prepend-path MODULEPATH ... however).

So somehow the code needs to look at the actual modulefile (to scan module use) and at the parsed output of "module show" and somehow merge those.

bartoldeman · 2017-03-15T03:18:31Z

... or only consider "module show" if Lmod is used so we get parsed lua syntax.
I can't think of an elegant solution now. Need to sleep over it.

boegel · 2017-03-15T06:40:11Z

easybuild/tools/modules.py

@@ -750,6 +750,14 @@ def modpath_extensions_for(self, mod_names):
            modtxt = self.read_module_file(mod_name)
            exts = [ext for tup in modpath_ext_regex.findall(modtxt) for ext in tup if ext]
            self.log.debug("Found $MODULEPATH extensions for %s: %s", mod_name, exts)
+            if exts:
+                modtxt = self.show(mod_name)


include comment to explain why you're going through show, since you already have the list of $MODULEPATH extensions?

boegel · 2017-03-15T06:49:31Z

test/framework/modules.py

@@ -404,6 +404,10 @@ def test_modpath_extensions_for(self):
        error_pattern = "Can't get value from a non-existing module"
        self.assertErrorRegex(EasyBuildError, error_pattern, self.modtool.modpath_extensions_for, ['nosuchmodule/1.2'])

+        realhome = os.environ.get('HOME')


no need to keep track of original $HOME

boegel · 2017-03-15T06:49:33Z

test/framework/modules.py

@@ -404,6 +404,10 @@ def test_modpath_extensions_for(self):
        error_pattern = "Can't get value from a non-existing module"
        self.assertErrorRegex(EasyBuildError, error_pattern, self.modtool.modpath_extensions_for, ['nosuchmodule/1.2'])

+        realhome = os.environ.get('HOME')
+        os.environ['HOME'] = os.path.join(self.test_prefix, 'HOME')
+        os.makedirs("%s/modules/Compiler/GCC/4.7.2" % os.environ['HOME'])


use mkdir(..., parents=True) from filetools (which will print out a decent error rather than a traceback should a problem occur)

why did you have to make this change btw?

Because with "module show" the if statement in the module file is parsed, so for

if isDir("/home/foo/modules/Compiler/GCC/4.7.2") then prepend_path("MODULEPATH", "/home/foo/modules/Compiler/GCC/4.7.2") end

module show will output just:

prepend_path("MODULEPATH", "/home/foo/modules/Compiler/GCC/4.7.2")

only if that directory exists, and nothing if it does not exist.

... so the test only passes if that directory exists

OK, but what if the directory is created later? You still want to exclude the module that may start extending $MODULEPATH with .../Compiler/GCC/4.7.2 in the future when the corresponding directory is created, no?

boegel · 2017-03-15T06:49:35Z

test/framework/modules.py

+        if realhome is not None:
+            os.environ['HOME'] = realhome
+        else:
+            del os.environ['HOME']


no need for this, the environment is restored after each test automatically

bartoldeman · 2017-03-15T11:30:46Z

I will install modulecmd.tcl before making further changes, so I can see exactly what it does without having to rely on Travis only... May be a bit slow today though (need to watch the kids since we had 36cm of snow here and schools are closed)

bartoldeman · 2017-03-15T12:41:50Z

For future reference: given this input file:

#%Module
module use /home/Compiler/intel/2013.5.192-GCC-4.8.3
module use "/home/Compiler/GCC/4.7.2"
prepend-path MODULEPATH "/home/MPI/GCC/4.7.2/OpenMPI/1.6.4"
if { [ file isdirectory $env(HOME) ] } {
    prepend-path MODULEPATH "$env(HOME)/MPI/GCC/4.7.2/OpenMPI/1.6.4.2"
    module use $env(HOME)
}

"module show" gives this

Lmod:

-----------------------------------------------------------------------------------------------------------------------------------------
   /home/oldeman/framework2/easybuild-framework/test/framework/1.0:
-----------------------------------------------------------------------------------------------------------------------------------------
prepend_path("MODULEPATH","/home/Compiler/intel/2013.5.192-GCC-4.8.3")
prepend_path("MODULEPATH","/home/Compiler/GCC/4.7.2")
prepend_path("MODULEPATH","/home/MPI/GCC/4.7.2/OpenMPI/1.6.4")
prepend_path("MODULEPATH","/home/oldeman/MPI/GCC/4.7.2/OpenMPI/1.6.4.2")
prepend_path("MODULEPATH","/home/oldeman")

C-modules 3.2.9 (different system)

-------------------------------------------------------------------
/home/boldeman/1.0:

module           use /home/Compiler/intel/2013.5.192-GCC-4.8.3
module           use /home/Compiler/GCC/4.7.2
prepend-path     MODULEPATH /home/MPI/GCC/4.7.2/OpenMPI/1.6.4
prepend-path     MODULEPATH /home/boldeman/MPI/GCC/4.7.2/OpenMPI/1.6.4.2
module           use /home/boldeman
-------------------------------------------------------------------

TCL modules:

-------------------------------------------------------------------
/home/oldeman/framework2/easybuild-framework/test/framework/1.0:

+(0):WARN:0: Directory '/home/Compiler/intel/2013.5.192-GCC-4.8.3' not found
+(0):WARN:0: Directory '/home/Compiler/GCC/4.7.2' not found
prepend-path    MODULEPATH      /home/MPI/GCC/4.7.2/OpenMPI/1.6.4
prepend-path    MODULEPATH      /home/oldeman/MPI/GCC/4.7.2/OpenMPI/1.6.4.2
-------------------------------------------------------------------

so one could argue that TCL modules is buggy here and we need to workaround this bug by reading the modulefile directly and grepping for "module use" statements. Or perhaps a newer TCL modules version fixes the bug and one could up the minimum version of TCL modules required?

on the other hand all module flavours correctly parse the if statement and expand environment variables in the show ouput.

bartoldeman · 2017-03-15T12:53:21Z

Note: TCL modules only very recently fixed this issue (Dec 2016):

https://sourceforge.net/p/modules/modules-tcl/ci/2d7654383ee586732e3272b49c93e47d8093e369/

This is backwards compatible and avoids bugs in TCL modules < 1.661.

This matches the actual syntax used for subdir-user-modules.

bartoldeman · 2017-03-15T17:14:22Z

I wonder if easybuild should use "prepend-path MODULEPATH " in generated TCL modulefiles instead of "module use", at least for the guarded extensions, to avoid the modulecmd.tcl issue?

boegel · 2017-03-16T08:22:38Z

Or perhaps a newer TCL modules version fixes the bug and one could up the minimum version of TCL modules required?

The pure Tcl environment modules implementation is a bit of a mess, since there are several variants around. Reliably checking the version is going to be quite difficult. We don't have any version checks right now for the Tcl-only env mods tool...

I wonder if easybuild should use "prepend-path MODULEPATH " in generated TCL modulefiles instead of "module use", at least for the guarded extensions, to avoid the modulecmd.tcl issue?

Maybe, but then what about modules that are already there and do have module use?

This was probably the initial reason why we are reading the module files themselves rather than using module show...

You mention that the motivation for this change is that you are extending $MODULEPATH using a path that includes $ARCH . Can you resolve $ARCH before it's added in the module file, or is the whole point to make the module dynamically rely on $ARCH?

boegel · 2017-03-16T08:26:09Z

test/framework/modules.py

-            "if { [ file isdirectory %s/modules/Compiler/GCC/4.7.2 ] } {" % os.environ['HOME'],
-            "    module use %s/modules/Compiler/GCC/4.7.2" % os.environ['HOME'],
+            "if { [ file isdirectory $env(HOME)/modules/Compiler/GCC/4.7.2 ] } {",
+            '    prepend-path MODULEPATH "$env(HOME)/modules/Compiler/GCC/4.7.2"',


I'm OK with changing the test to dynamically rely on $HOME, that's a better test indeed (especially with your $ARCH use case in mind), but why change module use to prepend-path?

Is that required to make the test pass (for modulecmd.tcl)? Because this doesn't match with how EasyBuild currently generates modules, it'll use module use instead in Tcl modules...

boegel · 2017-03-16T08:28:15Z

easybuild/tools/modules.py

        modpath_ext_regex = re.compile(modpath_ext_regex, re.M)

        modpath_exts = {}
        for mod_name in mod_names:
+            # we must include parsed MODULEPATH extensions where the module tool expands
+            # environment variables, concatenates strings, etc.
+            # The show output is actually sufficient except for modulecmd.tcl < 1.661


I'm not sure if this is correct, I think we still want to include $MODULEPATH extensions that are guarded, cfr. my other comment: if the directory is created at a later point, all of a sudden we'll have a module A again that loads a module B that extend the $MODULEPATH with the location where module A resides, which doesn't make sense...

boegel · 2017-03-16T08:29:47Z

easybuild/tools/modules.py

            modtxt = self.read_module_file(mod_name)
-            exts = [ext for tup in modpath_ext_regex.findall(modtxt) for ext in tup if ext]
+            use_exts = modpath_ext_use_regex.findall(modtxt)
+            modtxt = self.show(mod_name)


rename this modtxt to show_out or something, to make the distinction clear?

boegel · 2017-03-16T08:33:01Z

easybuild/tools/modules.py

+            use_exts = modpath_ext_use_regex.findall(modtxt)
+            modtxt = self.show(mod_name)
+            parsed_exts = [ext for tup in modpath_ext_regex.findall(modtxt) for ext in tup if ext]
+            exts = use_exts + [ext for ext in parsed_exts if ext not in use_exts]


With this, you'll sort of still have duplicates, just one with resolved variables and one without (for example), e.g. $env(HOME)/MPI/GCC/4.7.2/OpenMPI/1.6.4.2 and /home/example/MPI/GCC/4.7.2/OpenMPI/1.6.4.2...

This may be OK, but it depends for what the result of modpath_extensions_for is used.

In any case, we need to extend the dedicated for modpath_extensions_for further to highlight this, if we go through with this change.

This avoids the issue if the directory does not exist and the issues with modulecmd.tcl and "module use".

bartoldeman · 2017-03-16T13:16:51Z

To avoid all this issues I switched the code to read the original modulefile and manually expand environment variables / join paths for Lua.

I might as well open a new PR since the diff versus the original code is smaller than before now?

boegel · 2017-03-16T14:03:20Z

easybuild/tools/modules.py

+
+                    # join paths (Lua)
+                    if matches[i][3]:
+                        ext = os.path.join(*ext.replace(",","").replace('"','').split())


@bartoldeman There's quite a lot going on here, and it looks quite tedious to me as well.

If we go through with this, we should flesh this out in a separate function, so we can test in isolation...

def interpret_lua_statement(txt): """Interpret raw Lua statement: resolve environment variables, join paths wher `pathJoin` is specified""" return ... ... exts = [] for raw_ext in raw_exts: if lua: ext = process_raw_lua(raw_ext) else: ext = process_raw_tcl(raw_ext) exts.append(ext)

That being said, this looks... adventurous.

boegel · 2017-03-16T14:05:19Z

easybuild/tools/modules.py

+            # Need to expand environment variables and join paths, e.g. when --subdir-user-modules is used
+            for i, ext in enumerate(exts):
+
+                if not matches[i][2]: # can skip plain literal Lua match


I'm not sure what's going on here... How can you use the indexes of exts for matches? This looks wrong, since exts is basically a flattened list of the entries in matches (which is a list of tuples with several empty entries), or I'm missing something...

It works here because the matches are mutually exclusive. So the matches list looks like this:
[('','','','match'),('match','','',''),...]
only one of the 4 is filled in.

OK, but wouldn't it be clearer to just loop over matches then?

for tcl_use, tcl_prepend, lua_prepend_str, lua_prepend_pathjoin in matches: if tcl_use or tcl_prepend: # Tcl statement elif lua_prepend_pathjoin: # Lua statement with pathJoin included

boegel · 2017-03-16T14:11:59Z

easybuild/tools/modules.py

+                        envlocend = ext.find(")", envlocstart+len(envstr))
+                        if envlocend == -1:
+                            break
+                        envvar = ext[envlocstart+len(envstr):envlocend].strip('"')


Can't you just use a regular expression to extract the environment variable name?

(totally untested code below):

if lua: env_regex = re.compile('os.getenv$"(.*)"$') else: env_regex = re.compile('\$env$"(.*)"$' res = env_regex.search(ext) while res: envval = os.getenv(res.group(1), '') ext = env_regex.sub(envval, ext) res = env_regex.search(ext) ...

ok let me try with regex

boegel · 2017-03-16T14:12:54Z

easybuild/tools/modules.py

@@ -742,14 +742,44 @@ def modpath_extensions_for(self, mod_names):
            r'^\s*module\s+use\s+"?([^"\s]+)"?',  # 'module use' in Tcl module files
            r'^\s*prepend-path\s+MODULEPATH\s+"?([^"\s]+)"?',  # prepend to $MODULEPATH in Tcl modules
            r'^\s*prepend_path\(\"MODULEPATH\",\s*\"(\S+)\"',  # prepend to $MODULEPATH in Lua modules
+            r'^\s*prepend_path\(\"MODULEPATH\",\s*pathJoin\((.+)\)\)',  # prepend to $MODULEPATH in Lua modules using pathJoin


I don't think we're handling prepend_path("MODULEPATH", os.getenv("HOME")) correctly yet?

no ... however the generated modulefiles won't have that.
Would be a nice to have though for consistency.

boegel · 2017-03-16T14:23:39Z

@bartoldeman No need to open a new PR, having the history of how we reached this point may be relevant in the future.

Using shlex, regular expressions for environment variables, and named matches this is now more clearly and compactly written.

…d tests for them + fix some bugs

… in modpath_extensions_for

rename parse_raw_path_* methods to interpret_raw_path_*, add dedicated tests for them + fix some bugs

…se' actually exist

fix broken test, modulecmd.tcl requires that paths used for 'module use' actually exist

also interpret '[ file join x y ]' statements in interpret_raw_path_tcl

boegel · 2017-03-17T20:55:22Z

Going in to be included in EasyBuild v3.1.2, thanks @bartoldeman!

Use "module show" output to process MODULEPATH extensions.

f7ab42d

This way lmod can do some parsing of the lua file, when the MODULEPATH extension uses environment variables.

bartoldeman mentioned this pull request Mar 14, 2017

Incorrect module loads in modules built by users with HMNS, minimal-toolchain, recursive-unload #2162

Closed

bartoldeman added 2 commits March 14, 2017 13:47

Raise exception if module for which MODULEPATH is searched does not e…

58c226e

…xist.

boegel added this to the 3.1.2 milestone Mar 14, 2017

boegel requested changes Mar 14, 2017

View reviewed changes

bartoldeman added 3 commits March 14, 2017 22:59

Match exception output in test.

35307e5

Allow original tests involving $HOME to work by redefining $HOME

290be71

We can redefine $HOME in the test to os.path.join(self.test_prefix, 'HOME') as suggested by @boegel.

Parse both "module show" output and actual modulefile for MODULEFILE …

8f0e59b

…extensions Travis reported that modulecmd.tcl does not include the extensions in its module show output. Therefore the module show output is only valid if there is at least one match.

boegel requested changes Mar 15, 2017

View reviewed changes

bartoldeman added 5 commits March 15, 2017 13:51

Merge MODULEPATH extensions from modulefile and "module show".

3489e0f

This is backwards compatible and avoids bugs in TCL modules < 1.661.

No need to preserve and restore $HOME (suggested by @boegel)

27701d0

Use mkdir from filetools and add comment as suggested by @boegel.

3088fb3

Only search for "module use" in actual modulefile.

a3e7bc8

Use $env(HOME) (TCL) and os.getenv("HOME") (Lua) in test case.

fe3b882

This matches the actual syntax used for subdir-user-modules.

boegel reviewed Mar 16, 2017

View reviewed changes

boegel requested changes Mar 16, 2017

View reviewed changes

Expand environment variables and join paths manually, not via show().

8d2c713

This avoids the issue if the directory does not exist and the issues with modulecmd.tcl and "module use".

Don't quote module use to make it more like the original testcase.

8ef1ec0

boegel requested changes Mar 16, 2017

View reviewed changes

boegel reviewed Mar 16, 2017

View reviewed changes

bartoldeman and others added 2 commits March 17, 2017 01:18

Clean up parsing code.

4b141af

Using shlex, regular expressions for environment variables, and named matches this is now more clearly and compactly written.

rename parse_raw_path_* methods to interpret_raw_path_*, add dedicate…

54a8d87

…d tests for them + fix some bugs

boegel mentioned this pull request Mar 17, 2017

rename parse_raw_path_* methods to interpret_raw_path_*, add dedicated tests for them + fix some bugs bartoldeman/easybuild-framework#4

Merged

boegel and others added 5 commits March 17, 2017 17:26

add extra test cases for modpath_extensions_for + minor style changes…

fa2b3bf

… in modpath_extensions_for

don't touch raw strings in interpret_raw_path_lua

526e01e

avoid excessive whitespace

1ed4627

Merge pull request #4 from boegel/module_show_modulepath

bbbcb4d

rename parse_raw_path_* methods to interpret_raw_path_*, add dedicated tests for them + fix some bugs

fix broken test, modulecmd.tcl requires that paths used for 'module u…

895403a

…se' actually exist

boegel mentioned this pull request Mar 17, 2017

fix broken test, modulecmd.tcl requires that paths used for 'module use' actually exist bartoldeman/easybuild-framework#5

Merged

boegel approved these changes Mar 17, 2017

View reviewed changes

bartoldeman and others added 2 commits March 17, 2017 13:03

Merge pull request #5 from boegel/module_show_modulepath

23bda15

fix broken test, modulecmd.tcl requires that paths used for 'module use' actually exist

also interpret '[ file join x y ]' statements in interpret_raw_path_tcl

7b416d4

boegel mentioned this pull request Mar 17, 2017

also interpret '[ file join x y ]' statements in interpret_raw_path_tcl bartoldeman/easybuild-framework#6

Merged

Merge pull request #6 from boegel/module_show_modulepath

fa27b73

also interpret '[ file join x y ]' statements in interpret_raw_path_tcl

boegel mentioned this pull request Mar 17, 2017

add test to verify that modules installed in user HMNS module tree don't have superfluous load statements (cfr. #2162) #2172

Merged

boegel changed the title ~~Use "module show" output to process MODULEPATH extensions.~~ interpret statements that extend $MODULEPATH in modpath_extensions_for method Mar 17, 2017

boegel merged commit 9c85748 into easybuilders:develop Mar 17, 2017

		@@ -711,15 +711,6 @@ def loaded_modules(self):

		return loaded_modules

		def read_module_file(self, mod_name):

interpret statements that extend $MODULEPATH in modpath_extensions_for method #2104

interpret statements that extend $MODULEPATH in modpath_extensions_for method #2104

Conversation

bartoldeman commented Feb 2, 2017

bartoldeman commented Feb 2, 2017

boegel commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartoldeman commented Mar 15, 2017

bartoldeman commented Mar 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartoldeman commented Mar 15, 2017

bartoldeman commented Mar 15, 2017

bartoldeman commented Mar 15, 2017

bartoldeman commented Mar 15, 2017

boegel commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartoldeman commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boegel commented Mar 16, 2017

boegel commented Mar 17, 2017