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

add support for --review-pr #1142

Closed
wants to merge 63 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
dbed38c
add download_repo() method
Aug 13, 2014
b3eef1f
compare push
Aug 13, 2014
7dbbffb
change outputter
Aug 13, 2014
839e716
add tool to determine terminal size
Aug 13, 2014
422f5d1
don't need no ordering
Aug 13, 2014
01325ee
some minor fixes
Aug 13, 2014
e7f0386
fix exact matching
Aug 13, 2014
f92fffb
move review code to dedicated module
Aug 14, 2014
fc672b6
add SO reference to terminal.py
Aug 14, 2014
e3c3255
improve logging, make download_repo generic
Aug 14, 2014
e4be300
cleanup find_related_easyconfigs
Aug 14, 2014
31b43d3
add header, general cleanup of multi_diff
Aug 14, 2014
587976b
remove unneeded whitespace in main.py
Aug 14, 2014
5fb9d5d
use setdefault, __str__
Aug 14, 2014
88f118b
change to positive logic
Aug 14, 2014
10d5256
use constant instead of random strings
Aug 14, 2014
bdc096c
add header, note about failing imports
Aug 14, 2014
7daded0
creative use of setdefault
Aug 14, 2014
994f3eb
wrap long line
Aug 14, 2014
fe05cf1
remove 2/3 _download references
Aug 14, 2014
ce87340
cleanup sorting / filtering
Aug 14, 2014
f80be4e
cleanup ifs
Aug 14, 2014
702c811
fix colorize
Aug 14, 2014
9b69d78
add support to disable colors
Aug 14, 2014
841cc44
add check for not redownloading github archive
Aug 14, 2014
78bd67e
add some docstrings
Aug 14, 2014
27d2709
change some docstrings around
Aug 14, 2014
30a2774
syntax error
Aug 14, 2014
2ef71c0
copyright year
Aug 14, 2014
46ca7e3
fix all the things
Aug 14, 2014
cf64b86
Merge branch 'develop' into feature/add-review-pr-option
boegel Jan 10, 2015
438e7e4
tiny fixes in github.py
boegel Jan 10, 2015
3d7b65b
fix exit for --review-pr
boegel Jan 10, 2015
af6da72
bump to 2015 in copyright line in new modules
boegel Jan 10, 2015
1aa2eac
fix passing of 'per_page=GITHUB_MAX_PER_PAGE' to _do_request
boegel Jan 10, 2015
2c46169
move review_pr function to framework.easyconfig.tools module
boegel Jan 10, 2015
ee2fee1
parse easyconfigs before passing them down to find_related_easyconfigs
boegel Jan 10, 2015
4a6d7b2
fix/cleanup in find_related_easyconfigs: only consider easyconfig fil…
boegel Jan 10, 2015
61e07fa
minor enhancements in review_pr function
boegel Jan 10, 2015
07de284
don't validate easyconfigs when reviewing PRs
boegel Jan 10, 2015
9ddb85f
use raw strings for regex patterns
boegel Jan 10, 2015
fb86a63
some style cleanup in multi_diff.py
boegel Jan 10, 2015
6af5f39
move find_related_easyconfigs into framework.easyconfigs.tools + mino…
boegel Jan 10, 2015
b3c2dce
replace easybuild.tools.terminal module with simple det_terminal_size…
boegel Jan 10, 2015
9859dac
fix location of --review-pr and --color options
boegel Jan 10, 2015
d84b1fe
style cleanup in tools/github.py
boegel Jan 10, 2015
47e1ba3
minor tweaks in docstring + some cleanup in review_pr function
boegel Jan 12, 2015
61b687c
fix use of review_pr in main.py, style cleanup in multi_diff.py
boegel Jan 12, 2015
48b89c0
rename multi_diff to multidiff
boegel Jan 12, 2015
cb1e911
fix import order
boegel Jan 16, 2015
dc6cbc1
style fixes in multidiff.py
boegel Jan 16, 2015
e9373bd
style fixes in multidiff.py
boegel Jan 20, 2015
4744fbb
Merge branch 'develop' into feature/add-review-pr-option
boegel Apr 3, 2015
33a8e65
stop using log.error in multidiff.py
boegel Apr 3, 2015
c407580
bump version to 2.3.1dev
boegel Sep 2, 2015
7f45f71
bump version to 2.4.0dev
boegel Sep 2, 2015
30f2908
Merge branch 'master' into develop
boegel Sep 2, 2015
a165461
Merge branch 'develop' into feature/add-review-pr-option
boegel Sep 2, 2015
87eb367
fix extracting of comments from an easyconfig file that includes 'tai…
boegel Sep 3, 2015
2def3b1
add tail comment in test easyconfig
boegel Sep 3, 2015
07989fe
enhance test_dump_comments with trailing comment
boegel Sep 4, 2015
a0b94ab
Merge pull request #1381 from boegel/fix_extract_tail_comment
boegel Sep 4, 2015
ac13988
Merge branch 'develop' into feature/add-review-pr-option
boegel Sep 4, 2015
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions easybuild/framework/easyconfig/format/one.py
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,8 @@ def dump(self, ecfg, default_values, templ_const, templ_val):
params, _ = self._find_defined_params(ecfg, [[k] for k in LAST_PARAMS], default_values, templ_const, templ_val)
dump.extend(params)

dump.extend(self.comments['tail'])

return '\n'.join(dump)

def extract_comments(self, rawtxt):
Expand All @@ -286,6 +288,7 @@ def extract_comments(self, rawtxt):
'header' : [], # header comment lines
'inline' : {}, # inline comments
'iter': {}, # (inline) comments on elements of iterable values
'tail': [],
}

rawlines = rawtxt.split('\n')
Expand All @@ -301,13 +304,21 @@ def extract_comments(self, rawtxt):
if rawline.startswith('#'):
comment = []
# comment could be multi-line
while rawline.startswith('#') or not rawline:
while rawline is not None and (rawline.startswith('#') or not rawline):
# drop empty lines (that don't even include a #)
if rawline:
comment.append(rawline)
rawline = rawlines.pop(0)
key = rawline.split('=', 1)[0].strip()
self.comments['above'][key] = comment
# grab next line (if more lines are left)
if rawlines:
rawline = rawlines.pop(0)
else:
rawline = None

if rawline is None:
self.comments['tail'] = comment
else:
key = rawline.split('=', 1)[0].strip()
self.comments['above'][key] = comment

elif '#' in rawline: # inline comment
comment_key, comment_val = None, None
Expand Down
130 changes: 116 additions & 14 deletions easybuild/framework/easyconfig/tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,27 @@
@author: Fotis Georgatos (Uni.Lu, NTUA)
@author: Ward Poelmans (Ghent University)
"""

import glob
import os
import re
import sys
import tempfile
from distutils.version import LooseVersion
from vsc.utils import fancylogger

from easybuild.framework.easyconfig import EASYCONFIGS_PKG_SUBDIR
from easybuild.framework.easyconfig.easyconfig import ActiveMNS, create_paths, process_easyconfig
from easybuild.tools.build_log import EasyBuildError
from easybuild.tools.config import build_option
from easybuild.tools.filetools import find_easyconfigs, which, write_file
from easybuild.tools.github import fetch_easyconfigs_from_pr, download_repo
from easybuild.tools.modules import modules_tool
from easybuild.tools.multidiff import multidiff
from easybuild.tools.ordereddict import OrderedDict
from easybuild.tools.run import run_cmd
from easybuild.tools.toolchain import DUMMY_TOOLCHAIN_NAME
from easybuild.tools.utilities import quote_str

# optional Python packages, these might be missing
# failing imports are just ignored
# a NameError should be catched where these are used
Expand Down Expand Up @@ -68,17 +84,6 @@
"or apt-get install python-pygraphviz,"
"or brew install graphviz --with-bindings")

from easybuild.framework.easyconfig import EASYCONFIGS_PKG_SUBDIR
from easybuild.framework.easyconfig.easyconfig import ActiveMNS
from easybuild.framework.easyconfig.easyconfig import process_easyconfig
from easybuild.tools.build_log import EasyBuildError
from easybuild.tools.config import build_option
from easybuild.tools.filetools import find_easyconfigs, which, write_file
from easybuild.tools.github import fetch_easyconfigs_from_pr
from easybuild.tools.modules import modules_tool
from easybuild.tools.ordereddict import OrderedDict
from easybuild.tools.utilities import quote_str


_log = fancylogger.getLogger('easyconfig.tools', fname=False)

Expand Down Expand Up @@ -318,7 +323,7 @@ def det_easyconfig_paths(orig_paths):
return ec_files


def parse_easyconfigs(paths):
def parse_easyconfigs(paths, validate=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

use **kwargs, if you are going to pass it as-is to process_easyconfig (and add it to it the docstring)

"""
Parse easyconfig files
@params paths: paths to easyconfigs
Expand All @@ -335,7 +340,7 @@ def parse_easyconfigs(paths):
ec_files = find_easyconfigs(path, ignore_dirs=build_option('ignore_dirs'))
for ec_file in ec_files:
# only pass build specs when not generating easyconfig files
kwargs = {}
kwargs = {'validate': validate}
if not build_option('try_to_generate'):
kwargs['build_specs'] = build_option('build_specs')
ecs = process_easyconfig(ec_file, **kwargs)
Expand All @@ -359,3 +364,100 @@ def stats_to_str(stats):
txt += "%s%s: %s,\n" % (pref, quote_str(k), quote_str(v))
txt += "}"
return txt


def find_related_easyconfigs(path, ec):
"""
Find related easyconfigs for provided parsed easyconfig in specified path.

A list of easyconfigs for the same software (name) is returned,
matching the 1st criterion that yields a non-empty list.

Software version is considered more important than toolchain.

Toolchain is considered with exact same version prior to without version (only name).

Matching versionsuffix is considered prior to any versionsuffix.

Exact software version is considered prior to matching major/minor version numbers,
and only matching major version number. Any software version is considered last.

The following criteria are considered, in order (with 'version criterion' being either an
Copy link
Contributor

Choose a reason for hiding this comment

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

so which is it: the previous block of text or the list below?

exact version match, a major/minor version match, a major version match, or no version match).

(i) software version criterion, matching versionsuffix and toolchain name/version
Copy link
Contributor

Choose a reason for hiding this comment

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

so they all have software version criterion. don't copy paste, refactor.

(ii) software version criterion, matching versionsuffix and toolchain name (any toolchain version)
(iii) software version criterion, matching versionsuffix (any toolchain name/version)
(iv) software version criterion, matching toolchain name/version (any versionsuffix)
(v) software version criterion, matching toolchain name (any versionsuffix, toolchain version)
(vi) software version criterion (any versionsuffix, toolchain name/version)

If no related easyconfigs with a matching software name are found, an empty list is returned.
"""
name = ec.name
version = ec.version
versionsuffix = ec['versionsuffix']
toolchain_name = ec['toolchain']['name']
toolchain_name_pattern = r'-%s-\S+' % toolchain_name
toolchain = "%s-%s" % (toolchain_name, ec['toolchain']['version'])
toolchain_pattern = '-%s' % toolchain
if toolchain_name == DUMMY_TOOLCHAIN_NAME:
toolchain_name_pattern = ''
toolchain_pattern = ''

potential_paths = [glob.glob(ec_path) for ec_path in create_paths(path, name, '*')]
potential_paths = sum(potential_paths, []) # flatten

parsed_version = LooseVersion(version).version
version_patterns = [version] # exact version match
if len(parsed_version) >= 2:
version_patterns.append(r'%s\.%s\.[0-9_A-Za-z]+' % tuple(parsed_version[:2])) # major/minor version match
Copy link
Contributor

Choose a reason for hiding this comment

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

[0-9_A-Za-z] is \w, please use that (everywhere)

if parsed_version != parsed_version[0]:
version_patterns.append(r'%s\.[0-9-]+\.[0-9_A-Za-z]+' % parsed_version[0]) # major version match
Copy link
Contributor

Choose a reason for hiding this comment

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

0-9 is \d

version_patterns.append(r'[0-9._A-Za-z-]+') # any version

regexes = []
for version_pattern in version_patterns:
regexes.extend([
re.compile((r"^\S+/%s-%s%s%s.eb$" % (name, version_pattern, toolchain_pattern, versionsuffix))),
Copy link
Contributor

Choose a reason for hiding this comment

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

do the re.compile in a separate step on the whole list of patterns (use map).
make a pattern that holds all the common pattern r"^\S+/%s-%s%%s.eb$" %(name, version_pattern) is a good start

re.compile((r"^\S+/%s-%s%s%s.eb$" % (name, version_pattern, toolchain_name_pattern, versionsuffix))),
re.compile((r"^\S+/%s-%s-\S+%s.eb$" % (name, version_pattern, versionsuffix))),
re.compile((r"^\S+/%s-%s%s.eb$" % (name, version_pattern, toolchain_pattern))),
re.compile((r"^\S+/%s-%s%s.eb$" % (name, version_pattern, toolchain_name_pattern))),
re.compile((r"^\S+/%s-%s-\S+.eb$" % (name, version_pattern))),
])
_log.debug("found these potential paths: %s" % potential_paths)

for regex in regexes:
res = [p for p in potential_paths if regex.match(p)]
if res:
_log.debug("Related easyconfigs found using '%s': %s" % (regex.pattern, res))
break
else:
_log.debug("No related easyconfigs in potential paths using '%s'" % regex.pattern)
return res


def review_pr(pr, colored=True, branch='develop'):
"""
Print multi-diff overview between easyconfigs in specified PR and specified branch.
@param pr: pull request number in easybuild-easyconfigs repo to review
@param colored: boolean indicating whether a colored multi-diff should be generated
@param branch: easybuild-easyconfigs branch to compare with
"""
tmpdir = tempfile.mkdtemp()

download_repo_path = download_repo(branch=branch, path=tmpdir)
repo_path = os.path.join(download_repo_path, 'easybuild', 'easyconfigs')
pr_files = [path for path in fetch_easyconfigs_from_pr(pr) if path.endswith('.eb')]

ecs, _ = parse_easyconfigs([(fp, False) for fp in pr_files], validate=False)
for ec in ecs:
files = find_related_easyconfigs(repo_path, ec['ec'])
_log.debug("File in PR#%s %s has these related easyconfigs: %s" % (pr, ec['spec'], files))
if files:
diff = multidiff(ec['spec'], files, colored=colored)
msg = diff
else:
msg = "\n(no related easyconfigs found for %s)\n" % os.path.basename(ec['spec'])
print(msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

generate all the text, print it all once

Copy link
Contributor

Choose a reason for hiding this comment

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

and print it all at once in main

13 changes: 10 additions & 3 deletions easybuild/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
from easybuild.framework.easyblock import EasyBlock, build_and_install_one
from easybuild.framework.easyconfig import EASYCONFIGS_PKG_SUBDIR
from easybuild.framework.easyconfig.tools import alt_easyconfig_paths, dep_graph, det_easyconfig_paths
from easybuild.framework.easyconfig.tools import get_paths_for, parse_easyconfigs, skip_available
from easybuild.framework.easyconfig.tools import get_paths_for, parse_easyconfigs, review_pr, skip_available
from easybuild.framework.easyconfig.tweak import obtain_ec_for, tweak
from easybuild.tools.config import get_repository, get_repositorypath
from easybuild.tools.filetools import adjust_permissions, cleanup, write_file
Expand Down Expand Up @@ -231,6 +231,10 @@ def main(args=None, logfile=None, do_build=None, testing=False):
init_session_state.update({'module_list': modlist})
_log.debug("Initial session state: %s" % init_session_state)

# review specified PR
if options.review_pr:
review_pr(options.review_pr, colored=options.color)

# search for easyconfigs, if a query is specified
query = options.search or options.search_short
if query:
Expand All @@ -241,6 +245,9 @@ def main(args=None, logfile=None, do_build=None, testing=False):
if not easyconfigs_pkg_paths:
_log.warning("Failed to determine install path for easybuild-easyconfigs package.")

# command line options that do not require any easyconfigs to be specified
no_ec_opts = [options.aggregate_regtest, options.review_pr, options.search, options.search_short, options.regtest]

# determine paths to easyconfigs
paths = det_easyconfig_paths(orig_paths)
if paths:
Expand All @@ -250,7 +257,7 @@ def main(args=None, logfile=None, do_build=None, testing=False):
if 'name' in build_specs:
# try to obtain or generate an easyconfig file via build specifications if a software name is provided
paths = find_easyconfigs_by_specs(build_specs, robot_path, try_to_generate, testing=testing)
elif not any([options.aggregate_regtest, options.search, options.search_short, options.regtest]):
elif not any(no_ec_opts):
print_error(("Please provide one or multiple easyconfig files, or use software build "
"options to make EasyBuild search for easyconfigs"),
log=_log, opt_parser=eb_go.parser, exit_on_error=not testing)
Expand Down Expand Up @@ -280,7 +287,7 @@ def main(args=None, logfile=None, do_build=None, testing=False):
print_msg(txt, log=_log, silent=testing, prefix=False)

# cleanup and exit after dry run, searching easyconfigs or submitting regression test
if any([options.dry_run, options.dry_run_short, options.regtest, options.search, options.search_short]):
if any(no_ec_opts + [options.dry_run, options.dry_run_short]):
cleanup(logfile, eb_tmpdir, testing)
sys.exit(0)

Expand Down
Loading