warn if non-single-letter local variables not named 'local_*' or '_*' are used + enhance --fix-deprecated-easyconfigs to rename them

easybuild/framework/easyconfig/easyconfig.py

@@ -87,6 +87,9 @@
 # name of easyconfigs archive subdirectory
 EASYCONFIGS_ARCHIVE_DIR = '__archive__'
 
+# prefix for names of local variables in easyconfig files
+LOCAL_VAR_PREFIX = 'local_'
+
 
 try:
     import autopep8
@@ -116,6 +119,44 @@ def new_ec_method(self, key, *args, **kwargs):
     return new_ec_method
 
 
+def triage_easyconfig_params(variables, ec):
+    """
+    Triage supplied variables into known easyconfig parameters and other variables.
+
+    Unknown easyconfig parameters that have a single-letter name, or of which the name starts with 'local_'
+    are considered to be local variables.
+
+    :param variables: dictionary with names/values of variables that should be triaged
+    :param ec: dictionary with set of known easyconfig parameters
+
+    :return: 2-tuple with dict of names/values for known easyconfig parameters + unknown (non-local) variables
+    """
+    ec_params, unknown_keys = {}, []
+
+    for key in variables:
+        # validations are skipped, just set in the config
+        if key in ec.keys():
+            ec_params[key] = variables[key]
+            _log.info("setting config option %s: value %s (type: %s)", key, ec_params[key], type(ec_params[key]))
+        elif key in REPLACED_PARAMETERS:
+            _log.nosupport("Easyconfig parameter '%s' is replaced by '%s'" % (key, REPLACED_PARAMETERS[key]), '2.0')
+
+        # anything else is considered to be a local variable in the easyconfig file;
+        # to catch mistakes (using unknown easyconfig parameters),
+        # and to protect against using a local variable name that may later become a known easyconfig parameter,
+        # we require that non-single letter names of local variables start with 'local_'
+        elif key.startswith(LOCAL_VAR_PREFIX):
+            _log.debug("Ignoring local variable '%s' (value: %s)", key, variables[key])
+
+        # __builtins__ is always defined as a 'local' variables
+        # single-letter local variable names are allowed (mainly for use in list comprehensions)
+        # in Python 2, variables defined in list comprehensions leak to the outside (no longer the case in Python 3)
+        elif len(key) > 1 and key not in ['__builtins__']:
+            unknown_keys.append(key)
+
+    return ec_params, unknown_keys
+
+
 def toolchain_hierarchy_cache(func):
     """Function decorator to cache (and retrieve cached) toolchain hierarchy queries."""
     cache = {}
@@ -391,6 +432,14 @@ def __init__(self, path, extra_options=None, build_specs=None, validate=True, hi
         self.build_specs = build_specs
         self.parse()
 
+        if self.unknown_keys:
+            cnt = len(self.unknown_keys)
+            unknown_keys_msg = ', '.join(sorted(self.unknown_keys))
+            msg = "Use of %d unknown easyconfig parameters detected: %s\n" % (cnt, unknown_keys_msg)
+            msg += "If these are just local variables please rename them to start with '%s', " % LOCAL_VAR_PREFIX
+            msg += "or try using --fix-deprecated-easyconfigs to do this automatically."
+            print_warning(msg, silent=build_option('silent'))
+
         # check whether this easyconfig file is deprecated, and act accordingly if so
         self.check_deprecated(self.path)
 
@@ -481,6 +530,22 @@ def update(self, key, value, allow_duplicate=True):
         else:
             raise EasyBuildError("Can't update configuration value for %s, because it's not a string or list.", key)
 
+    def set_keys(self, params):
+        """
+        Set keys in this EasyConfig instance based on supplied easyconfig parameter values.
+
+        If any unknown easyconfig parameters are encountered here, an error is raised.
+
+        :param params: a dict value with names/values of easyconfig parameters to set
+        """
+        for key in params:
+            # validations are skipped, just set in the config
+            if key in self._config.keys():
+                self[key] = params[key]
+                self.log.info("setting easyconfig parameter %s: value %s (type: %s)", key, self[key], type(self[key]))
+            else:
+                raise EasyBuildError("Unknown easyconfig parameter: %s (value '%s')", key, params[key])
+
     def parse(self):
         """
         Parse the file and set options
@@ -516,18 +581,10 @@ def parse(self):
             raise EasyBuildError("You may have some typos in your easyconfig file: %s",
                                  ', '.join(["%s -> %s" % typo for typo in typos]))
 
-        # we need toolchain to be set when we call _parse_dependency
-        for key in ['toolchain'] + list(local_vars):
-            # validations are skipped, just set in the config
-            if key in self._config.keys():
-                self[key] = local_vars[key]
-                self.log.info("setting config option %s: value %s (type: %s)", key, self[key], type(self[key]))
-            elif key in REPLACED_PARAMETERS:
-                _log.nosupport("Easyconfig parameter '%s' is replaced by '%s'" % (key, REPLACED_PARAMETERS[key]), '2.0')
+        # set keys in current EasyConfig instance based on dict obtained by parsing easyconfig file
+        known_ec_params, self.unknown_keys = triage_easyconfig_params(local_vars, self._config)
 
-            # do not store variables we don't need
-            else:
-                self.log.debug("Ignoring unknown easyconfig parameter %s (value: %s)" % (key, local_vars[key]))
+        self.set_keys(known_ec_params)
 
         # templating is disabled when parse_hook is called to allow for easy updating of mutable easyconfig parameters
         # (see also comment in resolve_template)
@@ -2063,6 +2120,12 @@ def fix_deprecated_easyconfigs(paths):
             ectxt = dummy_tc_regex.sub("toolchain = SYSTEM", ectxt)
             fixed = True
 
+        # fix use of local variables with a name other than a single letter or 'local_*'
+        ec = EasyConfig(path)
+        for key in ec.unknown_keys:
+            regexp = re.compile(key, re.M)  # FIXME avoid replacing partial matches!
+            ectxt = regexp.sub('local_' + key, ectxt)
+
         if fixed:
             fixed_cnt += 1
             backup_path = find_backup_name_candidate(path + '.orig')

test/framework/easyconfig.py

@@ -664,7 +664,7 @@ def test_obtain_easyconfig(self):
                 'homepage = "http://example.com"',
                 'description = "test easyconfig"',
                 'toolchain = {"name": "%s", "version": "%s"}' % (tcname, tcver),
-                'foo_extra1 = "bar"',
+                'local_foo_extra1 = "bar"',
             ]))
         ]
 
@@ -1344,6 +1344,7 @@ def foo(key):
 
         easyconfig.parser.DEPRECATED_PARAMETERS = orig_deprecated_parameters
         reload(easyconfig.parser)
+        reload(easyconfig.easyconfig)
         easyconfig.easyconfig.EasyConfig = orig_EasyConfig
         easyconfig.easyconfig.ActiveMNS = orig_ActiveMNS
 
@@ -2347,7 +2348,7 @@ def test_hidden_toolchain(self):
             ec_file,
             '--dry-run',
         ]
-        outtxt = self.eb_main(args)
+        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))
 
@@ -2859,12 +2860,23 @@ def test_fix_deprecated_easyconfigs(self):
         """Test fix_deprecated_easyconfigs function."""
         test_ecs_dir = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'easyconfigs', 'test_ecs')
         toy_ec = os.path.join(test_ecs_dir, 't', 'toy', 'toy-0.0.eb')
-        toy_ectxt = read_file(toy_ec)
         gzip_ec = os.path.join(test_ecs_dir, 'g', 'gzip', 'gzip-1.4.eb')
 
+        gzip_ec_txt = read_file(gzip_ec)
+        toy_ec_txt = read_file(toy_ec)
+
         test_ec = os.path.join(self.test_prefix, 'test.eb')
-        tc_regex = re.compile('^toolchain = .*', re.M)
 
+        test_ectxt = toy_ec_txt
+        # inject local variables with names that need to be tweaked (or not for single-letter ones)
+        regex = re.compile('^(sanity_check_paths)', re.M)
+        test_ectxt = regex.sub(r'foobar = "foobar"\n\n\1', toy_ec_txt)
+        regex = re.compile('^(toolchain\s*=.*)$', re.M)
+        test_ectxt = regex.sub(r'\1\n\nsome_list = [x + "1" for x in ["one", "two", "three"]]', test_ectxt)
+        print test_ectxt
+
+        # test fixing the use of 'dummy' toolchain to SYSTEM
+        tc_regex = re.compile('^toolchain = .*', re.M)
         tc_strs = [
             "{'name': 'dummy', 'version': 'dummy'}",
             "{'name': 'dummy', 'version': ''}",
@@ -2874,12 +2886,16 @@ def test_fix_deprecated_easyconfigs(self):
         ]
 
         for tc_str in tc_strs:
-            write_file(test_ec, tc_regex.sub("toolchain = %s" % tc_str, toy_ectxt))
+            write_file(test_ec, tc_regex.sub("toolchain = %s" % tc_str, test_ectxt))
 
             test_ec_txt = read_file(test_ec)
             regex = re.compile("^toolchain = {.*'name': 'dummy'.*$", re.M)
             self.assertTrue(regex.search(test_ec_txt), "Pattern '%s' found in: %s" % (regex.pattern, test_ec_txt))
 
+            # easyconfig doesn't parse because of local variables with name other than 'local_*'
+            error_pattern = "Use of 2 unknown easyconfig parameters detected: foobar, some_list"
+            self.assertErrorRegex(EasyBuildError, error_pattern, EasyConfig, test_ec)
+
             self.mock_stderr(True)
             self.mock_stdout(True)
             fix_deprecated_easyconfigs([toy_ec, test_ec, gzip_ec])
@@ -2892,6 +2908,16 @@ def test_fix_deprecated_easyconfigs(self):
             regex = re.compile("^toolchain = SYSTEM$", re.M)
             self.assertTrue(regex.search(ectxt), "Pattern '%s' found in: %s" % (regex.pattern, ectxt))
 
+            self.assertEqual(gzip_ec_txt, read_file(gzip_ec))
+            self.assertEqual(toy_ec_txt, read_file(toy_ec))
+            self.assertTrue(test_ec_txt != read_file(test_ec))
+
+            # original easyconfig is backed up automatically
+            test_ecs = sorted([f for f in os.listdir(self.test_prefix) if f.startswith('test.eb')])
+            self.assertEqual(len(test_ecs), 2)
+            self.assertEqual(test_ec_txt, read_file(os.path.join(self.test_prefix, test_ecs[1])))
+
+            # parsing works now, toolchain is replaced with system toolchain
             ec = EasyConfig(test_ec)
             self.assertTrue(ec['toolchain'], {'name': 'system', 'version': 'system'})
 
@@ -2921,13 +2947,13 @@ def test_parse_list_comprehension_scope(self):
             "homepage = 'https://example.com'",
             "description = 'test'",
             "toolchain = SYSTEM",
-            "bindir = 'bin'",
-            "binaries = ['foo', 'bar']",
+            "local_bindir = 'bin'",
+            "local_binaries = ['foo', 'bar']",
             "sanity_check_paths = {",
             # using local variable 'bindir' in list comprehension in sensitive w.r.t. scope,
             # especially in Python 3 where list comprehensions have their own scope
             # cfr. https://github.com/easybuilders/easybuild-easyconfigs/pull/7848
-            "   'files': ['%s/%s' % (bindir, x) for x in binaries],",
+            "   'files': ['%s/%s' % (local_bindir, x) for x in local_binaries],",
             "   'dirs': [],",
             "}",
         ])
@@ -2939,6 +2965,37 @@ def test_parse_list_comprehension_scope(self):
         }
         self.assertEqual(ec['sanity_check_paths'], expected_sanity_check_paths)
 
+    def test_local_vars_detection(self):
+        """Test detection of using unknown easyconfig parameters that are likely local variables."""
+
+        test_ec = os.path.join(self.test_prefix, 'test.eb')
+        test_ectxt = '\n'.join([
+            "easyblock = 'ConfigureMake'",
+            "name = 'test'",
+            "version = '1.2.3'",
+            "homepage = 'https://example.com'",
+            "description = 'test'",
+            "toolchain = SYSTEM",
+            "foobar = 'xxx'",
+        ])
+        write_file(test_ec, test_ectxt)
+        expected_error = "Use of 1 unknown easyconfig parameters detected: foobar"
+        self.assertErrorRegex(EasyBuildError, expected_error, EasyConfig, test_ec)
+
+        # all unknown keys are detected at once, and reported alphabetically
+        # single-letter local variables are not a problem
+        test_ectxt = '\n'.join([
+            'zzz_test = ["one", "two"]',
+            test_ectxt,
+            'a = "blah"',
+            'test_list = [x for x in ["1", "2", "3"]]',
+            'an_unknown_key = 123',
+        ])
+        write_file(test_ec, test_ectxt)
+
+        expected_error = "Use of 4 unknown easyconfig parameters detected: an_unknown_key, foobar, test_list, zzz_test"
+        self.assertErrorRegex(EasyBuildError, expected_error, EasyConfig, test_ec)
+
 
 def suite():
     """ returns all the testcases in this module """

test/framework/easyconfigs/test_ecs/f/FFTW/FFTW-3.3.3-GCC-4.8.2-serial.eb

@@ -14,13 +14,13 @@ toolchainopts = {'optarch': True, 'pic': True}
 sources = [SOURCELOWER_TAR_GZ]
 source_urls = [homepage]
 
-common_configopts = "--enable-threads --enable-openmp --with-pic"
+local_common_configopts = "--enable-threads --enable-openmp --with-pic"
 
 configopts = [
-    common_configopts + " --enable-single --enable-sse2 --enable-mpi",
-    common_configopts + " --enable-long-double --enable-mpi",
-    common_configopts + " --enable-quad-precision",
-    common_configopts + " --enable-sse2 --enable-mpi",  # default as last
+    local_common_configopts + " --enable-single --enable-sse2 --enable-mpi",
+    local_common_configopts + " --enable-long-double --enable-mpi",
+    local_common_configopts + " --enable-quad-precision",
+    local_common_configopts + " --enable-sse2 --enable-mpi",  # default as last
 ]
 
 sanity_check_paths = {

test/framework/easyconfigs/test_ecs/f/FFTW/FFTW-3.3.7-GCC-6.4.0-2.28-serial.eb

@@ -14,13 +14,13 @@ toolchainopts = {'optarch': True, 'pic': True}
 sources = [SOURCELOWER_TAR_GZ]
 source_urls = [homepage]
 
-common_configopts = "--enable-openmp --with-pic"
+local_common_configopts = "--enable-openmp --with-pic"
 
 configopts = [
-    common_configopts + " --enable-single --enable-sse2",
-    common_configopts + " --enable-long-double",
-    common_configopts + " --enable-quad-precision",
-    common_configopts + " --enable-sse2",  # default as last
+    local_common_configopts + " --enable-single --enable-sse2",
+    local_common_configopts + " --enable-long-double",
+    local_common_configopts + " --enable-quad-precision",
+    local_common_configopts + " --enable-sse2",  # default as last
 ]
 
 sanity_check_paths = {

test/framework/easyconfigs/test_ecs/f/FFTW/FFTW-3.3.7-gompi-2018a.eb

@@ -13,13 +13,13 @@ toolchainopts = {'optarch': True, 'pic': True}
 sources = [SOURCELOWER_TAR_GZ]
 source_urls = [homepage]
 
-common_configopts = "--enable-openmp --with-pic"
+local_common_configopts = "--enable-openmp --with-pic"
 
 configopts = [
-    common_configopts + " --enable-single --enable-sse2 --enable-mpi",
-    common_configopts + " --enable-long-double --enable-mpi",
-    common_configopts + " --enable-quad-precision",
-    common_configopts + " --enable-sse2 --enable-mpi",  # default as last
+    local_common_configopts + " --enable-single --enable-sse2 --enable-mpi",
+    local_common_configopts + " --enable-long-double --enable-mpi",
+    local_common_configopts + " --enable-quad-precision",
+    local_common_configopts + " --enable-sse2 --enable-mpi",  # default as last
 ]
 
 sanity_check_paths = {

test/framework/easyconfigs/test_ecs/f/FFTW/FFTW-3.3.7-gompic-2018a.eb

@@ -13,13 +13,13 @@ toolchainopts = {'optarch': True, 'pic': True}
 sources = [SOURCELOWER_TAR_GZ]
 source_urls = [homepage]
 
-common_configopts = "--enable-threads --enable-openmp --with-pic"
+local_common_configopts = "--enable-threads --enable-openmp --with-pic"
 
 configopts = [
-    common_configopts + " --enable-single --enable-sse2 --enable-mpi",
-    common_configopts + " --enable-long-double --enable-mpi",
-    common_configopts + " --enable-quad-precision",
-    common_configopts + " --enable-sse2 --enable-mpi",  # default as last
+    local_common_configopts + " --enable-single --enable-sse2 --enable-mpi",
+    local_common_configopts + " --enable-long-double --enable-mpi",
+    local_common_configopts + " --enable-quad-precision",
+    local_common_configopts + " --enable-sse2 --enable-mpi",  # default as last
 ]
 
 sanity_check_paths = {

test/framework/easyconfigs/test_ecs/f/foss/foss-2018a.eb

@@ -9,27 +9,23 @@ OpenMPI for MPI support, OpenBLAS (BLAS and LAPACK support), FFTW and ScaLAPACK.
 
 toolchain = SYSTEM
 
-comp_name = 'GCC'
-comp_version = '6.4.0-2.28'
-comp = (comp_name, comp_version)
+local_comp = ('GCC', '6.4.0-2.28')
 
-blaslib = 'OpenBLAS'
-blasver = '0.2.20'
+local_blaslib = 'OpenBLAS'
+local_blasver = '0.2.20'
 
 # toolchain used to build foss dependencies
-comp_mpi_tc_name = 'gompi'
-comp_mpi_tc_ver = "%s" % version
-comp_mpi_tc = (comp_mpi_tc_name, comp_mpi_tc_ver)
+local_comp_mpi_tc = ('gompi', version)
 
 # compiler toolchain dependencies
 # we need GCC and OpenMPI as explicit dependencies instead of gompi toolchain
 # because of toolchain preperation functions
 dependencies = [
-    comp,
-    ('OpenMPI', '2.1.2', '', comp),  # part of gompi-2018a
-    (blaslib, blasver, '', comp),
-    ('FFTW', '3.3.7', '', comp_mpi_tc),
-    ('ScaLAPACK', '2.0.2', '-%s-%s' % (blaslib, blasver), comp_mpi_tc)
+    local_comp,
+    ('OpenMPI', '2.1.2', '', local_comp),  # part of gompi-2018a
+    (local_blaslib, local_blasver, '', local_comp),
+    ('FFTW', '3.3.7', '', local_comp_mpi_tc),
+    ('ScaLAPACK', '2.0.2', '-%s-%s' % (local_blaslib, local_blasver), local_comp_mpi_tc)
 ]
 
 moduleclass = 'toolchain'

test/framework/easyconfigs/test_ecs/g/GCC/GCC-4.6.3.eb

@@ -25,7 +25,8 @@ source_urls = [
     'http://www.multiprecision.org/mpc/download',  # MPC official
 ]
 
-languages = ['c', 'c++', 'fortran', 'lto']
+# commented out 'languages' setting since dummy GCC easyblock doesn't define this as a known easyconfig parameter
+# languages = ['c', 'c++', 'fortran', 'lto']
 
 # compiler class
 moduleclass = 'compiler'