add support for marking installed module file as new default version #2110

victorusu · 2017-02-09T15:31:46Z

This commit adds support to define a default module version on command
line.

The command line is:
--set-default-module

And both Tcl and Lua modules are supported.

release EasyBuild v3.1.0

This commit adds support to define a default module version on command line. The command line is: --set-as-default And both Tcl and Lua modules are supported.

pforai · 2017-02-09T21:50:28Z

LGTM

Adding unittest to * symlink and readlink (test.framework.filetools) * set_default (test.framework.module_generator) Currently the unittests pass, except for test.framework.module_generator test_set_default because the environment is cleaned up in the test and LMod complains about the cache. The results for the unittests are: python -m test.framework.filetools test_symlink_readlink_file INFO: This is (based on) vsc.install.shared_setup 0.10.22 Filtered FileToolsTest tests using 'test_symlink_readlink_file', retained 1/33 tests: test_symlink_readlink_file . ---------------------------------------------------------------------- Ran 1 test in 0.740s OK python -m test.framework.module_generator test_set_default INFO: This is (based on) vsc.install.shared_setup 0.10.22 Filtered TclModuleGeneratorTest tests using 'test_set_default', retained 1/16 tests: test_set_default Filtered LuaModuleGeneratorTest tests using 'test_set_default', retained 1/16 tests: test_set_default .E ====================================================================== ERROR: test_set_default (__main__.LuaModuleGeneratorTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/hvictor/Programs/easybuild/easybuild-framework/test/framework/module_generator.py", line 186, in test_set_default self.modtool.load([module_name]) File "easybuild/tools/modules.py", line 509, in load self.run_module('load', mod) File "easybuild/tools/modules.py", line 674, in run_module raise EasyBuildError("Changing environment as dictated by module failed: %s (%s)", err, out) EasyBuildError: 'Changing environment as dictated by module failed: name \'false\' is not defined (stdout: \nfalse\n, stderr: Lmod has detected the following error: The following module(s) are unknown: "dummy"\n\nPlease check the spelling or version number. Also try "module spider ..."\nIt is also possible your cache file is out-of-date try:\n $ module --ignore-cache load "dummy"\n\n\n\n)' ---------------------------------------------------------------------- Ran 2 tests in 2.653s FAILED (errors=1)

boegel · 2017-02-09T15:54:20Z

easybuild/tools/options.py

@@ -383,6 +383,9 @@ def override_options(self):
                                     None, 'store_true', False),
            'zip-logs': ("Zip logs that are copied to install directory, using specified command",
                         None, 'store_or_None', 'gzip'),
+
+                'set-as-default': ("Set the generated module as default", None, 'store_true', False),


indent is off here

boegel · 2017-02-09T15:55:52Z

easybuild/tools/module_generator.py

+        mod_filepath = os.path.join(module_folder_path, module_file)
+
+        default_filepath = os.path.join(module_folder_path, 'default')
+        os.symlink(mod_filepath, default_filepath)


what if there's already a symlink?

also, use symlink from filetools

boegel · 2017-02-09T15:58:26Z

easybuild/tools/module_generator.py

@@ -692,6 +704,14 @@ def getenv_cmd(self, envvar):
        """
        return 'os.getenv("%s")' % envvar

+    def set_as_default(self, module_folder_path, module_version):
+
+        module_file = module_version + '.lua'


use self.MODULE_FILE_EXTENSION rather than hardcoding .lua

boegel · 2017-02-09T15:59:01Z

easybuild/framework/easyblock.py

@@ -2132,6 +2172,8 @@ def make_module_step(self, fake=False):
        txt += self.make_module_extra()
        txt += self.make_module_footer()

+
+


moar whitespace!

boegel · 2017-02-09T15:59:29Z

easybuild/framework/easyblock.py

@@ -2163,6 +2205,8 @@ def make_module_step(self, fake=False):
            if not fake:
                self.make_devel_module()

+        self._set_module_as_default(fake=fake)


I'd include the if build_option(...) here instead?

boegel · 2017-02-09T16:07:39Z

easybuild/framework/easyblock.py

+                    self.dry_run_msg("Generating default module file %s\n", mod_filepath)
+            else:
+                self.module_generator.set_as_default(mod_folderpath, self.version)
+                self.log.info("Module default version file %s written", mod_filepath)


this should not be here, should log in modulegenerator instead

boegel · 2017-02-09T16:12:32Z

easybuild/framework/easyblock.py

+                self.module_generator.set_as_default(mod_folderpath, self.version)
+                self.log.info("Module default version file %s written", mod_filepath)
+
+                # invalidate relevant 'module avail'/'module show' cache entries


?! comment does not make any sense?

all of this can be removed totally

boegel · 2017-02-09T16:13:42Z

easybuild/framework/easyblock.py

@@ -2075,6 +2075,46 @@ def _sanity_check_step(self, custom_paths=None, custom_commands=None, extension=
        else:
            self.log.debug("Sanity check passed!")

+    def _set_module_as_default(self, fake=False):


boegel · 2017-02-10T00:48:01Z

easybuild/framework/easyblock.py

+        sets the default module version except if we are in dry run.
+        """
+        mod_filepath = self.module_generator.get_module_filepath()
+        mod_folderpath = os.path.dirname(mod_filepath)


why not combine these lines together, mod_filepath is only used in one place?

boegel · 2017-02-10T00:49:30Z

easybuild/framework/easyblock.py

+        mod_folderpath = os.path.dirname(mod_filepath)
+
+        if not self.dry_run:
+            self.module_generator.set_as_default(mod_folderpath, self.version)


maybe emit a meaningful message in the case of dry run?

if self.dry_run: dry_run_msg("Marked %s v%s as default version" % (self.name, self.version)) else: self.module_generator.set_as_default(mod_folderpath, self.version)

boegel · 2017-02-10T00:50:14Z

easybuild/framework/easyblock.py

@@ -2139,6 +2151,7 @@ def make_module_step(self, fake=False):
                for line in txt.split('\n'):
                    self.dry_run_msg(' ' * 4 + line)

+            self.dry_run_msg("Generating file to set default module version to %s\n", self.version)


hmm, drop this, see dry run msg in _set_module_as_default?

boegel · 2017-02-10T00:51:16Z

easybuild/tools/filetools.py

+    :param symlink_path: symlink file path
+    """
+
+    # note: we can't use try-except-finally, because Python 2.4 doesn't support it as a single block


minimal required version is Python 2.6 now, so you can use try/except/finally

but, not relevant here, so please just drop this comment

boegel · 2017-02-10T00:51:40Z

easybuild/tools/filetools.py

+    # note: we can't use try-except-finally, because Python 2.4 doesn't support it as a single block
+    try:
+        target_symlink_path = os.readlink(symlink_path)
+    except IOError, err:


should be OSError rather than IOError?

also, use

except OSError as err:

boegel · 2017-02-10T00:59:50Z

test/framework/module_generator.py

@@ -40,13 +40,16 @@
 from easybuild.tools import config
 from easybuild.tools.module_generator import ModuleGeneratorLua, ModuleGeneratorTcl
 from easybuild.tools.module_naming_scheme.utilities import is_valid_module_name
+from easybuild.tools.filetools import mkdir, write_file


sort line alphabetically please?

boegel · 2017-02-10T01:00:07Z

test/framework/module_generator.py

 from easybuild.framework.easyblock import EasyBlock
 from easybuild.framework.easyconfig.easyconfig import EasyConfig, ActiveMNS
 from easybuild.tools.build_log import EasyBuildError
 from easybuild.tools.modules import Lmod
 from easybuild.tools.utilities import quote_str
 from test.framework.utilities import EnhancedTestCase, TestLoaderFiltered, find_full_path, init_config

+from vsc.utils.missing import nub
+from easybuild.tools.modules import curr_module_paths, reset_module_caches


this should be moved up...

boegel · 2017-02-10T01:00:17Z

test/framework/module_generator.py

+        Test load part in generated module file.
+        """
+        # if not self.MODULE_GENERATOR_CLASS == ModuleGeneratorTcl:
+        #     return


please remove?

boegel · 2017-02-10T01:01:28Z

test/framework/module_generator.py

+        mkdir(base_path)
+
+        # creating package module
+        module_name = 'dummy'


dummy has a particular meaning, use just a random module name instead like foobar or whatever

boegel · 2017-02-10T01:01:52Z

test/framework/module_generator.py

+        if txt:
+            txt += '\n'
+
+        foo_version = 'foo_version'


why not make this 'proper' versions like 1.0 and 2.0?

Fixed MR and all unittests pass now python -m test.framework.filetools test_symlink_readlink_file INFO: This is (based on) vsc.install.shared_setup 0.10.22 Filtered FileToolsTest tests using 'test_symlink_readlink_file', retained 1/33 tests: test_symlink_readlink_file . ---------------------------------------------------------------------- Ran 1 test in 0.763s OK python -m test.framework.module_generator test_set_default INFO: This is (based on) vsc.install.shared_setup 0.10.22 Filtered TclModuleGeneratorTest tests using 'test_set_default', retained 1/16 tests: test_set_default Filtered LuaModuleGeneratorTest tests using 'test_set_default', retained 1/16 tests: test_set_default .. ---------------------------------------------------------------------- Ran 2 tests in 2.831s OK

* Removed bug on remove_file - that did not remove invalid links * Removed own introduced bug on symlink that called build_options too * early * Removed own introduced bug that set full path links to default LMod * modules

This commit is comprised of: * tests remove_file for valid and invalid symlinks * improved error message for symlink errors

boegel · 2017-02-14T19:26:56Z

easybuild/tools/filetools.py

@@ -180,7 +204,8 @@ def remove_file(path):
        return

    try:
-        if os.path.exists(path):
+        # note: file may also be a broken symlink...
+        if os.path.exists(path) or os.path.islink(path):


@victorusu please add a dedicated test case for this fixed bug?

I have added as test_remove_symlinks

boegel · 2017-02-14T19:27:32Z

easybuild/tools/module_generator.py

@@ -452,6 +456,20 @@ def getenv_cmd(self, envvar):
        """
        return '$env(%s)' % envvar

+    # noinspection PyPackageRequirements,PyPackageRequirements


@victorusu what's this about (and why twice the same thing)?

I removed. It was a mistake...

boegel · 2017-02-14T19:28:28Z

easybuild/tools/module_generator.py

@@ -693,6 +711,29 @@ def getenv_cmd(self, envvar):
        return 'os.getenv("%s")' % envvar


+    def set_as_default(self, module_folder_path, module_version):
+        """
+        Create file called "default" inside the package's module folder in order


it's not a file, but a symlink?

boegel · 2017-02-14T19:28:53Z

easybuild/tools/module_generator.py

+        default_filepath = os.path.join(module_folder_path, 'default')
+
+        if os.path.islink(default_filepath):
+            points_to = readlink(default_filepath)


rename to link_target?

boegel · 2017-02-14T19:29:18Z

easybuild/tools/module_generator.py

+        cwd = os.getcwd()
+        #os.chdir(module_folder_path)
+        symlink(module_version + self.MODULE_FILE_EXTENSION, default_filepath)
+        #os.chdir(cwd)


please clean up the uncommented code, and you can drop the cwd = line too (not used anymore)

boegel · 2017-02-14T19:31:10Z

test/framework/filetools.py

+        # checking if file is symlink
+        self.assertTrue(os.path.islink(link))
+        # reading link target and comparing to file name
+        self.assertEqual(fp, ft.readlink(link))


hmm, be careful here, what if fp is a file in a symlinked location?

maybe use os.path.realpath or ft.readlink when defining fp?

boegel · 2017-02-14T19:31:36Z

test/framework/module_generator.py

@@ -46,7 +48,7 @@
 from easybuild.tools.modules import Lmod
 from easybuild.tools.utilities import quote_str
 from test.framework.utilities import EnhancedTestCase, TestLoaderFiltered, find_full_path, init_config
-
+from vsc.utils.missing import nub


move this up, next to the other imports from vsc.utils?

boegel · 2017-02-14T19:32:07Z

test/framework/module_generator.py

+        # removing all available modules from the environment
+        mod_paths = [p for p in nub(curr_module_paths()) if os.path.exists(p)]
+        for mod_path in nub(mod_paths):
+            self.modtool.unuse(mod_path)


do you really need to do this? if not, then don't?

This is an attempt to remove the issues related to LMod 5.8

boegel · 2017-02-16T13:26:21Z

@victorusu if you consider this fit for rereview, please update PR title and ping me in a comment

victorusu · 2017-02-20T09:34:48Z

@boegel, I changed the symlink function in order to allow non-full absolute source paths. Which is a required for older versions of LMod (< 7.1 at least). So now, all the LMod tests are passing.
I am still experiencing a failure due to TMod. The test test_set_default is creating .lua module files, even though the env module system is TMod. So I wonder if this is the desired behaviour and I should add an if statement like the following in order to skip this case?

if self.MODULE_GENERATOR_CLASS == ModuleGeneratorLua and os.getenv("EASYBUILD_MODULE_SYNTAX") == "Tcl":
pass

Thanks!

boegel · 2017-02-20T14:13:55Z

@victorusu How can the test still be creating Lua module file, we're using self.modgen.MODULE_FILE_EXTENSION to make sure we create the right type of module files?

Unless I'm missing something, the test should work unconditionally regardless of which modules tool & syntax is being used?

boegel · 2017-02-20T14:23:11Z

OK, I think I see the problem now...

The tests in test/framework/module_generator.py are being run twice automagically, once for Tcl syntax, and once for Lua syntax.

However, running them with Lua syntax only works when Lmod is being used as a modules tool, of course.

So, yes @victorusu, you'll need to add a check like:

if self.MODULE_GENERATOR_CLASS == ModuleGeneratorLua and not isinstance(self.modtool, Lmod):
    pass

victorusu · 2017-02-20T14:26:19Z

@boegel, If think the issue is related to the fact that the test.suite calls

suite.addTests(TestLoaderFiltered().loadTestsFromTestCase(TclModuleGeneratorTest, sys.argv[1:]))
suite.addTests(TestLoaderFiltered().loadTestsFromTestCase(LuaModuleGeneratorTest, sys.argv[1:]))

So, the generator is Lua but the environment is Tcl. This issue was not observed before because no, previous, test generated any module. The current tests only load and unload modules.

Isn't it?

This commit skips the creation of luamodules for the Tcl Environment module system

boegel · 2017-02-21T16:22:41Z

test/framework/module_generator.py

+        Test load part in generated module file.
+        """
+        if self.MODULE_GENERATOR_CLASS == ModuleGeneratorLua and not isinstance(self.modtool, Lmod):
+            pass


@victorusu: you'll need to make this a return rather than a pass to actually skip the test

Yes, thanks!

victorusu · 2017-02-22T18:37:53Z

@boegel, The unittests are finally passing! 👍
I removed the WIP from the title. I am waiting for feedback. :-)
Thanks!

boegel

final review, a couple more minor things

largest remark is about making the option name a bit clearer

boegel · 2017-02-23T20:43:49Z

easybuild/tools/filetools.py

+    Create a symlink at the specified path to the given path.
+
+    :param source_path: source file path
+    :param symlink_path: symlink file path


use_abspath_source is not documented in docstring, please fix

boegel · 2017-02-23T20:44:50Z

easybuild/tools/filetools.py

+            _log.info("Symlinked %s to %s", os.path.abspath(source_path), symlink_path)
+        else:
+            os.symlink(source_path, symlink_path)
+            _log.info("Symlinked %s to %s", source_path, symlink_path)


you can shorten this a bit:

if use_abspath_source: source_path = os.path.abspath(source_path) os.symlink(source_path, symlink_path) _log.info("Symlinked %s to %s", source_path, symlink_path)

boegel · 2017-02-23T20:45:25Z

easybuild/tools/module_generator.py


+# noinspection PyPackageRequirements


please remove?

boegel · 2017-02-23T20:46:28Z

easybuild/tools/module_generator.py

+            remove_file(default_filepath)
+            self.log.info("Removed default version marking from %s.", link_target)
+        elif os.path.exists(default_filepath):
+            raise EasyBuildError('Found an unexpected file called default in dir %s' % module_folder_path)


s/called default/named 'default'/g

boegel · 2017-02-23T20:47:11Z

easybuild/tools/options.py

@@ -383,6 +383,8 @@ def override_options(self):
                                     None, 'store_true', False),
            'zip-logs': ("Zip logs that are copied to install directory, using specified command",
                         None, 'store_or_None', 'gzip'),
+            'set-as-default': ("Set the generated module as default", None, 'store_true', False),


maybe rename to --set-default-module? leaves less room for misinterpretation...

boegel · 2017-02-23T20:48:54Z

test/framework/filetools.py

+        # checking if file is symlink
+        self.assertTrue(os.path.islink(link))
+        # reading link target and comparing to file name
+        self.assertEqual(os.path.realpath(fp), ft.readlink(link))


hmm, I think this may fail, e.g. if self.test_prefix is a symlink...

os.path.realpath will give you the fully resolved location, while ft.readlink will only resolve one symlink 'level', I think?

You are right! Setting

self.assertEqual(os.path.realpath(fp), os.path.realpath(ft.readlink(link)))

worked for me.

Yes, ok, but then the test is worthless, no?

I'm starting to think we should change ft.readlink to ft.resolve_symlink, and basically make it call os.path.realpath in a try-except?

boegel · 2017-02-23T20:49:46Z

test/framework/module_generator.py

+        Test load part in generated module file.
+        """
+        if self.MODULE_GENERATOR_CLASS == ModuleGeneratorLua and not isinstance(self.modtool, Lmod):
+            return


include a short comment to explain why you're bailing out here, this looks suspicious, but we're not cheating here :)

…path, enhance test for symlink & resolve_path

boegel · 2017-03-23T20:21:27Z

@victorusu victorusu#1 should make this good to go

rename filetools.readlink to filetools.resolve_path, use os.path.realpath, enhance test for symlink & resolve_path

boegel · 2017-03-24T08:23:00Z

Going in, thanks @victorusu!

boegel and others added 2 commits February 3, 2017 13:03

Merge pull request easybuilders#2106 from hpcugent/develop

027438d

release EasyBuild v3.1.0

Setting default module version

ee04aaf

This commit adds support to define a default module version on command line. The command line is: --set-as-default And both Tcl and Lua modules are supported.

boegel requested changes Feb 10, 2017

View reviewed changes

victorusu added 3 commits February 10, 2017 10:00

Fixed minor assert on unittest

d3f718c

Fix unittest error for extended-dry-build

4b2b0ed

victorusu changed the title ~~Default module version~~ WIP: Default module version Feb 10, 2017

victorusu added 3 commits February 10, 2017 13:53

Fixing not accepted empty modules on LMod versions

45bbd79

Setting debig_lmod fallback to false

f3f93b2

Fixing some test bugs

b66bb75

* Removed bug on remove_file - that did not remove invalid links * Removed own introduced bug on symlink that called build_options too * early * Removed own introduced bug that set full path links to default LMod * modules

boegel mentioned this pull request Feb 10, 2017

calling build_option too early causes a mess #2115

Open

Unittest to remove symlinks

ee34cc6

This commit is comprised of: * tests remove_file for valid and invalid symlinks * improved error message for symlink errors

boegel reviewed Feb 14, 2017

View reviewed changes

boegel requested changes Feb 14, 2017

View reviewed changes

boegel reviewed Feb 14, 2017

View reviewed changes

boegel requested changes Feb 14, 2017

View reviewed changes

boegel added this to the 3.2.0 milestone Feb 14, 2017

victorusu added 2 commits February 16, 2017 09:28

Making the symlink fully local

2799443

This is an attempt to remove the issues related to LMod 5.8

Addressing the remaining PR issues

e06280a

Fixing PR remarks

0751576

This commit skips the creation of luamodules for the Tcl Environment module system

boegel reviewed Feb 21, 2017

View reviewed changes

Corrected the test_set_default based on PR remarks

bf96fcb

victorusu changed the title ~~WIP: Default module version~~ Default module version Feb 22, 2017

boegel changed the title ~~Default module version~~ add support for marking installed module file as new default version Feb 23, 2017

boegel requested changes Feb 23, 2017

View reviewed changes

boegel mentioned this pull request Feb 24, 2017

alphabetically sort functions and methods in module_generator module #2132

Merged

victorusu and others added 3 commits February 27, 2017 15:19

Fixing MR remarks

f6d03d2

Fixing MR remarks

e9c97e9

rename filetools.readlink to filetools.resolve_path, use os.path.real…

7282896

…path, enhance test for symlink & resolve_path

boegel mentioned this pull request Mar 23, 2017

rename filetools.readlink to filetools.resolve_path, use os.path.realpath, enhance test for symlink & resolve_path victorusu/easybuild-framework#1

Merged

Merge pull request #1 from boegel/default-version

51488d8

rename filetools.readlink to filetools.resolve_path, use os.path.realpath, enhance test for symlink & resolve_path

boegel approved these changes Mar 24, 2017

View reviewed changes

boegel merged commit 297a07b into easybuilders:develop Mar 24, 2017

boegel mentioned this pull request Mar 24, 2017

sync with develop damianam/easybuild-framework#1

Merged

		@@ -2132,6 +2172,8 @@ def make_module_step(self, fake=False):
		txt += self.make_module_extra()
		txt += self.make_module_footer()

add support for marking installed module file as new default version #2110

add support for marking installed module file as new default version #2110

Conversation

victorusu commented Feb 9, 2017 • edited Loading

pforai commented Feb 9, 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

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

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

victorusu Feb 16, 2017 • edited Loading

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

Choose a reason for hiding this comment

boegel commented Feb 16, 2017

victorusu commented Feb 20, 2017

boegel commented Feb 20, 2017

boegel commented Feb 20, 2017

victorusu commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorusu commented Feb 22, 2017

boegel left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boegel commented Mar 23, 2017

boegel commented Mar 24, 2017

victorusu commented Feb 9, 2017 •

edited

Loading

victorusu Feb 16, 2017 •

edited

Loading