[Android] Refactor the buildbot scripts to prepare for downstream usage.

[Android] Refactor the buildbot scripts to prepare for downstream usage.

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

[Siva Chandra, 2013-06-18 02:10:52]

On 2013/06/18 02:07:58, Siva Chandra wrote:

Hi,

We can discuss on VC tomorrow after you go over the CL once. I can run through
what I think about how we can use it downstream. Once we agree on the overall
design, I can start pushing jobs on to the try server. I have tested patch set 1
locally though.

Thanks,
Siva Chandra

[Siva Chandra, 2013-06-19 01:01:27]

PTaL at patch set 2. It has the modifications that we discussed offline.

[Siva Chandra, 2013-06-19 19:20:05]

On 2013/06/19 01:01:27, Siva Chandra wrote:
> PTaL at patch set 2. It has the modifications that we discussed offline.

I have now added a function to spawn logcat monitor as it can be used downstream
as well. PTaL.

[Siva Chandra, 2013-06-20 15:51:26]

On 2013/06/19 19:20:05, Siva Chandra wrote:
> On 2013/06/19 01:01:27, Siva Chandra wrote:
> > PTaL at patch set 2. It has the modifications that we discussed offline.
> 
> I have now added a function to spawn logcat monitor as it can be used
downstream
> as well. PTaL.

ping?

[Siva Chandra, 2013-06-22 14:35:05]

On 2013/06/20 15:51:26, Siva Chandra wrote:
> On 2013/06/19 19:20:05, Siva Chandra wrote:
> > On 2013/06/19 01:01:27, Siva Chandra wrote:
> > > PTaL at patch set 2. It has the modifications that we discussed offline.
> > 
> > I have now added a function to spawn logcat monitor as it can be used
> downstream
> > as well. PTaL.
> 
> ping?

Patch set 9 is good for a review.

[Siva Chandra, 2013-06-24 23:32:11]

On 2013/06/22 14:35:05, Siva Chandra wrote:
> On 2013/06/20 15:51:26, Siva Chandra wrote:
> > On 2013/06/19 19:20:05, Siva Chandra wrote:
> > > On 2013/06/19 01:01:27, Siva Chandra wrote:
> > > > PTaL at patch set 2. It has the modifications that we discussed offline.
> > > 
> > > I have now added a function to spawn logcat monitor as it can be used
> > downstream
> > > as well. PTaL.
> > 
> > ping?
> 
> Patch set 9 is good for a review.

Patch set 10 is green on the tryservers. PTaL.

[Siva Chandra, 2013-06-26 20:26:22]

ping?

[Isaac (away), 2013-06-28 01:39:12]

I like the general idea.  Here are some initial comments to get started on.

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
File build/android/buildbot/bb_device_steps.py (right):

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_device_steps.py:34: #   test_data: data folder in
format destination:source.
need documentation for additional options.

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_device_steps.py:36: 'name', 'apk', 'apk_package',
'test_apk', 'test_data', 'host_driven_root',
too many args for this -- can we group into subtuples like bb_run_bot?

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_device_steps.py:177: if test.screenshot:
Are screenshots ever off?

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_device_steps.py:240: LOGCAT_DIR])
sleep should be here

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

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_run_bot.py:18: BotConfig = collections.namedtuple(
Rename these to start with _

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_run_bot.py:28: def CreateBotConfig(bot_id,
host_object, test_object=None):
Just call these BotConfig, HostConfig and TestConfig

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_run_bot.py:88: def GetCommands(options, bot_config,
host_step_script, device_step_script):
These args are fragile, I'd prefer overrides to the scripts be part of
bot_config object.

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_run_bot.py:190: def GetBotConfig(bot_step_map,
bot_id):
Please name this something more generic, such as 'GetBestMatch' and remove
references to 'bot'.

[Siva Chandra, 2013-06-28 21:34:10]

PTAL. Addressed all comments.

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
File build/android/buildbot/bb_device_steps.py (right):

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_device_steps.py:34: #   test_data: data folder in
format destination:source.
On 2013/06/28 01:39:12, Isaac wrote:
> need documentation for additional options.

Done.

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_device_steps.py:36: 'name', 'apk', 'apk_package',
'test_apk', 'test_data', 'host_driven_root',
On 2013/06/28 01:39:12, Isaac wrote:
> too many args for this -- can we group into subtuples like bb_run_bot?

I have removed one arg. TBF, I do not understand the meaning of these args to
logically group them together and reduce the number of args further.

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_device_steps.py:177: if test.screenshot:
On 2013/06/28 01:39:12, Isaac wrote:
> Are screenshots ever off?

Yes. For example, downstream chrome-driver tests bot.

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_device_steps.py:240: LOGCAT_DIR])
On 2013/06/28 01:39:12, Isaac wrote:
> sleep should be here

Done.

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

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_run_bot.py:18: BotConfig = collections.namedtuple(
On 2013/06/28 01:39:12, Isaac wrote:
> Rename these to start with _

Done.

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_run_bot.py:28: def CreateBotConfig(bot_id,
host_object, test_object=None):
On 2013/06/28 01:39:12, Isaac wrote:
> Just call these BotConfig, HostConfig and TestConfig

Done.

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_run_bot.py:88: def GetCommands(options, bot_config,
host_step_script, device_step_script):
On 2013/06/28 01:39:12, Isaac wrote:
> These args are fragile, I'd prefer overrides to the scripts be part of
> bot_config object.

Done.

https://chromiumcodereview.appspot.com/17114007/diff/37001/build/android/buil...
build/android/buildbot/bb_run_bot.py:190: def GetBotConfig(bot_step_map,
bot_id):
On 2013/06/28 01:39:12, Isaac wrote:
> Please name this something more generic, such as 'GetBestMatch' and remove
> references to 'bot'.

Done.

[Isaac (away), 2013-06-29 09:55:38]

https://chromiumcodereview.appspot.com/17114007/diff/46001/build/android/buil...
File build/android/buildbot/bb_device_steps.py (right):

https://chromiumcodereview.appspot.com/17114007/diff/46001/build/android/buil...
build/android/buildbot/bb_device_steps.py:298: cmd_str += ' | tee %s' %
options.logcat_dump_output
I'm not sure I like using bash here -- could we always save to file and then cat
back?  We also don't necessarily need to take the logcat_dump_output option --
WDYT about always saving in the same place?

You'd need to add a 'stdout' option to run command and then do something like:

with open(<logcat file path>, 'w') as f:
   RunCmd([adb_logcat_printer.py, LOGCAT_DIR], stdout=f)

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

https://chromiumcodereview.appspot.com/17114007/diff/46001/build/android/buil...
build/android/buildbot/bb_run_bot.py:188: def GetBestMatch(step_map, name):
Let's name these vars 'id_map', and 'id'

Add function comment and delete command on line 218-221:

Return an object from the map, looking first for an exact id match.
If this fails, look for an id which is a substring of the specified id.
Choose the longest of all substring matches.

https://chromiumcodereview.appspot.com/17114007/diff/46001/build/android/buil...
build/android/buildbot/bb_run_bot.py:190: if not steps:
let's name this variable 'config'

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

https://chromiumcodereview.appspot.com/17114007/diff/46001/build/android/buil...
build/android/buildbot/bb_utils.py:92: def RunAllSteps(step_cmds, options):
I think this would be better to keep this code inline and not make a function.

[Siva Chandra, 2013-07-01 23:58:52]

Addressed all comments. PTAL.

https://chromiumcodereview.appspot.com/17114007/diff/46001/build/android/buil...
File build/android/buildbot/bb_device_steps.py (right):

https://chromiumcodereview.appspot.com/17114007/diff/46001/build/android/buil...
build/android/buildbot/bb_device_steps.py:298: cmd_str += ' | tee %s' %
options.logcat_dump_output
On 2013/06/29 09:55:39, Isaac wrote:
> I'm not sure I like using bash here -- could we always save to file and then
cat
> back?  We also don't necessarily need to take the logcat_dump_output option --
> WDYT about always saving in the same place?
> 
> You'd need to add a 'stdout' option to run command and then do something like:
> 
> with open(<logcat file path>, 'w') as f:
>    RunCmd([adb_logcat_printer.py, LOGCAT_DIR], stdout=f)

Done.

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

https://chromiumcodereview.appspot.com/17114007/diff/46001/build/android/buil...
build/android/buildbot/bb_run_bot.py:188: def GetBestMatch(step_map, name):
On 2013/06/29 09:55:39, Isaac wrote:
> Let's name these vars 'id_map', and 'id'
> 
> Add function comment and delete command on line 218-221:
> 
> Return an object from the map, looking first for an exact id match.
> If this fails, look for an id which is a substring of the specified id.
> Choose the longest of all substring matches.

Done.

https://chromiumcodereview.appspot.com/17114007/diff/46001/build/android/buil...
build/android/buildbot/bb_run_bot.py:190: if not steps:
On 2013/06/29 09:55:39, Isaac wrote:
> let's name this variable 'config'

Done.

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

https://chromiumcodereview.appspot.com/17114007/diff/46001/build/android/buil...
build/android/buildbot/bb_utils.py:92: def RunAllSteps(step_cmds, options):
On 2013/06/29 09:55:39, Isaac wrote:
> I think this would be better to keep this code inline and not make a function.

Done.

[Isaac (away), 2013-07-02 00:17:23]

LGTM, thanks!

[commit-bot: I haz the power, 2013-07-02 12:09:00]

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

[commit-bot: I haz the power, 2013-07-02 14:02:16]

Change committed as 209694