Use devil's adb with each bisect iteration, also start/stop daemons.

Use devil's adb with each bisect iteration, also start/stop daemons.

In order to cleanly accomplish this change, custom android code has been moved out of the auto bisect loop and into two context managers. Desktop and other non-Android builds will use noop context managers instead.

This change cleans up a lot of nested logic in the bisect and makes it a little less commingled with calling code.

BUG=660852

[ghost stip (do not use), 2016-11-07 23:56:42]

Description was changed from

==========
Use devil's adb with each bisect iteration, also start/stop daemons.

In order to cleanly accomplish this change, custom android code has been moved
out of the auto bisect loop and into two context managers. Desktop and other
non-Android builds will use noop context managers instead.

This change cleans up a lot of nested logic in the bisect and makes it a little
less commingled with calling code.

BUG=660852
==========

to

==========
Use devil's adb with each bisect iteration, also start/stop daemons.

In order to cleanly accomplish this change, custom android code has been moved
out of the auto bisect loop and into two context managers. Desktop and other
non-Android builds will use noop context managers instead.

This change cleans up a lot of nested logic in the bisect and makes it a little
less commingled with calling code.

BUG=660852
==========

[ghost stip (do not use), 2016-11-07 23:56:42]

stip@chromium.org changed reviewers:
+ dtu@chromium.org, jbudorick@chromium.org

[ghost stip (do not use), 2016-11-07 23:57:48]

ptal.

note that this is a "live" CL. I'm about to take what I have here and copy it to
the _staging modules, but that will be a separate CL.

also, this will break internal when landed. luckily the CL to get it running
internally isn't large.

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/api.py (left):

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:323: else:
diff didn't like this indent for some reason

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

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:330: if
self.m.chromium.c.TARGET_PLATFORM == 'android' and not skip_download:
I would have added this part to the context manager but I was worried about the
skip_download thing

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:410: with
self.build_context_mgr(api):
😎

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_android/api.py (left):

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1156: def
common_tests_final_steps(self, logcat_gs_bucket='chromium-android'):
I had to pull this out so that we could make the logic in the context manager
cleaner. I checked and it doesn't seem like people use anything except the
defaults anyway.

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

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1188: self.use_devil_adb()
this is the "new logic" of the CL. does it look okay?

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1196:
self.stack_tool_steps()
do we need this? do we want it?

[jbudorick, 2016-11-08 00:18:29]

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

[jbudorick, 2016-11-08 00:18:32]

Really like where this is going.

+robertocn

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

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:66: # test_context_mgr is run
for each overall bisect run (i.e., once).
build_context_mgr

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_android/api.py (left):

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1156: def
common_tests_final_steps(self, logcat_gs_bucket='chromium-android'):
On 2016/11/07 23:57:48, stip wrote:
> I had to pull this out so that we could make the logic in the context manager
> cleaner. I checked and it doesn't seem like people use anything except the
> defaults anyway.

You could also do something like

  def create_android_test_wrapper(self, gs_bucket):
    @contextlib.contextmanager
    def android_build_wrapper(api):
      ...

    return android_build_wrapper

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

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:323:
devil_env.config.PrefetchPaths(dependencies=['adb'])
We might want to modify this to also start the adb server, i.e. add

  from devil.android.sdk import adb_wrapper
  ...
  adb_wrapper.AdbWrapper.StartServer()

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1188: self.use_devil_adb()
On 2016/11/07 23:57:48, stip wrote:
> this is the "new logic" of the CL. does it look okay?

I'm not sure that we'll want to do this every time we use android_test_wrapper.

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1196:
self.stack_tool_steps()
On 2016/11/07 23:57:48, stip wrote:
> do we need this? do we want it?

Yes and yes. Us not having stack symbolization on the swarming bots has been a
bit of an issue.

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

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/api.py:619:
self.m.chromium_android.logcat_gs_bucket = 'chromium-android'
If this is the default, why do we need to specify it?

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipes/a...
File scripts/slave/recipes/android/perf.py (right):

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipes/a...
scripts/slave/recipes/android/perf.py:192: api.chromium_android.logcat_gs_bucket
= 'chromium-android'
Same question.

[RobertoCN, 2016-11-08 19:16:56]

Lovely. I can't wait to see this in staging as well. LGTM

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

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/api.py:59: It is used for a placeholder
for code that should be wrapped around ever
nit: every

[ghost stip (do not use), 2016-11-08 20:56:33]

Speaking of staging, I've uploaded https://codereview.chromium.org/2486923002/
which will land this on staging first. If that is good, I can rebase this CL on
that (and update it) so we can land it for real.

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_android/api.py (left):

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1156: def
common_tests_final_steps(self, logcat_gs_bucket='chromium-android'):
On 2016/11/08 00:18:31, jbudorick wrote:
> On 2016/11/07 23:57:48, stip wrote:
> > I had to pull this out so that we could make the logic in the context
manager
> > cleaner. I checked and it doesn't seem like people use anything except the
> > defaults anyway.
> 
> You could also do something like
> 
>   def create_android_test_wrapper(self, gs_bucket):
>     @contextlib.contextmanager
>     def android_build_wrapper(api):
>       ...
> 
>     return android_build_wrapper

logcat_dump might be called outside of it though, and presumably you'd want them
in the same location

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

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1188: self.use_devil_adb()
On 2016/11/08 00:18:31, jbudorick wrote:
> On 2016/11/07 23:57:48, stip wrote:
> > this is the "new logic" of the CL. does it look okay?
> 
> I'm not sure that we'll want to do this every time we use
android_test_wrapper.

isn't that the whole point of doing this work?

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1196:
self.stack_tool_steps()
On 2016/11/08 00:18:31, jbudorick wrote:
> On 2016/11/07 23:57:48, stip wrote:
> > do we need this? do we want it?
> 
> Yes and yes. Us not having stack symbolization on the swarming bots has been a
> bit of an issue.

ok, I wasn't sure if I was duplicating some other logic somewhere

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipes/a...
File scripts/slave/recipes/android/perf.py (right):

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipes/a...
scripts/slave/recipes/android/perf.py:192: api.chromium_android.logcat_gs_bucket
= 'chromium-android'
On 2016/11/08 00:18:31, jbudorick wrote:
> Same question.

sure, was just re-implementing the same logic. I can remove it

[jbudorick, 2016-11-08 21:37:08]

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_android/api.py (left):

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1156: def
common_tests_final_steps(self, logcat_gs_bucket='chromium-android'):
On 2016/11/08 20:56:33, stip wrote:
> On 2016/11/08 00:18:31, jbudorick wrote:
> > On 2016/11/07 23:57:48, stip wrote:
> > > I had to pull this out so that we could make the logic in the context
> manager
> > > cleaner. I checked and it doesn't seem like people use anything except the
> > > defaults anyway.
> > 
> > You could also do something like
> > 
> >   def create_android_test_wrapper(self, gs_bucket):
> >     @contextlib.contextmanager
> >     def android_build_wrapper(api):
> >       ...
> > 
> >     return android_build_wrapper
> 
> logcat_dump might be called outside of it though, and presumably you'd want
them
> in the same location

At that point, I think it's on the caller.

We could also do this w/ a config setting.

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

https://codereview.chromium.org/2485763002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1188: self.use_devil_adb()
On 2016/11/08 20:56:32, stip wrote:
> On 2016/11/08 00:18:31, jbudorick wrote:
> > On 2016/11/07 23:57:48, stip wrote:
> > > this is the "new logic" of the CL. does it look okay?
> > 
> > I'm not sure that we'll want to do this every time we use
> android_test_wrapper.
> 
> isn't that the whole point of doing this work?

For bisect, yes. I would be interested in using this beyond bisect.

[ghost stip (do not use), 2016-11-09 02:01:49]

ok. take a look at the changes I've made in
https://codereview.chromium.org/2486923002/ to address this and let me know what
you think