From 1881eac7c209832b117585384413483d4159b6a8 Mon Sep 17 00:00:00 2001 From: Jarvis Schultz Date: Mon, 21 Oct 2019 16:49:11 -0600 Subject: [PATCH 1/2] Updated regex for extracting '-l' and '-j' args for make Summary of improvements: - No longer consuming args that happen to begin `jX` or `lX` (where `X` is a sequence if integers. - Fixed bug where an empty 'j' arg would throw a `ValueError` when trying to convert an empty string (`''`) to an int - Both `-l/--load-average` and `-j/--jobs` can now be empty (which is supported by GNU Make - The `-l/--load-average` arg will now accept a float value (e.g. `catkin build -l4.0`) which is supported by GNU Make --- catkin_tools/argument_parsing.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/catkin_tools/argument_parsing.py b/catkin_tools/argument_parsing.py index dd9ea974..651441fb 100644 --- a/catkin_tools/argument_parsing.py +++ b/catkin_tools/argument_parsing.py @@ -218,7 +218,7 @@ def extract_cmake_and_make_arguments(args): def extract_jobs_flags_values(mflags): - """Gets the values of tha make jobs flags + """Gets the values of the make jobs flags :param mflags: string of space separated make arguments :type mflags: str @@ -226,9 +226,9 @@ def extract_jobs_flags_values(mflags): :rtype: dict """ - regex = r'(?:^|\s)(?:-?(j|l)(\s*[0-9]+|\s|$))' + \ + regex = r'(?:^|\s)(?:-(j|l)(\s*\d+(?:\.\d*)?|\s|$))' + \ r'|' + \ - r'(?:^|\s)(?:(?:--)?(jobs|load-average)(?:(?:=|\s+)([0-9]+)|(?:\s|$)))' + r'(?:^|\s)(?:(?:--)(jobs|load-average)(?:(?:=|\s+)(\d+(?:\.\d*)?)|(?:\s|$)))' jobs_dict = {} @@ -237,9 +237,9 @@ def extract_jobs_flags_values(mflags): v = v.strip() value = value.strip() if k == 'j' or key == 'jobs': - jobs_dict['jobs'] = int(v or value) if (v or value) else '' + 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) + jobs_dict['load-average'] = float(v or value) if v or value else None return jobs_dict @@ -252,11 +252,14 @@ def extract_jobs_flags(mflags): :returns: list of make jobs flags :rtype: list """ - regex = r'(?:^|\s)(-?(?:j|l)(?:\s*[0-9]+|\s|$))' + \ - r'|' + \ - r'(?:^|\s)((?:--)?(?:jobs|load-average)(?:(?:=|\s+)[0-9]+|(?:\s|$)))' + 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 = [m[0] or m[1] for m in matches] + matches = [[a for a in m if a][0] for m in matches] filtered_flags = [m.strip() for m in matches] if matches else [] return filtered_flags From de97676c6b84985c05e90c443983510eec72f3bd Mon Sep 17 00:00:00 2001 From: Jarvis Schultz Date: Sun, 12 Jan 2020 20:29:49 -0700 Subject: [PATCH 2/2] Added test cases for make args regular expressions Test cases also helped to make regexes slightly more robust. --- catkin_tools/argument_parsing.py | 6 +- tests/unit/test_job_flag_regex.py | 98 +++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 3 deletions(-) create mode 100644 tests/unit/test_job_flag_regex.py diff --git a/catkin_tools/argument_parsing.py b/catkin_tools/argument_parsing.py index 651441fb..db6c9a90 100644 --- a/catkin_tools/argument_parsing.py +++ b/catkin_tools/argument_parsing.py @@ -226,9 +226,9 @@ def extract_jobs_flags_values(mflags): :rtype: dict """ - regex = r'(?:^|\s)(?:-(j|l)(\s*\d+(?:\.\d*)?|\s|$))' + \ + regex = r'(?:^|\s)+?(?:-)(j|l)(?:=|(?:\s+?)|$)?(\d*(?:\.\d*)?)?(?!-)' + \ r'|' + \ - r'(?:^|\s)(?:(?:--)(jobs|load-average)(?:(?:=|\s+)(\d+(?:\.\d*)?)|(?:\s|$)))' + r'(?:^|\s)+?(?:--)(jobs|load-average)(?:=|(?:\s+?)|$)?(\d*(?:\.\d*)?)?(?!-)' jobs_dict = {} @@ -257,7 +257,7 @@ def extract_jobs_flags(mflags): 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|$))(?!-)' + 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 [] diff --git a/tests/unit/test_job_flag_regex.py b/tests/unit/test_job_flag_regex.py new file mode 100644 index 00000000..4c42a112 --- /dev/null +++ b/tests/unit/test_job_flag_regex.py @@ -0,0 +1,98 @@ +from catkin_tools.argument_parsing import extract_jobs_flags +from catkin_tools.argument_parsing import extract_jobs_flags_values + + +JOB_FLAG_ERR = "`extract_job_flags` failed to correctly extract flags from %s" + + +def check_only_strings_in_list_util(mflags, args_list): + """Utility function for testing regular expressions used in argument parsing + + Tests if all space-separated values in mflags are in args_list and + that there are no extra entries in args_list. If either of these + tests fail, we return false. + + :param mflags: space separated string of arguments + :type mflags: str + :param args_list: list of strings to test against + :type args_list: list + :returns: if tests pass + :rtype: bool + """ + split_flags = mflags.split() + if len(args_list) != len(split_flags): + return False + for arg in args_list: + if arg not in split_flags: + return False + else: + first_index = next( + i for i, val in enumerate(split_flags) if val == arg) + split_flags.pop(first_index) + if not split_flags: + return True + else: + return False + + +def test_job_flag_filtering_jl_packages(): + """Test ensures packages that start with `j` or `l` are not converted to make job args:""" + args = "jpkg lpkg -j1 -l1" + filtered = extract_jobs_flags(args) + assert check_only_strings_in_list_util( + "-j1 -l1", filtered), JOB_FLAG_ERR % args + args = "--jobs=1 j2pkg l2pkg --load-average=1" + filtered = extract_jobs_flags(args) + assert check_only_strings_in_list_util( + "--jobs=1 --load-average=1", filtered), JOB_FLAG_ERR % args + args = "--jobs=1 j2pkg --verbose --no-deps l2pkg --load-average=1 --dry-run --start-with j2pkg" + filtered = extract_jobs_flags(args) + assert check_only_strings_in_list_util( + "--jobs=1 --load-average=1", filtered), JOB_FLAG_ERR % args + + +def test_job_flag_filtering_empty_jl(): + """Test ensures proper capture of -j/-l args without a value (accepted by GNU Make)""" + args = "pkg1 pkg2 -j" + filtered = extract_jobs_flags(args) + assert check_only_strings_in_list_util( + "-j", filtered), JOB_FLAG_ERR % args + args = "pkg1 pkg2 -l" + filtered = extract_jobs_flags(args) + assert check_only_strings_in_list_util( + "-l", filtered), JOB_FLAG_ERR % args + args = "--jobs pkg1 --verbose --no-deps pkg2 --load-average --dry-run --start-with pkg1" + filtered = extract_jobs_flags(args) + assert check_only_strings_in_list_util( + "--jobs --load-average", filtered), 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" + filtered = extract_jobs_flags(args) + assert check_only_strings_in_list_util( + "--load-average=1.23", filtered), JOB_FLAG_ERR % args + args = "-l2 pkg1 pkg2 --load-average=1.23 --no-deps --load-average=1 --load-average -l" + filtered = extract_jobs_flags(args) + assert check_only_strings_in_list_util( + "-l2 --load-average=1.23 --load-average=1 --load-average -l", filtered), JOB_FLAG_ERR % args + + +def test_job_flag_values(): + """Test to ensure values are properly extracted from list of job flags""" + flags = "-j1 -j2 --jobs=3 --jobs 4" + valdict = extract_jobs_flags_values(flags) + assert valdict['jobs'] == 4 + flags = "-l1 -l2 --load-average=3 --load-average 4" + valdict = extract_jobs_flags_values(flags) + assert abs(valdict['load-average'] - 4.0) < 1e-12 + flags = "-l -j" + 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 + assert valdict['jobs'] is None and abs( + valdict['load-average'] - 4.0) < 1e-12