run_isolated: support non-isolated commands

run_isolated: support non-isolated commands

Make --isolated option optional:
* if not specified, treat args as a command, so run_isolated.py becomes a
  wrapper
* --isolate-server option is required only if --isolated is specified or an
  arg contains ${ISOLATED_OUTDIR}
* even if input isolated hash is not provided, we still upload output files
  if any, so name "run_isolated" still makes sense

R=maruel@chromium.org
BUG=601022
Committed: https://github.com/luci/luci-py/commit/ff263d1538f3434f1d92ee14c22de87beab596e4

[nodir, 2016-04-29 22:40:45]

Description was changed from

==========
run_isolated: support non-isolated commands

Make --isolated option optional:
- if not specified, treat args as a command, so run_isolated.py becomes a
  wrapper
- --isolate-server option is required only if --isolated is specified or an
  arg contains ${ISOLATED_OUTDIR}
- even if input isolated hash is not provided, we still upload output files
  if any, so name "run_isolated" still makes sense

R=maruel@chromium.org
BUG=601022
==========

to

==========
run_isolated: support non-isolated commands

Make --isolated option optional:
* if not specified, treat args as a command, so run_isolated.py becomes a
  wrapper
* --isolate-server option is required only if --isolated is specified or an
  arg contains ${ISOLATED_OUTDIR}
* even if input isolated hash is not provided, we still upload output files
  if any, so name "run_isolated" still makes sense

R=maruel@chromium.org
BUG=601022
==========

[nodir, 2016-04-29 22:41:22]

PTAL

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

I'm OOO today, will try to complete the review later today at worst tomorrow
morning.

https://codereview.chromium.org/1932143003/diff/20001/client/isolateserver.py
File client/isolateserver.py (right):

https://codereview.chromium.org/1932143003/diff/20001/client/isolateserver.py...
client/isolateserver.py:2184: parser, options, set_exception_handler,
required=True):
Can you not make it a default arg and update all call sites and bump version?

https://codereview.chromium.org/1932143003/diff/20001/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/1932143003/diff/20001/client/run_isolated.py#...
client/run_isolated.py:146: return [arg.replace(ISOLATED_OUTDIR_PARAMETER,
out_dir) for arg in command]
The reason for the previous code was things like:

"${ISOLATED_OUTDIR}/foo/bar"

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

https://codereview.chromium.org/1932143003/diff/20001/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/1932143003/diff/20001/client/run_isolated.py#...
client/run_isolated.py:456: the .isolated file. Otherwise, the command line to
execute.
I'm not a fan of this ambivalence.

https://codereview.chromium.org/1932143003/diff/20001/client/run_isolated.py#...
client/run_isolated.py:520: usage='%prog <options> [args]',
[command to run or extra args]

https://codereview.chromium.org/1932143003/diff/20001/client/run_isolated.py#...
client/run_isolated.py:572: parser.error('--isolated or args is required.')
s/args/command to run/ ?

https://codereview.chromium.org/1932143003/diff/20001/client/run_isolated.py#...
client/run_isolated.py:596: with storage or tools.noop_context():
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:37:42]

Patchset #4 (id:60001) has been deleted

[nodir, 2016-05-02 17:38:27]

Patchset #4 (id:80001) has been deleted

[nodir, 2016-05-02 17:43:10]

Patchset #4 (id:100001) has been deleted

[nodir, 2016-05-02 17:43:23]

https://codereview.chromium.org/1932143003/diff/20001/client/isolateserver.py
File client/isolateserver.py (right):

https://codereview.chromium.org/1932143003/diff/20001/client/isolateserver.py...
client/isolateserver.py:2184: parser, options, set_exception_handler,
required=True):
On 2016/05/02 14:18:14, M-A Ruel wrote:
> Can you not make it a default arg and update all call sites and bump version?

Done in a separate patchset

https://codereview.chromium.org/1932143003/diff/20001/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/1932143003/diff/20001/client/run_isolated.py#...
client/run_isolated.py:146: return [arg.replace(ISOLATED_OUTDIR_PARAMETER,
out_dir) for arg in command]
On 2016/05/02 14:18:14, M-A Ruel wrote:
> The reason for the previous code was things like:
> 
> "${ISOLATED_OUTDIR}/foo/bar"

got it, reverted, added comment

https://codereview.chromium.org/1932143003/diff/20001/client/run_isolated.py#...
client/run_isolated.py:456: the .isolated file. Otherwise, the command line to
execute.
On 2016/05/02 14:51:17, M-A Ruel wrote:
> I'm not a fan of this ambivalence.

fixed

https://codereview.chromium.org/1932143003/diff/20001/client/run_isolated.py#...
client/run_isolated.py:520: usage='%prog <options> [args]',
On 2016/05/02 14:51:17, M-A Ruel wrote:
> [command to run or extra args]

Done.

https://codereview.chromium.org/1932143003/diff/20001/client/run_isolated.py#...
client/run_isolated.py:572: parser.error('--isolated or args is required.')
On 2016/05/02 14:51:17, M-A Ruel wrote:
> s/args/command to run/ ?

Done.

https://codereview.chromium.org/1932143003/diff/20001/client/run_isolated.py#...
client/run_isolated.py:596: with storage or tools.noop_context():
On 2016/05/02 14:51:17, M-A Ruel wrote:
> 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.

Done.

[M-A Ruel, 2016-05-03 13:55:23]

lgtm, thanks!

https://codereview.chromium.org/1932143003/diff/120001/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/1932143003/diff/120001/client/run_isolated.py...
client/run_isolated.py:445: command, isolated_hash, storage, cache,
leak_temp_dir, result_json, root_dir,
80 cols

[nodir, 2016-05-03 16:37:21]

The CQ bit was checked by nodir@chromium.org

[nodir, 2016-05-03 16:37:21]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/1932143003/#ps140001
(title: "80cols")

[commit-bot: I haz the power, 2016-05-03 16:37:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1932143003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1932143003/140001

[commit-bot: I haz the power, 2016-05-03 16:39:58]

Description was changed from

==========
run_isolated: support non-isolated commands

Make --isolated option optional:
* if not specified, treat args as a command, so run_isolated.py becomes a
  wrapper
* --isolate-server option is required only if --isolated is specified or an
  arg contains ${ISOLATED_OUTDIR}
* even if input isolated hash is not provided, we still upload output files
  if any, so name "run_isolated" still makes sense

R=maruel@chromium.org
BUG=601022
==========

to

==========
run_isolated: support non-isolated commands

Make --isolated option optional:
* if not specified, treat args as a command, so run_isolated.py becomes a
  wrapper
* --isolate-server option is required only if --isolated is specified or an
  arg contains ${ISOLATED_OUTDIR}
* even if input isolated hash is not provided, we still upload output files
  if any, so name "run_isolated" still makes sense

R=maruel@chromium.org
BUG=601022

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

[commit-bot: I haz the power, 2016-05-03 16:39:59]

Committed patchset #5 (id:140001) as
https://github.com/luci/luci-py/commit/ff263d1538f3434f1d92ee14c22de87beab596e4