Merge pull request #4070 from akesandgren/allow_dict_for_patches

boegel · web-flow · commit 83e68bd1958d · 2022-09-10T19:09:29.000+01:00
fix type-checking of patches to allow dict values + correctly handle patches specified as dict values in --new-pr
diff --git a/easybuild/framework/easyconfig/types.py b/easybuild/framework/easyconfig/types.py
@@ -349,6 +349,32 @@ def to_list_of_strings_and_tuples(spec):
     return str_tup_list
 
 
+def to_list_of_strings_and_tuples_and_dicts(spec):
+    """
+    Convert a 'list of dicts and tuples/lists and strings' to a 'list of dicts and tuples and strings'
+
+    Example:
+        ['foo', ['bar', 'baz']]
+        to
+        ['foo', ('bar', 'baz')]
+    """
+    str_tup_list = []
+
+    if not isinstance(spec, (list, tuple)):
+        raise EasyBuildError("Expected value to be a list, found %s (%s)", spec, type(spec))
+
+    for elem in spec:
+        if isinstance(elem, (string_type, tuple, dict)):
+            str_tup_list.append(elem)
+        elif isinstance(elem, list):
+            str_tup_list.append(tuple(elem))
+        else:
+            raise EasyBuildError("Expected elements to be of type string, tuple, dict or list, got %s (%s)",
+                                 elem, type(elem))
+
+    return str_tup_list
+
+
 def to_sanity_check_paths_dict(spec):
     """
     Convert a sanity_check_paths dict as received by yaml (a dict with list values that contain either lists or strings)
@@ -526,6 +552,7 @@ def ensure_iterable_license_specs(specs):
         'key_types': [str],
     }
 ))
+STRING_OR_TUPLE_OR_DICT_LIST = (list, as_hashable({'elem_types': [str, TUPLE_OF_STRINGS, STRING_DICT]}))
 SANITY_CHECK_PATHS_DICT = (dict, as_hashable({
     'elem_types': {
         SANITY_CHECK_PATHS_FILES: [STRING_OR_TUPLE_LIST],
@@ -544,7 +571,8 @@ def ensure_iterable_license_specs(specs):
 CHECKSUMS = (list, as_hashable({'elem_types': [str, tuple, STRING_DICT, CHECKSUM_LIST]}))
 
 CHECKABLE_TYPES = [CHECKSUM_LIST, CHECKSUMS, DEPENDENCIES, DEPENDENCY_DICT, LIST_OF_STRINGS,
-                   SANITY_CHECK_PATHS_DICT, STRING_DICT, STRING_OR_TUPLE_LIST, TOOLCHAIN_DICT, TUPLE_OF_STRINGS]
+                   SANITY_CHECK_PATHS_DICT, STRING_DICT, STRING_OR_TUPLE_LIST, STRING_OR_TUPLE_OR_DICT_LIST,
+                   TOOLCHAIN_DICT, TUPLE_OF_STRINGS]
 
 # easy types, that can be verified with isinstance
 EASY_TYPES = [string_type, bool, dict, int, list, str, tuple]
@@ -555,7 +583,7 @@ def ensure_iterable_license_specs(specs):
     'docurls': LIST_OF_STRINGS,
     'name': string_type,
     'osdependencies': STRING_OR_TUPLE_LIST,
-    'patches': STRING_OR_TUPLE_LIST,
+    'patches': STRING_OR_TUPLE_OR_DICT_LIST,
     'sanity_check_paths': SANITY_CHECK_PATHS_DICT,
     'toolchain': TOOLCHAIN_DICT,
     'version': string_type,
@@ -575,4 +603,5 @@ def ensure_iterable_license_specs(specs):
     TOOLCHAIN_DICT: to_toolchain_dict,
     SANITY_CHECK_PATHS_DICT: to_sanity_check_paths_dict,
     STRING_OR_TUPLE_LIST: to_list_of_strings_and_tuples,
+    STRING_OR_TUPLE_OR_DICT_LIST: to_list_of_strings_and_tuples_and_dicts,
 }
diff --git a/easybuild/tools/github.py b/easybuild/tools/github.py
@@ -1800,6 +1800,15 @@ def new_pr(paths, ecs, title=None, descr=None, commit_msg=None):
             for patch in ec.asdict()['patches']:
                 if isinstance(patch, tuple):
                     patch = patch[0]
+                elif isinstance(patch, dict):
+                    patch_info = {}
+                    for key in patch.keys():
+                        patch_info[key] = patch[key]
+                    if 'name' not in patch_info.keys():
+                        raise EasyBuildError("Wrong patch spec '%s', when using a dict 'name' entry must be supplied",
+                                             str(patch))
+                    patch = patch_info['name']
+
                 if patch not in paths['patch_files'] and not os.path.isfile(os.path.join(os.path.dirname(ec_path),
                                                                             patch)):
                     print_warning("new patch file %s, referenced by %s, is not included in this PR" %
diff --git a/test/framework/options.py b/test/framework/options.py
@@ -4390,6 +4390,53 @@ def test_github_new_update_pr(self):
         ]
         self._assert_regexs(regexs, txt, assert_true=False)
 
+    def test_new_pr_warning_missing_patch(self):
+        """Test warning printed by --new-pr (dry run only) when a specified patch file could not be found."""
+
+        if self.github_token is None:
+            print("Skipping test_new_pr_warning_missing_patch, no GitHub token available?")
+            return
+
+        topdir = os.path.dirname(os.path.abspath(__file__))
+        test_ecs = os.path.join(topdir, 'easyconfigs', 'test_ecs')
+        test_ec = os.path.join(self.test_prefix, 'test.eb')
+        copy_file(os.path.join(test_ecs, 't', 'toy', 'toy-0.0-gompi-2018a-test.eb'), test_ec)
+
+        patches_regex = re.compile(r'^patches = .*', re.M)
+        test_ec_txt = read_file(test_ec)
+
+        patch_fn = 'this_patch_does_not_exist.patch'
+        test_ec_txt = patches_regex.sub('patches = ["%s"]' % patch_fn, test_ec_txt)
+        write_file(test_ec, test_ec_txt)
+
+        new_pr_out_regex = re.compile(r"Opening pull request", re.M)
+        warning_regex = re.compile("new patch file %s, referenced by .*, is not included in this PR" % patch_fn, re.M)
+
+        args = [
+            '--new-pr',
+            '--github-user=%s' % GITHUB_TEST_ACCOUNT,
+            test_ec,
+            '-D',
+        ]
+        stdout, stderr = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False)
+
+        new_pr_out_error_msg = "Pattern '%s' should be found in: %s" % (new_pr_out_regex.pattern, stdout)
+        self.assertTrue(new_pr_out_regex.search(stdout), new_pr_out_error_msg)
+
+        warning_error_msg = "Pattern '%s' should be found in: %s" % (warning_regex.pattern, stderr)
+        self.assertTrue(warning_regex.search(stderr), warning_error_msg)
+
+        # try again with patch specified via a dict value
+        test_ec_txt = patches_regex.sub('patches = [{"name": "%s", "alt_location": "foo"}]' % patch_fn, test_ec_txt)
+        write_file(test_ec, test_ec_txt)
+
+        stdout, stderr = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False)
+
+        new_pr_out_error_msg = "Pattern '%s' should be found in: %s" % (new_pr_out_regex.pattern, stdout)
+        self.assertTrue(new_pr_out_regex.search(stdout), new_pr_out_error_msg)
+        warning_error_msg = "Pattern '%s' should be found in: %s" % (warning_regex.pattern, stderr)
+        self.assertTrue(warning_regex.search(stderr), warning_error_msg)
+
     def test_github_sync_pr_with_develop(self):
         """Test use of --sync-pr-with-develop (dry run only)."""
         if self.github_token is None:
diff --git a/test/framework/type_checking.py b/test/framework/type_checking.py
@@ -37,8 +37,9 @@
 from easybuild.framework.easyconfig.types import LIST_OF_STRINGS, SANITY_CHECK_PATHS_DICT, STRING_OR_TUPLE_LIST
 from easybuild.framework.easyconfig.types import TOOLCHAIN_DICT
 from easybuild.framework.easyconfig.types import is_value_of_type, to_checksums, to_dependencies, to_dependency
-from easybuild.framework.easyconfig.types import to_list_of_strings, to_list_of_strings_and_tuples, to_toolchain_dict
-from easybuild.framework.easyconfig.types import to_sanity_check_paths_dict
+from easybuild.framework.easyconfig.types import to_list_of_strings, to_list_of_strings_and_tuples
+from easybuild.framework.easyconfig.types import to_list_of_strings_and_tuples_and_dicts
+from easybuild.framework.easyconfig.types import to_sanity_check_paths_dict, to_toolchain_dict
 from easybuild.tools.build_log import EasyBuildError
 from easybuild.tools.py2vs3 import string_type
 
@@ -225,6 +226,25 @@ def test_check_type_of_param_value_checksums(self):
         for inp in inputs:
             self.assertEqual(check_type_of_param_value('checksums', inp), (True, inp))
 
+    def test_check_type_of_param_value_patches(self):
+        """Test check_type_of_param_value function for patches."""
+
+        # patches values that do not need to be converted
+        inputs = (
+            [],  # empty list of patches
+            # single patch, different types
+            ['foo.patch'],  # only filename
+            [('foo.patch', '1')],  # filename + patch level
+            [('foo.patch', 'subdir')],  # filename + subdir to apply patch in
+            [{'name': 'foo.patch', 'level': '1'}],  # filename + patch level, as dict value
+            # multiple patches, mix of different types
+            ['1.patch', '2.patch', '3.patch'],
+            ['1.patch', ('2.patch', '2'), {'name': '3.patch'}],
+            ['1.patch', {'name': '2.patch', 'level': '2'}, ('3.patch', '3')],
+        )
+        for inp in inputs:
+            self.assertEqual(check_type_of_param_value('patches', inp), (True, inp))
+
     def test_convert_value_type(self):
         """Test convert_value_type function."""
         # to string
@@ -614,11 +634,47 @@ def test_to_list_of_strings_and_tuples(self):
         self.assertEqual(to_list_of_strings_and_tuples(('foo', ['bar', 'baz'])), ['foo', ('bar', 'baz')])
 
         # conversion failures
-        self.assertErrorRegex(EasyBuildError, "Expected value to be a list", to_list_of_strings_and_tuples, 'foo')
+        error_regex = "Expected value to be a list"
+        self.assertErrorRegex(EasyBuildError, error_regex, to_list_of_strings_and_tuples, 'foo')
+        self.assertErrorRegex(EasyBuildError, error_regex, to_list_of_strings_and_tuples, 1)
+        self.assertErrorRegex(EasyBuildError, error_regex, to_list_of_strings_and_tuples, {'foo': 'bar'})
         error_msg = "Expected elements to be of type string, tuple or list"
         self.assertErrorRegex(EasyBuildError, error_msg, to_list_of_strings_and_tuples, ['foo', 1])
         self.assertErrorRegex(EasyBuildError, error_msg, to_list_of_strings_and_tuples, (1,))
 
+    def test_to_list_of_strings_and_tuples_and_dicts(self):
+        """Test to_list_of_strings_and_tuples_and_dicts function."""
+
+        # no conversion, already right type
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts([]), [])
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts([()]), [()])
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts(['foo']), ['foo'])
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts([{}]), [{}])
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts([('foo', 'bar')]), [('foo', 'bar')])
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts([('foo', 'bar'), 'baz']), [('foo', 'bar'), 'baz'])
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts([('x',), 'y', {'z': 1}]), [('x',), 'y', {'z': 1}])
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts(['y', {'z': 1}]), ['y', {'z': 1}])
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts([{'z': 1}, ('x',)]), [{'z': 1}, ('x',)])
+
+        # actual conversion
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts(()), [])
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts(('foo',)), ['foo'])
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts([['bar', 'baz']]), [('bar', 'baz')])
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts((['bar', 'baz'],)), [('bar', 'baz')])
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts(['foo', ['bar', 'baz']]), ['foo', ('bar', 'baz')])
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts(('foo', ['bar', 'baz'])), ['foo', ('bar', 'baz')])
+        self.assertEqual(to_list_of_strings_and_tuples_and_dicts(('x', ['y'], {'z': 1})), ['x', ('y',), {'z': 1}])
+
+        # conversion failures
+        error_regex = "Expected value to be a list"
+        self.assertErrorRegex(EasyBuildError, error_regex, to_list_of_strings_and_tuples_and_dicts, 'foo')
+        self.assertErrorRegex(EasyBuildError, error_regex, to_list_of_strings_and_tuples_and_dicts, 1)
+        self.assertErrorRegex(EasyBuildError, error_regex, to_list_of_strings_and_tuples_and_dicts, {'foo': 'bar'})
+        error_msg = "Expected elements to be of type string, tuple, dict or list"
+        self.assertErrorRegex(EasyBuildError, error_msg, to_list_of_strings_and_tuples_and_dicts, ['foo', 1])
+        self.assertErrorRegex(EasyBuildError, error_msg, to_list_of_strings_and_tuples_and_dicts, (1,))
+        self.assertErrorRegex(EasyBuildError, error_msg, to_list_of_strings_and_tuples_and_dicts, (1, {'foo': 'bar'}))
+
     def test_to_sanity_check_paths_dict(self):
         """Test to_sanity_check_paths_dict function."""
         # no conversion, already right type
