Setting --run-python-script flag for runtest.py invocation.

Setting --run-python-script flag for runtest.py invocation.

The lack of this flag is breaking windows and android bisects.
Also handling the case when dashboard sends 'python' as part of the
command to run.

This is to fix a defect introduced by https://codereview.chromium.org/1875263002/ where instead of calling run_benchmark (for example) directly, we invoke runtest.py which will take care of among some other things, opening and killing xvfb when running under linux.

R=prasadv,sullivan
BUG=606281
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300165

[sullivan, 2016-04-25 18:43:12]

Needs some more context!

* Can you update the bug to clarify exactly what broke?
* Can you update the CL description to explain this is a follow-on to
https://codereview.chromium.org/1875263002/ ?
* Inline comments about the actual code.

https://codereview.chromium.org/1915183002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/bisect_tester/perf_test.py (right):

https://codereview.chromium.org/1915183002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/bisect_tester/perf_test.py:208: run_python_command
= True
The code could use some more comments explaining exactly what's going on:

1) We need this python_mode=True and need to strip 'python' out of the command
for runtest.py? Are more details available about why?

2) The dashboard sends 'python' as the command: why do we handle that case here
instead of change the dashboard?

3) Why do we check command_parts[0].startswith('python') and then drop the
entire thing if it does? Could it be anything other than 'python'? If it could,
what are we dropping?

4) Why do we effectively have two names for what appears to be telling
run_test.py to prepend 'python ' to the command (run_python_command and
python_mode)?

[RobertoCN, 2016-04-25 18:57:47]

Description was changed from

==========
Setting --run-python-script flag for runtest.py invocation.

The lack of this flag is breaking windows and android bisects.
Also handling the case when dashboard sends 'python' as part of the
command to run.

R=prasadv,sullivan
BUG=606281
==========

to

==========
Setting --run-python-script flag for runtest.py invocation.

The lack of this flag is breaking windows and android bisects.
Also handling the case when dashboard sends 'python' as part of the
command to run.

This is to fix a defect introduced by
https://codereview.chromium.org/1875263002/ where instead of calling
run_benchmark (for example) directly, we invoke runtest.py which will take care
of among some other things, opening and killing xvfb when running under linux.

R=prasadv,sullivan
BUG=606281
==========

[RobertoCN, 2016-04-25 19:17:44]

On 2016/04/25 18:43:12, sullivan wrote:
> Needs some more context!
> 
> * Can you update the bug to clarify exactly what broke?
Done.


> * Can you update the CL description to explain this is a follow-on to
Done.


> https://codereview.chromium.org/1875263002/ ?
> * Inline comments about the actual code.
> 
>
https://codereview.chromium.org/1915183002/diff/1/scripts/slave/recipe_module...
> File scripts/slave/recipe_modules/bisect_tester/perf_test.py (right):
> 
>
https://codereview.chromium.org/1915183002/diff/1/scripts/slave/recipe_module...
> scripts/slave/recipe_modules/bisect_tester/perf_test.py:208:
run_python_command
> = True
> The code could use some more comments explaining exactly what's going on:
>
Added some.

 
> 1) We need this python_mode=True and need to strip 'python' out of the command
> for runtest.py? Are more details available about why?
>
runtest.py assumes it's its responsibility to prepend the call to the
interpreter depending on the platform.
 
> 2) The dashboard sends 'python' as the command: why do we handle that case
here
> instead of change the dashboard?
I think dashboard should stop sending python as the command, but first we need
to make sure bisect works regardless.
Added a TODO to remove the clause once dashboard is fixed in this regard.

> 3) Why do we check command_parts[0].startswith('python') and then drop the
> entire thing if it does? Could it be anything other than 'python'? If it
could,
> what are we dropping?
This is a mistake, changed it to a strict equality comparison.

> 4) Why do we effectively have two names for what appears to be telling
> run_test.py to prepend 'python ' to the command (run_python_command and
> python_mode)?
run_python_command is the name of the property inside runtest.py as well ad the
command line flag '--run-python-command', but someone decided to name the
parameter to the runtest() api 'python_mode', for simplicity I just renamed all
instances to 'python_mode'.

[sullivan, 2016-04-25 19:25:05]

lgtm

Thanks for cleaning up the documentation around this fix!

https://codereview.chromium.org/1915183002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/bisect_tester/perf_test.py (right):

https://codereview.chromium.org/1915183002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bisect_tester/perf_test.py:209: # part of the
command.
One final nit: Please file a dashboard bug and link it here.

Also, I just realized: when it's fixed on the dashboard side, how will the
bisect know to set python_mode=True here?

[sullivan, 2016-04-25 19:25:07]

lgtm

lgtm

Thanks for cleaning up the documentation around this fix!

[RobertoCN, 2016-04-25 19:34:32]

https://codereview.chromium.org/1915183002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/bisect_tester/perf_test.py (right):

https://codereview.chromium.org/1915183002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/bisect_tester/perf_test.py:208: run_python_command
= True
On 2016/04/25 18:43:12, sullivan wrote:
> The code could use some more comments explaining exactly what's going on:
> 
> 1) We need this python_mode=True and need to strip 'python' out of the command
> for runtest.py? Are more details available about why?
> 
> 2) The dashboard sends 'python' as the command: why do we handle that case
here
> instead of change the dashboard?
> 
> 3) Why do we check command_parts[0].startswith('python') and then drop the
> entire thing if it does? Could it be anything other than 'python'? If it
could,
> what are we dropping?
> 
> 4) Why do we effectively have two names for what appears to be telling
> run_test.py to prepend 'python ' to the command (run_python_command and
> python_mode)?

Done.

https://codereview.chromium.org/1915183002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/bisect_tester/perf_test.py (right):

https://codereview.chromium.org/1915183002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bisect_tester/perf_test.py:209: # part of the
command.
On 2016/04/25 19:25:04, sullivan wrote:
> One final nit: Please file a dashboard bug and link it here.
> 
> Also, I just realized: when it's fixed on the dashboard side, how will the
> bisect know to set python_mode=True here?

It will go into the if _is_telemetry_command(...) like it does for mac and linux
where we also set it to True. (And note that this is only necessary because
there's no '.py' extension on the command, otherwise runtest.py would infer it
correctly)

https://codereview.chromium.org/1915183002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bisect_tester/perf_test.py:209: # part of the
command.
On 2016/04/25 19:25:04, sullivan wrote:
> One final nit: Please file a dashboard bug and link it here.
> 
> Also, I just realized: when it's fixed on the dashboard side, how will the
> bisect know to set python_mode=True here?

Done.

[RobertoCN, 2016-04-25 19:37:59]

The CQ bit was checked by robertocn@chromium.org

[RobertoCN, 2016-04-25 19:38:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/1915183002/#ps2 (title:
"Adding link to dashboard bug.")

[commit-bot: I haz the power, 2016-04-25 19:38:07]

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

[commit-bot: I haz the power, 2016-04-25 19:42:02]

Description was changed from

==========
Setting --run-python-script flag for runtest.py invocation.

The lack of this flag is breaking windows and android bisects.
Also handling the case when dashboard sends 'python' as part of the
command to run.

This is to fix a defect introduced by
https://codereview.chromium.org/1875263002/ where instead of calling
run_benchmark (for example) directly, we invoke runtest.py which will take care
of among some other things, opening and killing xvfb when running under linux.

R=prasadv,sullivan
BUG=606281
==========

to

==========
Setting --run-python-script flag for runtest.py invocation.

The lack of this flag is breaking windows and android bisects.
Also handling the case when dashboard sends 'python' as part of the
command to run.

This is to fix a defect introduced by
https://codereview.chromium.org/1875263002/ where instead of calling
run_benchmark (for example) directly, we invoke runtest.py which will take care
of among some other things, opening and killing xvfb when running under linux.

R=prasadv,sullivan
BUG=606281

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300165
==========

[commit-bot: I haz the power, 2016-04-25 19:42:03]

Committed patchset #3 (id:2) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=300165