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 type-checking of patches to allow dict values + correctly handle patches specified as dict values in --new-pr #4070

Merged
merged 8 commits into from
Sep 10, 2022

Conversation

akesandgren
Copy link
Contributor

@akesandgren akesandgren commented Aug 30, 2022

Required to make PR #3994 complete.

@akesandgren akesandgren force-pushed the allow_dict_for_patches branch from f01a502 to 8e41344 Compare August 30, 2022 11:57
@easybuilders easybuilders deleted a comment from boegelbot Aug 30, 2022
Add STRING_OR_TUPLE_OR_DICT_LIST for testing validity of "patches" format.
Required to make PR# 3994 complete.
@akesandgren akesandgren force-pushed the allow_dict_for_patches branch from 8e41344 to 0349826 Compare August 30, 2022 12:13
This is likely to simplistic (or plain wrong) but it's a first attempt.
@@ -349,6 +349,31 @@ def to_list_of_strings_and_tuples(spec):
return str_tup_list


def to_list_of_strings_and_tuples_and_dicts(spec):
"""
Convert a 'list of lists and strings' to a 'list of dicts and tuples and strings'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convert a 'list of dicts and tuples/lists and strings' to a 'list of dicts and tuples and strings'

@akesandgren
Copy link
Contributor Author

akesandgren commented Aug 31, 2022

With this in place I now get the below error when trying to do --new-pr on an easyconfig with

patches = [
    {'name': 'AmberTools-20_cmake-locate-netcdf.patch', 'alt_location': 'AmberTools'},
    {'name': 'AmberTools-20_fix_missing_MPI_LIBRARY_error.patch', 'alt_location': 'AmberTools'},

Error:

Traceback (most recent call last):
  File "/usr/lib/python3.8/runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/home/a/ake/Projects/easybuild-dev/easybuild-framework/easybuild/main.py", line 597, in <module>
    main()
  File "/home/a/ake/Projects/easybuild-dev/easybuild-framework/easybuild/main.py", line 496, in main
    new_pr(categorized_paths, ordered_ecs)
  File "/home/a/ake/Projects/easybuild-dev/easybuild-framework/easybuild/tools/github.py", line 1803, in new_pr
    if patch not in paths['patch_files'] and not os.path.isfile(os.path.join(os.path.dirname(ec_path),
  File "/usr/lib/python3.8/posixpath.py", line 90, in join
    genericpath._check_arg_types('join', a, *p)
  File "/usr/lib/python3.8/genericpath.py", line 152, in _check_arg_types
    raise TypeError(f'{funcname}() argument must be str, bytes, or '
TypeError: join() argument must be str, bytes, or os.PathLike object, not 'dict'

Question:
How much checking should be done in new_pr for patches in alt_locations?

@akesandgren
Copy link
Contributor Author

Regarding testing of this...

  • test a full? easyconfig with source and patches in alt_location
  • test new-pr with a alt_location enabled patches

Ideas on how? @boegel

@boegel boegel changed the title allow patches to be dicts. fix type-checking of patches to allow dict values Aug 31, 2022
@boegel boegel changed the title fix type-checking of patches to allow dict values fix type-checking of patches to allow dict values + correctly handle patches specified as dict values in --new-pr Sep 10, 2022
@boegel
Copy link
Member

boegel commented Sep 10, 2022

@akesandgren Extensive tests that cover the added function, and the overall fixes being implemented here, in 6aa3459, 941886c, 5ace8be

@boegel boegel merged commit 83e68bd into easybuilders:develop Sep 10, 2022
@akesandgren akesandgren deleted the allow_dict_for_patches branch September 12, 2022 10:51
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.

3 participants