Review fixes

Author: Flamefire

easybuild/framework/easyconfig/easyconfig.py

@@ -809,37 +809,30 @@ def validate_iterate_opts_lists(self):
         Configure/build/install options specified as lists should have same length.
         """
 
-        # Disable templating as only the existance is of interest, the actual resolved value is not
-        prev_enable_templating = self.enable_templating
-        self.enable_templating = False
-
         # configure/build/install options may be lists, in case of an iterated build
         # when lists are used, they should be all of same length
         # list of length 1 are treated as if it were strings in EasyBlock
         opt_counts = []
         for opt in ITERATE_OPTIONS:
 
             # only when builddependencies is a list of lists are we iterating over them
-            if opt == 'builddependencies' and not all(isinstance(e, list) for e in self[opt]):
+            if opt == 'builddependencies' and not all(isinstance(e, list) for e in self.get_ref(opt)):
                 continue
 
+            opt_value = self.get(opt, None, resolve=False)
             # anticipate changes in available easyconfig parameters (e.g. makeopts -> buildopts?)
-            if self.get(opt, None) is None:
-                self.enable_templating = prev_enable_templating
+            if opt_value is None:
                 raise EasyBuildError("%s not available in self.cfg (anymore)?!", opt)
 
             # keep track of list, supply first element as first option to handle
-            if isinstance(self[opt], (list, tuple)):
-                opt_counts.append((opt, len(self[opt])))
+            if isinstance(opt_value, (list, tuple)):
+                opt_counts.append((opt, len(opt_value)))
 
         # make sure that options that specify lists have the same length
         list_opt_lengths = [length for (opt, length) in opt_counts if length > 1]
         if len(nub(list_opt_lengths)) > 1:
-            self.enable_templating = prev_enable_templating
             raise EasyBuildError("Build option lists for iterated build should have same length: %s", opt_counts)
 
-        self.enable_templating = prev_enable_templating
-
         return True
 
     def start_iterating(self):
@@ -1531,12 +1524,13 @@ def __setitem__(self, key, value):
                                  key, value)
 
     @handle_deprecated_or_replaced_easyconfig_parameters
-    def get(self, key, default=None):
+    def get(self, key, default=None, resolve=True):
         """
         Gets the value of a key in the config, with 'default' as fallback.
+        :param resolve: if False, disables templating via calling get_ref, else resolves template values
         """
         if key in self:
-            return self[key]
+            return self[key] if resolve else self.get_ref(key)
         else:
             return default
 

easybuild/framework/extension.py

@@ -41,6 +41,7 @@
 from easybuild.tools.build_log import EasyBuildError, raise_nosupport
 from easybuild.tools.filetools import change_dir
 from easybuild.tools.run import run_cmd
+from easybuild.tools.py2vs3 import string_type
 
 
 def resolve_exts_filter_template(exts_filter, ext):
@@ -51,7 +52,7 @@ def resolve_exts_filter_template(exts_filter, ext):
     :return (cmd, input) as a tuple of strings
     """
 
-    if isinstance(exts_filter, str) or len(exts_filter) != 2:
+    if isinstance(exts_filter, string_type) or len(exts_filter) != 2:
         raise EasyBuildError('exts_filter should be a list or tuple of ("command","input")')
 
     cmd, cmdinput = exts_filter
@@ -71,11 +72,13 @@ def resolve_exts_filter_template(exts_filter, ext):
     }
 
     try:
-        return (cmd % tmpldict, cmdinput % tmpldict if cmdinput else None)
+        cmd = cmd % tmpldict
+        cmdinput = cmdinput % tmpldict if cmdinput else None
     except KeyError as err:
         msg = "KeyError occurred on completing extension filter template: %s; "
         msg += "'name'/'version' keys are no longer supported, should use 'ext_name'/'ext_version' instead"
         raise_nosupport(msg % err, '2.0')
+    return cmd, cmdinput
 
 
 class Extension(object):

easybuild/tools/build_log.py

@@ -95,6 +95,7 @@ def raise_nosupport(msg, ver):
     nosupport_msg = "NO LONGER SUPPORTED since v%s: %s; see %s for more information"
     raise_easybuilderror(nosupport_msg, ver, msg, DEPRECATED_DOC_URL)
 
+
 class EasyBuildLog(fancylogger.FancyLogger):
     """
     The EasyBuild logger, with its own error and exception functions.

easybuild/tools/modules.py

@@ -180,18 +180,17 @@ def __init__(self, mod_paths=None, testing=False):
         if mod_paths is not None:
             self.set_mod_paths(mod_paths)
 
-        cmd_path = which(self.cmd, log_ok=False, log_error=False)
-        # only use command path in environment variable if command in not available in $PATH
-        if which(self.cmd) is None:
-            if env_cmd_path is not None:
+        if env_cmd_path:
+            cmd_path = which(self.cmd, log_ok=False, log_error=False)
+            # only use command path in environment variable if command in not available in $PATH
+            if cmd_path is None:
                 self.log.debug("Set %s command via environment variable %s: %s",
                                self.NAME, self.COMMAND_ENVIRONMENT, self.cmd)
                 self.cmd = env_cmd_path
-
-        # check whether paths obtained via $PATH and $LMOD_CMD are different
-        elif env_cmd_path and cmd_path != env_cmd_path:
-            self.log.debug("Different paths found for %s command '%s' via which/$PATH and $%s: %s vs %s",
-                           self.NAME, self.COMMAND, self.COMMAND_ENVIRONMENT, cmd_path, env_cmd_path)
+            # check whether paths obtained via $PATH and $LMOD_CMD are different
+            elif cmd_path != env_cmd_path:
+                self.log.debug("Different paths found for %s command '%s' via which/$PATH and $%s: %s vs %s",
+                               self.NAME, self.COMMAND, self.COMMAND_ENVIRONMENT, cmd_path, env_cmd_path)
 
         # make sure the module command was found
         if self.cmd is None:
@@ -676,7 +675,7 @@ def modulefile_path(self, mod_name, strip_ext=False):
         :param strip_ext: strip (.lua) extension from module fileame (if present)"""
         # (possible relative) path is always followed by a ':', and may be prepended by whitespace
         # this works for both environment modules and Lmod
-        modpath_re = re.compile('^\s*(?P<modpath>[^/\n]*/[^\s]+):$', re.M)
+        modpath_re = re.compile(r'^\s*(?P<modpath>[^/\n]*/[^\s]+):$', re.M)
         modpath = self.get_value_from_modulefile(mod_name, modpath_re)
 
         if strip_ext and modpath.endswith('.lua'):
@@ -941,7 +940,7 @@ def file_join(res):
             """Helper function to compose joined path."""
             return os.path.join(*[x.strip('"') for x in res.groups()])
 
-        res = re.sub('\[\s+file\s+join\s+(.*)\s+(.*)\s+\]', file_join, res)
+        res = re.sub(r'\[\s+file\s+join\s+(.*)\s+(.*)\s+\]', file_join, res)
 
         # also interpret all $env(...) parts
         res = re.sub(r'\$env\((?P<key>[^)]*)\)', lambda res: os.getenv(res.group('key'), ''), res)
@@ -1160,7 +1159,7 @@ def run_module(self, *args, **kwargs):
         # this is required for the DEISA variant of modulecmd.tcl which is commonly used
         def tweak_stdout(txt):
             """Tweak stdout before it's exec'ed as Python code."""
-            modulescript_regex = "^exec\s+[\"'](?P<modulescript>/tmp/modulescript_[0-9_]+)[\"']$"
+            modulescript_regex = r"^exec\s+[\"'](?P<modulescript>/tmp/modulescript_[0-9_]+)[\"']$"
             return re.sub(modulescript_regex, r"execfile('\1')", txt)
 
         tweak_stdout_fn = None
@@ -1368,7 +1367,7 @@ def module_wrapper_exists(self, mod_name):
 
         # first consider .modulerc.lua with Lmod 7.8 (or newer)
         if StrictVersion(self.version) >= StrictVersion('7.8'):
-            mod_wrapper_regex_template = '^module_version\("(?P<wrapped_mod>.*)", "%s"\)$'
+            mod_wrapper_regex_template = r'^module_version\("(?P<wrapped_mod>.*)", "%s"\)$'
             res = super(Lmod, self).module_wrapper_exists(mod_name, modulerc_fn='.modulerc.lua',
                                                           mod_wrapper_regex_template=mod_wrapper_regex_template)
 

test/framework/build_log.py

@@ -36,8 +36,9 @@
 from unittest import TextTestRunner
 
 from easybuild.base.fancylogger import getLogger, logToFile, setLogFormat
-from easybuild.tools.build_log import LOGGING_FORMAT, EasyBuildError, EasyBuildLog, dry_run_msg, dry_run_warning
-from easybuild.tools.build_log import init_logging, print_error, print_msg, print_warning, stop_logging, time_str_since
+from easybuild.tools.build_log import (
+    LOGGING_FORMAT, EasyBuildError, EasyBuildLog, dry_run_msg, dry_run_warning, init_logging, print_error, print_msg,
+    print_warning, stop_logging, time_str_since, raise_nosupport)
 from easybuild.tools.filetools import read_file, write_file
 
 
@@ -146,9 +147,10 @@ def test_easybuildlog(self):
         logtxt_regex = re.compile(r'^%s' % expected_logtxt, re.M)
         self.assertTrue(logtxt_regex.search(logtxt), "Pattern '%s' found in %s" % (logtxt_regex.pattern, logtxt))
 
-        self.assertErrorRegex(EasyBuildError, "DEPRECATED \(since .*: kaput", log.deprecated, "kaput", older_ver)
-        self.assertErrorRegex(EasyBuildError, "DEPRECATED \(since .*: 2>1", log.deprecated, "2>1", '2.0', '1.0')
-        self.assertErrorRegex(EasyBuildError, "DEPRECATED \(since .*: 2>1", log.deprecated, "2>1", '2.0', max_ver='1.0')
+        self.assertErrorRegex(EasyBuildError, r"DEPRECATED \(since .*: kaput", log.deprecated, "kaput", older_ver)
+        self.assertErrorRegex(EasyBuildError, r"DEPRECATED \(since .*: 2>1", log.deprecated, "2>1", '2.0', '1.0')
+        self.assertErrorRegex(EasyBuildError, r"DEPRECATED \(since .*: 2>1", log.deprecated, "2>1", '2.0',
+                              max_ver='1.0')
 
         # wipe log so we can reuse it
         write_file(tmplog, '')
@@ -422,6 +424,10 @@ def test_init_logging(self):
 
         stop_logging(logfile, logtostdout=True)
 
+    def test_raise_nosupport(self):
+        self.assertErrorRegex(EasyBuildError, 'NO LONGER SUPPORTED since v42: foobar;',
+                              raise_nosupport, 'foobar', 42)
+
 
 def suite():
     """ returns all the testcases in this module """

test/framework/easyconfig.py

@@ -58,6 +58,7 @@
 from easybuild.framework.easyconfig.tools import categorize_files_by_type, check_sha256_checksums, dep_graph
 from easybuild.framework.easyconfig.tools import find_related_easyconfigs, get_paths_for, parse_easyconfigs
 from easybuild.framework.easyconfig.tweak import obtain_ec_for, tweak_one
+from easybuild.framework.extension import resolve_exts_filter_template
 from easybuild.toolchains.system import SystemToolchain
 from easybuild.tools.build_log import EasyBuildError
 from easybuild.tools.config import module_classes
@@ -198,7 +199,7 @@ def test_validation(self):
         # introduce "TypeError: format requires mapping" issue"
         self.contents = self.contents.replace("syntax_error'", "foo = '%(name)s %s' % version")
         self.prep()
-        error_pattern = "Parsing easyconfig file failed: format requires a mapping \(line 8\)"
+        error_pattern = r"Parsing easyconfig file failed: format requires a mapping \(line 8\)"
         self.assertErrorRegex(EasyBuildError, error_pattern, EasyConfig, self.eb_file)
 
     def test_system_toolchain_constant(self):
@@ -854,7 +855,7 @@ def test_obtain_easyconfig(self):
         # hidden dependencies must be included in list of dependencies
         res = obtain_ec_for(specs, [self.test_prefix], None)
         self.assertEqual(res[0], True)
-        error_pattern = "Hidden deps with visible module names .* not in list of \(build\)dependencies: .*"
+        error_pattern = r"Hidden deps with visible module names .* not in list of \(build\)dependencies: .*"
         self.assertErrorRegex(EasyBuildError, error_pattern, EasyConfig, res[1])
         remove_file(res[1])
 
@@ -1559,7 +1560,6 @@ def test_quote_str(self):
             'foo': '"foo"',
             'foo\'bar': '"foo\'bar"',
             'foo\'bar"baz': '"""foo\'bar"baz"""',
-            "foo'bar\"baz": '"""foo\'bar"baz"""',
             "foo\nbar": '"foo\nbar"',
             'foo bar': '"foo bar"'
         }
@@ -2499,9 +2499,9 @@ def test_copy_easyconfigs(self):
         # verify whether copied easyconfig gets cleaned up (stripping out 'Built with' comment + build stats)
         txt = read_file(copied_toy_ec)
         regexs = [
-            "# Built with EasyBuild",
-            "# Build statistics",
-            "buildstats\s*=",
+            r"# Built with EasyBuild",
+            r"# Build statistics",
+            r"buildstats\s*=",
         ]
         for regex in regexs:
             regex = re.compile(regex, re.M)
@@ -2708,8 +2708,8 @@ def test_hidden_toolchain(self):
             '--dry-run',
         ]
         outtxt = self.eb_main(args, raise_error=True)
-        self.assertTrue(re.search('module: GCC/\.4\.9\.2', outtxt))
-        self.assertTrue(re.search('module: gzip/1\.6-GCC-4\.9\.2', outtxt))
+        self.assertTrue(re.search(r'module: GCC/\.4\.9\.2', outtxt))
+        self.assertTrue(re.search(r'module: gzip/1\.6-GCC-4\.9\.2', outtxt))
 
     def test_categorize_files_by_type(self):
         """Test categorize_files_by_type"""
@@ -2853,8 +2853,8 @@ def test_verify_easyconfig_filename(self):
         # incorrect spec
         specs['versionsuffix'] = ''
         error_pattern = "filename '%s' does not match with expected filename 'toy-0.0-gompi-2018a.eb' " % toy_ec_name
-        error_pattern += "\(specs: name: 'toy'; version: '0.0'; versionsuffix: ''; "
-        error_pattern += "toolchain name, version: 'gompi', '2018a'\)"
+        error_pattern += r"\(specs: name: 'toy'; version: '0.0'; versionsuffix: ''; "
+        error_pattern += r"toolchain name, version: 'gompi', '2018a'\)"
         self.assertErrorRegex(EasyBuildError, error_pattern, verify_easyconfig_filename, toy_ec, specs)
         specs['versionsuffix'] = '-test'
 
@@ -2863,8 +2863,8 @@ def test_verify_easyconfig_filename(self):
         toy_ec = os.path.join(self.test_prefix, 'toy.eb')
         write_file(toy_ec, toy_txt)
         error_pattern = "filename 'toy.eb' does not match with expected filename 'toy-0.0-gompi-2018a-test.eb' "
-        error_pattern += "\(specs: name: 'toy'; version: '0.0'; versionsuffix: '-test'; "
-        error_pattern += "toolchain name, version: 'gompi', '2018a'\)"
+        error_pattern += r"\(specs: name: 'toy'; version: '0.0'; versionsuffix: '-test'; "
+        error_pattern += r"toolchain name, version: 'gompi', '2018a'\)"
         self.assertErrorRegex(EasyBuildError, error_pattern, verify_easyconfig_filename, toy_ec, specs)
 
         # incorrect file contents
@@ -2998,7 +2998,7 @@ def test_check_sha256_checksums(self):
         toy_ec = os.path.join(test_ecs_dir, 't', 'toy', 'toy-0.0.eb')
         toy_ec_txt = read_file(toy_ec)
 
-        checksums_regex = re.compile('^checksums = \[\[(.|\n)*\]\]', re.M)
+        checksums_regex = re.compile(r'^checksums = \[\[(.|\n)*\]\]', re.M)
 
         # wipe out specified checksums, to make check fail
         test_ec = os.path.join(self.test_prefix, 'toy-0.0-fail.eb')
@@ -3013,7 +3013,7 @@ def test_check_sha256_checksums(self):
         self.assertTrue(res[0].startswith('Checksums missing for one or more sources/patches in toy-0.0-fail.eb'))
 
         # test use of whitelist regex patterns: check passes because easyconfig is whitelisted by filename
-        for regex in ['toy-.*', '.*-0\.0-fail\.eb']:
+        for regex in [r'toy-.*', r'.*-0\.0-fail\.eb']:
             res = check_sha256_checksums(ecs, whitelist=[regex])
             self.assertFalse(res)
 
@@ -3033,7 +3033,7 @@ def test_check_sha256_checksums(self):
         # re-test with right checksum in place
         toy_sha256 = '44332000aa33b99ad1e00cbd1a7da769220d74647060a10e807b916d73ea27bc'
         test_ec_txt = checksums_regex.sub('checksums = ["%s"]' % toy_sha256, toy_ec_txt)
-        test_ec_txt = re.sub('patches = \[(.|\n)*\]', '', test_ec_txt)
+        test_ec_txt = re.sub(r'patches = \[(.|\n)*\]', '', test_ec_txt)
 
         test_ec = os.path.join(self.test_prefix, 'toy-0.0-ok.eb')
         write_file(test_ec, test_ec_txt)
@@ -3557,6 +3557,55 @@ def test_unexpected_version_keys_caught(self):
 
             self.assertRaises(EasyBuildError, EasyConfig, test_ec)
 
+    def test_resolve_exts_filter_template(self):
+        class TestExtension(object):
+            def __init__(self, values):
+                self.name = values['name']
+                self.version = values.get('version')
+                self.src = values.get('src')
+                self.options = values.get('options', {})
+
+        error_msg = 'exts_filter should be a list or tuple'
+        self.assertErrorRegex(EasyBuildError, error_msg, resolve_exts_filter_template,
+                              '[ 1 == 1 ]', {})
+        self.assertErrorRegex(EasyBuildError, error_msg, resolve_exts_filter_template,
+                              ['[ 1 == 1 ]'], {})
+        self.assertErrorRegex(EasyBuildError, error_msg, resolve_exts_filter_template,
+                              ['[ 1 == 1 ]', 'true', 'false'], {})
+
+        test_cases = [
+            # Minimal case: just name
+            (['%(ext_name)s', None],
+             {'name': 'foo'},
+             ('foo', None),
+             ),
+            # Minimal case with input
+            (['%(ext_name)s', '>%(ext_name)s'],
+             {'name': 'foo'},
+             ('foo', '>foo'),
+             ),
+            # All values
+            (['%(ext_name)s-%(ext_version)s-%(src)s', '>%(ext_name)s-%(ext_version)s-%(src)s'],
+             {'name': 'foo', 'version': 42, 'src': 'bar.tgz'},
+             ('foo-42-bar.tgz', '>foo-42-bar.tgz'),
+             ),
+            # options dict is accepted
+            (['%(ext_name)s-%(ext_version)s-%(src)s', '>%(ext_name)s-%(ext_version)s-%(src)s'],
+             {'name': 'foo', 'version': 42, 'src': 'bar.tgz', 'options': {'dummy': 'value'}},
+             ('foo-42-bar.tgz', '>foo-42-bar.tgz'),
+             ),
+            # modulename overwrites name
+            (['%(ext_name)s-%(ext_version)s-%(src)s', '>%(ext_name)s-%(ext_version)s-%(src)s'],
+             {'name': 'foo', 'version': 42, 'src': 'bar.tgz', 'options': {'modulename': 'baz'}},
+             ('baz-42-bar.tgz', '>baz-42-bar.tgz'),
+             ),
+        ]
+        for exts_filter, ext, expected_value in test_cases:
+            value = resolve_exts_filter_template(exts_filter, ext)
+            self.assertEqual(value, expected_value)
+            value = resolve_exts_filter_template(exts_filter, TestExtension(ext))
+            self.assertEqual(value, expected_value)
+
 
 def suite():
     """ returns all the testcases in this module """