add support for Environment Modules 4 by ebagrenrut · Pull Request #2365 · easybuilders/easybuild-framework

ebagrenrut · 2017-12-20T22:16:54Z

Add a new class EnvironmentModules4. Introduces a MAX_VERSION to ModulesTool that can be used to set a maximum version for a particular module tool. Limits EnvironmentModulesC to a maximum version of 3.2.10 (the last release of the C implementation of Environment Modules). Limits EnvironmentModulesTcl to a maximum version of 1.923 (the last version of modules-tcl).

boegel · 2017-12-25T09:31:32Z

-        if self.REQ_VERSION is None:
-            self.log.debug("No version requirement defined.")
+        if self.REQ_VERSION is None and self.MAX_VERSION is None:
+                self.log.debug("No version requirement defined.")


No need for the extra indent here?

boegel · 2017-12-25T09:32:12Z

-            if StrictVersion(self.version) < StrictVersion(self.REQ_VERSION):
-                raise EasyBuildError("EasyBuild requires v%s >= v%s (no rc), found v%s",
-                                     self.__class__.__name__, self.REQ_VERSION, self.version)
+            if self.REQ_VERSION is None: # MAX_VERSION is not None


Why not check directly whether self.MAX_VERSION is different from None?

elif self. MAX_VERSION is not None:

boegel · 2017-12-25T09:32:27Z

+                                         self.__class__.__name__, self.version, self.MAX_VERSION)
+                else:
+                    self.log.debug('Version %s matches requirement <= %s' % (self.version, self.MAX_VERSION))
+            elif self.MAX_VERSION is None: # REQ_VERSION is not None


You should change this to an if, to make sure that if both self.REQ_VERSION and self.MAX_VERSION are specified, that both are checked?

if self. REQ_VERSION is not None:

You're right, my logic is broken. Will fix.

boegel · 2017-12-25T09:37:17Z

+            if self.REQ_VERSION is None: # MAX_VERSION is not None
+                self.log.debug("No required minimum version defined.")
+                if StrictVersion(self.version) > StrictVersion(self.MAX_VERSION):
+                    raise EasyBuildError("EasyBuild requires v%s <= v%s (no rc), found v%s",


You should probably drop the no rc part here, since using a release candidate of the highest allowed version is older than that version, so it's OK to use it.

boegel · 2017-12-25T09:38:02Z

+                    raise EasyBuildError("EasyBuild requires v%s <= v%s (no rc), found v%s",
+                                         self.__class__.__name__, self.version, self.MAX_VERSION)
+                else:
+                    self.log.debug('Version %s matches requirement <= %s' % (self.version, self.MAX_VERSION))


Style nitpicking: Please use , rather than %:

self.log.debug('Version %s matches requirement <= %s', self.version, self.MAX_VERSION)

boegel · 2017-12-25T09:38:40Z

+                    raise EasyBuildError("EasyBuild requires v%s >= v%s (no rc), found v%s",
+                                         self.__class__.__name__, self.REQ_VERSION, self.version)
+                else:
+                    self.log.debug('Version %s matches requirement >= %s' % (self.version, self.REQ_VERSION))


Use , rather than %:

self.log.debug('Version %s matches requirement >= %s', self.version, self.REQ_VERSION)

boegel · 2017-12-25T09:42:25Z

+                if StrictVersion(self.REQ_VERSION) > StrictVersion(self.MAX_VERSION):
+                    raise EasyBuildError("v%s, REQ_VERSION must be <= MAX_VERSION", self.__class__)
+                else:
+                    self.log.debug('Version %s matches requirements >= %s, <= %s' % (self.version, self.REQ_VERSION, self.MAX_VERSION))


I would drop this whole block, and use two if conditions above.

boegel · 2017-12-25T09:43:02Z

    """Interface to (C) environment modules (modulecmd)."""
    COMMAND = "modulecmd"
    REQ_VERSION = '3.2.10'
+    MAX_VERSION = '3.2.10'


Make this a bit more liberal, in case people have local made local modifications and bumped this version.

MAX_VERSION = '3.99' # fictional version, just to indicate that version 4.0.0 and up are not compatible

boegel · 2017-12-25T09:44:16Z

        """Update after new modules were added."""
        pass

+class EnvironmentModules4(EnvironmentModulesC):


Maybe EnvironmentModulesTcl4 is a better name?

boegel · 2017-12-25T09:45:59Z

    COMMAND_SHELL = ['tclsh']
    VERSION_OPTION = ''
    REQ_VERSION = None
+    MAX_VERSION = '1.923'


I would leave this unspecified, there's little point to it?

If somebody uses EnvironmentModulesTcl with Modules v4.0, it will fail anyway because eb can't determine the version?

…ed the hastily-named EnvironmentModules4 class until there is a better implementation. Style corrections.

… into env-modules-4

…EnvironmentModules 4.0+

…tead of string concatenation. Add a second blank line after EnvironmentModules class to match style of other classes.

boegel · 2017-12-29T09:24:40Z


+class EnvironmentModules(EnvironmentModulesTcl):
+    """Interface to environment modules 4.0+"""
+    COMMAND = os.path.join(os.environ['MODULESHOME'], 'libexec', 'modulecmd.tcl')


This should be done more carefully, if $MODULESHOME is not defined, this will lead to a nasty crash/traceback.

My suggestion is to use a fallback value, like this:

COMMAND = os.path.join(os.getenv('MODULESHOME', 'MODULESHOME_NOT_DEFINED'), 'libexec', 'modulecmd.tcl')

DO we need both MODULESHOME and MODULESHOME_NOT_DEFINED? If using os.getenv and MODULESHOME is undefined, None will be returned and the traceback won't occur. In that case, eb would look for None/libexec/modulecmd.tcl and throw an exception.

@ebagrenrut Yes, we need the fallback value, since os.path.join won't be happy with the None value:

>>> os.path.join(None, 'libexec', 'modulecmd.tcl') Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/posixpath.py", line 70, in join elif path == '' or path.endswith('/'): AttributeError: 'NoneType' object has no attribute 'endswith'

also test against Modules v4

…ad of os.environment when getting value of MODULESHOME. The latter results in a traceback if MODULESHOME is undefined.

@boegel

…work into env-modules-4 Modules 4 tests from @boegel

…value in case MODULESHOME is undefined

…odule purge. It causes an extraneous argument error with Modules 4+

don't pass empty argument to 'module purge'

…(>=4.0) like EnvironmentModulesTcl (<=1.293)

…s 4+ behaves similarly to ModulesC, so also append GCCcore/6.2.0 if we are an instance of EnvironmentModules

… (+ break up long comment)

update test_load, Modules v4.0 does not reload already loaded modules

boegel · 2018-01-04T20:45:17Z

@ebagrenrut The last failing test is because the bootstrap script is not aware of the new EnvironmentModules class yet.

That's not hard to fix, but when that is fixed there's another problem: the test for the bootstrap script will still fail because it will try to install the latest EasyBuild release (v3.5.0), which doesn't know about EnvironmentModules yet. This leads to:

eb: error: option --modules-tool: invalid choice: 'EnvironmentModules' (choose from 'EnvironmentModulesC', 'EnvironmentModulesTcl', 'Lmod')

In other words, a chicken-or-egg problem.

So, I think we should just bypass the testing of the bootstrap script for now under EnvironmentModules, and open another PR to fix the bootstrap script once EasyBuild v3.5.1 (which includes this PR that adds support for Modules v4).

boegel · 2018-01-05T08:04:06Z

@ebagrenrut Change in Travis config file to bypass testing of bootstrap script done in ebagrenrut#4.

…s tool

disable testing of bootstrap script when Modules v4 is used as modules tool

boegel · 2018-01-05T16:33:35Z

This looks good to go now, thanks for you help with this @ebagrenrut and @xdelaruelle!

I'll follow up with another PR to re-enable testing of the bootstrap script with Modules v4, which can only be merged once EasyBuild v3.5.1 is released...

First attempt at support for new Environment Modules 4

c1dfa4d

ebagrenrut mentioned this pull request Dec 20, 2017

also test against environment modules (Tcl) 4.0.0 #2341

Closed

boegel requested changes Dec 25, 2017

View reviewed changes

boegel added this to the 3.5.1 milestone Dec 25, 2017

boegel added the enhancement label Dec 25, 2017

boegel mentioned this pull request Dec 27, 2017

bootstrap_eb.py fails to find Tcl Modules installation #2366

Closed

ebagrenrut added 5 commits December 28, 2017 12:20

Corrected logic around new MAX_VERSION setting for ModulesTool. Remov…

9cd2c63

…ed the hastily-named EnvironmentModules4 class until there is a better implementation. Style corrections.

Removed superfluous MAX_VERSION from EnvironmentModulesTcl

cda2bd2

Merge branch 'develop' of github.com:easybuilders/easybuild-framework…

1a80b81

… into env-modules-4

Added new class EnvironmentModules(EnvironmentModulesTcl) to support …

26f414a

…EnvironmentModules 4.0+

In EvironmentModules class, use os.path.join when setting COMMAND ins…

52c5bcb

…tead of string concatenation. Add a second blank line after EnvironmentModules class to match style of other classes.

boegel requested changes Dec 29, 2017

View reviewed changes

boegel and others added 6 commits December 29, 2017 10:37

also test against Modules v4

6c6bb61

Merge pull request #1 from boegel/env-modules-4

5f15f59

also test against Modules v4

When setting COMMAND in EnvironmentModules class, use os.getenv inste…

656f37f

…ad of os.environment when getting value of MODULESHOME. The latter results in a traceback if MODULESHOME is undefined.

Merge branch 'env-modules-4' of github.com:ebagrenrut/easybuild-frame…

03ddd5a

…work into env-modules-4 Modules 4 tests from @boegel

When setting COMMAND in EnvironmentModules class, provide a fallback …

29dfe3d

…value in case MODULESHOME is undefined

don't pass empty argument to 'module purge'

585f1ee

boegel mentioned this pull request Dec 29, 2017

don't pass empty argument to 'module purge' ebagrenrut/easybuild-framework#2

Merged

ebagrenrut and others added 6 commits December 29, 2017 13:51

In purge method of ModulesTool class, do not pass empty argument to m…

763a2e0

…odule purge. It causes an extraneous argument error with Modules 4+

Merge pull request #2 from boegel/env-modules-4

d86561a

don't pass empty argument to 'module purge'

In test_avail method of ModulesTest class, handle EnvironmentModules …

1c85daa

…(>=4.0) like EnvironmentModulesTcl (<=1.293)

In test_avail method of ModulesTest class, handle EnvironmentModules …

c59565e

…(>=4.0) like EnvironmentModulesTcl (<=1.293)

import EnvironmentModules class

6b6588b

EnvironmentModules is a subclass of EnvironmentModulesTcl, but Module…

2ce48cc

…s 4+ behaves similarly to ModulesC, so also append GCCcore/6.2.0 if we are an instance of EnvironmentModules

easybuilders deleted a comment from boegelbot Dec 30, 2017

boegel changed the title ~~First attempt at support for Environment Modules 4~~ add support for Environment Modules 4 Dec 30, 2017

update test_load, Modules v4.0 does not reload already loaded modules…

0b7d344

… (+ break up long comment)

boegel mentioned this pull request Dec 30, 2017

update test_load, Modules v4.0 does not reload already loaded modules ebagrenrut/easybuild-framework#3

Merged

Merge pull request #3 from boegel/env-modules-4

d6dbdda

update test_load, Modules v4.0 does not reload already loaded modules

easybuilders deleted a comment from boegelbot Jan 4, 2018

boegel mentioned this pull request Jan 4, 2018

disable testing of bootstrap script when Modules v4 is used as modules tool ebagrenrut/easybuild-framework#4

Merged

boegel and others added 2 commits January 5, 2018 09:48

disable testing of bootstrap script when Modules v4 is used as module…

792e134

…s tool

Merge pull request #4 from boegel/env-modules-4

86ff039

disable testing of bootstrap script when Modules v4 is used as modules tool

easybuilders deleted a comment from boegelbot Jan 5, 2018

boegel approved these changes Jan 5, 2018

View reviewed changes

boegel merged commit 596dbc8 into easybuilders:develop Jan 5, 2018

boegel mentioned this pull request Jan 5, 2018

update bootstrap script to be compatible with Modules v4 #2370

Closed

Conversation

ebagrenrut commented Dec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel commented Jan 4, 2018

Uh oh!

boegel commented Jan 5, 2018

Uh oh!

boegel commented Jan 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants