enhance documentation of checksums easyconfig parameter#853
Closed
Flamefire wants to merge 1 commit intoeasybuilders:developfrom
Closed
enhance documentation of checksums easyconfig parameter#853Flamefire wants to merge 1 commit intoeasybuilders:developfrom
Flamefire wants to merge 1 commit intoeasybuilders:developfrom
Conversation
Add more examples of which formats are possible. Also contains not-yet implemented or broken features.
This was referenced Jan 12, 2023
Merged
Closed
Member
|
@Flamefire Do you mind re-doing this PR in the new easybuild-docs repository, where we've ported the EasyBuild docs sources to MarkDown? As soon as we switch https://docs.easybuild.md to the new documentation, we'll trash the whole |
Contributor
Author
|
Oh, yes makes sense. Anyway this is but a draft meant for discussion. |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add more examples of which formats are possible.
Also contains not-yet implemented or broken features!
This is related to easybuilders/easybuild-framework#4142, easybuilders/easybuild-framework#4177, easybuilders/easybuild-framework#4150, easybuilders/easybuild-framework#4159, easybuilders/easybuild-framework#4164
We have https://github.com/easybuilders/easybuild-framework/blob/e3681ae53628400096f3e29d58e787bf1173f27b/test/framework/type_checking.py#L184-L225
and https://github.com/easybuilders/easybuild-framework/blob/e3681ae53628400096f3e29d58e787bf1173f27b/test/framework/type_checking.py#L709-L719
which tests various formats for checksums.
We have code in
get_checksum_forwhich supportschecksumsbeing a dict instead of a list: https://github.com/easybuilders/easybuild-framework/blob/e3681ae53628400096f3e29d58e787bf1173f27b/easybuild/framework/easyblock.py#L369However this is not supported by the type-checking code and hence
to_checksumswill be called which would iterate over the dicts keys mistaking them for checksums!!! https://github.com/easybuilders/easybuild-framework/blob/e3681ae53628400096f3e29d58e787bf1173f27b/easybuild/framework/easyconfig/types.py#L475Furthermore this is made more difficult by the YEB files which use a YAML format where tuples are not supported. E.g. a test file has this:
This is a checksum entry for a single file. Obviously the inner-most 2-element lists should be converted to tuples and the outer-most list should be a list. But it isn't clear what to do with the 2nd list: Are those alternative checksums where only one needs to match or additional checksums that all need to match? Both cases should be supported.
I would argue that an additional level can be used:
checksums: [[['mainchecksum', 'altchecksum']]]The type conversion code can deduce that after the 2nd level of lists only tuples may be specified as a list (i.e. an AND) inside a list (already an AND) doesn't make sense, so the 2nd level list is an AND consisting only of a single element, which is redundant but ok.And finally specifying
Nonein a dict currently yields the same error as not specifying it, see easybuilders/easybuild-framework#4142So things to decide:
checksumsto be a dict? This would make the base check (len(checksums) = len(srcs+patches)) impossible. So I'd keep it a list.to_checksumswithNonevalues in dicts and recursion easybuild-framework#4159 as I see no reason why you'd want that. Or is there any?None? In a tuple it doesn't make sense, so I'd disallow it there. For a dict should we differ between having a key-None entry or not having any entry especially related to--enforce-checksums?I see 2 ways here:
As for the missing-key case: I'd handle it the same as not specifying it: Error when
enforce_checksumsis active else treat as matched.NOT to be merged right away but should be used for discussion and once everything is decided this PR can be finalized.