Return specific files, not just those in $(ISOLATED_OUTDIR)

client/run_isolated.py

Index: client/run_isolated.py
diff --git a/client/run_isolated.py b/client/run_isolated.py
index bf580e271eda208a28ba84b3f44eff05283193d4..f8b65b56825164611361cef763d829aa23af0436 100755
--- a/client/run_isolated.py
+++ b/client/run_isolated.py
@@ -36,6 +36,7 @@ import json
 import logging
 import optparse
 import os
+import shutil
 import sys
 import tempfile
 import time
@@ -289,6 +290,17 @@ def fetch_and_map(isolated_hash, storage, cache, outdir, use_symlinks):
   }
 
 
+def move_outputs_to_outdir(run_dir, out_dir, outputs):
+  """ Moves/overwrites any named outputs to out_dir so they can be uploaded."""

[M-A Ruel, 2016/10/26 15:43:40]

"""Moves/overwrites ...

[aludwin, 2016/10/26 17:32:14]

Done.

+  if outputs == None:

[M-A Ruel, 2016/10/26 15:43:40]

if not outputs:

[aludwin, 2016/10/26 17:32:14]

Done.

+    return
+  isolateserver.create_directories(out_dir, outputs)
+  for o in outputs:
+    shutil.copyfile(

[M-A Ruel, 2016/10/26 15:43:40]

this doesn't fit a line?

please use instead:
file_path.link_file(os.path.join(out_dir, o), os.path.join(run_dir, o),
file_PATH.HARDLINK_WITH_FALLBACK)

this will be faster and works in case of MAX_PATH.

[aludwin, 2016/10/26 17:32:14]

Done.

+        os.path.join(run_dir, o),
+        os.path.join(out_dir, o))
+
+
 def delete_and_upload(storage, out_dir, leak_temp_dir):
   """Deletes the temporary run directory and uploads results back.
 
@@ -301,7 +313,6 @@ def delete_and_upload(storage, out_dir, leak_temp_dir):
           behind.
     - stats: uploading stats.
   """
-
   # Upload out_dir and generate a .isolated file out of this directory. It is
   # only done if files were written in the directory.
   outputs_ref = None
@@ -351,9 +362,9 @@ def delete_and_upload(storage, out_dir, leak_temp_dir):
 
 
 def map_and_run(
-    command, isolated_hash, storage, isolate_cache, leak_temp_dir, root_dir,
-    hard_timeout, grace_period, bot_file, extra_args, install_packages_fn,
-    use_symlinks):
+    command, isolated_hash, storage, isolate_cache, outputs, leak_temp_dir,
+    root_dir, hard_timeout, grace_period, bot_file, extra_args,
+    install_packages_fn, use_symlinks):
   """Runs a command with optional isolated input/output.
 
   See run_tha_test for argument documentation.
@@ -439,6 +450,11 @@ def map_and_run(
       cwd = os.path.normpath(os.path.join(cwd, bundle.relative_cwd))
       command = bundle.command + extra_args
 
+    # If we have an explicit list of files to return, make sure their
+    # directories exist now

[M-A Ruel, 2016/10/26 15:43:39]

add period.

[aludwin, 2016/10/26 17:32:14]

Done.

+    if storage and outputs != None:

[M-A Ruel, 2016/10/26 15:43:39]

if storage and outputs:

[aludwin, 2016/10/26 17:32:14]

Done.

+      isolateserver.create_directories(run_dir, outputs)
+
     command = tools.fix_python_path(command)
     command = process_command(command, out_dir, bot_file)
     file_path.ensure_command_has_abs_path(command, cwd)
@@ -456,6 +472,19 @@ def map_and_run(
     logging.exception('internal failure: %s', e)
     result['internal_failure'] = str(e)
     on_error.report(None)
+
+  # Try to copy files to the output directory, if specified. They must *all*
+  # exist; we'll treat this as a task failure if we can't copy the files for

[M-A Ruel, 2016/10/26 15:43:40]

can the outputs be directories? I feel this should be supported too. This can be
differentiated with a trailing /.

I mean, let's get this in and decide later.

Probably worth asking Ola.

[aludwin, 2016/10/26 17:32:14]

Currently, Bazel doesn't support this - but it will. I wasn't planning on
working on it until then.

+  # some reason.
+  try:
+    if out_dir:
+      move_outputs_to_outdir(run_dir, out_dir, outputs)
+  except Exception as e:

[M-A Ruel, 2016/10/26 15:43:40]

Please catch the exceptions at move_outputs_to_outdir() and only catch the
relevant onces.

Also, having only a subset of the outputs should still save the ones that were
there.

Finally, please don't overwrite exit_code if it were already set to non-zero.
Actually, I'm not feeling good about this, this is a layering violation. I mean,
if the files are not there, it's up to the caller to handle this. So I feel this
shouldn't be enforced.

[aludwin, 2016/10/26 17:32:14]

Done.

+    sys.stderr.write('<Could not return requested files: %s>' % e)
+    result['exit_code'] = 1
+    return result
+
+  # Clean up
   finally:
     try:
       if leak_temp_dir:
@@ -512,9 +541,9 @@ def map_and_run(
 
 
 def run_tha_test(
-    command, isolated_hash, storage, isolate_cache, leak_temp_dir, result_json,
-    root_dir, hard_timeout, grace_period, bot_file, extra_args,
-    install_packages_fn, use_symlinks):
+    command, isolated_hash, storage, isolate_cache, outputs,
+    leak_temp_dir, result_json, root_dir, hard_timeout, grace_period,
+    bot_file, extra_args, install_packages_fn, use_symlinks):
   """Runs an executable and records execution metadata.
 
   Either command or isolated_hash must be specified.
@@ -577,9 +606,9 @@ def run_tha_test(
 
   # run_isolated exit code. Depends on if result_json is used or not.
   result = map_and_run(
-      command, isolated_hash, storage, isolate_cache, leak_temp_dir, root_dir,
-      hard_timeout, grace_period, bot_file, extra_args, install_packages_fn,
-      use_symlinks)
+      command, isolated_hash, storage, isolate_cache, outputs,
+      leak_temp_dir, root_dir, hard_timeout, grace_period, bot_file,
+      extra_args, install_packages_fn, use_symlinks)
   logging.info('Result:\n%s', tools.format_json(result, dense=True))
 
   if result_json:
@@ -734,6 +763,14 @@ def create_option_parser():
       help='Path to a file describing the state of the host. The content is '
            'defined by on_before_task() in bot_config.')
   parser.add_option(
+      '--output', action='append',
+      help='Specifies an output to return. If no outputs are specified, all '
+           'files located in $(ISOLATED_OUT_DIR) will be returned; '
+           'otherwise, outputs in both $(ISOLATED_OUT_DIR) and those '
+           'specified by --output option (there can be multiple) will be '
+           'returned. Note that if a file in OUT_DIR has the same path '
+           'as an --output option, the --output version will be returned.')
+  parser.add_option(
       '-a', '--argsfile',
       # This is actually handled in parse_args; it's included here purely so it
       # can make it into the help text.
@@ -846,15 +883,15 @@ def main(args):
         # Hashing schemes used by |storage| and |isolated_cache| MUST match.
         assert storage.hash_algo == isolated_cache.hash_algo
         return run_tha_test(
-            command, options.isolated, storage, isolated_cache,
+            command, options.isolated, storage, isolated_cache, options.output,
             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)
     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)
+        command, options.isolated, None, isolated_cache, options.output,
+        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