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.

This change is only for the staging recipe. If shown to work, we can land https://codereview.chromium.org/2485763002/.

BUG=660852
Committed: https://chromium.googlesource.com/chromium/tools/build/+/71e8f921fdc7abc1cea52ef5cf8247069c305bc7

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

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.

This change is only for the staging recipe. If shown to work, we can land
https://codereview.chromium.org/2485763002/.

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.

This change is only for the staging recipe. If shown to work, we can land
https://codereview.chromium.org/2485763002/.

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

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

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

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

ptal!

this is just for staging

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

ptal

https://codereview.chromium.org/2486923002/diff/30001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect_staging/api.py (left):

https://codereview.chromium.org/2486923002/diff/30001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect_staging/api.py:11: import os
os was an unnecessary import

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

https://codereview.chromium.org/2486923002/diff/30001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1166: def
android_build_wrapper(self, logcat_gs_bucket='chromium-android'):
this wacky construction of a contextmanager-generating function is so we can
have consistently defined logcat_gs_bucket. I think it's overkill, but jbudorick
seemed to prefer this over my initial solution in
https://codereview.chromium.org/2485763002/

https://codereview.chromium.org/2486923002/diff/30001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1193: with
self.android_test_wrapper(logcat_gs_bucket)(bisect_obj):
this split is so android_test_wrapper can be used independently outside of
bisect and not have the repeated use_devil_adb

https://codereview.chromium.org/2486923002/diff/30001/scripts/slave/recipe_mo...
File
scripts/slave/recipe_modules/chromium_android/example.expected/use_devil_adb_basic.json
(right):

https://codereview.chromium.org/2486923002/diff/30001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/example.expected/use_devil_adb_basic.json:818:
"\nimport sys\nsys.path.append(sys.argv[1])\nfrom devil import devil_env\nfrom
devil.android.sdk import
adb_wrapper\ndevil_env.config.Initialize()\ndevil_env.config.PrefetchPaths(dependencies=['adb'])\nadb_wrapper.AdbWrapper.StartServer()\n",
because prod and staging recipes import the same devil_env, this has an effect
here. hopefully this minimal change is ok in prod

[jbudorick, 2016-11-09 14:32:36]

Should there be changes to
https://codesearch.chromium.org/chromium/build/scripts/slave/recipes/bisectio...
?

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

https://codereview.chromium.org/2486923002/diff/30001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1166: def
android_build_wrapper(self, logcat_gs_bucket='chromium-android'):
On 2016/11/09 01:55:56, stip wrote:
> this wacky construction of a contextmanager-generating function is so we can
> have consistently defined logcat_gs_bucket. I think it's overkill, but
jbudorick
> seemed to prefer this over my initial solution in
> https://codereview.chromium.org/2485763002/

On looking at this, a config setting probably would be cleaner, but I could live
with this.

https://codereview.chromium.org/2486923002/diff/30001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1193: with
self.android_test_wrapper(logcat_gs_bucket)(bisect_obj):
On 2016/11/09 01:55:56, stip wrote:
> this split is so android_test_wrapper can be used independently outside of
> bisect and not have the repeated use_devil_adb

I think this belongs in the bisect modules rather than in here.

[RobertoCN, 2016-11-09 22:10:31]

Bisectwise this looks good to me. Idk about chromium_android though. In that, I
defer to JB.

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

https://codereview.chromium.org/2486923002/diff/30001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1193: with
self.android_test_wrapper(logcat_gs_bucket)(bisect_obj):
On 2016/11/09 14:32:36, jbudorick wrote:
> On 2016/11/09 01:55:56, stip wrote:
> > this split is so android_test_wrapper can be used independently outside of
> > bisect and not have the repeated use_devil_adb
> 
> I think this belongs in the bisect modules rather than in here.

+1

[RobertoCN, 2016-11-09 22:11:51]

LGTM % John's comments.

[ghost stip (do not use), 2016-11-09 23:05:37]

ptal. the scripts/slave/recipes/bisection/android_bisect_staging.py change is
there, it's just buried in all the expectations. look about halfway down

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

https://codereview.chromium.org/2486923002/diff/30001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1193: with
self.android_test_wrapper(logcat_gs_bucket)(bisect_obj):
On 2016/11/09 14:32:36, jbudorick wrote:
> On 2016/11/09 01:55:56, stip wrote:
> > this split is so android_test_wrapper can be used independently outside of
> > bisect and not have the repeated use_devil_adb
> 
> I think this belongs in the bisect modules rather than in here.

Done.

[jbudorick, 2016-11-09 23:51:46]

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

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1168: def
wrapper(bisect_obj):
nit: This shouldn't be "bisect_obj" if it's here.

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1174:
bisect_obj.m.chromium_android.common_tests_setup_steps(
When doing something like

  with android_build_wrapper()():
    with android_test_wrapper():

we will spawn the monitors twice. That seems wrong.

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1180:
bisect_obj.ensure_checkout()
...?

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1187: def
wrapper(_bisect_obj):
nit: same

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipes/b...
File
scripts/slave/recipes/bisection/android_bisect_staging.expected/basic_recipe_android_nexus6_perf_bisect.json
(right):

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/android_bisect_staging.expected/basic_recipe_android_nexus6_perf_bisect.json:579:
"name": "spawn_logcat_monitor (2)"
We're spawning second copies of the monitors here.

[ghost stip (do not use), 2016-11-10 00:51:57]

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

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1180:
bisect_obj.ensure_checkout()
On 2016/11/09 23:51:45, jbudorick wrote:
> ...?

so we're in chromium_android. ensure_checkout is part of auto_bisect, and
auto_bisect_staging. I could hardcode the ensure_checkout, but then that defeats
the split between _staging and prod. I can pull it out and make it part of the
bisect wrapper, but then we're reordering things.

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipes/b...
File
scripts/slave/recipes/bisection/android_bisect_staging.expected/basic_recipe_android_nexus6_perf_bisect.json
(right):

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/android_bisect_staging.expected/basic_recipe_android_nexus6_perf_bisect.json:579:
"name": "spawn_logcat_monitor (2)"
On 2016/11/09 23:51:46, jbudorick wrote:
> We're spawning second copies of the monitors here.

for the first iteration, yes. I'm assuming logcat_dump will close it? if so, I
can put one in the build_wrapper to close off the one before the test wrapper
starts.

[jbudorick, 2016-11-10 01:00:30]

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

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1180:
bisect_obj.ensure_checkout()
On 2016/11/10 00:51:57, stip wrote:
> On 2016/11/09 23:51:45, jbudorick wrote:
> > ...?
> 
> so we're in chromium_android. ensure_checkout is part of auto_bisect, and
> auto_bisect_staging. I could hardcode the ensure_checkout, but then that
defeats
> the split between _staging and prod. I can pull it out and make it part of the

Not following you here.

> bisect wrapper, but then we're reordering things.

This definitely should be in bisect, not in chromium_android. Not sure about
reordering, though.

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipes/b...
File scripts/slave/recipes/bisection/android_bisect_staging.py (right):

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/android_bisect_staging.py:144:
api.chromium_android.android_build_wrapper())
For the build wrapper, you could do


  def android_bisect_build_wrapper(bisect_obj):
    with api.chromium_android.android_build_wrapper()(bisect_obj):
      try:
        yield
      finally:
        bisect_obj.ensure_checkout()

which would preserve order.

[ghost stip (do not use), 2016-11-10 01:05:33]

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

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1180:
bisect_obj.ensure_checkout()
On 2016/11/10 01:00:29, jbudorick wrote:
> On 2016/11/10 00:51:57, stip wrote:
> > On 2016/11/09 23:51:45, jbudorick wrote:
> > > ...?
> > 
> > so we're in chromium_android. ensure_checkout is part of auto_bisect, and
> > auto_bisect_staging. I could hardcode the ensure_checkout, but then that
> defeats
> > the split between _staging and prod. I can pull it out and make it part of
the
> 
> Not following you here.
> 
> > bisect wrapper, but then we're reordering things.
> 
> This definitely should be in bisect, not in chromium_android. Not sure about
> reordering, though.

if we're okay with ensure_checkout coming after common_tests_final_steps, then
this all becomes much simpler

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipes/b...
File scripts/slave/recipes/bisection/android_bisect_staging.py (right):

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/android_bisect_staging.py:144:
api.chromium_android.android_build_wrapper())
On 2016/11/10 01:00:29, jbudorick wrote:
> For the build wrapper, you could do
> 
> 
>   def android_bisect_build_wrapper(bisect_obj):
>     with api.chromium_android.android_build_wrapper()(bisect_obj):
>       try:
>         yield
>       finally:
>         bisect_obj.ensure_checkout()
> 
> which would preserve order.

That would put ensure_checkout() after
chromium_android.common_tests_final_steps(). Maybe that's cool, but I don't
know.

[jbudorick, 2016-11-10 01:06:06]

On 2016/11/10 01:05:33, stip wrote:
>
https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipe_mo...
> File scripts/slave/recipe_modules/chromium_android/api.py (right):
> 
>
https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipe_mo...
> scripts/slave/recipe_modules/chromium_android/api.py:1180:
> bisect_obj.ensure_checkout()
> On 2016/11/10 01:00:29, jbudorick wrote:
> > On 2016/11/10 00:51:57, stip wrote:
> > > On 2016/11/09 23:51:45, jbudorick wrote:
> > > > ...?
> > > 
> > > so we're in chromium_android. ensure_checkout is part of auto_bisect, and
> > > auto_bisect_staging. I could hardcode the ensure_checkout, but then that
> > defeats
> > > the split between _staging and prod. I can pull it out and make it part of
> the
> > 
> > Not following you here.
> > 
> > > bisect wrapper, but then we're reordering things.
> > 
> > This definitely should be in bisect, not in chromium_android. Not sure about
> > reordering, though.
> 
> if we're okay with ensure_checkout coming after common_tests_final_steps, then
> this all becomes much simpler
> 
>
https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipes/b...
> File scripts/slave/recipes/bisection/android_bisect_staging.py (right):
> 
>
https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipes/b...
> scripts/slave/recipes/bisection/android_bisect_staging.py:144:
> api.chromium_android.android_build_wrapper())
> On 2016/11/10 01:00:29, jbudorick wrote:
> > For the build wrapper, you could do
> > 
> > 
> >   def android_bisect_build_wrapper(bisect_obj):
> >     with api.chromium_android.android_build_wrapper()(bisect_obj):
> >       try:
> >         yield
> >       finally:
> >         bisect_obj.ensure_checkout()
> > 
> > which would preserve order.
> 
> That would put ensure_checkout() after
> chromium_android.common_tests_final_steps(). Maybe that's cool, but I don't
> know.

No, it won't.

[jbudorick, 2016-11-10 01:17:37]

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

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1168: def
wrapper(bisect_obj):
On 2016/11/09 23:51:46, jbudorick wrote:
> nit: This shouldn't be "bisect_obj" if it's here.

(and once ensure_checkout moves into bisect, this should be `api` w/ bisect
passing in bisect_obj.m)

[ghost stip (do not use), 2016-11-11 01:08:17]

ptal again

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

https://codereview.chromium.org/2486923002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1168: def
wrapper(bisect_obj):
On 2016/11/10 01:17:37, jbudorick wrote:
> On 2016/11/09 23:51:46, jbudorick wrote:
> > nit: This shouldn't be "bisect_obj" if it's here.
> 
> (and once ensure_checkout moves into bisect, this should be `api` w/ bisect
> passing in bisect_obj.m)

Done.

[jbudorick, 2016-11-11 01:15:55]

https://codereview.chromium.org/2486923002/diff/50001/scripts/slave/recipes/b...
File scripts/slave/recipes/bisection/android_bisect_staging.py (right):

https://codereview.chromium.org/2486923002/diff/50001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/android_bisect_staging.py:148: def
android_bisect_test_wrapper(api):
You have api here and new_api above. I'm guessing you're doing the latter to not
shadow api in the outer scope. Should you do the same here?

[jbudorick, 2016-11-11 01:16:19]

meant to add, lgtm w/ q

On 2016/11/11 01:15:55, jbudorick wrote:
>
https://codereview.chromium.org/2486923002/diff/50001/scripts/slave/recipes/b...
> File scripts/slave/recipes/bisection/android_bisect_staging.py (right):
> 
>
https://codereview.chromium.org/2486923002/diff/50001/scripts/slave/recipes/b...
> scripts/slave/recipes/bisection/android_bisect_staging.py:148: def
> android_bisect_test_wrapper(api):
> You have api here and new_api above. I'm guessing you're doing the latter to
not
> shadow api in the outer scope. Should you do the same here?

[ghost stip (do not use), 2016-11-11 02:19:18]

https://codereview.chromium.org/2486923002/diff/50001/scripts/slave/recipes/b...
File scripts/slave/recipes/bisection/android_bisect_staging.py (right):

https://codereview.chromium.org/2486923002/diff/50001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/android_bisect_staging.py:148: def
android_bisect_test_wrapper(api):
On 2016/11/11 01:15:55, jbudorick wrote:
> You have api here and new_api above. I'm guessing you're doing the latter to
not
> shadow api in the outer scope. Should you do the same here?

sure, why not

[ghost stip (do not use), 2016-11-11 02:19:36]

The CQ bit was checked by stip@chromium.org

[ghost stip (do not use), 2016-11-11 02:19:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertocn@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2486923002/#ps60001
(title: "Also add new_api to android_bisect_test_wrapper.")

[commit-bot: I haz the power, 2016-11-11 02:19:41]

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-11-11 02:27:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 02:27:30]

Try jobs failed on following builders:
  Build Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/326a009f39f75e10)

[ghost stip (do not use), 2016-11-11 20:11:58]

The CQ bit was checked by stip@chromium.org

[ghost stip (do not use), 2016-11-11 20:11:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertocn@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2486923002/#ps70001
(title: "Rebase onto latest master.")

[commit-bot: I haz the power, 2016-11-11 20:12:03]

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-11-11 20:24:07]

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.

This change is only for the staging recipe. If shown to work, we can land
https://codereview.chromium.org/2485763002/.

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.

This change is only for the staging recipe. If shown to work, we can land
https://codereview.chromium.org/2485763002/.

BUG=660852

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/71e8f921fdc7abc1cea5...
==========

[commit-bot: I haz the power, 2016-11-11 20:24:09]

Committed patchset #7 (id:70001) as
https://chromium.googlesource.com/chromium/tools/build/+/71e8f921fdc7abc1cea5...

[ghost stip (do not use), 2016-11-14 21:58:16]

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.

This change is only for the staging recipe. If shown to work, we can land
https://codereview.chromium.org/2485763002/.

BUG=660852

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/71e8f921fdc7abc1cea5...
==========

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.

This change is only for the staging recipe. If shown to work, we can land
https://codereview.chromium.org/2485763002/.

BUG=660852

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/71e8f921fdc7abc1cea5...
==========