From 6889b79a338ebd7f24b6b9a5a7781d47828ad280 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 6 Jan 2023 11:23:59 +0100 Subject: [PATCH 1/4] Fix the checksum type check The `None` case was missed and due to the unrestricted `tuple` elem_type it may return valid for actually invalid entries. So restrict that beeing overly cautious so it may wrongly return invalid. But in that case the conversion function will be called which can do more elaborate verification. Add test checking for None in checksums. --- easybuild/framework/easyconfig/types.py | 24 +++++++++++++++++++----- test/framework/type_checking.py | 18 ++++++++++++++++-- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/easybuild/framework/easyconfig/types.py b/easybuild/framework/easyconfig/types.py index f902716dfa..0f7e0c8167 100644 --- a/easybuild/framework/easyconfig/types.py +++ b/easybuild/framework/easyconfig/types.py @@ -613,19 +613,33 @@ def ensure_iterable_license_specs(specs): })) # checksums is a list of checksums, one entry per file (source/patch) # each entry can be: +# None # a single checksum value (string) # a single checksum value of a specified type (2-tuple, 1st element is checksum type, 2nd element is checksum) # a list of checksums (of different types, perhaps different formats), which should *all* be valid -# a dictionary with a mapping from filename to checksum value -CHECKSUM_LIST = (list, as_hashable({'elem_types': [str, tuple, STRING_DICT]})) -CHECKSUMS = (list, as_hashable({'elem_types': [str, tuple, STRING_DICT, CHECKSUM_LIST]})) +# a tuple of checksums (of different types, perhaps different formats), where one should be valid +# a dictionary with a mapping from filename to checksum (None, value, type&value, alternatives) -CHECKABLE_TYPES = [CHECKSUM_LIST, CHECKSUMS, DEPENDENCIES, DEPENDENCY_DICT, LIST_OF_STRINGS, +# Type & value, value may be an int for type "size" +# This is a bit too permissive as it allows the first element to be an int and doesn't restrict the number of elements +CHECKSUM_TUPLE = (tuple, as_hashable({'elem_types': [str, int]})) +CHECKSUM_DICT = (dict, as_hashable( + { + 'elem_types': [type(None), str, CHECKSUM_TUPLE], + 'key_types': [str], + } +)) +CHECKSUM_LIST = (list, as_hashable({'elem_types': [str, CHECKSUM_TUPLE, CHECKSUM_DICT]})) + +CHECKSUMS = (list, as_hashable({'elem_types': [type(None), str, CHECKSUM_LIST, CHECKSUM_TUPLE, CHECKSUM_DICT]})) + +CHECKABLE_TYPES = [CHECKSUM_DICT, CHECKSUM_LIST, CHECKSUM_TUPLE, CHECKSUMS, + DEPENDENCIES, DEPENDENCY_DICT, LIST_OF_STRINGS, SANITY_CHECK_PATHS_DICT, SANITY_CHECK_PATHS_ENTRY, STRING_DICT, STRING_OR_TUPLE_LIST, STRING_OR_TUPLE_DICT, STRING_OR_TUPLE_OR_DICT_LIST, TOOLCHAIN_DICT, TUPLE_OF_STRINGS] # easy types, that can be verified with isinstance -EASY_TYPES = [str, bool, dict, int, list, str, tuple] +EASY_TYPES = [str, bool, dict, int, list, str, tuple, type(None)] # type checking is skipped for easyconfig parameters names not listed in PARAMETER_TYPES PARAMETER_TYPES = { diff --git a/test/framework/type_checking.py b/test/framework/type_checking.py index ded577ef69..eb549fb265 100644 --- a/test/framework/type_checking.py +++ b/test/framework/type_checking.py @@ -220,10 +220,24 @@ def test_check_type_of_param_value_checksums(self): {'foo.txt': sha256_checksum1, 'bar.txt': sha256_checksum2}, # 3 alternative checksums for a single file, one match is sufficient (sha256_checksum1, sha256_checksum2, sha256_checksum3), - ] + # two alternative checksums for a single file (not to be confused by checksum-type & -value tuple) + (sha256_checksum1, md5_checksum), + # three alternative checksums for a single file of different types + (sha256_checksum1, ('md5', md5_checksum), {'foo.txt': sha256_checksum1}), + # alternative checksums in dicts are also allowed + {'foo.txt': (sha256_checksum2, sha256_checksum3), 'bar.txt': (sha256_checksum1, md5_checksum)}, + # Same but with lists -> all must match for each file + {'foo.txt': [sha256_checksum2, sha256_checksum3], 'bar.txt': [sha256_checksum1, md5_checksum]}, + ], + # None is allowed, meaning skip the checksum + [ + None, + # Also in mappings + {'foo.txt': sha256_checksum1, 'bar.txt': None}, + ], ] for inp in inputs: - self.assertEqual(check_type_of_param_value('checksums', inp), (True, inp)) + self.assertTrue(check_type_of_param_value('checksums', inp), 'Failed for ' + str(inp)) def test_check_type_of_param_value_patches(self): """Test check_type_of_param_value function for patches.""" From 63867726bcb9dd0a3033be4866ab2f6fa7dc0ec9 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 17 Nov 2023 12:22:32 +0100 Subject: [PATCH 2/4] Fix assertion --- test/framework/type_checking.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/framework/type_checking.py b/test/framework/type_checking.py index eb549fb265..a1a4c7a8d7 100644 --- a/test/framework/type_checking.py +++ b/test/framework/type_checking.py @@ -237,7 +237,9 @@ def test_check_type_of_param_value_checksums(self): ], ] for inp in inputs: - self.assertTrue(check_type_of_param_value('checksums', inp), 'Failed for ' + str(inp)) + type_ok, newval = check_type_of_param_value('checksums', inp) + self.assertIs(type_ok, True, 'Failed for ' + str(inp)) + self.assertEqual(newval, inp) def test_check_type_of_param_value_patches(self): """Test check_type_of_param_value function for patches.""" From 86ec6d07645b196b958666888e62c13c249f5304 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 17 Nov 2023 12:25:59 +0100 Subject: [PATCH 3/4] Allow checksum+type in lists, tuples & dicts and disallow nested dicts --- easybuild/framework/easyconfig/types.py | 12 +++++++----- test/framework/type_checking.py | 17 ++++++++++------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/easybuild/framework/easyconfig/types.py b/easybuild/framework/easyconfig/types.py index 0f7e0c8167..bdae6ebca7 100644 --- a/easybuild/framework/easyconfig/types.py +++ b/easybuild/framework/easyconfig/types.py @@ -622,18 +622,20 @@ def ensure_iterable_license_specs(specs): # Type & value, value may be an int for type "size" # This is a bit too permissive as it allows the first element to be an int and doesn't restrict the number of elements -CHECKSUM_TUPLE = (tuple, as_hashable({'elem_types': [str, int]})) +CHECKSUM_AND_TYPE = (tuple, as_hashable({'elem_types': [str, int]})) +CHECKSUM_LIST = (list, as_hashable({'elem_types': [str, CHECKSUM_AND_TYPE]})) +CHECKSUM_TUPLE = (tuple, as_hashable({'elem_types': [str, CHECKSUM_AND_TYPE]})) CHECKSUM_DICT = (dict, as_hashable( { - 'elem_types': [type(None), str, CHECKSUM_TUPLE], + 'elem_types': [type(None), str, CHECKSUM_AND_TYPE, CHECKSUM_TUPLE, CHECKSUM_LIST], 'key_types': [str], } )) -CHECKSUM_LIST = (list, as_hashable({'elem_types': [str, CHECKSUM_TUPLE, CHECKSUM_DICT]})) -CHECKSUMS = (list, as_hashable({'elem_types': [type(None), str, CHECKSUM_LIST, CHECKSUM_TUPLE, CHECKSUM_DICT]})) +CHECKSUMS = (list, as_hashable({'elem_types': [type(None), str, CHECKSUM_AND_TYPE, + CHECKSUM_LIST, CHECKSUM_TUPLE, CHECKSUM_DICT]})) -CHECKABLE_TYPES = [CHECKSUM_DICT, CHECKSUM_LIST, CHECKSUM_TUPLE, CHECKSUMS, +CHECKABLE_TYPES = [CHECKSUM_AND_TYPE, CHECKSUM_LIST, CHECKSUM_TUPLE, CHECKSUM_DICT, CHECKSUMS, DEPENDENCIES, DEPENDENCY_DICT, LIST_OF_STRINGS, SANITY_CHECK_PATHS_DICT, SANITY_CHECK_PATHS_ENTRY, STRING_DICT, STRING_OR_TUPLE_LIST, STRING_OR_TUPLE_DICT, STRING_OR_TUPLE_OR_DICT_LIST, TOOLCHAIN_DICT, TUPLE_OF_STRINGS] diff --git a/test/framework/type_checking.py b/test/framework/type_checking.py index a1a4c7a8d7..77c0716bd9 100644 --- a/test/framework/type_checking.py +++ b/test/framework/type_checking.py @@ -174,10 +174,12 @@ def test_check_type_of_param_value_sanity_check_paths(self): def test_check_type_of_param_value_checksums(self): """Test check_type_of_param_value function for checksums.""" - md5_checksum = 'fa618be8435447a017fd1bf2c7ae9224' - sha256_checksum1 = 'fa618be8435447a017fd1bf2c7ae922d0428056cfc7449f7a8641edf76b48265' - sha256_checksum2 = 'b5f9cb06105c1d2d30719db5ffb3ea67da60919fb68deaefa583deccd8813551' - sha256_checksum3 = '033be54514a03e255df75c5aee8f9e672f663f93abb723444caec8fe43437bde' + # Using (actually invalid) prefix to better detect those in case of errors + md5_checksum = 'md518be8435447a017fd1bf2c7ae9224' + sha256_checksum1 = 'sha18be8435447a017fd1bf2c7ae922d0428056cfc7449f7a8641edf76b48265' + sha256_checksum2 = 'sha2cb06105c1d2d30719db5ffb3ea67da60919fb68deaefa583deccd8813551' + sha256_checksum3 = 'sha3e54514a03e255df75c5aee8f9e672f663f93abb723444caec8fe43437bde' + filesize = 45617379 # valid values for 'checksums' easyconfig parameters inputs = [ @@ -190,6 +192,7 @@ def test_check_type_of_param_value_checksums(self): # one checksum of specific type (as 2-tuple) [('md5', md5_checksum)], [('sha256', sha256_checksum1)], + [('size', filesize)], # alternative checksums for a single file (n-tuple) [(sha256_checksum1, sha256_checksum2)], [(sha256_checksum1, sha256_checksum2, sha256_checksum3)], @@ -213,17 +216,17 @@ def test_check_type_of_param_value_checksums(self): # two checksums for a single file, *both* should match [sha256_checksum1, md5_checksum], # three checksums for a single file, *all* should match - [sha256_checksum1, ('md5', md5_checksum), {'foo.txt': sha256_checksum1}], + [sha256_checksum1, ('md5', md5_checksum), ('size', filesize)], # single checksum for a single file sha256_checksum1, # filename-to-checksum mapping - {'foo.txt': sha256_checksum1, 'bar.txt': sha256_checksum2}, + {'foo.txt': sha256_checksum1, 'bar.txt': sha256_checksum2, 'baz.txt': ('size', filesize)}, # 3 alternative checksums for a single file, one match is sufficient (sha256_checksum1, sha256_checksum2, sha256_checksum3), # two alternative checksums for a single file (not to be confused by checksum-type & -value tuple) (sha256_checksum1, md5_checksum), # three alternative checksums for a single file of different types - (sha256_checksum1, ('md5', md5_checksum), {'foo.txt': sha256_checksum1}), + (sha256_checksum1, ('md5', md5_checksum), ('size', filesize)), # alternative checksums in dicts are also allowed {'foo.txt': (sha256_checksum2, sha256_checksum3), 'bar.txt': (sha256_checksum1, md5_checksum)}, # Same but with lists -> all must match for each file From 099fc5bec222e82e9f37374de2d1dc543c72096c Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 17 Nov 2023 14:43:15 +0100 Subject: [PATCH 4/4] Allow dicts of checksums outside of other dicts for backwards compatibility Not sure if that makes sense but at least for EB 4.x we need this. --- easybuild/framework/easyconfig/types.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/easybuild/framework/easyconfig/types.py b/easybuild/framework/easyconfig/types.py index bdae6ebca7..344c8ee136 100644 --- a/easybuild/framework/easyconfig/types.py +++ b/easybuild/framework/easyconfig/types.py @@ -631,11 +631,15 @@ def ensure_iterable_license_specs(specs): 'key_types': [str], } )) +# At the top-level we allow tuples/lists containing a dict +CHECKSUM_LIST_W_DICT = (list, as_hashable({'elem_types': [str, CHECKSUM_AND_TYPE, CHECKSUM_DICT]})) +CHECKSUM_TUPLE_W_DICT = (tuple, as_hashable({'elem_types': [str, CHECKSUM_AND_TYPE, CHECKSUM_DICT]})) CHECKSUMS = (list, as_hashable({'elem_types': [type(None), str, CHECKSUM_AND_TYPE, - CHECKSUM_LIST, CHECKSUM_TUPLE, CHECKSUM_DICT]})) + CHECKSUM_LIST_W_DICT, CHECKSUM_TUPLE_W_DICT, CHECKSUM_DICT]})) -CHECKABLE_TYPES = [CHECKSUM_AND_TYPE, CHECKSUM_LIST, CHECKSUM_TUPLE, CHECKSUM_DICT, CHECKSUMS, +CHECKABLE_TYPES = [CHECKSUM_AND_TYPE, CHECKSUM_LIST, CHECKSUM_TUPLE, + CHECKSUM_LIST_W_DICT, CHECKSUM_TUPLE_W_DICT, CHECKSUM_DICT, CHECKSUMS, DEPENDENCIES, DEPENDENCY_DICT, LIST_OF_STRINGS, SANITY_CHECK_PATHS_DICT, SANITY_CHECK_PATHS_ENTRY, STRING_DICT, STRING_OR_TUPLE_LIST, STRING_OR_TUPLE_DICT, STRING_OR_TUPLE_OR_DICT_LIST, TOOLCHAIN_DICT, TUPLE_OF_STRINGS]