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

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

Merged

Conversation

boegel
Copy link
Member

@boegel boegel commented Mar 7, 2019

… 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.

…-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
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' (git@github.com: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
Member

@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
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
Copy link
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
Member

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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants