From 9a3cc3172b1f72e3e1d364d7dfd000abb5712e53 Mon Sep 17 00:00:00 2001 From: Timon Engelke Date: Wed, 3 Jun 2020 22:48:51 +0200 Subject: [PATCH] Cleanup of jobs flag parsing (#610) Summary of the changes: * change log space location flag from -l to -L to avoid conflicts (closes #567) * clean up regular expressions that were not really readable and add explanatory comments to the new ones * add two additional small tests regarding the job flags * add `-l` to parser to be shown on `--help` * specify type of `-l`, `-j` and `-p` options for more useful error messages --- catkin_tools/argument_parsing.py | 51 ++++++++++++++++++------------- catkin_tools/spaces/log.py | 2 +- tests/unit/test_job_flag_regex.py | 10 ++++++ 3 files changed, 41 insertions(+), 22 deletions(-) diff --git a/catkin_tools/argument_parsing.py b/catkin_tools/argument_parsing.py index e583de3f..16d557e0 100644 --- a/catkin_tools/argument_parsing.py +++ b/catkin_tools/argument_parsing.py @@ -59,10 +59,12 @@ def add_cmake_and_make_and_catkin_make_args(parser): """ add = parser.add_argument - add('-j', '--jobs', default=None, + add('-j', '--jobs', default=None, type=int, help='Maximum number of build jobs to be distributed across active packages. (default is cpu count)') - add('-p', '--parallel-packages', metavar='PACKAGE_JOBS', dest='parallel_jobs', default=None, + add('-p', '--parallel-packages', metavar='PACKAGE_JOBS', dest='parallel_jobs', default=None, type=int, help='Maximum number of packages allowed to be built in parallel (default is cpu count)') + add('-l', '--load-average', default=None, type=float, + help='Maximum load average before no new build jobs are scheduled') # Deprecated flags kept for compatibility add('--parallel-jobs', '--parallel', action='store_true', dest='parallel_jobs', help=argparse.SUPPRESS) @@ -225,21 +227,20 @@ def extract_jobs_flags_values(mflags): :returns: dictionary mapping jobs flags to jobs flags values :rtype: dict """ + jobs_dict = {'jobs': None, 'load-average': None} - regex = r'(?:^|\s)+?(?:-)(j|l)(?:=|(?:\s+?)|$)?(\d*(?:\.\d*)?)?(?!-)' + \ - r'|' + \ - r'(?:^|\s)+?(?:--)(jobs|load-average)(?:=|(?:\s+?)|$)?(\d*(?:\.\d*)?)?(?!-)' + # These regular expressions use (?P...) for named capture groups + # (^|\s) and (?=$|\s) make sure that the flag is surrounded by whitespace - jobs_dict = {} + regex = r'(^|\s)(-j\s*|--jobs(=|\s+))(?P\d*)(?=$|\s)' + for m in re.finditer(regex, mflags): + if m.group('jobs'): + jobs_dict['jobs'] = int(m.group('jobs')) - matches = re.findall(regex, mflags) or [] - for k, v, key, value in matches: - v = v.strip() - value = value.strip() - if k == 'j' or key == 'jobs': - jobs_dict['jobs'] = int(v or value) if (v or value) else None - elif k == 'l' or key == 'load-average': - jobs_dict['load-average'] = float(v or value) if v or value else None + regex = r'(^|\s)(-l\s*|--load-average(=|\s+))(?P\d*\.?\d*)(?=$|\s)' + for m in re.finditer(regex, mflags): + if m.group('load'): + jobs_dict['load-average'] = float(m.group('load')) return jobs_dict @@ -254,13 +255,21 @@ def extract_jobs_flags(mflags): """ if not mflags: return [] - regex = r'(?:^|\s)(-j(?:(?:\s)*(?:\d)*)|\s|$)(?!-)|' + \ - r'(?:^|\s)(-l(?:(?:\s)*(?:\d*(?:\.\d*)?)|\s|$))(?!-)|' + \ - r'(?:^|\s)(--jobs(?:(?:=|\s*)(?:\d)*)|\s|$)(?!-)|' + \ - r'(?:^|\s)(--load-average(?:(?:=|\s*)(?:\d*(?:\.\d*)?)|\s|$))(?!-)' - matches = re.findall(regex, mflags) or [] - matches = [[a for a in m if a][0] for m in matches] - filtered_flags = [m.strip() for m in matches] if matches else [] + + # Each line matches a flag type, i.e. -j, -l, --jobs, --load-average + # (?:^|\s) and (?=$|\s) make sure that the flag is surrounded by whitespace + # (?:...) is just a group that will not be captured, this is necessary because the whole flag should be captured + # The upper two expressions are simple, they just match the flag, optional whitespace and an optional number + # The bottom two expressions are more complicated because the long flag may be # followed by '=' and a number, + # whitespace and a number or nothing + regex = r'(?:^|\s)(-j\s*\d*)(?=$|\s)|' + \ + r'(?:^|\s)(-l\s*\d*\.?\d*)(?=$|\s)|' + \ + r'(?:^|\s)(--jobs(?:(?:=|\s+)\d+)?)(?=$|\s)|' + \ + r'(?:^|\s)(--load-average(?:(?:=|\s+)\d*\.?\d+)?)(?=$|\s)' + + filtered_flags = [] + for match in re.findall(regex, mflags): + filtered_flags.extend([m.strip() for m in match if m]) return filtered_flags or None diff --git a/catkin_tools/spaces/log.py b/catkin_tools/spaces/log.py index 169aa05c..7ae6b6e4 100644 --- a/catkin_tools/spaces/log.py +++ b/catkin_tools/spaces/log.py @@ -14,7 +14,7 @@ description = dict( default='logs', - short_flag='-l', + short_flag='-L', space='Log Space', description='Output generated during the build stages.' ) diff --git a/tests/unit/test_job_flag_regex.py b/tests/unit/test_job_flag_regex.py index 4c42a112..d60787f6 100644 --- a/tests/unit/test_job_flag_regex.py +++ b/tests/unit/test_job_flag_regex.py @@ -67,6 +67,12 @@ def test_job_flag_filtering_empty_jl(): "--jobs --load-average", filtered), JOB_FLAG_ERR % args +def test_no_job_flag(): + args = "jpkg1 lpkg2" + filtered = extract_jobs_flags(args) + assert filtered is None, JOB_FLAG_ERR % args + + def test_job_flag_load_float(): """Test ensures floating point values are handled correctly for load average""" args = "pkg1 pkg2 --load-average=1.23" @@ -91,6 +97,10 @@ def test_job_flag_values(): valdict = extract_jobs_flags_values(flags) assert 'load-average' in valdict and 'jobs' in valdict assert valdict['load-average'] is None and valdict['jobs'] is None + flags = "--load-average --jobs" + valdict = extract_jobs_flags_values(flags) + assert 'load-average' in valdict and 'jobs' in valdict + assert valdict['load-average'] is None and valdict['jobs'] is None flags = '-j -l2 -j -l --load-average=4' valdict = extract_jobs_flags_values(flags) assert 'load-average' in valdict and 'jobs' in valdict