Add the recipe_modules/blimp for integration bot

Add the recipe_modules/blimp for integration bot

By using script '//src/blimp/tools/client_engine_integration.py', to integrate client+engine via port forwarding on bot.

BUG=656122
Committed: https://chromium.googlesource.com/chromium/tools/build/+/86c5ac5eecf8c7ad492fb0219de6464b4485ccb9

[shenghuazhang, 2016-10-14 23:52:55]

Description was changed from

==========
Add the recipe_modules/blimp for integration bot

BUG=656122
==========

to

==========
Add the recipe_modules/blimp for integration bot

By using script '//src/blimp/tools/client_engine_integration.py', to integrate
client+engine via port forwarding on bot.

BUG=656122
==========

[shenghuazhang, 2016-10-14 23:54:41]

shenghuazhang@chromium.org changed reviewers:
+ mikecase@chromium.org, nyquist@chromium.org

[mikecase (-- gone --), 2016-10-15 00:05:38]

Some quick comments

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/__init__.py (right):

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/__init__.py:8: 'gsutil',
Doesnt look like you are using gsutil module

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/__init__.py:13: 'recipe_engine/step',
A lot of these DEPS you are not using. Delete all the ones you dont need.

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/api.py (right):

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/api.py:9: def get_config_defaults(self):
not sure what this does. Do you need it?

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/api.py:14: def run_engine_forwarder(self,
output_linux_dir, **kwargs):
I would change this to be a context manager.

So it would be like 

@contextlib.contextmanager
def engine_forwarder():
  _run_engine_forwarder()
  yield
  _stop_engine_forwarder()

Then people would use it like....

with engine_forwarder():
  do stuff

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/config.py (right):

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/config.py:11:
client_engine_integration_script = Static(
nit: I think you want to indent this 2 more spaces.

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/example.py (right):

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/example.py:25: 'test_utils',
Most of these DEPS you are not using. Delete all the ones you dont need.

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipes/b...
File scripts/slave/recipes/blimp/integration.py (left):

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipes/b...
scripts/slave/recipes/blimp/integration.py:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
Did you mean to delete this file?

[shenghuazhang, 2016-10-17 23:22:07]

Remain get_config_defaults since I guess that's needed?

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/__init__.py (right):

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/__init__.py:8: 'gsutil',
On 2016/10/15 00:05:37, mikecase wrote:
> Doesnt look like you are using gsutil module

Done.

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/__init__.py:13: 'recipe_engine/step',
On 2016/10/15 00:05:37, mikecase wrote:
> A lot of these DEPS you are not using. Delete all the ones you dont need.

Done.

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/api.py (right):

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/api.py:9: def get_config_defaults(self):
On 2016/10/15 00:05:37, mikecase wrote:
> not sure what this does. Do you need it?

Seems like BaseConfig(CHECKOUT_PATH, **_kwargs) in config.py is using this by
default? It somehow calls 'CHECKOUT_PATH' defining in this method.

If remove this, otherwise, the train result would show TypeError:
'TypeError: BaseConfig() takes exactly 1 argument (0 given)'

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/api.py:14: def run_engine_forwarder(self,
output_linux_dir, **kwargs):
On 2016/10/15 00:05:37, mikecase wrote:
> I would change this to be a context manager.
> 
> So it would be like 
> 
> @contextlib.contextmanager
> def engine_forwarder():
>   _run_engine_forwarder()
>   yield
>   _stop_engine_forwarder()
> 
> Then people would use it like....
> 
> with engine_forwarder():
>   do stuff

Done.

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/config.py (right):

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/config.py:11:
client_engine_integration_script = Static(
On 2016/10/15 00:05:37, mikecase wrote:
> nit: I think you want to indent this 2 more spaces.

Done.

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/example.py (right):

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/example.py:25: 'test_utils',
On 2016/10/15 00:05:37, mikecase wrote:
> Most of these DEPS you are not using. Delete all the ones you dont need.

Done.

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipes/b...
File scripts/slave/recipes/blimp/integration.py (left):

https://codereview.chromium.org/2416253003/diff/20001/scripts/slave/recipes/b...
scripts/slave/recipes/blimp/integration.py:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/10/15 00:05:37, mikecase wrote:
> Did you mean to delete this file?

Oh this shouldn't be here... This file shouldn't be touched. Will revert it in
next patch.

[mikecase (-- gone --), 2016-10-17 23:38:35]

Looks great ^_^ Just mostly nits then I think it's good.

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/api.py (right):

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/api.py:16: def _run_engine_forwarder(self,
output_linux_dir, **kwargs):
suuuper nit: I would personally name this just _start_engine_forwarder. 

Matches the '[Blimp] Starting engine and forwarder' better too

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/api.py:26: # Installs apk in client and runs
blimp.
nit: Change this comment to a proper docstring like...

def load_client(self, output_linux_dir, apk_path, **kwargs):
  """Installs apk in client and runs blimp."""

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/api.py:38: def _stop_engine_forwarder(self,
output_linux_dir, **kwargs):
Nit: Move this function next to _run_engine_forwarder for organization.

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/api.py:49: def engine_forwarder_process(self,
output_linux_dir, **kwargs):
suuuper nit: I would personally name this just engine_forwarder()

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/api.py:50:
self._run_engine_forwarder(output_linux_dir, **kwargs)
Not 100% sure, but Im guessing you want to put this line inside the try: block
below.

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/config.py (right):

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/config.py:20: pass
Also, if you don't want to deal with this config nonsense, I'd be fine with you
just setting the client_engine_integration_script path directly in the
recipe_module.

But, I think what you have should work.

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/example.py (right):

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/example.py:56:
api.chromium.c.build_config_fs)
nit: These indents are off.

This might fit on 1 line.

android_debug_dir = android_build.join(api.chromium.c.build_config_fs)

Otherwise indent like this...

android_debug_dir = android_build.join(
    api.chromium.c.build_config_fs)

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/example.py:58:
api.chromium.c.build_config_fs)
same thing here.

[shenghuazhang, 2016-10-18 20:39:07]

shenghuazhang@chromium.org changed reviewers:
+ jbudorick@chromium.org

[shenghuazhang, 2016-10-18 20:39:08]

Refactor cl

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/api.py (right):

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/api.py:16: def _run_engine_forwarder(self,
output_linux_dir, **kwargs):
On 2016/10/17 23:38:35, mikecase wrote:
> suuuper nit: I would personally name this just _start_engine_forwarder. 
> 
> Matches the '[Blimp] Starting engine and forwarder' better too

Done.

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/api.py:26: # Installs apk in client and runs
blimp.
On 2016/10/17 23:38:35, mikecase wrote:
> nit: Change this comment to a proper docstring like...
> 
> def load_client(self, output_linux_dir, apk_path, **kwargs):
>   """Installs apk in client and runs blimp."""

Done.

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/api.py:38: def _stop_engine_forwarder(self,
output_linux_dir, **kwargs):
On 2016/10/17 23:38:35, mikecase wrote:
> Nit: Move this function next to _run_engine_forwarder for organization.

Done.

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/api.py:49: def engine_forwarder_process(self,
output_linux_dir, **kwargs):
On 2016/10/17 23:38:35, mikecase wrote:
> suuuper nit: I would personally name this just engine_forwarder()

Done.

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/api.py:50:
self._run_engine_forwarder(output_linux_dir, **kwargs)
On 2016/10/17 23:38:35, mikecase wrote:
> Not 100% sure, but Im guessing you want to put this line inside the try: block
> below.

Done.

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/config.py (right):

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/config.py:20: pass
On 2016/10/17 23:38:35, mikecase wrote:
> Also, if you don't want to deal with this config nonsense, I'd be fine with
you
> just setting the client_engine_integration_script path directly in the
> recipe_module.
> 
> But, I think what you have should work.

Yeah that would be more explicit with your way. 

I would be adding server steps to the recipe later, which probably needs to add
more config as well (but I am not 100% sure).

How about leave it here now for later use? And I would change to your way if no
more config added :)

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/example.py (right):

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/example.py:56:
api.chromium.c.build_config_fs)
On 2016/10/17 23:38:35, mikecase wrote:
> nit: These indents are off.
> 
> This might fit on 1 line.
> 
> android_debug_dir = android_build.join(api.chromium.c.build_config_fs)
> 
> Otherwise indent like this...
> 
> android_debug_dir = android_build.join(
>     api.chromium.c.build_config_fs)

OH the indents are really like... Where could I learn the rules of the indents?
PS. done this part.

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/example.py:58:
api.chromium.c.build_config_fs)
On 2016/10/17 23:38:35, mikecase wrote:
> same thing here.

Done.

[mikecase (-- gone --), 2016-10-18 20:52:39]

lgtm

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/example.py (right):

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/example.py:56:
api.chromium.c.build_config_fs)
On 2016/10/18 at 20:39:08, shenghuazhang wrote:
> On 2016/10/17 23:38:35, mikecase wrote:
> > nit: These indents are off.
> > 
> > This might fit on 1 line.
> > 
> > android_debug_dir = android_build.join(api.chromium.c.build_config_fs)
> > 
> > Otherwise indent like this...
> > 
> > android_debug_dir = android_build.join(
> >     api.chromium.c.build_config_fs)
> 
> OH the indents are really like... Where could I learn the rules of the
indents?
> PS. done this part.

I think for indents you either do....

===> Move all args to next line, indent 4 spaces

function(
    a, b, c, d, e)

function(
    a, b, c,
    d, e)

===> Move some args to next line, line up the args

function(a, b, c,
         d, e)

X Don't do this.

function(a, b, c,
    d, e)


I think that is the preferred style anyway

[jbudorick, 2016-10-18 20:56:10]

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/blimp/example.py (right):

https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/blimp/example.py:56:
api.chromium.c.build_config_fs)
On 2016/10/18 20:52:39, mikecase wrote:
> On 2016/10/18 at 20:39:08, shenghuazhang wrote:
> > On 2016/10/17 23:38:35, mikecase wrote:
> > > nit: These indents are off.
> > > 
> > > This might fit on 1 line.
> > > 
> > > android_debug_dir = android_build.join(api.chromium.c.build_config_fs)
> > > 
> > > Otherwise indent like this...
> > > 
> > > android_debug_dir = android_build.join(
> > >     api.chromium.c.build_config_fs)
> > 
> > OH the indents are really like... Where could I learn the rules of the
> indents?
> > PS. done this part.
> 
> I think for indents you either do....
> 
> ===> Move all args to next line, indent 4 spaces
> 
> function(
>     a, b, c, d, e)
> 
> function(
>     a, b, c,
>     d, e)
> 
> ===> Move some args to next line, line up the args
> 
> function(a, b, c,
>          d, e)
> 
> X Don't do this.
> 
> function(a, b, c,
>     d, e)
> 
> 
> I think that is the preferred style anyway
> 

Correct: https://google.github.io/styleguide/pyguide.html#Indentation

(we generally prefer the google python style guide)

[jbudorick, 2016-10-19 15:46:13]

lgtm though

On 2016/10/18 20:56:10, jbudorick wrote:
>
https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
> File scripts/slave/recipe_modules/blimp/example.py (right):
> 
>
https://codereview.chromium.org/2416253003/diff/60001/scripts/slave/recipe_mo...
> scripts/slave/recipe_modules/blimp/example.py:56:
> api.chromium.c.build_config_fs)
> On 2016/10/18 20:52:39, mikecase wrote:
> > On 2016/10/18 at 20:39:08, shenghuazhang wrote:
> > > On 2016/10/17 23:38:35, mikecase wrote:
> > > > nit: These indents are off.
> > > > 
> > > > This might fit on 1 line.
> > > > 
> > > > android_debug_dir = android_build.join(api.chromium.c.build_config_fs)
> > > > 
> > > > Otherwise indent like this...
> > > > 
> > > > android_debug_dir = android_build.join(
> > > >     api.chromium.c.build_config_fs)
> > > 
> > > OH the indents are really like... Where could I learn the rules of the
> > indents?
> > > PS. done this part.
> > 
> > I think for indents you either do....
> > 
> > ===> Move all args to next line, indent 4 spaces
> > 
> > function(
> >     a, b, c, d, e)
> > 
> > function(
> >     a, b, c,
> >     d, e)
> > 
> > ===> Move some args to next line, line up the args
> > 
> > function(a, b, c,
> >          d, e)
> > 
> > X Don't do this.
> > 
> > function(a, b, c,
> >     d, e)
> > 
> > 
> > I think that is the preferred style anyway
> > 
> 
> Correct: https://google.github.io/styleguide/pyguide.html#Indentation
> 
> (we generally prefer the google python style guide)

[shenghuazhang, 2016-10-22 02:05:10]

The CQ bit was checked by shenghuazhang@chromium.org

[commit-bot: I haz the power, 2016-10-22 02:05:16]

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-22 02:16:33]

Description was changed from

==========
Add the recipe_modules/blimp for integration bot

By using script '//src/blimp/tools/client_engine_integration.py', to integrate
client+engine via port forwarding on bot.

BUG=656122
==========

to

==========
Add the recipe_modules/blimp for integration bot

By using script '//src/blimp/tools/client_engine_integration.py', to integrate
client+engine via port forwarding on bot.

BUG=656122

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/86c5ac5eecf8c7ad492f...
==========

[commit-bot: I haz the power, 2016-10-22 02:16:33]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/tools/build/+/86c5ac5eecf8c7ad492f...