Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2162123002:
Bisection debugging helper script (Closed)
|
Created:
4 years, 5 months ago by manasijm Modified:
4 years, 5 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionBisection debugging helper script
BUG=none
R=stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=34e88480eda520540d253fe1662e18d63551c033
Patch Set 1 #
Total comments: 39
Patch Set 2 : Address some comments #Patch Set 3 : Format #Patch Set 4 : Allow joining ranges with comma #Patch Set 5 : Format and correct typos #Patch Set 6 : 'Use extend insead of +' #Patch Set 7 : Use half open ranges instead of closed ranges #
Total comments: 5
Patch Set 8 : Add Documentation #Patch Set 9 : Allow templated command input #Patch Set 10 : Sorted output and step estimate #Patch Set 11 : Fix log(0,2) #
Total comments: 23
Patch Set 12 : Address comments #
Total comments: 19
Patch Set 13 : Address Comments #
Messages
Total messages: 16 (2 generated)
manasijm@google.com changed reviewers: + eholk@chromium.org, jpp@chromium.org, kschimpf@google.com, stichnot@chromium.org
https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-test.py File pydir/bisection-test.py (right): https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-test.py#new... pydir/bisection-test.py:17: help='Crash list, single values or x-separated combinations like 2x4') why x? https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-test.py#new... pydir/bisection-test.py:39: excluded.add(num) It seems you're not using excluded for anything. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-test.py#new... pydir/bisection-test.py:43: if len(crash_combination) == 1: no need to special case here, right? https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py File pydir/bisection-tool.py (right): https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:5: import subprocess add an empty line https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:21: class Timeout(Exception): No need for this class, just raise Exception("timeout") https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:41: print p.returncode remove this print, it will get lost in the script's output. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:46: if main_range[0] == main_range[1]: Are you sure this is what you want to do here? I would assume you want to run the test program even for single-element ranges... https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:54: exit_code_1 = run(cmd, [first_half] + include_ranges[1:], timeout) I personally would discourage using recursion for this. I would keep a stack of ranges, and I would keep run()ing until that stack becomes empty. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:58: if exit_code_1 == 0 and exit_code_2 == 0: I understand what you're trying to accomplish, but I do not understand how it accomplishes that... https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:61: partial_1 = find_crashes(cmd, [first_half] + [second_half] + include_ranges[1:], timeout) 80-col, as per the Google python style guide. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:63: if isinstance(partial_1[0], list) : This is python. You should be careful about the values you return! This is only happening because of line 68, when you concatenate two lists and wrap the result around another list. I propose that you modify this code so that you always ensure find_crashes returns a list of "crashes." And you ensure that by using list's super convenient "extend" method. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:65: partial_2 = find_crashes(cmd, [second_half] + [first_half] + include_ranges[1:], timeout) 80-col https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:74: result = result + find_crashes(cmd, [second_half] + include_ranges[1:], timeout) 80-col https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:88: help='Timeout for each invocation of the input command') 80-col https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:109: if len(fail_list) > 0 : this is python. you can just if fail_list: print "Failing Functions" for fail in fail_list: print fail https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:114: #TODO(manasijm) : Pretty print, assocoiate these numbers with filenames # TODO - note the space Also, add an empty line between this and the if below.
moar comments.... https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py File pydir/bisection-tool.py (right): https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:8: def build_command(input_cmd, include_ranges): why not have build command return an object: def Runner(object): def __init__(input_cmd, timeout): self._input_cmd = input_cmd self._timeout = timeout self._num_tries = 0 def Run(included_ranges): def timeout_handler(signum, frame): raise Exception("timeout") self._num_tries += 1 cmd = self.input_cmd for i in included_ranges: cmd += "-i {start}:{end}".format(start=i[0], end=i[1] + 1) p = subprocess.Popen(cmd, shell=True, cwd=None, ...) if self._timeout != -1: install signal handlers try: _, _ = p.communicate() if self._timeout != -1: remove signal handlers catch Exception: cancel p With this interface, find_crashes' signature becomes: def find_crashes(runner, include_ranges): Which I personally prefer. The current signature has two parameters (cmd, and timeout) that are closely related to each other, and would be better off "bundled" together. It also gets rid of the num_tries global. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:58: if exit_code_1 == 0 and exit_code_2 == 0: On 2016/07/20 14:50:08, John wrote: > I understand what you're trying to accomplish, but I do not understand how it > accomplishes that... Now I understand... This method's signature is too complicated. You're convoluting the concepts of "include_ranges", and "current_interval". You probably want a signature like def find_crashes(cmd, interval, include_ranges, timeout) then the code would naturally show what is your bisect interval. Then, later on, when you're trying to figure out the "a:b crashes, but a:mid and mid:b don't" case below, you would invoke the method as follows: find_crashes(cmd, first_half, [second_half] + include_ranges, timeout) find_crashes(cmd, second_half, [first_half] + include_ranges, timeout) https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:97: find_all = args.all this is only ever use in find_crashes. I strongly suggest you pass this as a parameter to that method instead of using a global. Also, as the Google Python Style guide is fairly specific about which types of data are candidates for globals, ans find_all is definitively not one of them.
https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py File pydir/bisection-tool.py (right): https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:11: result = result + " -i " + str(i[0])+":"+str(i[1] + 1) For more awesomeness, I would really like it if one could somehow control on the command line how 'result' is formed based on include_ranges. For example. If you're doing szbuild.py bisection, you do as above: -i a1:b1 -i a2:b2 ... -i an:bn But Subzero itself has its own bisection range syntax, e.g.: -verbose-focus a1:b1,a2:b2,...,an:bn Furthermore, these ranges might need to be injected into the middle of the command line instead of appended to the end: ./pydir/szbuild.py -o ./a.out -i a1:b1 ; ./a.out
https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-test.py File pydir/bisection-test.py (right): https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-test.py#new... pydir/bisection-test.py:17: help='Crash list, single values or x-separated combinations like 2x4') On 2016/07/20 14:50:08, John wrote: > why x? Anything other than : would be fine, I guess. Chose x to better communicate that it is a combination and not a range. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-test.py#new... pydir/bisection-test.py:39: excluded.add(num) On 2016/07/20 14:50:08, John wrote: > It seems you're not using excluded for anything. Right, it should be included.remove(num) https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-test.py#new... pydir/bisection-test.py:43: if len(crash_combination) == 1: On 2016/07/20 14:50:08, John wrote: > no need to special case here, right? Done. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py File pydir/bisection-tool.py (right): https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:5: import subprocess On 2016/07/20 14:50:08, John wrote: > add an empty line Done. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:8: def build_command(input_cmd, include_ranges): On 2016/07/20 16:11:58, John wrote: > why not have build command return an object: > > def Runner(object): > def __init__(input_cmd, timeout): > self._input_cmd = input_cmd > self._timeout = timeout > self._num_tries = 0 > > def Run(included_ranges): > def timeout_handler(signum, frame): > raise Exception("timeout") > > self._num_tries += 1 > cmd = self.input_cmd > for i in included_ranges: > cmd += "-i {start}:{end}".format(start=i[0], end=i[1] + 1) > > p = subprocess.Popen(cmd, shell=True, cwd=None, ...) > if self._timeout != -1: > install signal handlers > > try: > _, _ = p.communicate() > if self._timeout != -1: > remove signal handlers > catch Exception: > cancel p > > With this interface, find_crashes' signature becomes: > > def find_crashes(runner, include_ranges): > > Which I personally prefer. The current signature has two parameters (cmd, and > timeout) that are closely related to each other, and would be better off > "bundled" together. It also gets rid of the num_tries global. Done. Thanks! https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:21: class Timeout(Exception): On 2016/07/20 14:50:08, John wrote: > No need for this class, just > > raise Exception("timeout") Done. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:41: print p.returncode On 2016/07/20 14:50:08, John wrote: > remove this print, it will get lost in the script's output. It seems helpful to determine how close the script is to ending. And eyeballing which ranges are failing. Making it harder to miss ! https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:46: if main_range[0] == main_range[1]: On 2016/07/20 14:50:08, John wrote: > Are you sure this is what you want to do here? I would assume you want to run > the test program even for single-element ranges... Acknowledged. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:65: partial_2 = find_crashes(cmd, [second_half] + [first_half] + include_ranges[1:], timeout) On 2016/07/20 14:50:08, John wrote: > 80-col Done. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:74: result = result + find_crashes(cmd, [second_half] + include_ranges[1:], timeout) On 2016/07/20 14:50:08, John wrote: > 80-col Done. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:88: help='Timeout for each invocation of the input command') On 2016/07/20 14:50:08, John wrote: > 80-col Done. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:97: find_all = args.all On 2016/07/20 16:11:58, John wrote: > this is only ever use in find_crashes. I strongly suggest you pass this as a > parameter to that method instead of using a global. > > Also, as the Google Python Style guide is fairly specific about which types of > data are candidates for globals, ans find_all is definitively not one of them. Done. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:109: if len(fail_list) > 0 : On 2016/07/20 14:50:08, John wrote: > this is python. you can just > > if fail_list: > print "Failing Functions" > for fail in fail_list: > print fail Done. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:114: #TODO(manasijm) : Pretty print, assocoiate these numbers with filenames On 2016/07/20 14:50:08, John wrote: > # TODO - note the space > > Also, add an empty line between this and the if below. Done.
https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py File pydir/bisection-tool.py (right): https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:11: result = result + " -i " + str(i[0])+":"+str(i[1] + 1) On 2016/07/20 18:32:02, stichnot wrote: > For more awesomeness, I would really like it if one could somehow control on the > command line how 'result' is formed based on include_ranges. > > For example. If you're doing szbuild.py bisection, you do as above: > -i a1:b1 -i a2:b2 ... -i an:bn > But Subzero itself has its own bisection range syntax, e.g.: > -verbose-focus a1:b1,a2:b2,...,an:bn > > Furthermore, these ranges might need to be injected into the middle of the > command line instead of appended to the end: > ./pydir/szbuild.py -o ./a.out -i a1:b1 ; ./a.out Done. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:61: partial_1 = find_crashes(cmd, [first_half] + [second_half] + include_ranges[1:], timeout) On 2016/07/20 14:50:08, John wrote: > 80-col, as per the Google python style guide. Done. https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:63: if isinstance(partial_1[0], list) : On 2016/07/20 14:50:08, John wrote: > This is python. You should be careful about the values you return! > > This is only happening because of line 68, when you concatenate two lists and > wrap the result around another list. > > I propose that you modify this code so that you always ensure find_crashes > returns a list of "crashes." And you ensure that by using list's super > convenient "extend" method. Done.
https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-test.py File pydir/bisection-test.py (right): https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-test.py#new... pydir/bisection-test.py:17: help='Crash list, single values or x-separated combinations like 2x4') On 2016/07/20 21:15:23, manasijm wrote: > On 2016/07/20 14:50:08, John wrote: > > why x? > > Anything other than : would be fine, I guess. > Chose x to better communicate that it is a combination and not a range. well, maybe ','? :) https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py File pydir/bisection-tool.py (right): https://codereview.chromium.org/2162123002/diff/1/pydir/bisection-tool.py#new... pydir/bisection-tool.py:41: print p.returncode On 2016/07/20 21:15:24, manasijm wrote: > On 2016/07/20 14:50:08, John wrote: > > remove this print, it will get lost in the script's output. > > It seems helpful to determine how close the script is to ending. > And eyeballing which ranges are failing. > Making it harder to miss ! Maybe add a string describing what's going on, then? just printing the range might be a bit confusing... https://codereview.chromium.org/2162123002/diff/120001/pydir/bisection-tool.py File pydir/bisection-tool.py (right): https://codereview.chromium.org/2162123002/diff/120001/pydir/bisection-tool.p... pydir/bisection-tool.py:21: if isinstance(i, int): I would always pass a range. If you want a single element, then just pass (n, n+1) This simplifies the logic, and makes it easier to read the code (fewer code paths to keep in memory.) https://codereview.chromium.org/2162123002/diff/120001/pydir/bisection-tool.p... pydir/bisection-tool.py:76: partial_result = flatten(find_crashes(runner, first_half, [second_half] you **REALLY** shouldn't need to flatten your result. You should figure out why you're returning a list of lists instead, and fix that. https://codereview.chromium.org/2162123002/diff/120001/pydir/bisection-tool.p... pydir/bisection-tool.py:82: return [partial_result] why do you return partial result wrapped in a list? partial result is a list already (or something with an extend method...) https://codereview.chromium.org/2162123002/diff/120001/pydir/bisection-tool.p... pydir/bisection-tool.py:85: if exit_code_1 != 0: I don't see you using "find_all" here, so you might find multiple crashes here anyways... https://codereview.chromium.org/2162123002/diff/120001/pydir/bisection-tool.p... pydir/bisection-tool.py:128: if runner.Run([initial_range]) != 0: why not crashes = find_crashes(runner, initial_range, [], args.all) if not crashes: print "Pass" else: for c in crashes: print c (i.e., you don't need to invoke run here...)
https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.py File pydir/bisection-tool.py (right): https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:36: raise RuntimeError("Timeout") For whatever reason, our scripts prefer 'strings' rather than "strings". https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:74: print "===Remaining Steps===: " + str(self.estimate(included_ranges)) Should probably rephrase the message to convey that it is not an exact step count. E.g. '===Remaining Steps (approx)===:' https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:86: def find_crashes(runner, current_interval, include_ranges, find_all): Maybe find_failures? It finds more than just crashes. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:142: For subzero itself crashing, Capitalize Subzero https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:143: bisection-tool.py --cmd 'pnacl-sz -translate-only=' --comma-join I think there should be only a single space character after 'pnacl-sz'. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:153: dest='cmd', Probably append this line to the previous line. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:162: argparser.add_argument('--all', dest='all', action='store_true') All of these args should have help strings. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:162: argparser.add_argument('--all', dest='all', action='store_true') Instead of --all and --no-all, you could do something like: add_argument('--all', type=int, choices=[0,1], default=1) This allows: --all --all=1 --all=0 and reduces the number of args. Same for comma-join/separate and template/no-template. This is what's currently done for 'szbuild.py --force'. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:184: print "Pass" # The whole input range works, maybe check subzero build flags? Another possibility is (if you expected it not to pass) is that you should increase the --end value (if bisecting per-instruction or per-variable transformations on a large function). https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:187: print "Failing Functions:" Here and in the comment above, we aren't necessarily talking about functions. Maybe "items"? https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:195: # TODO(manasijm): Pretty print, associate these numbers with filenames I don't think you need this TODO, since this utility is now more general than just szbuild.py-style "-i" functionality. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:198: main() Indent this only 2 characters?
https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.py File pydir/bisection-tool.py (right): https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:36: raise RuntimeError("Timeout") On 2016/07/22 13:35:43, stichnot wrote: > For whatever reason, our scripts prefer 'strings' rather than "strings". Done. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:74: print "===Remaining Steps===: " + str(self.estimate(included_ranges)) On 2016/07/22 13:35:43, stichnot wrote: > Should probably rephrase the message to convey that it is not an exact step > count. E.g. > '===Remaining Steps (approx)===:' Done. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:86: def find_crashes(runner, current_interval, include_ranges, find_all): On 2016/07/22 13:35:43, stichnot wrote: > Maybe find_failures? It finds more than just crashes. Done. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:142: For subzero itself crashing, On 2016/07/22 13:35:42, stichnot wrote: > Capitalize Subzero Done. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:143: bisection-tool.py --cmd 'pnacl-sz -translate-only=' --comma-join On 2016/07/22 13:35:42, stichnot wrote: > I think there should be only a single space character after 'pnacl-sz'. Done. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:153: dest='cmd', On 2016/07/22 13:35:43, stichnot wrote: > Probably append this line to the previous line. Done. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:162: argparser.add_argument('--all', dest='all', action='store_true') On 2016/07/22 13:35:42, stichnot wrote: > Instead of --all and --no-all, you could do something like: > > add_argument('--all', type=int, choices=[0,1], default=1) > > This allows: > --all > --all=1 > --all=0 > and reduces the number of args. > > Same for comma-join/separate and template/no-template. > > This is what's currently done for 'szbuild.py --force'. Done. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:184: print "Pass" # The whole input range works, maybe check subzero build flags? On 2016/07/22 13:35:43, stichnot wrote: > Another possibility is (if you expected it not to pass) is that you should > increase the --end value (if bisecting per-instruction or per-variable > transformations on a large function). Done. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:187: print "Failing Functions:" On 2016/07/22 13:35:43, stichnot wrote: > Here and in the comment above, we aren't necessarily talking about functions. > Maybe "items"? Done. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:195: # TODO(manasijm): Pretty print, associate these numbers with filenames On 2016/07/22 13:35:43, stichnot wrote: > I don't think you need this TODO, since this utility is now more general than > just szbuild.py-style "-i" functionality. Done. https://codereview.chromium.org/2162123002/diff/200001/pydir/bisection-tool.p... pydir/bisection-tool.py:198: main() On 2016/07/22 13:35:43, stichnot wrote: > Indent this only 2 characters? Done.
https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-test.py File pydir/bisection-test.py (right): https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-test.p... pydir/bisection-test.py:7: bisection-tool.py --cmd \'./pydir/bisection-test.py -c 2x3\' \ I would change the escaped single-quotes into double-quotes. https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-test.p... pydir/bisection-test.py:23: for string in args.include : No space before the colon. https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-test.p... pydir/bisection-test.py:55: main() Make sure this last line ends with a newline. Also, indent 2 spaces. https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.py File pydir/bisection-tool.py (right): https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.p... pydir/bisection-tool.py:8: import math Sort imports. https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.p... pydir/bisection-tool.py:74: print '===Remaining Steps (approx)===: '\ Add a space before the backslash? https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.p... pydir/bisection-tool.py:91: Spaces around the slash https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.p... pydir/bisection-tool.py:165: argparser.add_argument('--all', type=int, choices=[0,1], default=1, So it looks like I was wrong in my earlier comment saying that "--all" is the same as "--all=1". In fact, in this case "--all" *requires* a 0/1 argument. :( The interwebs say that your original "--all" and "--no-all" are the more standard solution, optionally with the use of 'parser.add_mutually_exclusive_group(...)' to prevent using --all and --no-all at the same time. So I would say, either go back to your original version, or change the documentation examples to explicitly use =0 or =1 . https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.p... pydir/bisection-tool.py:168: argparser.add_argument('--comma_join', type=int, choices=[0,1], default=0, Arg name string should be --comma-join . (hyphen instead of underscore) https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.p... pydir/bisection-tool.py:173: help='Replace %i in the cmd string with the ranges') use '%%i' here, otherwise the --help option breaks. https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.p... pydir/bisection-tool.py:201: main() Make sure this last line ends with a newline.
https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-test.py File pydir/bisection-test.py (right): https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-test.p... pydir/bisection-test.py:23: for string in args.include : On 2016/07/24 14:30:44, stichnot wrote: > No space before the colon. Done. https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-test.p... pydir/bisection-test.py:55: main() On 2016/07/24 14:30:44, stichnot wrote: > Make sure this last line ends with a newline. > > Also, indent 2 spaces. Done. https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.py File pydir/bisection-tool.py (right): https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.p... pydir/bisection-tool.py:8: import math On 2016/07/24 14:30:44, stichnot wrote: > Sort imports. Done. https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.p... pydir/bisection-tool.py:74: print '===Remaining Steps (approx)===: '\ On 2016/07/24 14:30:44, stichnot wrote: > Add a space before the backslash? Done. https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.p... pydir/bisection-tool.py:91: On 2016/07/24 14:30:44, stichnot wrote: > Spaces around the slash Done. https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.p... pydir/bisection-tool.py:165: argparser.add_argument('--all', type=int, choices=[0,1], default=1, On 2016/07/24 14:30:45, stichnot wrote: > So it looks like I was wrong in my earlier comment saying that "--all" is the > same as "--all=1". In fact, in this case "--all" *requires* a 0/1 argument. :( > > The interwebs say that your original "--all" and "--no-all" are the more > standard solution, optionally with the use of > 'parser.add_mutually_exclusive_group(...)' to prevent using --all and --no-all > at the same time. > > So I would say, either go back to your original version, or change the > documentation examples to explicitly use =0 or =1 . Done. https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.p... pydir/bisection-tool.py:168: argparser.add_argument('--comma_join', type=int, choices=[0,1], default=0, On 2016/07/24 14:30:45, stichnot wrote: > Arg name string should be --comma-join . (hyphen instead of underscore) Done. https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.p... pydir/bisection-tool.py:173: help='Replace %i in the cmd string with the ranges') On 2016/07/24 14:30:44, stichnot wrote: > use '%%i' here, otherwise the --help option breaks. Done. https://codereview.chromium.org/2162123002/diff/220001/pydir/bisection-tool.p... pydir/bisection-tool.py:201: main() On 2016/07/24 14:30:44, stichnot wrote: > Make sure this last line ends with a newline. Done.
lgtm
Description was changed from ========== Bisection debugging helper script BUG=none ========== to ========== Bisection debugging helper script BUG=none R=stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) manually as 34e88480eda520540d253fe1662e18d63551c033 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
