Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2443663002:
Pass args in file from task_runner to run_isolated (Closed)
DescriptionPass args in file from task_runner to run_isolated
BUG=
Committed: https://github.com/luci/luci-py/commit/b57b137e5018958549e2731b10209161ed65ec75
Patch Set 1 #
Total comments: 15
Patch Set 2 : Respond to code review comments other than changes to options processing #
Total comments: 7
Patch Set 3 : Parse argsfile with argparse instead of custom code #Patch Set 4 : Bump run_isolated version number #
Total comments: 8
Patch Set 5 : More review feedback #
Total comments: 13
Patch Set 6 : Reviews for PS5 #Patch Set 7 : Rebase to latest master #
Dependent Patchsets: Messages
Total messages: 21 (5 generated)
aludwin@google.com changed reviewers: + maruel@google.com
maruel@chromium.org changed reviewers: + maruel@chromium.org
https://codereview.chromium.org/2443663002/diff/1/appengine/swarming/swarming... File appengine/swarming/swarming_bot/bot_code/task_runner.py (right): https://codereview.chromium.org/2443663002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/bot_code/task_runner.py:423: fail_on_start = lambda exit_code, stdout: fail_without_command(remote, wrap remote on next line so they are aligned. https://codereview.chromium.org/2443663002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/bot_code/task_runner.py:434: "Could not write args to %s: %s" % (args_path, e)) single quotes, align all args the same. https://codereview.chromium.org/2443663002/diff/1/appengine/swarming/swarming... File appengine/swarming/swarming_bot/bot_code/task_runner_test.py (right): https://codereview.chromium.org/2443663002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/bot_code/task_runner_test.py:1100: self.maxDiff = None Remove, you just have to pass -v on the command line if you want to disable maxDiff. https://codereview.chromium.org/2443663002/diff/1/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2443663002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:704: def create_option_parser(): def create_option_parser(add_file_flag): https://codereview.chromium.org/2443663002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:708: log_file=RUN_ISOLATED_LOG_FILE) if add_file_flag: parser.add_option('--file', '-f', help='Use a response file') https://codereview.chromium.org/2443663002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:763: parser = create_option_parser() parser = create_option_parser(True) https://codereview.chromium.org/2443663002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:765: if options.file: if args: parser.error('Can\'t use both --file and arguments') parser = create_option_parser(False) with open(options.file, 'r') as f: args = json.loads(f.read()) options, args = parser.parse_args(options.file) actually I'd probably prefer options2 then try to merge both but it's tricky. Reading /usr/lib/python2.7/optparse.py, the magic class to look at is Values. So something like: options2, args = parser.parse_args(options.file) options._update_careful(options2.__dict__) https://codereview.chromium.org/2443663002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:826: 2 lines
https://codereview.chromium.org/2443663002/diff/1/appengine/swarming/swarming... File appengine/swarming/swarming_bot/bot_code/task_runner.py (right): https://codereview.chromium.org/2443663002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/bot_code/task_runner.py:423: fail_on_start = lambda exit_code, stdout: fail_without_command(remote, On 2016/10/21 20:15:19, M-A Ruel wrote: > wrap remote on next line so they are aligned. Done. https://codereview.chromium.org/2443663002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/bot_code/task_runner.py:434: "Could not write args to %s: %s" % (args_path, e)) On 2016/10/21 20:15:19, M-A Ruel wrote: > single quotes, align all args the same. Done. https://codereview.chromium.org/2443663002/diff/1/appengine/swarming/swarming... File appengine/swarming/swarming_bot/bot_code/task_runner_test.py (right): https://codereview.chromium.org/2443663002/diff/1/appengine/swarming/swarming... appengine/swarming/swarming_bot/bot_code/task_runner_test.py:1100: self.maxDiff = None On 2016/10/21 20:15:20, M-A Ruel wrote: > Remove, you just have to pass -v on the command line if you want to disable > maxDiff. Yeesh. Would have been nice if someone said that on the internet! Of course, I'd forgotten to take this one out. https://codereview.chromium.org/2443663002/diff/1/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2443663002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:765: > actually I'd probably prefer options2 then try to merge both but it's tricky. > options._update_careful(options2.__dict__) Looks kind of dangerous, relying on an internal function? I'd be happier going with the explicit solution, I think. What will the "with open" idiom do if there's an exception? https://codereview.chromium.org/2443663002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:826: On 2016/10/21 20:15:20, M-A Ruel wrote: > 2 lines Done.
https://codereview.chromium.org/2443663002/diff/1/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2443663002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:765: So it looks like parser.parse_args can only be called once per process. The issue appears to be that OptionParserWithLogging ties together the act of parsing options (which you should be able to do multiple times) with the act of setting up process-wide logging (see utils/logging_utils.py: prepare_logging). I suspect that these two things are tied together so that the logs capture any messages from argument processing, but *also* allow the argument processing to set the log file. Or something. Regardless, I'd rather not play with this code in this changelist. I also think there's some justification to the method I put in, which is to explicitly check for "-f file" and *no other options* - basically, "-f" overrides anything else that would be on the command line and I don't think it's that bad having a separate code path for it. Any other recommendations? https://codereview.chromium.org/2443663002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:826: On 2016/10/21 20:32:00, aludwin wrote: > On 2016/10/21 20:15:20, M-A Ruel wrote: > > 2 lines > > Done. Actually, un-done - see comment above.
https://codereview.chromium.org/2443663002/diff/20001/appengine/swarming/swar... File appengine/swarming/swarming_bot/bot_code/task_runner.py (right): https://codereview.chromium.org/2443663002/diff/20001/appengine/swarming/swar... appengine/swarming/swarming_bot/bot_code/task_runner.py:430: args_file = open(args_path, "w") with open(... https://codereview.chromium.org/2443663002/diff/20001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2443663002/diff/20001/client/run_isolated.py#... client/run_isolated.py:30: __version__ = '0.8.5' don't forget to bump. https://codereview.chromium.org/2443663002/diff/20001/client/run_isolated.py#... client/run_isolated.py:828: if len(args) == 2 and args[0] == '-f': then create a temporary optparse with only the -f flag supported. Make sure to not handle --help though.
https://codereview.chromium.org/2443663002/diff/20001/appengine/swarming/swar... File appengine/swarming/swarming_bot/bot_code/task_runner.py (right): https://codereview.chromium.org/2443663002/diff/20001/appengine/swarming/swar... appengine/swarming/swarming_bot/bot_code/task_runner.py:430: args_file = open(args_path, "w") On 2016/10/24 16:26:52, M-A Ruel wrote: > with open(... Do I put that inside a try-catch?
https://codereview.chromium.org/2443663002/diff/20001/appengine/swarming/swar... File appengine/swarming/swarming_bot/bot_code/task_runner.py (right): https://codereview.chromium.org/2443663002/diff/20001/appengine/swarming/swar... appengine/swarming/swarming_bot/bot_code/task_runner.py:430: args_file = open(args_path, "w") On 2016/10/24 17:05:48, aludwin wrote: > On 2016/10/24 16:26:52, M-A Ruel wrote: > > with open(... > > Do I put that inside a try-catch? yes, so it looks like: try: with open(args_path, 'w') as f: json.dump(args, f) except (IOError, OSError) as e: ... Notes: - single quotes - use f instead of args_file - order the exception class name alphabetically
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2443663002/diff/20001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2443663002/diff/20001/client/run_isolated.py#... client/run_isolated.py:30: __version__ = '0.8.5' On 2016/10/24 16:26:52, M-A Ruel wrote: > don't forget to bump. I haven't changed the file format itself, so is this necessary?
https://codereview.chromium.org/2443663002/diff/20001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2443663002/diff/20001/client/run_isolated.py#... client/run_isolated.py:30: __version__ = '0.8.5' On 2016/10/24 18:32:05, aludwin wrote: > On 2016/10/24 16:26:52, M-A Ruel wrote: > > don't forget to bump. > > I haven't changed the file format itself, so is this necessary? You add a new flag. The API changes.
https://codereview.chromium.org/2443663002/diff/80001/appengine/swarming/swar... File appengine/swarming/swarming_bot/bot_code/task_runner.py (right): https://codereview.chromium.org/2443663002/diff/80001/appengine/swarming/swar... appengine/swarming/swarming_bot/bot_code/task_runner.py:430: with open(args_path, "w") as f: single quotes https://codereview.chromium.org/2443663002/diff/80001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2443663002/diff/80001/client/run_isolated.py#... client/run_isolated.py:765: # it's not documented here; instead, it's documented in create_option_parser It's not? https://codereview.chromium.org/2443663002/diff/80001/client/run_isolated.py#... client/run_isolated.py:768: file_argparse = argparse.ArgumentParser() file_argparse = argparse.ArgumentParser(add_help=False) Yep it's not documented, why do you ask? :P https://codereview.chromium.org/2443663002/diff/80001/client/run_isolated.py#... client/run_isolated.py:778: except Exception as e: except (IOError, OSError, ValueError): as e: print >> sys.stderr, 'Couldn't read arguments: %s' % e sys.exit(1) well, I'd prefer to return instead of sys.exit..
https://codereview.chromium.org/2443663002/diff/80001/appengine/swarming/swar... File appengine/swarming/swarming_bot/bot_code/task_runner.py (right): https://codereview.chromium.org/2443663002/diff/80001/appengine/swarming/swar... appengine/swarming/swarming_bot/bot_code/task_runner.py:430: with open(args_path, "w") as f: On 2016/10/24 20:25:09, M-A Ruel wrote: > single quotes Done. https://codereview.chromium.org/2443663002/diff/80001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2443663002/diff/80001/client/run_isolated.py#... client/run_isolated.py:765: # it's not documented here; instead, it's documented in create_option_parser On 2016/10/24 20:25:09, M-A Ruel wrote: > It's not? Whoops, I forgot to actually document it in create_option_parser. Fixed. https://codereview.chromium.org/2443663002/diff/80001/client/run_isolated.py#... client/run_isolated.py:768: file_argparse = argparse.ArgumentParser() On 2016/10/24 20:25:09, M-A Ruel wrote: > file_argparse = argparse.ArgumentParser(add_help=False) > > Yep it's not documented, why do you ask? :P Done. https://codereview.chromium.org/2443663002/diff/80001/client/run_isolated.py#... client/run_isolated.py:778: except Exception as e: On 2016/10/24 20:25:09, M-A Ruel wrote: > except (IOError, OSError, ValueError): as e: > print >> sys.stderr, 'Couldn't read arguments: %s' % e > sys.exit(1) > > well, I'd prefer to return instead of sys.exit.. We don't need to sys.exit or return. After this line, "args" will be empty so when we call parser.parse_args(args), we'll get the help and error out then. I'll add a comment.
https://codereview.chromium.org/2443663002/diff/100001/appengine/swarming/swa... File appengine/swarming/swarming_bot/bot_code/task_runner.py (right): https://codereview.chromium.org/2443663002/diff/100001/appengine/swarming/swa... appengine/swarming/swarming_bot/bot_code/task_runner.py:432: except (OSError, IOError) as e: (IOError, OSError) https://codereview.chromium.org/2443663002/diff/100001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2443663002/diff/100001/client/run_isolated.py... client/run_isolated.py:774: # because -f is exclusive with all other options and arguments. -f? https://codereview.chromium.org/2443663002/diff/100001/client/run_isolated.py... client/run_isolated.py:776: file_argparse.add_argument("-a", "--argsfile") single quotes https://codereview.chromium.org/2443663002/diff/100001/client/run_isolated.py... client/run_isolated.py:778: if not file_args.argsfile == None: don't use this pattern, do: if file_args.argsfile: https://codereview.chromium.org/2443663002/diff/100001/client/run_isolated.py... client/run_isolated.py:779: if len(nonfile_args) > 0: if nonfile_args: https://codereview.chromium.org/2443663002/diff/100001/client/run_isolated.py... client/run_isolated.py:784: args = json.loads(f.read()) args = json.load(f) https://codereview.chromium.org/2443663002/diff/100001/client/run_isolated.py... client/run_isolated.py:789: print >> sys.stderr, "Couldn't read arguments: %s" % e use single quote and escape \'
https://codereview.chromium.org/2443663002/diff/100001/appengine/swarming/swa... File appengine/swarming/swarming_bot/bot_code/task_runner.py (right): https://codereview.chromium.org/2443663002/diff/100001/appengine/swarming/swa... appengine/swarming/swarming_bot/bot_code/task_runner.py:432: except (OSError, IOError) as e: On 2016/10/24 21:19:28, M-A Ruel wrote: > (IOError, OSError) Done. https://codereview.chromium.org/2443663002/diff/100001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2443663002/diff/100001/client/run_isolated.py... client/run_isolated.py:774: # because -f is exclusive with all other options and arguments. On 2016/10/24 21:19:28, M-A Ruel wrote: > -f? Now --argsfile; fixed. https://codereview.chromium.org/2443663002/diff/100001/client/run_isolated.py... client/run_isolated.py:776: file_argparse.add_argument("-a", "--argsfile") On 2016/10/24 21:19:28, M-A Ruel wrote: > single quotes Done. https://codereview.chromium.org/2443663002/diff/100001/client/run_isolated.py... client/run_isolated.py:778: if not file_args.argsfile == None: On 2016/10/24 21:19:28, M-A Ruel wrote: > don't use this pattern, do: > if file_args.argsfile: Done. https://codereview.chromium.org/2443663002/diff/100001/client/run_isolated.py... client/run_isolated.py:779: if len(nonfile_args) > 0: On 2016/10/24 21:19:28, M-A Ruel wrote: > if nonfile_args: Done, didn't realize [] was falsy. https://codereview.chromium.org/2443663002/diff/100001/client/run_isolated.py... client/run_isolated.py:789: print >> sys.stderr, "Couldn't read arguments: %s" % e On 2016/10/24 21:19:28, M-A Ruel wrote: > use single quote and escape \' Done.
lgtm
The CQ bit was checked by aludwin@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Pass args in file from task_runner to run_isolated BUG= ========== to ========== Pass args in file from task_runner to run_isolated BUG= Committed: https://github.com/luci/luci-py/commit/b57b137e5018958549e2731b10209161ed65ec75 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://github.com/luci/luci-py/commit/b57b137e5018958549e2731b10209161ed65ec75 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
