From 2c11d214e35999fa066f02957ecb70babf4b4bff 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 42be1b99e4..ba81aeaccd 100644 --- a/easybuild/framework/easyconfig/types.py +++ b/easybuild/framework/easyconfig/types.py @@ -608,19 +608,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 = [string_type, bool, dict, int, list, str, tuple] +EASY_TYPES = [string_type, 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 8b7edd3215..1b6aaaa75d 100644 --- a/test/framework/type_checking.py +++ b/test/framework/type_checking.py @@ -221,10 +221,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 ececf17b3c8f1b33bb4370618d1b954569a2665d 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 1b6aaaa75d..41db011a49 100644 --- a/test/framework/type_checking.py +++ b/test/framework/type_checking.py @@ -238,7 +238,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 3651e51d4631ce323c78b4a8a2f5acd0d34c4bba 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 ba81aeaccd..ee1b2fe07f 100644 --- a/easybuild/framework/easyconfig/types.py +++ b/easybuild/framework/easyconfig/types.py @@ -617,18 +617,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 41db011a49..9d4fe4bc2c 100644 --- a/test/framework/type_checking.py +++ b/test/framework/type_checking.py @@ -175,10 +175,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 = [ @@ -191,6 +193,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)], @@ -214,17 +217,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 61f616e36f0096c5a04900e7af31fefe954377b4 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 ee1b2fe07f..ba0aecb559 100644 --- a/easybuild/framework/easyconfig/types.py +++ b/easybuild/framework/easyconfig/types.py @@ -626,11 +626,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]