run_isolated: support non-isolated commands

client/run_isolated.py

Index: client/run_isolated.py
diff --git a/client/run_isolated.py b/client/run_isolated.py
index f74a8391dca5839bdb1fa94093c4aad20b49c584..8dafaa493c355d06aaad04242b8737870fb56a35 100755
--- a/client/run_isolated.py
+++ b/client/run_isolated.py
@@ -3,10 +3,15 @@
 # Use of this source code is governed by the Apache v2.0 license that can be
 # found in the LICENSE file.
 
-"""Reads a .isolated, creates a tree of hardlinks and runs the test.
+"""Runs a command with optional isolated input/output.
 
-To improve performance, it keeps a local cache. The local cache can safely be
-deleted.
+Despite name "run_isolated", can run a generic non-isolated command specified as
+args.
+
+If input isolated hash is provided, fetches it, creates a tree of hard links,
+appends args to the command in the fetched isolated and runs it.
+To improve performance, keeps a local cache.
+The local cache can safely be deleted.
 
 Any ${ISOLATED_OUTDIR} on the command line will be replaced by the location of a
 temporary directory upon execution of the command specified in the .isolated
@@ -14,7 +19,7 @@ file. All content written to this directory will be uploaded upon termination
 and the .isolated file describing this directory will be printed to stdout.
 """
 
-__version__ = '0.6.1'
+__version__ = '0.7.0'
 
 import base64
 import logging
@@ -36,10 +41,11 @@ from utils import tools
 from utils import zip_package
 
 import auth
-import isolated_format
 import isolateserver
 
 
+ISOLATED_OUTDIR_PARAMETER = '${ISOLATED_OUTDIR}'
+
 # Absolute path to this file (can be None if running from zip on Mac).
 THIS_FILE_PATH = os.path.abspath(__file__) if __file__ else None
 
@@ -136,12 +142,8 @@ def change_tree_read_only(rootdir, read_only):
 
 def process_command(command, out_dir):
   """Replaces isolated specific variables in a command line."""
-  def fix(arg):
-    if '${ISOLATED_OUTDIR}' in arg:
-      return arg.replace('${ISOLATED_OUTDIR}', out_dir).replace('/', os.sep)
-    return arg
-
-  return [fix(arg) for arg in command]
+  out_dir = out_dir.replace('/', os.sep)
+  return [arg.replace(ISOLATED_OUTDIR_PARAMETER, out_dir) for arg in command]

[M-A Ruel, 2016/05/02 14:18:14]

The reason for the previous code was things like:

"${ISOLATED_OUTDIR}/foo/bar"

[nodir, 2016/05/02 17:43:22]

got it, reverted, added comment

 
 
 def run_command(command, cwd, tmp_dir, hard_timeout, grace_period):
@@ -302,9 +304,14 @@ def delete_and_upload(storage, out_dir, leak_temp_dir):
 
 
 def map_and_run(
-    isolated_hash, storage, cache, leak_temp_dir, root_dir, hard_timeout,
-    grace_period, extra_args):
-  """Maps and run the command. Returns metadata about the result."""
+    isolated_hash, args, storage, cache, leak_temp_dir, root_dir, hard_timeout,
+    grace_period):
+  """Runs a command with optional isolated input/output.
+
+  See run_tha_test for argument documentation.
+
+  Returns metadata about the result.
+  """
   result = {
     'duration': None,
     'exit_code': None,
@@ -336,26 +343,30 @@ def map_and_run(
   run_dir = make_temp_dir(prefix + u'run', root_dir)
   out_dir = make_temp_dir(prefix + u'out', root_dir)
   tmp_dir = make_temp_dir(prefix + u'tmp', root_dir)
+  cwd = run_dir
+
   try:
-    bundle, result['stats']['download'] = fetch_and_measure(
-        isolated_hash=isolated_hash,
-        storage=storage,
-        cache=cache,
-        outdir=run_dir)
-    if not bundle.command:
-      # Handle this as a task failure, not an internal failure.
-      sys.stderr.write(
-          '<The .isolated doesn\'t declare any command to run!>\n'
-          '<Check your .isolate for missing \'command\' variable>\n')
-      if os.environ.get('SWARMING_TASK_ID'):
-        # Give an additional hint when running as a swarming task.
-        sys.stderr.write('<This occurs at the \'isolate\' step>\n')
-      result['exit_code'] = 1
-      return result
+    command = args
+    if isolated_hash:
+      bundle, result['stats']['download'] = fetch_and_measure(
+          isolated_hash=isolated_hash,
+          storage=storage,
+          cache=cache,
+          outdir=run_dir)
+      if not bundle.command:
+        # Handle this as a task failure, not an internal failure.
+        sys.stderr.write(
+            '<The .isolated doesn\'t declare any command to run!>\n'
+            '<Check your .isolate for missing \'command\' variable>\n')
+        if os.environ.get('SWARMING_TASK_ID'):
+          # Give an additional hint when running as a swarming task.
+          sys.stderr.write('<This occurs at the \'isolate\' step>\n')
+        result['exit_code'] = 1
+        return result
 
-    change_tree_read_only(run_dir, bundle.read_only)
-    cwd = os.path.normpath(os.path.join(run_dir, bundle.relative_cwd))
-    command = bundle.command + extra_args
+      change_tree_read_only(run_dir, bundle.read_only)
+      cwd = os.path.normpath(os.path.join(cwd, bundle.relative_cwd))
+      command = bundle.command + args
     file_path.ensure_command_has_abs_path(command, cwd)
     sys.stdout.flush()
     start = time.time()
@@ -424,18 +435,25 @@ def map_and_run(
 
 
 def run_tha_test(
-    isolated_hash, storage, cache, leak_temp_dir, result_json, root_dir,
-    hard_timeout, grace_period, extra_args):
-  """Downloads the dependencies in the cache, hardlinks them into a temporary
-  directory and runs the executable from there.
+    isolated_hash, args, storage, cache, leak_temp_dir, result_json, root_dir,
+    hard_timeout, grace_period):
+  """Runs an executable and records execution metadata.
+
+  If isolated_hash is specified, downloads the dependencies in the cache,
+  hardlinks them into a temporary directory and runs the command specified in
+  the .isolated.
 
   A temporary directory is created to hold the output files. The content inside
   this directory will be uploaded back to |storage| packaged as a .isolated
   file.
 
   Arguments:
-    isolated_hash: the SHA-1 of the .isolated file that must be retrieved to
-                   recreate the tree of files to run the target executable.
+    isolated_hash: if not empty, the SHA-1 of the .isolated file that must be
+                   retrieved to recreate the tree of files to run the target
+                   executable. The command specified in the .isolated is
+                   executed.
+    args: if isolated_hash is not empty, arguments to append to the command in
+          the .isolated file. Otherwise, the command line to execute.

[M-A Ruel, 2016/05/02 14:51:17]

I'm not a fan of this ambivalence.

[nodir, 2016/05/02 17:43:22]

fixed

     storage: an isolateserver.Storage object to retrieve remote objects. This
              object has a reference to an isolateserver.StorageApi, which does
              the actual I/O.
@@ -451,12 +469,13 @@ def run_tha_test(
     hard_timeout: kills the process if it lasts more than this amount of
                   seconds.
     grace_period: number of seconds to wait between SIGTERM and SIGKILL.
-    extra_args: optional arguments to add to the command stated in the .isolate
-                file.
 
   Returns:
     Process exit code that should be used.
   """
+  if any(ISOLATED_OUTDIR_PARAMETER in a for a in args):
+    assert storage is not None, 'storage is None although outdir is specified'
+
   if result_json:
     # Write a json output file right away in case we get killed.
     result = {
@@ -470,8 +489,8 @@ def run_tha_test(
 
   # run_isolated exit code. Depends on if result_json is used or not.
   result = map_and_run(
-      isolated_hash, storage, cache, leak_temp_dir, root_dir, hard_timeout,
-      grace_period, extra_args)
+      isolated_hash, args, storage, cache, leak_temp_dir, root_dir,
+      hard_timeout, grace_period)
   logging.info('Result:\n%s', tools.format_json(result, dense=True))
   if result_json:
     # We've found tests to delete 'work' when quitting, causing an exception
@@ -498,7 +517,7 @@ def run_tha_test(
 
 def main(args):
   parser = logging_utils.OptionParserWithLogging(
-      usage='%prog <options>',
+      usage='%prog <options> [args]',

[M-A Ruel, 2016/05/02 14:51:17]

[command to run or extra args]

[nodir, 2016/05/02 17:43:22]

Done.

       version=__version__,
       log_file=RUN_ISOLATED_LOG_FILE)
   parser.add_option(
@@ -518,7 +537,7 @@ def main(args):
   data_group = optparse.OptionGroup(parser, 'Data source')
   data_group.add_option(
       '-s', '--isolated',
-      help='Hash of the .isolated to grab from the isolate server')
+      help='Hash of the .isolated to grab from the isolate server.')
   isolateserver.add_isolate_server_options(data_group)
   parser.add_option_group(data_group)
 
@@ -549,22 +568,37 @@ def main(args):
     cache.cleanup()
     return 0
 
+  if not options.isolated and not args:
+    parser.error('--isolated or args is required.')

[M-A Ruel, 2016/05/02 14:51:17]

s/args/command to run/ ?

[nodir, 2016/05/02 17:43:22]

Done.

+
   auth.process_auth_options(parser, options)
-  isolateserver.process_isolate_server_options(parser, options, True)
+
+  isolateserver.process_isolate_server_options(
+      parser, options, True, required=False)
+  if not options.isolate_server:
+    if options.isolated:
+      parser.error('--isolated requires --isolate-server')
+    if ISOLATED_OUTDIR_PARAMETER in args:
+      parser.error(
+          '%s in args requires --isolate-server' % ISOLATED_OUTDIR_PARAMETER)
 
   if options.root_dir:
     options.root_dir = unicode(os.path.abspath(options.root_dir))
   if options.json:
     options.json = unicode(os.path.abspath(options.json))
-  if not options.isolated:
-    parser.error('--isolated is required.')
-  with isolateserver.get_storage(
-      options.isolate_server, options.namespace) as storage:
-    # Hashing schemes used by |storage| and |cache| MUST match.
+
+  storage = None
+  if options.isolate_server:
+    storage = isolateserver.get_storage(
+        options.isolate_server, options.namespace)
     assert storage.hash_algo == cache.hash_algo
+
+  with storage or tools.noop_context():

[M-A Ruel, 2016/05/02 14:51:17]

while I understand the goal to not copy paste the function call twice, I'm not
sure this is worth, I think it's fine to just copy paste lines 598-601.

[nodir, 2016/05/02 17:43:22]

Done.

+    # Hashing schemes used by |storage| and |cache| MUST match.
     return run_tha_test(
-        options.isolated, storage, cache, options.leak_temp_dir, options.json,
-        options.root_dir, options.hard_timeout, options.grace_period, args)
+        options.isolated, args, storage, cache, options.leak_temp_dir,
+        options.json, options.root_dir, options.hard_timeout,
+        options.grace_period)
 
 
 if __name__ == '__main__':