Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix to_checksums with None values in dicts and recursion #4159

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Jan 4, 2023

Having a 'src': None entry in a dict for checksums is as valid as having a None entry directly in the list. However the current function didn't handle it and crashed.
Fix that as well as a few corner cases especially in the recursive case by introducing a new function for handling checksum entries in the checksum list and limiting the recursiveness.

It will now throw an EasyBuildError similar to e.g. to_dependency instead of trying to continue and fail at a random other place.
I extended the testcases to cover the original issue (None as a value in a dict) and some more correct and error cases

Please carefully review and check if you can come up with any valid or invalid checksums entry that might break using the new function. Especially regarding the YEB compatibility this was pretty hard as for lists of strings you cannot know if those are 2 checksums without a type that should both match or a checksum and type. In EC-files (Python) one can use a tuple for that but then again it is hard to tell if that tuple is a checksum and type or 2 alternative checksums.

Fixes #4142

@Flamefire
Copy link
Contributor Author

I traced the failure to a difference in YEB and EB formats and the validator which seemingly has a bug:

The toy-0.0.* contains a single source with alternative checksums:

checksums: [[
    'be662daa971a640e40be5c804d9d7d10',  # default [MD5]
    '44332000aa33b99ad1e00cbd1a7da769220d74647060a10e807b916d73ea27bc',  # default (SHA256)
    ['adler32', '0x998410035'],
    ['crc32', '0x1553842328'],
    ['md5', 'be662daa971a640e40be5c804d9d7d10'],
    ['sha1', 'f618096c52244539d0e89867405f573fdb0b55b0'],
    ['size', 273],
]]

The difference is:

  • YEB: `[['be...', '44...', [...], [...], [...], [...], [...]]]``
  • EC: [['be...', '44...', (...), (...), (...), (...), (...)]]

I.e. the EC correctly uses tuples while the YEB uses lists. However actually both are wrong and need to be converted because alternative checksums must be specified as a tuple:

elif isinstance(checksum, tuple):

However the current definition of the checksums type is:

CHECKSUM_LIST = (list, as_hashable({'elem_types': [str, tuple, STRING_DICT]}))
CHECKSUMS = (list, as_hashable({'elem_types': [str, tuple, STRING_DICT, CHECKSUM_LIST]}))

I.e. the we wrongly allow a list where we expect a tuple.
Furthermore this doesn't cover all possible use cases (e.g. dicts with None entries or alternatives) and allows wrong entries, e.g. tuples of any types. As I don't see that it is possible to express all valid entries I'd purposely restrict this to less than is possible and make use of to_checksums to do the final verification.

@boegel
Copy link
Member

boegel commented May 23, 2023

@Flamefire This needs another sync with develop after merge of #4255

@boegel boegel modified the milestones: 4.9.2, release after 4.9.2 Jun 6, 2024
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.
Having a `'src': None` entry in a dict for checksums is as valid as
having a `None` entry directly in the list. However the current function
didn't handle it and crashed.
Fix that as well as a few corner cases especially in the recursive case
by introducing a new function for handling checksum entries in the
checksum list and limiting the recursiveness.

Fixes easybuilders#4142
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

checksum dict with a checksum value of None doesn't work
2 participants