add option to make sanity_check_paths arch dependent #3845
Conversation
Flamefire
force-pushed
the
alternative_sanity_check_paths
branch
from
eb40cb2 to
b789d2b
Compare
|
@Flamefire Are the failing tests making sense to you? |
|
@boegel They are secondary. The primary issue is that I basically changed the allowed format to accept dicts in addition to strings and tuples in the list. See https://github.com/easybuilders/easybuild-framework/pull/3845/files#diff-c63ede120b91884bda5e7d0df599ac170866c050ccec036bdf61c3d456846f43R572 The conversion function works, however it seems for yeb-files (the YAMLs) the type-spec is used and fails. |
Flamefire
force-pushed
the
alternative_sanity_check_paths
branch
from
b789d2b to
9332d65
Compare
|
Ah I needed to add |
Flamefire
force-pushed
the
alternative_sanity_check_paths
branch
from
9332d65 to
81e59c3
Compare
Flamefire
force-pushed
the
alternative_sanity_check_paths
branch
from
81e59c3 to
ddf7d89
Compare
|
@boegel All green now. |
Flamefire
force-pushed
the
alternative_sanity_check_paths
branch
from
4f6b4a5 to
78c95a3
Compare
`False` is a valid value to return to skip a dependency for an architecture, so don't error in that case.
Flamefire
force-pushed
the
alternative_sanity_check_paths
branch
from
78c95a3 to
761c732
Compare
Flamefire
force-pushed
the
alternative_sanity_check_paths
branch
2 times, most recently
from
de23dc0 to
390d625
Compare
Mention that `dict`s are also an option
Flamefire
force-pushed
the
alternative_sanity_check_paths
branch
from
390d625 to
8f21e20
Compare
| if isinstance(elem, (string_type, tuple)): | ||
| result.append(elem) | ||
| elif isinstance(elem, list): | ||
| result.append(tuple(elem)) | ||
| elif isinstance(elem, dict): | ||
| for key, value in elem.items(): | ||
| if not isinstance(key, string_type): | ||
| raise EasyBuildError("Expected keys to be of type string, got %s (%s)", key, type(key)) | ||
| elif isinstance(value, list): | ||
| elem[key] = tuple(value) | ||
| elif not isinstance(value, (string_type, tuple)): | ||
| raise EasyBuildError("Expected elements to be of type string, tuple or list, got %s (%s)", | ||
| value, type(value)) | ||
| result.append(elem) |
There was a problem hiding this comment.
Strictly speaking, every value in a list (turned tuple) or tuple, should be of string_type, or at least not list/tuple/dicts,
and we should perhaps check that too, or?
The question then becomes, are we doing that strictly in all the other to_xxx functions?
I don't think we need to change this here, but rather take a look at all of them at the same time in a separate PR.
There was a problem hiding this comment.
You are right that should be done as one should (be able to) assume that what comes out from those functions is valid.
Maybe keep that in an issue
Semi-related: #4177
|
Going in, thanks @Flamefire! |
boegel
changed the title
|
@Flamefire Can you also update the documentation to mention that this is now supported? |
Not in the next couple weeks at least. A quick message is all I can do at the moment, a rebase when required is pushing my time limits already. |
3 participants
pick_system_specific_valueto be used bypick_dep_versionExample:
'files': [{'arch=x86_64': 'correct.a'}, 'all_archs.a']Note that the arch-specific stuff are single files or alternatives (tuple), not a full list of files (again) to keep it simple.