libyuv: Add Android Testers

libyuv: Add Android Testers

BUG=643144, 589180, 644212
Committed: https://chromium.googlesource.com/chromium/tools/build/+/ac3894c8293b42e21b891722965f982118bf6a5a

[kjellander_chromium, 2016-09-06 06:40:27]

Patchset #1 (id:1) has been deleted

[kjellander_chromium, 2016-09-06 06:45:04]

kjellander@chromium.org changed reviewers:
+ ehmaldonado@chromium.org, machenbach@chromium.org

[kjellander_chromium, 2016-09-06 06:45:05]

*phew* it's a lot of work to wire this up.
Luckily I was able to copy-paste a lot from our recipes.

The added buildnumber property (required by the chromium_android recipe module)
accounts for most of the noise in the  expectation updates - the interesting
part is the Android Testers.

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

https://codereview.chromium.org/2306373003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/libyuv/api.py:59: @property
All new methods below except runtests are pretty much a plain copy-paste from
the webrtc recipe module.

https://codereview.chromium.org/2306373003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/libyuv/builders.py (right):

https://codereview.chromium.org/2306373003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/libyuv/builders.py:42: 'build_gs_bucket':
'chromium-webrtc',
I think it's fine sharing our bucket since we're owners of the project and
there's a directory named after each master being created in it anyway.

[kjellander_chromium, 2016-09-06 07:17:43]

Description was changed from

==========
libyuv: Add Android Testers

BUG=643144
==========

to

==========
libyuv: Add Android Testers

BUG=643144, 589180
==========

[Michael Achenbach, 2016-09-06 08:17:49]

lgtm with comments:

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

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/libyuv/api.py:98: 'Testers cannot be forced without
providing revision information.'
nit: add spaces where the string wraps.

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/libyuv/api.py:102: # Ensure old build directory is
not used is by removing it.
nit: grammar. "is" too much? Or maybe restructure this sentence...

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/libyuv/api.py:123:
self.m.chromium_android.logcat_dump(gs_bucket='chromium-android')
Maybe keep gs bucket generic?
self.master_config.get('build_gs_bucket')

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
File
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json
(left):

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json:177:
"GYP_CHROMIUM_NO_ACTION": "1",
Is chromium's gyp off by default now?

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
File
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json
(right):

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json:24:
"src@12345",
svn revision? guess that's old test data... like in v8 :(

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json:69:
"linux-386",
Does it need to align with the hardware (which is 64 bits) or with the target
bits (which are 32)? Probably all unused here, right? Maybe avoid ensure_goma on
bots that don't compile?

Looks the same on webrtc standalone. So I guess it's ok for now...

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json:167:
"gs://chromium-webrtc/client.libyuv/Android Debug/full-build-linux.zip",
This looks confusing, but diving into 
https://cs.chromium.org/chromium/build/scripts/slave/extract_build.py?q=extra...
enlightened me that the revisions seems to be added before .zip in the end.
Anyways, horrible legacy. It would be nice if diving into these scripts wouldn't
be necessary to ensure consistency.

Looks the same on webrtc standalone - so guess it's ok...

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json:441:
"[SLAVE_BUILD]/src/out/Debug/full_log",
Webrtc standalone has [BUILDER_CACHE] here, but I assume that waterfall was
switched to different infrastructure, regarding remote_run?

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
File scripts/slave/recipes/libyuv/libyuv.py (right):

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.py:63: assert
bot_config.get('parent_buildername') is None, (
Very strict. We don't ensure many things that we don't expect to be there (i.e.
there are a lot more similar things that we don't check - to also keep the tests
maintainable).

You could instead in some prod code location ensure that parent_buildername=None
equals bot_type in ('builder', 'builder_tester'). E.g. in the
should_download_build property?

[kjellander_chromium, 2016-09-06 09:40:10]

Description was changed from

==========
libyuv: Add Android Testers

BUG=643144, 589180
==========

to

==========
libyuv: Add Android Testers

BUG=643144, 589180, 644212
==========

[kjellander_chromium, 2016-09-06 10:52:34]

Thanks for thorough review. I got plenty of additional cleanup to do now :)

PTAL

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

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/libyuv/api.py:98: 'Testers cannot be forced without
providing revision information.'
On 2016/09/06 08:17:49, machenbach (slow) wrote:
> nit: add spaces where the string wraps.

Done.

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/libyuv/api.py:102: # Ensure old build directory is
not used is by removing it.
On 2016/09/06 08:17:49, machenbach (slow) wrote:
> nit: grammar. "is" too much? Or maybe restructure this sentence...

Done.

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/libyuv/api.py:123:
self.m.chromium_android.logcat_dump(gs_bucket='chromium-android')
On 2016/09/06 08:17:49, machenbach (slow) wrote:
> Maybe keep gs bucket generic?
> self.master_config.get('build_gs_bucket')

right, I should get a separate bucket for libyuv for this and the archived
builds. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=644212 for
that.

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
File
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json
(left):

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json:177:
"GYP_CHROMIUM_NO_ACTION": "1",
On 2016/09/06 08:17:49, machenbach (slow) wrote:
> Is chromium's gyp off by default now?

No, libyuv is still using GYP, so GYP needs to run as part of the hooks.

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
File
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json
(right):

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json:24:
"src@12345",
On 2016/09/06 08:17:49, machenbach (slow) wrote:
> svn revision? guess that's old test data... like in v8 :(

Heh, OK I updated it to something obviously being a Git hash.

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json:69:
"linux-386",
On 2016/09/06 08:17:49, machenbach (slow) wrote:
> Does it need to align with the hardware (which is 64 bits) or with the target
> bits (which are 32)? Probably all unused here, right? 

I'm not familiar with this, but it seems it's aligning with TARGET_BITS.

> Maybe avoid ensure_goma on bots that don't compile?
> 
> Looks the same on webrtc standalone. So I guess it's ok for now...

Good point. I'll cleanup ensure_goma on non-compiling bots for both libyuv and
webrtc in a follow-up CL.

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json:167:
"gs://chromium-webrtc/client.libyuv/Android Debug/full-build-linux.zip",
On 2016/09/06 08:17:49, machenbach (slow) wrote:
> This looks confusing, but diving into 
>
https://cs.chromium.org/chromium/build/scripts/slave/extract_build.py?q=extra...
> enlightened me that the revisions seems to be added before .zip in the end.
> Anyways, horrible legacy. It would be nice if diving into these scripts
wouldn't
> be necessary to ensure consistency.
> 
> Looks the same on webrtc standalone - so guess it's ok...

Yeah, but I'll wait for a new bucket for libyuv before I submit this.

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json:441:
"[SLAVE_BUILD]/src/out/Debug/full_log",
On 2016/09/06 08:17:49, machenbach (slow) wrote:
> Webrtc standalone has [BUILDER_CACHE] here, but I assume that waterfall was
> switched to different infrastructure, regarding remote_run?

Yeah, it's Pawel's remote_run work. I don't think we need to bother about that
for libyuv (until he says so).

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
File scripts/slave/recipes/libyuv/libyuv.py (right):

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.py:63: assert
bot_config.get('parent_buildername') is None, (
On 2016/09/06 08:17:49, machenbach (slow) wrote:
> Very strict. We don't ensure many things that we don't expect to be there
(i.e.
> there are a lot more similar things that we don't check - to also keep the
tests
> maintainable).
> 
> You could instead in some prod code location ensure that
parent_buildername=None
> equals bot_type in ('builder', 'builder_tester'). E.g. in the
> should_download_build property?

Let's just remove this check instead (it was copy-pasted and I will remove it
from the webrtc recipe in a separate CL).

[kjellander_chromium, 2016-09-06 11:01:14]

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
File
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json
(right):

https://codereview.chromium.org/2306373003/diff/40001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json:69:
"linux-386",
On 2016/09/06 10:52:34, kjellander_chromium wrote:
> On 2016/09/06 08:17:49, machenbach (slow) wrote:
> > Does it need to align with the hardware (which is 64 bits) or with the
target
> > bits (which are 32)? Probably all unused here, right? 
> 
> I'm not familiar with this, but it seems it's aligning with TARGET_BITS.
> 
> > Maybe avoid ensure_goma on bots that don't compile?
> > 
> > Looks the same on webrtc standalone. So I guess it's ok for now...
> 
> Good point. I'll cleanup ensure_goma on non-compiling bots for both libyuv and
> webrtc in a follow-up CL.

Changed my mind: I did this update for libyuv in PS#4 since it makes more sense.

[Michael Achenbach, 2016-09-06 11:22:15]

still lgtm

https://codereview.chromium.org/2306373003/diff/80001/scripts/slave/recipes/l...
File
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json
(right):

https://codereview.chromium.org/2306373003/diff/80001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json:70:
"GYP_DEFINES": "OS=android component=static_library fastbuild=1
gomadir='[BUILD]/goma' target_arch=arm use_goma=1",
Hmm - that looks not optimal. Optimal would be no mention of gomadir at all. But
never mind. These kinds of glitches are everywhere.

[kjellander_chromium, 2016-09-06 11:24:32]

https://codereview.chromium.org/2306373003/diff/80001/scripts/slave/recipes/l...
File
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json
(right):

https://codereview.chromium.org/2306373003/diff/80001/scripts/slave/recipes/l...
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json:70:
"GYP_DEFINES": "OS=android component=static_library fastbuild=1
gomadir='[BUILD]/goma' target_arch=arm use_goma=1",
On 2016/09/06 11:22:15, machenbach (slow) wrote:
> Hmm - that looks not optimal. Optimal would be no mention of gomadir at all.
But
> never mind. These kinds of glitches are everywhere.

Yeah I noticed that too, but I would assume the Chrome team is working actively
for removing GYP_* environment variables now, so hopefully this can be gone
reasonably soon.

[ehmaldonado_chromium, 2016-09-07 09:22:00]

On 2016/09/06 11:24:32, kjellander_chromium wrote:
>
https://codereview.chromium.org/2306373003/diff/80001/scripts/slave/recipes/l...
> File
>
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json
> (right):
> 
>
https://codereview.chromium.org/2306373003/diff/80001/scripts/slave/recipes/l...
>
scripts/slave/recipes/libyuv/libyuv.expected/client_libyuv_android_tester_arm32_debug__nexus_5x_.json:70:
> "GYP_DEFINES": "OS=android component=static_library fastbuild=1
> gomadir='[BUILD]/goma' target_arch=arm use_goma=1",
> On 2016/09/06 11:22:15, machenbach (slow) wrote:
> > Hmm - that looks not optimal. Optimal would be no mention of gomadir at all.
> But
> > never mind. These kinds of glitches are everywhere.
> 
> Yeah I noticed that too, but I would assume the Chrome team is working
actively
> for removing GYP_* environment variables now, so hopefully this can be gone
> reasonably soon.

LGTM

[kjellander_chromium, 2016-09-09 06:06:41]

The google storage bucket is now created, so let's try this out!

[kjellander_chromium, 2016-09-09 06:06:57]

The CQ bit was checked by kjellander@chromium.org

[commit-bot: I haz the power, 2016-09-09 06:07: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-09-09 06:09:52]

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

[commit-bot: I haz the power, 2016-09-09 06:09:53]

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

[kjellander_chromium, 2016-09-09 06:28:59]

The CQ bit was checked by kjellander@chromium.org

[kjellander_chromium, 2016-09-09 06:29:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
ehmaldonado@chromium.org, machenbach@chromium.org
Link to the patchset: https://codereview.chromium.org/2306373003/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2016-09-09 06:29: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-09-09 06:40:13]

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

[commit-bot: I haz the power, 2016-09-09 06:40:14]

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

[kjellander_chromium, 2016-09-09 06:54:29]

The CQ bit was checked by kjellander@chromium.org

[kjellander_chromium, 2016-09-09 06:54:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
ehmaldonado@chromium.org, machenbach@chromium.org
Link to the patchset: https://codereview.chromium.org/2306373003/#ps120001
(title: "Added test case to cover missing coverage")

[commit-bot: I haz the power, 2016-09-09 06:54:35]

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-09-09 07:06:47]

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

[commit-bot: I haz the power, 2016-09-09 07:06:48]

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

[kjellander_chromium, 2016-09-14 05:06:46]

The CQ bit was checked by kjellander@chromium.org

[kjellander_chromium, 2016-09-14 05:06:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
ehmaldonado@chromium.org, machenbach@chromium.org
Link to the patchset: https://codereview.chromium.org/2306373003/#ps140001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-09-14 05:06:52]

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-09-14 05:09:40]

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

[commit-bot: I haz the power, 2016-09-14 05:09:41]

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

[kjellander_chromium, 2016-09-14 07:05:31]

The CQ bit was checked by kjellander@chromium.org

[kjellander_chromium, 2016-09-14 07:05:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
ehmaldonado@chromium.org, machenbach@chromium.org
Link to the patchset: https://codereview.chromium.org/2306373003/#ps150001
(title: "Rebased again")

[commit-bot: I haz the power, 2016-09-14 07:05:43]

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-09-14 07:20:53]

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

[commit-bot: I haz the power, 2016-09-14 07:21:00]

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

[kjellander_chromium, 2016-09-14 09:19:04]

The CQ bit was checked by kjellander@chromium.org

[kjellander_chromium, 2016-09-14 09:19:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
ehmaldonado@chromium.org, machenbach@chromium.org
Link to the patchset: https://codereview.chromium.org/2306373003/#ps170001
(title: "Fixed WebRTC expectation")

[commit-bot: I haz the power, 2016-09-14 09:19:08]

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-09-14 09:24:07]

Description was changed from

==========
libyuv: Add Android Testers

BUG=643144, 589180, 644212
==========

to

==========
libyuv: Add Android Testers

BUG=643144, 589180, 644212

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

[commit-bot: I haz the power, 2016-09-14 09:24:08]

Committed patchset #9 (id:170001) as
https://chromium.googlesource.com/chromium/tools/build/+/ac3894c8293b42e21b89...