Skip to content

warn about potentially missing patches in --new-pr#3759

Merged
akesandgren merged 5 commits intoeasybuilders:developfrom
migueldiascosta:missing_patches
Feb 15, 2022
Merged

warn about potentially missing patches in --new-pr#3759
akesandgren merged 5 commits intoeasybuilders:developfrom
migueldiascosta:missing_patches

Conversation

@migueldiascosta
Copy link
Copy Markdown
Member

@migueldiascosta migueldiascosta commented Jun 28, 2021 •
edited
Loading

requested in #1674, at least

not sure if it always makes sense to automatically add patches, but we can at least warn about potentially missing ones...

@migueldiascosta migueldiascosta changed the title warn about potentially missing patches in --new-pr warn about potentially missing patches in --new-pr (WIP) Jun 28, 2021
@boegel boegel added this to the release after 4.4.1 milestone Jul 3, 2021
@boegel boegel modified the milestones: 4.4.2, release after 4.4.2 Sep 1, 2021
@boegel boegel modified the milestones: 4.5.0, 4.x Oct 13, 2021
akesandgren
akesandgren previously approved these changes Feb 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

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

LGTM

@akesandgren akesandgren dismissed their stale review February 14, 2022 10:03

Premature review

@akesandgren
Copy link
Copy Markdown
Contributor

akesandgren commented Feb 14, 2022

What about --new-pr on easyconfig that uses patch files that already exist in the repo?
We don't want to warn about them do we?

@migueldiascosta migueldiascosta marked this pull request as draft February 15, 2022 04:43
@migueldiascosta migueldiascosta force-pushed the missing_patches branch 3 times, most recently from c2b9109 to 6a543d7 Compare February 15, 2022 06:42
@migueldiascosta
Copy link
Copy Markdown
Member Author

migueldiascosta commented Feb 15, 2022

What about --new-pr on easyconfig that uses patch files that already exist in the repo? We don't want to warn about them do we?

right, addressed in 799c3b4

akesandgren
akesandgren reviewed Feb 15, 2022
View reviewed changes
Comment thread easybuild/tools/github.py Outdated
for ec, ec_path in zip(file_info['ecs'], file_info['paths_in_repo']):
for patch in ec.asdict()['patches']:
if patch not in paths['patch_files'] and not os.path.isfile(os.path.join(os.path.dirname(ec_path), patch)):
print_warning("new patch %s, referenced by %s, is not included in this PR" % (patch, ec.filename()))
Copy link
Copy Markdown
Contributor

@akesandgren akesandgren Feb 15, 2022

Choose a reason for hiding this comment

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

perhaps "new patch file"?? but do as you please.

@akesandgren
Copy link
Copy Markdown
Contributor

akesandgren commented Feb 15, 2022

All in all, this looks good and it's something I could have benefited from multiple times... ooohh so many times...

@akesandgren akesandgren reopened this Feb 15, 2022
@akesandgren
Copy link
Copy Markdown
Contributor

akesandgren commented Feb 15, 2022

oops hit the wrong button

@akesandgren
Copy link
Copy Markdown
Contributor

akesandgren commented Feb 15, 2022

Change to non-draft?

@migueldiascosta migueldiascosta changed the title warn about potentially missing patches in --new-pr (WIP) warn about potentially missing patches in --new-pr Feb 15, 2022
@migueldiascosta migueldiascosta marked this pull request as ready for review February 15, 2022 09:06
akesandgren
akesandgren previously approved these changes Feb 15, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

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

LGTM

@migueldiascosta
Copy link
Copy Markdown
Member Author

migueldiascosta commented Feb 15, 2022 •
edited
Loading

@akesandgren wait, the tests requiring github token are failing

@migueldiascosta migueldiascosta marked this pull request as draft February 15, 2022 10:01
@akesandgren
Copy link
Copy Markdown
Contributor

akesandgren commented Feb 15, 2022

I have one more question, is the file_info['ecs'] and file_info['paths_in_repo'] guaranteed to match in number of elements?
Haven't had time to check the code yet...

@migueldiascosta
Copy link
Copy Markdown
Member Author

migueldiascosta commented Feb 15, 2022

that's a good point - I'd say yes (

easybuild-framework/easybuild/framework/easyconfig/easyconfig.py

Lines 2402 to 2442 in 23071bd

def det_file_info(paths, target_dir):
"""
Determine useful information on easyconfig files relative to a target directory,
before any actual operation (e.g. copying) is performed
:param paths: list of paths to easyconfig files
:param target_dir: target directory
:return: dict with useful information on easyconfig files (corresponding EasyConfig instances, paths, status)
relative to a target directory
"""
file_info = {
'ecs': [],
'paths': [],
'paths_in_repo': [],
'new': [],
'new_folder': [],
'new_file_in_existing_folder': [],
}
for path in paths:
ecs = process_easyconfig(path, validate=False)
if len(ecs) == 1:
file_info['paths'].append(path)
file_info['ecs'].append(ecs[0]['ec'])
soft_name = file_info['ecs'][-1].name
ec_filename = file_info['ecs'][-1].filename()
target_path = det_location_for(path, target_dir, soft_name, ec_filename)
new_file = not os.path.exists(target_path)
new_folder = not os.path.exists(os.path.dirname(target_path))
file_info['new'].append(new_file)
file_info['new_folder'].append(new_folder)
file_info['new_file_in_existing_folder'].append(new_file and not new_folder)
file_info['paths_in_repo'].append(target_path)
else:
raise EasyBuildError("Multiple EasyConfig instances obtained from easyconfig file %s", path)
return file_info
) and something similar is assumed in copy_easyconfigs itself (

easybuild-framework/easybuild/framework/easyconfig/easyconfig.py

Line 2455 in 23071bd

for path, target_path in zip(file_info['paths'], file_info['paths_in_repo']):
), but this could probably be made clearer and/or more robust...

@akesandgren
Copy link
Copy Markdown
Contributor

akesandgren commented Feb 15, 2022

Have you run the framework test suite after this commit?

@migueldiascosta
Copy link
Copy Markdown
Member Author

migueldiascosta commented Feb 15, 2022

Have you run the framework test suite after this commit?

I ran the (50) tests that require the github token locally, yes, they all pass now

@migueldiascosta migueldiascosta marked this pull request as ready for review February 15, 2022 11:19
akesandgren
akesandgren approved these changes Feb 15, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

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

LGTM

@akesandgren
Copy link
Copy Markdown
Contributor

akesandgren commented Feb 15, 2022

Going in, thanks @migueldiascosta!

@akesandgren akesandgren merged commit f5ea74a into easybuilders:develop Feb 15, 2022
@branfosj branfosj mentioned this pull request Feb 19, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akesandgren akesandgren akesandgren approved these changes

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

4.5.4

Development

Successfully merging this pull request may close these issues.

3 participants

@migueldiascosta @akesandgren @boegel