Pass args in file from task_runner to run_isolated

client/run_isolated.py

 #!/usr/bin/env python
 # Copyright 2012 The LUCI Authors. All rights reserved.
 # Use of this source code is governed under the Apache License, Version 2.0
 # that can be found in the LICENSE file.
 
 """Runs a command with optional isolated input/output.
 
 Despite name "run_isolated", can run a generic non-isolated command specified as
 args.
 
@@ skipping 13 matching lines @@
 Any ${SWARMING_BOT_FILE} on the command line will be replaced by the value of
 the --bot-file parameter. This file is used by a swarming bot to communicate
 state of the host to tasks. It is written to by the swarming bot's
 on_before_task() hook in the swarming server's custom bot_config.py.
 """
 
 __version__ = '0.8.5'
 
 import base64
 import collections
+import json
 import logging
 import optparse
 import os
 import sys
 import tempfile
 import time
 
 from third_party.depot_tools import fix_encoding
 
 from utils import file_path
@@ skipping 649 matching lines @@
       'duration': total_duration,
       'get_client_duration': get_client_duration,
     },
     'cipd_pins': {
       'client_package': client_package,
       'packages': package_pins,
     }
   }
 
 
 def create_option_parser():

[M-A Ruel, 2016/10/21 20:15:20]

def create_option_parser(add_file_flag):

   parser = logging_utils.OptionParserWithLogging(
       usage='%prog <options> [command to run or extra args]',
       version=__version__,
       log_file=RUN_ISOLATED_LOG_FILE)

[M-A Ruel, 2016/10/21 20:15:20]

if add_file_flag:
  parser.add_option('--file', '-f', help='Use a response file')

   parser.add_option(
       '--clean', action='store_true',
       help='Cleans the cache, trimming it necessary and remove corrupted items '
            'and returns without executing anything; use with -v to know what '
            'was done')
   parser.add_option(
       '--no-clean', action='store_true',
       help='Do not clean the cache automatically on startup. This is meant for '
            'bots where a separate execution with --clean was done earlier so '
            'doing it again is redundant')
@@ skipping 34 matching lines @@
       '--root-dir', help='Use a directory instead of a random one')
   parser.add_option_group(debug_group)
 
   auth.add_auth_options(parser)
 
   parser.set_defaults(cache='cache', cipd_cache='cipd_cache')
   return parser
 
 
 def main(args):
   parser = create_option_parser()

[M-A Ruel, 2016/10/21 20:15:20]

parser = create_option_parser(True)

   options, args = parser.parse_args(args)
 

[M-A Ruel, 2016/10/21 20:15:20]

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__)

[aludwin, 2016/10/21 20:32:00]

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?

[aludwin, 2016/10/24 13:33:31]

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?

   isolated_cache = isolateserver.process_cache_options(options)
   if options.clean:
     if options.isolated:
       parser.error('Can\'t use --isolated with --clean.')
     if options.isolate_server:
       parser.error('Can\'t use --isolate-server with --clean.')
     if options.json:
       parser.error('Can\'t use --json with --clean.')
     isolated_cache.cleanup()
     return 0
@@ skipping 40 matching lines @@
             options.hard_timeout, options.grace_period, options.bot_file, args,
             install_packages_fn, options.use_symlinks)
     return run_tha_test(
         command, options.isolated, None, isolated_cache, options.leak_temp_dir,
         options.json, options.root_dir, options.hard_timeout,
         options.grace_period, options.bot_file, args, install_packages_fn,
         options.use_symlinks)
   except cipd.Error as ex:
     print >> sys.stderr, ex.message
     return 1
 

[M-A Ruel, 2016/10/21 20:15:20]

2 lines

[aludwin, 2016/10/21 20:32:00]

Done.

[aludwin, 2016/10/24 13:33:31]

Actually, un-done - see comment above.

+def get_args(args):
+  if len(args) == 2 and args[0] == '-f':
+    # If the only args are "-f filename", load those
+    # instead.
+    argfile = args[1]
+    args = []
+    try:
+      f = open(argfile, 'r')
+      args = json.loads(f.read())
+      f.close()
+    except Exception as e:
+      print "Couldn't read arguments: %s"%e
+  return args
 
 if __name__ == '__main__':
   subprocess42.inhibit_os_error_reporting()
   # Ensure that we are always running with the correct encoding.
   fix_encoding.fix_encoding()
   file_path.enable_symlink()
-  sys.exit(main(sys.argv[1:]))
+
+  sys.exit(main(get_args(sys.argv[1:])))