Skip to content

use temporary checkout of repository as robot path for --new-pr and --update-pr to determine locations for patch files#2803

Merged
migueldiascosta merged 4 commits intoeasybuilders:developfrom
boegel:new_update_pr_robot_path
Apr 4, 2019
Merged

use temporary checkout of repository as robot path for --new-pr and --update-pr to determine locations for patch files#2803
migueldiascosta merged 4 commits intoeasybuilders:developfrom
boegel:new_update_pr_robot_path

Conversation

@boegel
Copy link
Copy Markdown
Member

@boegel boegel commented Mar 7, 2019 •
edited
Loading

… so only that is considered when figuring out to which easyconfig the specified patch files belong to

Marked WIP since need to check whether not considering the easyconfigs in the local robot path makes sense... Removed the WIP, there's no point in checking easyconfigs in the local robot path imho.

@boegel
add temporary checkout of repository to robot path for --new-pr and -…
8319d72 
…-update-pr so it's considered when figuring out to which easyconfig the specified patch files belong to
@boegel boegel added the bug fix label Mar 7, 2019
@boegel boegel added this to the next release (3.8.2) milestone Mar 7, 2019
@boegel boegel requested a review from vanzod March 7, 2019 19:10
@boegel
Copy link
Copy Markdown
Member Author

boegel commented Mar 7, 2019

@migueldiascosta Thoughts on this change?

I think it should be OK, relying on local easyconfigs for this was never a good idea imho...

This fixes the following issue reported by @vanzod:

$ echo "+++ foo\n--- bar" > PyTorch-0.3.1_Fix_pushback_type.patch

$ eb --update-pr 7827 PyTorch-0.3.1_Fix_pushback_type.patch --pr-commit-msg 'testing 123' -D
== temporary log file in case of crash /tmp/eb-YH_Cgj/easybuild-CfQnX2.log
== Determined branch name corresponding to easybuilders/easybuild-easyconfigs PR #7827: 20190307100351_new_pr_PyTorch031
== copying /Users/kehoste/work/easybuild-easyconfigs...
== fetching branch '20190307100351_new_pr_PyTorch031' from https://github.com/vanzod/easybuild-easyconfigs.git...
== copying easyconfigs to /tmp/eb-YH_Cgj/git-working-dir0_Xsu8/easybuild-easyconfigs...
== determining software names for patch files...
Matching easyconfig for PyTorch-0.3.1_Fix_pushback_type.patch not found on the first try:
scanning all easyconfigs to determine where patch file belongs (this may take a while)...
2281 of 2281 easyconfigs checked
ERROR: Failed to determine software name to which patch file PyTorch-0.3.1_Fix_pushback_type.patch relates

With this patch:

$ eb --update-pr 7827 PyTorch-0.3.1_Fix_pushback_type.patch --pr-commit-msg 'testing 123' -D
== temporary log file in case of crash /tmp/eb-KQ2F36/easybuild-Yzwuv8.log
== Determined branch name corresponding to easybuilders/easybuild-easyconfigs PR #7827: 20190307100351_new_pr_PyTorch031
== copying /Users/kehoste/work/easybuild-easyconfigs...
== fetching branch '20190307100351_new_pr_PyTorch031' from https://github.com/vanzod/easybuild-easyconfigs.git...
== copying easyconfigs to /tmp/eb-KQ2F36/git-working-dirnmmbqG/easybuild-easyconfigs...
== determining software names for patch files...
Matching easyconfig for PyTorch-0.3.1_Fix_pushback_type.patch not found on the first try:
scanning all easyconfigs to determine where patch file belongs (this may take a while)...
2359 of 2534 easyconfigs checked
== copying patch files to /tmp/eb-KQ2F36/git-working-dirnmmbqG/easybuild-easyconfigs...
== pushing branch '20190307100351_new_pr_PyTorch031' to remote 'github_vanzod_eYRMB' ([email protected]:vanzod/easybuild-easyconfigs.git) [DRY RUN]
Overview of changes:
 .../PyTorch/PyTorch-0.3.1_Fix_pushback_type.patch  | 37 ++--------------------
 1 file changed, 2 insertions(+), 35 deletions(-)

Updated easybuilders/easybuild-easyconfigs PR #7827 by pushing to branch vanzod/20190307100351_new_pr_PyTorch031 [DRY RUN]
== Temporary log file(s) /tmp/eb-KQ2F36/easybuild-Yzwuv8.log* have been removed.
== Temporary directory /tmp/eb-KQ2F36 has been removed.

@migueldiascosta
Copy link
Copy Markdown
Member

migueldiascosta commented Mar 8, 2019

@boegel at some point you changed from extending the robot path to not using it at all, so the title and docstrings should be changed accordingly?

also, what about if one is using --new-pr to add a new easyconfig, eventually for a new software, together with a patch?

@boegel boegel changed the title add temporary checkout of repository to robot path for --new-pr and --update-pr (WIP) use temporary checkout of repository as robot path for --new-pr and --update-pr (WIP) Apr 4, 2019
@boegel
Copy link
Copy Markdown
Member Author

boegel commented Apr 4, 2019

@migueldiascosta Using --new-pr with an easyconfig + accompanying patch file is unaffected by this change, since we do two passes in det_patch_specs: first, we check whether any of easyconfigs specified to --new-pr needs the patch. If that's not the case, we fall back to scanning the whole repository for a match, and it's only that latter part that we're changing here.

@boegel boegel changed the title use temporary checkout of repository as robot path for --new-pr and --update-pr (WIP) use temporary checkout of repository as robot path for --new-pr and --update-pr Apr 4, 2019
@boegel boegel changed the title use temporary checkout of repository as robot path for --new-pr and --update-pr use temporary checkout of repository as robot path for --new-pr and --update-pr to determine locations for patch files Apr 4, 2019
migueldiascosta
migueldiascosta approved these changes Apr 4, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@migueldiascosta migueldiascosta 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

migueldiascosta commented Apr 4, 2019

Going in, thanks @boegel!

@migueldiascosta migueldiascosta merged commit d86266e into easybuilders:develop Apr 4, 2019
@boegel boegel deleted the new_update_pr_robot_path branch April 4, 2019 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@migueldiascosta migueldiascosta migueldiascosta approved these changes

@vanzod vanzod Awaiting requested review from vanzod

Assignees

No one assigned

Labels

bug fix

Projects

None yet

Milestone

3.9.0

Development

Successfully merging this pull request may close these issues.

2 participants

@boegel @migueldiascosta