add support for --preview-pr #2331

migueldiascosta · 2017-10-23T02:12:21Z

Is something like this useful to anyone else?

(I'm calling it when --new-pr is run with -x, but of course, a new option could be created)

boegel · 2017-10-23T08:20:58Z

Hmm, just gave this a quick try, doesn't seem to work for me, possibly because of the filename I'm using?

$ ls -l CP2K.eb
-rw-r--r--  1 kehoste  staff  1860 Oct 20 14:25 CP2K.eb
$ eb CP2K.eb --new-pr -x
== temporary log file in case of crash /tmp/eb-Ebn_P5/easybuild-_tfsfo.log
ERROR: Can't find path /private/tmp/eb-Ebn_P5/tmp0F3jsT/easybuilders/CP2K.eb

Also, I think it makes more sense to park this under a dedicated option like --preview-pr?

migueldiascosta · 2017-10-23T08:39:41Z

it doesn't seem to be the filename, it does work for me if I use it.. strange
regarding the dedicated option, yes, I suppose so, if other people find it useful (and we get it to actually work :) )

boegel · 2017-10-23T08:42:25Z

== 2017-10-23 10:40:48,937 github.py:353 DEBUG Repo easybuild-easyconfigs at branch develop extracted into /private/tmp/eb-81EYLU/tmpDcQKjl/easybuilders/easybuild-easyconfigs-develop
== 2017-10-23 10:40:48,989 build_log.py:158 ERROR EasyBuild crashed with an error (at ?:124 in __init__): Can't find path /private/tmp/eb-81EYLU/tmpDcQKjl/easybuilders/CP2K.eb (at easybuild/framework/easyconfig/tools.py:375 in parse_easyconfigs)
ERROR: Can't find path /private/tmp/eb-81EYLU/tmpDcQKjl/easybuilders/CP2K.eb

Does this help at all?

I also see it or other easyconfigs that are named <name>.eb...

migueldiascosta · 2017-10-23T08:54:46Z

it's looking for it under /private/tmp/... instead of /tmp/..., I guess I'll need to make sure it is copied to the correct path (where does the private come from in your environment?) or the correct path is used...

boegel · 2017-10-23T09:52:02Z

@migueldiascosta That's not the problem, /tmp and /private/tmp are the same thing...

$ ls -l /tmp
lrwxr-xr-x@ 1 root  wheel  11 Dec 26  2016 /tmp -> private/tmp

boegel · 2017-10-23T11:35:13Z

Hmm, doesn't seem to be related to filename at all:

$ eb --new-pr netCDF-Fortran-4.4.4-foss-2017b.eb netCDF-4.4.1.1-foss-2017b.eb HDF5-1.10.1-foss-2017b.eb -x
== temporary log file in case of crash /tmp/eb-tn6hu6/easybuild-KJzqnn.log
ERROR: Can't find path /private/tmp/eb-tn6hu6/tmp_AQfBZ/easybuilders/netCDF-Fortran-4.4.4-foss-2017b.eb

migueldiascosta · 2017-10-23T11:54:56Z

ok, I can reproduce if I pass the eb filename without a path, I was always using one. I suppose I need to explicitly copy the file to the tmp folder.

Can you just check with an absolute path to make sure that's the problem?

boegel · 2017-10-24T10:44:19Z

easybuild/framework/easyconfig/tools.py

+        else:
+            lines.extend(['', "(no related easyconfigs found for %s)\n" % os.path.basename(ec['spec'])])
+
+    return '\n'.join(lines)


@migueldiascosta This is working as expected now, but there's way too much copy-pasting going on here when comparing preview_pr with review_pr.
The only thing that differs is the list of files (pr_files) and a part of the log message.

Can we create a private method (_pr_diff_common or something like that) which accepts these two as additional arguments?

@boegel definitely, will do

Current approach of enhancing the existing review_pr is way better 👍

boegel · 2017-10-24T10:45:37Z

easybuild/main.py

+        if options.preview_pr:
+            print preview_pr(paths, colored=use_color(options.color))
+            cleanup(logfile, eb_tmpdir, testing, silent=True)
+            sys.exit(0)


Hmm, can we avoid doing cleanup/exit here? How is this handled for --new-pr?

I think it's because options.new_pr is in no_ec_opts, there's a cleanup later for those, I suppose options.preview_pr can also be added to them (although it's a bit counterintuitive that these don't require any easyconfigs to be specified?)

boegel · 2017-10-24T17:48:37Z

easybuild/framework/easyconfig/tools.py

+    elif paths:
+        pr_files = [path[0] for path in paths]
+    else:
+        return "no easyconfigs or pr specified"


I would raise an error instead here, one of both mus be provided or something is wrong in the way review_pr is used...

boegel · 2017-10-24T17:49:41Z

easybuild/framework/easyconfig/tools.py

+    if pr:
+        pr_files = [path for path in fetch_easyconfigs_from_pr(pr) if path.endswith('.eb')]
+    elif paths:
+        pr_files = [path[0] for path in paths]


Hmm, shouldn't we pass the paths without the generated part down to review_pr instead?

in main the paths from det_easyconfig_paths are transformed to these tuples and I used those, but I suppose I can call det_easyconfig_paths in main again, or store the original paths when it is called the first time

boegel · 2017-10-24T17:52:26Z

easybuild/framework/easyconfig/tools.py

+        if pr:
+            _log.debug("File in PR#%s %s has these related easyconfigs: %s" % (pr, ec['spec'], files))
+        else:
+            _log.debug("File in new PR %s has these related easyconfigs: %s" % (ec['spec'], files))


Avoid repeating the whole thing:

if pr: pr_msg = "PR#%s" % pr else: pr_msg = "new PR" _log.debug("File in %s %s has these related easyconfigs: %s" % (pr_msg, ec['spec'], files))

boegel · 2017-10-24T17:52:42Z

easybuild/main.py


    # command line options that do not require any easyconfigs to be specified
-    no_ec_opts = [options.aggregate_regtest, options.new_pr, options.regtest, options.update_pr, search_query]
+    no_ec_opts = [options.aggregate_regtest, options.new_pr, options.preview_pr, options.regtest, options.update_pr, search_query]


line too long, needs wrapping...

Also, can't we use no_ec_opts or new_update_pr below, and avoid including options.new_pr, options.update_pr and options.preview_pr in no_ec_opts (since it's wrong, sematically)

actually, an update_pr could just add or remove a patch, without any easyconfig in the command line...?

OK, sure, but it doesn't make much sense for --new-pr and --preview-pr (although in theory you could also only use a patch file there)

boegel · 2017-10-24T17:57:37Z

easybuild/main.py

    forced = options.force or options.rebuild
    dry_run_mode = options.dry_run or options.dry_run_short
-    new_update_pr = options.new_pr or options.update_pr
+    new_update_pr = options.new_pr or options.update_pr or options.preview_pr


maybe this should be renamed to new_update_preview_pr?

boegel · 2017-10-24T17:58:03Z

easybuild/main.py

-        if options.new_pr:
+        if options.preview_pr:
+            print review_pr(paths=paths, colored=use_color(options.color))
+        elif options.new_pr:


keep these alphabetically sorted, so new_pr before preview_pr

boegel · 2017-10-25T19:42:56Z

easybuild/framework/easyconfig/tools.py

+            pr_msg = "PR#%s" % pr
+        else:
+            pr_msg = "new PR"
+            _log.debug("File in %s %s has these related easyconfigs: %s" % (pr_msg, ec['spec'], files))


the _log.debug should not be under the else:, should be intended left one level so a log message is also emitted for --review-pr

boegel

also missing: a dedicated test for the --preview-pr option, let me know if you need help with that

boegel · 2017-10-25T19:46:46Z

easybuild/main.py


    # cleanup and exit after dry run, searching easyconfigs or submitting regression test
-    if any(no_ec_opts + [options.check_conflicts, dry_run_mode, options.dump_env_script, options.inject_checksums]):
+    if any(no_ec_opts + [new_update_preview_pr, options.check_conflicts, dry_run_mode, options.dump_env_script, options.inject_checksums]):


now this line is too long, should be shortened somehow...

maybe like this:

stop_options = [options.check_conflicts, dry_run_mode, options.dump_env_script, options.inject_checksums] if no_ec_opts or new_update_preview_pr or any(stop_options): cleanup(logfile, eb_tmpdir, testing) sys.exit(0)

boegel · 2017-10-26T06:59:37Z

lgtm, just missing a dedicated test for --preview-pr, and an accompanying PR to update the docs?

boegel

lgtm

boegel · 2017-10-30T07:55:12Z

@migueldiascosta Good to go, thanks!

Can you look into an update for the documentation too?

fix bug that slipped in with #2331

boegel · 2017-10-31T10:37:04Z

documentation added in easybuilders/easybuild#388, thanks again @migueldiascosta!

review-pr before submitting new-pr

ce1b274

boegel added this to the 3.5.0 milestone Oct 23, 2017

migueldiascosta added 2 commits October 24, 2017 08:58

use det_easyconfig_paths

792cc08

new option --preview-pr

29aea71

boegel changed the title ~~review-pr before submitting new-pr~~ add support for --preview-pr Oct 24, 2017

boegel requested changes Oct 24, 2017

View reviewed changes

reduce code duplication

468f154

boegel requested changes Oct 24, 2017

View reviewed changes

migueldiascosta added 5 commits October 25, 2017 09:24

raise error if no PR # or easyconfig path is specified

e2b0a97

avoid code repetition

77ddd19

fix style

79c9913

redefine no_ec_opts and new_update_preview_pr

3ba5bf9

use original determined paths

a086775

boegel requested changes Oct 25, 2017

View reviewed changes

migueldiascosta added 3 commits October 26, 2017 08:39

fix indentation

f2c8bfd

shorten long line

bde283d

if no_ec_opts should be if any(no_ec_opts)

25e9b56

easybuilders deleted a comment from boegelbot Oct 26, 2017

add test

ca98281

boegel approved these changes Oct 30, 2017

View reviewed changes

boegel merged commit b5562ac into easybuilders:develop Oct 30, 2017

wpoely86 added a commit that referenced this pull request Oct 31, 2017

Merge pull request #2337 from boegel/fix_bug

9da714a

fix bug that slipped in with #2331

boegel added the enhancement label Nov 22, 2017

migueldiascosta mentioned this pull request Nov 23, 2017

RFF: allow running --review-pr *before* --new-pr #2147

Closed

add support for --preview-pr #2331

add support for --preview-pr #2331

Uh oh!

Conversation

migueldiascosta commented Oct 23, 2017

Uh oh!

boegel commented Oct 23, 2017

Uh oh!

migueldiascosta commented Oct 23, 2017

Uh oh!

boegel commented Oct 23, 2017

Uh oh!

migueldiascosta commented Oct 23, 2017

Uh oh!

boegel commented Oct 23, 2017

Uh oh!

boegel commented Oct 23, 2017

Uh oh!

migueldiascosta commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

migueldiascosta Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel commented Oct 26, 2017

Uh oh!

boegel left a comment

Choose a reason for hiding this comment

Uh oh!

boegel commented Oct 30, 2017

Uh oh!

boegel commented Oct 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

migueldiascosta Oct 24, 2017 •

edited

Loading