[Android] Refactor bb_host_steps to prepare for downstream usage.

[Android] Refactor bb_host_steps to prepare for downstream usage.

- Refactored bb_host_steps.py to take a list of steps.
- Moved run step logic into bb_utils

BUG=249997
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206555

[Siva Chandra, 2013-06-12 18:50:30]

Review patch set 5. I have tested it locally.

The change is according to what we discussed offline except that I am not using
OrderedDict at it is available only starting Python 2.7.

[Isaac (away), 2013-06-12 19:49:05]

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
File build/android/buildbot/bb_host_steps.py (left):

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
build/android/buildbot/bb_host_steps.py:61: else:
what happened here?

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
File build/android/buildbot/bb_host_steps.py (right):

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
build/android/buildbot/bb_host_steps.py:36: build_type =
options.factory_properties.get('target', 'Debug')
check out bb_device_steps for how to put this into options.  We should not
repeat this code in multiple places.

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
build/android/buildbot/bb_host_steps.py:108: ('runhooks', RunHooks),
this should not be a separate step, it should be called whenever compile is
requested.

The order should be:
compile
extract_build
<all tests>
zip_build

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
build/android/buildbot/bb_host_steps.py:132: if options.steps:
lines 132-140 should be moved to a function, like 'def RunSteps(all_steps,
options)', probably in bb_utils if you're expecting to change bb_device_steps
the same way.

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
build/android/buildbot/bb_host_steps.py:134: unknown_steps = set(steps) - set([x
for x, _ in host_steps])
generator is preferred style:

set(step for step, _ in host_steps)

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
File build/android/buildbot/bb_run_bot.py (right):

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
build/android/buildbot/bb_run_bot.py:84: property_args +
unneeded linebreak

[Siva Chandra, 2013-06-12 21:21:35]

PTAL. The latest patchset addresses your comments and also passes presubmit on
my machine. The try jobs are in the queue.

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
File build/android/buildbot/bb_host_steps.py (left):

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
build/android/buildbot/bb_host_steps.py:61: else:
On 2013/06/12 19:49:05, Isaac wrote:
> what happened here?

This change assumes that one would not request an experimental compile without a
'compile' first. With the previous logic, we have had to make two calls to
Compile, one for the 'compile' step, and another for the 'Experimental Compile'
step.

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
File build/android/buildbot/bb_host_steps.py (right):

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
build/android/buildbot/bb_host_steps.py:36: build_type =
options.factory_properties.get('target', 'Debug')
On 2013/06/12 19:49:05, Isaac wrote:
> check out bb_device_steps for how to put this into options.  We should not
> repeat this code in multiple places.

Done.

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
build/android/buildbot/bb_host_steps.py:108: ('runhooks', RunHooks),
On 2013/06/12 19:49:05, Isaac wrote:
> this should not be a separate step, it should be called whenever compile is
> requested.
> 
> The order should be:
> compile
> extract_build
> <all tests>
> zip_build

Done.

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
build/android/buildbot/bb_host_steps.py:132: if options.steps:
On 2013/06/12 19:49:05, Isaac wrote:
> lines 132-140 should be moved to a function, like 'def RunSteps(all_steps,
> options)', probably in bb_utils if you're expecting to change bb_device_steps
> the same way.

Done.

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
build/android/buildbot/bb_host_steps.py:134: unknown_steps = set(steps) - set([x
for x, _ in host_steps])
On 2013/06/12 19:49:05, Isaac wrote:
> generator is preferred style:
> 
> set(step for step, _ in host_steps)

Done.

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
File build/android/buildbot/bb_run_bot.py (right):

https://chromiumcodereview.appspot.com/15817022/diff/11001/build/android/buil...
build/android/buildbot/bb_run_bot.py:84: property_args +
On 2013/06/12 19:49:05, Isaac wrote:
> unneeded linebreak

Done.

[Isaac (away), 2013-06-12 21:37:35]

Looks great -- I'll take one last look w/ changes.

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
File build/android/buildbot/bb_host_steps.py (right):

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
build/android/buildbot/bb_host_steps.py:34: def RunHooks(options):
This can still take build_type

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
build/android/buildbot/bb_host_steps.py:59: build_args = '
'.join(options.build_args.split(','))
This is not how compile.py takes this argument.  For each item in the list you
have to pass a separate --build-args=<item>

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
build/android/buildbot/bb_host_steps.py:122: help='Comma separated list of build
targets.')
if these are targets, name this option 'build-targets'

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
File build/android/buildbot/bb_utils.py (right):

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
build/android/buildbot/bb_utils.py:83: if options.steps:
maybe change to

if not options.steps:
  return

and remove line 82

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
build/android/buildbot/bb_utils.py:87: return sys.exit('Unknown steps %s' %
list(unknown_steps))
sys.exit does not return

[Siva Chandra, 2013-06-13 00:09:56]

PTaL at patch set 7. All comments addressed. Local presubmit successful.

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
File build/android/buildbot/bb_host_steps.py (right):

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
build/android/buildbot/bb_host_steps.py:34: def RunHooks(options):
On 2013/06/12 21:37:35, Isaac wrote:
> This can still take build_type

Done.

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
build/android/buildbot/bb_host_steps.py:59: build_args = '
'.join(options.build_args.split(','))
On 2013/06/12 21:37:35, Isaac wrote:
> This is not how compile.py takes this argument.  For each item in the list you
> have to pass a separate --build-args=<item>

Build args are all args which are passed to ninja (or a build tool in general)
as is. Going by how downstream is, I know that ninja can take all targets
simultaneously. I think make can also take all of them simultaneously. Let me
know if I got my understanding wrong somewhere.

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
build/android/buildbot/bb_host_steps.py:122: help='Comma separated list of build
targets.')
On 2013/06/12 21:37:35, Isaac wrote:
> if these are targets, name this option 'build-targets'

Done.

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
File build/android/buildbot/bb_utils.py (right):

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
build/android/buildbot/bb_utils.py:83: if options.steps:
On 2013/06/12 21:37:35, Isaac wrote:
> maybe change to
> 
> if not options.steps:
>   return
> 
> and remove line 82

Done.

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
build/android/buildbot/bb_utils.py:87: return sys.exit('Unknown steps %s' %
list(unknown_steps))
On 2013/06/12 21:37:35, Isaac wrote:
> sys.exit does not return

Done.

[Isaac (away), 2013-06-13 00:20:55]

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
File build/android/buildbot/bb_host_steps.py (right):

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
build/android/buildbot/bb_host_steps.py:59: build_args = '
'.join(options.build_args.split(','))
On 2013/06/13 00:09:56, Siva Chandra wrote:
> On 2013/06/12 21:37:35, Isaac wrote:
> > This is not how compile.py takes this argument.  For each item in the list
you
> > have to pass a separate --build-args=<item>
> 
> Build args are all args which are passed to ninja (or a build tool in general)
> as is. Going by how downstream is, I know that ninja can take all targets
> simultaneously. I think make can also take all of them simultaneously. Let me
> know if I got my understanding wrong somewhere.

I don't think it is how the argument is intended to be used.  Check out the
options section of compile.py.

https://chromiumcodereview.appspot.com/15817022/diff/30001/build/android/buil...
File build/android/buildbot/bb_utils.py (right):

https://chromiumcodereview.appspot.com/15817022/diff/30001/build/android/buil...
build/android/buildbot/bb_utils.py:88: sys.exit('Unknown steps %s' %
list(unknown_steps))
sys.exit doesn't take a string.

You want

print >>sys.stderr, 'FATAL Unknown steps %s' %...
sys.exit(1)

Rememeber to locally test functions like this and make sure they behave as
expecting.  In this case if you passed an unknown set of steps you would not
have seen an error message.

[Siva Chandra, 2013-06-13 18:04:34]

PTaL. I made the changes you suggested.

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
File build/android/buildbot/bb_host_steps.py (right):

https://chromiumcodereview.appspot.com/15817022/diff/21001/build/android/buil...
build/android/buildbot/bb_host_steps.py:59: build_args = '
'.join(options.build_args.split(','))
On 2013/06/13 00:20:55, Isaac wrote:
> On 2013/06/13 00:09:56, Siva Chandra wrote:
> > On 2013/06/12 21:37:35, Isaac wrote:
> > > This is not how compile.py takes this argument.  For each item in the list
> you
> > > have to pass a separate --build-args=<item>
> > 
> > Build args are all args which are passed to ninja (or a build tool in
general)
> > as is. Going by how downstream is, I know that ninja can take all targets
> > simultaneously. I think make can also take all of them simultaneously. Let
me
> > know if I got my understanding wrong somewhere.
> 
> I don't think it is how the argument is intended to be used.  Check out the
> options section of compile.py.

The help text for '--build-args' only says, "arguments to pass to the build
tool."

I have changed to invoke the build tool separately for each build target anyway.

https://chromiumcodereview.appspot.com/15817022/diff/30001/build/android/buil...
File build/android/buildbot/bb_utils.py (right):

https://chromiumcodereview.appspot.com/15817022/diff/30001/build/android/buil...
build/android/buildbot/bb_utils.py:88: sys.exit('Unknown steps %s' %
list(unknown_steps))
On 2013/06/13 00:20:55, Isaac wrote:
> sys.exit doesn't take a string.

Per my own testing and per documentation here,
http://docs.python.org/2.6/library/sys.html#sys.exit, what I had was equivalent
to what you suggested. I have changed it according to your suggestion anyway.

[Siva Chandra, 2013-06-15 00:52:53]

ping?

[Isaac (away), 2013-06-15 00:57:22]

LGTM -- sorry for delay.

Also you are right about sys.exit, I didn't know that. Either way is fine.

[commit-bot: I haz the power, 2013-06-15 01:04:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sivachandra@chromium.org/15817022/40001

[Isaac (away), 2013-06-15 01:07:27]

sorry -- one other nit -- can you plus improve the description / add a bug id?

[Isaac (away), 2013-06-15 01:07:38]

please*

[Siva Chandra, 2013-06-15 01:20:45]

On 2013/06/15 01:07:38, Isaac wrote:
> please*

Done.

[commit-bot: I haz the power, 2013-06-15 03:57:46]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-15 04:52:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sivachandra@chromium.org/15817022/40001

[commit-bot: I haz the power, 2013-06-15 04:52:26]

Change committed as 206555