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

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

BUG=

Committed: https://github.com/luci/luci-py/commit/ba9fd6cdeb951478fee252d183629fd0a86449cc

[aludwin, 2016-10-26 15:28:27]

Patchset #2 (id:20001) has been deleted

[aludwin, 2016-10-26 15:28:35]

Patchset #1 (id:1) has been deleted

[aludwin, 2016-10-26 15:33:13]

Patchset #1 (id:40001) has been deleted

[aludwin, 2016-10-26 15:34:09]

aludwin@google.com changed reviewers:
+ maruel@google.com

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

maruel@chromium.org changed reviewers:
+ maruel@chromium.org
- maruel@google.com

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

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py#...
client/run_isolated.py:294: """ Moves/overwrites any named outputs to out_dir so
they can be uploaded."""
"""Moves/overwrites ...

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py#...
client/run_isolated.py:295: if outputs == None:
if not outputs:

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py#...
client/run_isolated.py:299: shutil.copyfile(
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.

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py#...
client/run_isolated.py:454: # directories exist now
add period.

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py#...
client/run_isolated.py:455: if storage and outputs != None:
if storage and outputs:

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py#...
client/run_isolated.py:477: # exist; we'll treat this as a task failure if we
can't copy the files for
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.

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py#...
client/run_isolated.py:482: except Exception as e:
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]

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py#...
client/run_isolated.py:294: """ Moves/overwrites any named outputs to out_dir so
they can be uploaded."""
On 2016/10/26 15:43:40, M-A Ruel wrote:
> """Moves/overwrites ...

Done.

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py#...
client/run_isolated.py:295: if outputs == None:
On 2016/10/26 15:43:40, M-A Ruel wrote:
> if not outputs:

Done.

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py#...
client/run_isolated.py:299: shutil.copyfile(
On 2016/10/26 15:43:40, M-A Ruel wrote:
> 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.

Done.

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py#...
client/run_isolated.py:454: # directories exist now
On 2016/10/26 15:43:39, M-A Ruel wrote:
> add period.

Done.

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py#...
client/run_isolated.py:455: if storage and outputs != None:
On 2016/10/26 15:43:39, M-A Ruel wrote:
> if storage and outputs:

Done.

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py#...
client/run_isolated.py:477: # exist; we'll treat this as a task failure if we
can't copy the files for
On 2016/10/26 15:43:40, M-A Ruel wrote:
> 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.

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

https://codereview.chromium.org/2440353004/diff/60001/client/run_isolated.py#...
client/run_isolated.py:482: except Exception as e:
On 2016/10/26 15:43:40, M-A Ruel wrote:
> 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.

Done.

[aludwin, 2016-10-26 17:35:00]

Description was changed from

==========
Return specific files, not just those in $(ISOLATED_OUT_DIR)

BUG=
==========

to

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

BUG=
==========

[M-A Ruel, 2016-10-27 19:22:20]

small nits

https://codereview.chromium.org/2440353004/diff/140001/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/2440353004/diff/140001/client/run_isolated.py...
client/run_isolated.py:294: def move_outputs_to_outdir(run_dir, out_dir,
outputs):
link_outputs_to_outdir()

otherwise this is confusing.

https://codereview.chromium.org/2440353004/diff/140001/client/run_isolated.py...
client/run_isolated.py:295: """Moves any named outputs to out_dir so they can be
uploaded.
add an empty line after

https://codereview.chromium.org/2440353004/diff/140001/client/run_isolated.py...
client/run_isolated.py:308: sys.stderr.write('<Could not return file %s: %s>' %
(o, e))
Add a # TODO(aludwin): Surface this error.

I think this should eventually be an internal error but I'm not 100% confident.

https://codereview.chromium.org/2440353004/diff/140001/client/run_isolated.py...
client/run_isolated.py:488: # Try to copy files to the output directory, if
specified.
s/Try to copy/Link/

[aludwin, 2016-10-27 22:00:06]

https://codereview.chromium.org/2440353004/diff/140001/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/2440353004/diff/140001/client/run_isolated.py...
client/run_isolated.py:294: def move_outputs_to_outdir(run_dir, out_dir,
outputs):
On 2016/10/27 19:22:19, M-A Ruel wrote:
> link_outputs_to_outdir()
> 
> otherwise this is confusing.

Done.

https://codereview.chromium.org/2440353004/diff/140001/client/run_isolated.py...
client/run_isolated.py:295: """Moves any named outputs to out_dir so they can be
uploaded.
On 2016/10/27 19:22:19, M-A Ruel wrote:
> add an empty line after

Done.

https://codereview.chromium.org/2440353004/diff/140001/client/run_isolated.py...
client/run_isolated.py:308: sys.stderr.write('<Could not return file %s: %s>' %
(o, e))
On 2016/10/27 19:22:19, M-A Ruel wrote:
> Add a # TODO(aludwin): Surface this error.
> 
> I think this should eventually be an internal error but I'm not 100%
confident.

Done.

https://codereview.chromium.org/2440353004/diff/140001/client/run_isolated.py...
client/run_isolated.py:488: # Try to copy files to the output directory, if
specified.
On 2016/10/27 19:22:20, M-A Ruel wrote:
> s/Try to copy/Link/

Done.

[M-A Ruel, 2016-10-27 22:21:09]

lgtm

[aludwin, 2016-10-27 22:23:15]

The CQ bit was checked by aludwin@google.com

[commit-bot: I haz the power, 2016-10-27 22:23:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 22:57:41]

Description was changed from

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

BUG=
==========

to

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

BUG=

Committed:
https://github.com/luci/luci-py/commit/ba9fd6cdeb951478fee252d183629fd0a86449cc
==========

[commit-bot: I haz the power, 2016-10-27 22:57:42]

Committed patchset #6 (id:160001) as
https://github.com/luci/luci-py/commit/ba9fd6cdeb951478fee252d183629fd0a86449cc