Enable all Android builders to use 'chrome_with_codecs'.

Enable all Android builders to use 'chrome_with_codecs'.

Now that we have a unified media pipeline on Android, we need to
set these flags to ensure proprietary codec tests and such continue
to work properly.

This does *NOT* enable 'chrome_with_codecs' for the builder which
archives the publicly advertised Chrome for Android builds at:

https://storage.googleapis.com/chromium-browser-continuous/index.html

BUG=570762
TEST=none

Committed: https://crrev.com/56fd27e36643e9e47c1ea9c07115a89a16523724
Cr-Commit-Position: refs/heads/master@{#380254}

[DaleCurtis, 2016-03-07 22:34:00]

Description was changed from

==========
Enable all Android builders to use 'chrome_with_codecs'.

Now that we have a unified media pipeline on Android, we need to
set these flags to ensure proprietary codec tests and such continue
to work properly.

BUG=
==========

to

==========
Enable all Android builders to use 'chrome_with_codecs'.

Now that we have a unified media pipeline on Android, we need to
set these flags to ensure proprietary codec tests and such continue
to work properly.

BUG=570762
TEST=none
==========

[DaleCurtis, 2016-03-07 22:34:00]

dalecurtis@chromium.org changed reviewers:
+ dpranke@chromium.org, jbudorick@chromium.org

[DaleCurtis, 2016-03-07 22:36:04]

This rejiggers things slightly now that we're enabling this everywhere except
for the artifact builders. I believe this is closer in spirit to what dpranke@
was asking for via chat.

https://codereview.chromium.org/1773883002/diff/1/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/1773883002/diff/1/tools/mb/mb_config.pyl#newc...
tools/mb/mb_config.pyl:494: # Take care when changing any of these builders to
ensure that you do not
I'm not a fan of this of enforcement by comment, so if there's a way to check
this at submit time through either a PRESUBMIT or some test please let me know
:)

[DaleCurtis, 2016-03-07 22:37:15]

Description was changed from

==========
Enable all Android builders to use 'chrome_with_codecs'.

Now that we have a unified media pipeline on Android, we need to
set these flags to ensure proprietary codec tests and such continue
to work properly.

BUG=570762
TEST=none
==========

to

==========
Enable all Android builders to use 'chrome_with_codecs'.

Now that we have a unified media pipeline on Android, we need to
set these flags to ensure proprietary codec tests and such continue
to work properly.

This does *NOT* enable 'chrome_with_codecs' for the builder which
archives the publicly advertised Chrome for Android builds at:

https://storage.googleapis.com/chromium-browser-continuous/index.html

BUG=570762
TEST=none
==========

[jbudorick, 2016-03-07 22:59:41]

not crazy about "android_without_codecs,"  but this otherwise seems fine to me.

[Dirk Pranke, 2016-03-09 02:26:54]

comments below on the approach.

Also, I talked to kerz@ about this briefly and he was of the opinion that the
buckets are public by an accident of history and don't really need to be, as
long as the tester builders can read from them.

Whether we can get away with making them be non-public for the desktop bots is
debatable, but we can probably do that fairly safely for Android, so maybe file
a follow-up bug to investigate that further?

https://codereview.chromium.org/1773883002/diff/1/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/1773883002/diff/1/tools/mb/mb_config.pyl#newc...
tools/mb/mb_config.pyl:39: 'android_gyp_release_bot_minimal_symbols':
['android_without_codecs', 'gyp', 'release_bot_minimal_symbols'],
I would actually change this name to
'android_without_codecs_gyp_release_bot_minimal_symbols' to be crystal-clear,
even if it is exceptionally verbose (the upside is that it at least follows the
normal config convention names). 

I would then make sure the comments for the 'android' mixin and the 'chromium'
waterfall are clear as to what they're doing, and omit the comment here.

https://codereview.chromium.org/1773883002/diff/1/tools/mb/mb_config.pyl#newc...
tools/mb/mb_config.pyl:73: 'gyp_release_bot_android': ['gyp', 'release_bot',
'android', 'chrome_with_codecs'],
This change isn't needed, right?

https://codereview.chromium.org/1773883002/diff/1/tools/mb/mb_config.pyl#newc...
tools/mb/mb_config.pyl:187: 'mixins': ['chrome_with_codecs'],
Can we change this to:

  'android': {
    'mixins': ['android_without_codecs', 'chrome_with_codecs'],
  }

I think this is stylistically a bit clearer and cleaner.

Also, add a comment as to why we build w/ codecs by default on most bots but not
on the p/chromium bots ?

https://codereview.chromium.org/1773883002/diff/1/tools/mb/mb_config.pyl#newc...
tools/mb/mb_config.pyl:494: # Take care when changing any of these builders to
ensure that you do not
On 2016/03/07 22:36:04, DaleCurtis wrote:
> I'm not a fan of this of enforcement by comment, so if there's a way to check
> this at submit time through either a PRESUBMIT or some test please let me know
> :)

I agree that we should probably enforce this more strictly. the presubmit checks
run `mb validate`,
so we can change mb validate to check that no bot on the chromium waterfall
contains the chrome_with_codecs
defines.

[DaleCurtis, 2016-03-09 04:19:58]

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=593222 -- PTAL.

https://codereview.chromium.org/1773883002/diff/1/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/1773883002/diff/1/tools/mb/mb_config.pyl#newc...
tools/mb/mb_config.pyl:39: 'android_gyp_release_bot_minimal_symbols':
['android_without_codecs', 'gyp', 'release_bot_minimal_symbols'],
On 2016/03/09 at 02:26:53, Dirk Pranke wrote:
> I would actually change this name to
'android_without_codecs_gyp_release_bot_minimal_symbols' to be crystal-clear,
even if it is exceptionally verbose (the upside is that it at least follows the
normal config convention names). 
> 
> I would then make sure the comments for the 'android' mixin and the 'chromium'
waterfall are clear as to what they're doing, and omit the comment here.

Done.

https://codereview.chromium.org/1773883002/diff/1/tools/mb/mb_config.pyl#newc...
tools/mb/mb_config.pyl:73: 'gyp_release_bot_android': ['gyp', 'release_bot',
'android', 'chrome_with_codecs'],
On 2016/03/09 at 02:26:53, Dirk Pranke wrote:
> This change isn't needed, right?

Correct, stale change. Removed.

https://codereview.chromium.org/1773883002/diff/1/tools/mb/mb_config.pyl#newc...
tools/mb/mb_config.pyl:187: 'mixins': ['chrome_with_codecs'],
On 2016/03/09 at 02:26:53, Dirk Pranke wrote:
> Can we change this to:
> 
>   'android': {
>     'mixins': ['android_without_codecs', 'chrome_with_codecs'],
>   }
> 
> I think this is stylistically a bit clearer and cleaner.
> 
> Also, add a comment as to why we build w/ codecs by default on most bots but
not on the p/chromium bots ?

Good suggestion. Done.

https://codereview.chromium.org/1773883002/diff/1/tools/mb/mb_config.pyl#newc...
tools/mb/mb_config.pyl:494: # Take care when changing any of these builders to
ensure that you do not
On 2016/03/09 at 02:26:53, Dirk Pranke wrote:
> On 2016/03/07 22:36:04, DaleCurtis wrote:
> > I'm not a fan of this of enforcement by comment, so if there's a way to
check
> > this at submit time through either a PRESUBMIT or some test please let me
know
> > :)
> 
> I agree that we should probably enforce this more strictly. the presubmit
checks run `mb validate`,
> so we can change mb validate to check that no bot on the chromium waterfall
contains the chrome_with_codecs
> defines.

Done. Added a unittest for that too.

[Dirk Pranke, 2016-03-09 19:47:08]

lgtm w/ comment. Thanks for going above and beyond :). If my suggestion doesn't
work, feel free to land as-is and I'll take a look.

https://codereview.chromium.org/1773883002/diff/20001/tools/mb/mb_unittest.py
File tools/mb/mb_unittest.py (right):

https://codereview.chromium.org/1773883002/diff/20001/tools/mb/mb_unittest.py...
tools/mb/mb_unittest.py:456: mbw = self.fake_mbw()
This is, I think, somewhat more complicated than it needs to be. Can't you just
move this whole test_validate() function into the regular UnitTest class and
rewrite it as:

  def test_validate(self):
    mbw = self.fake_mbw()
    mbw.files[mbw.default_config] = TEST_BAD_CONFIG
    mbw.ParseArgs(['validate'])
    self.check(['validate'], ret=1, mbw=mbw, out=TES_BAD_CONFIG_ERR) 

?

[DaleCurtis, 2016-03-09 21:24:05]

Patchset #3 (id:40001) has been deleted

[DaleCurtis, 2016-03-09 21:28:59]

Patchset #3 (id:60001) has been deleted

[DaleCurtis, 2016-03-09 21:35:28]

Patchset #3 (id:80001) has been deleted

[DaleCurtis, 2016-03-09 21:36:27]

Thanks for review!

https://codereview.chromium.org/1773883002/diff/20001/tools/mb/mb_unittest.py
File tools/mb/mb_unittest.py (right):

https://codereview.chromium.org/1773883002/diff/20001/tools/mb/mb_unittest.py...
tools/mb/mb_unittest.py:456: mbw = self.fake_mbw()
On 2016/03/09 at 19:47:07, Dirk Pranke wrote:
> This is, I think, somewhat more complicated than it needs to be. Can't you
just move this whole test_validate() function into the regular UnitTest class
and rewrite it as:
> 
>   def test_validate(self):
>     mbw = self.fake_mbw()
>     mbw.files[mbw.default_config] = TEST_BAD_CONFIG
>     mbw.ParseArgs(['validate'])
>     self.check(['validate'], ret=1, mbw=mbw, out=TES_BAD_CONFIG_ERR) 
> 
> ?

Whoops, yeah it was late when I wrote the other code is my excuse :) It didn't
work quite as you wrote since when mb raises MBErr the prints to stderr, stdout
never happen. So I extended check() to support exceptions.

[DaleCurtis, 2016-03-09 21:40:28]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-03-09 21:40:28]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1773883002/#ps120001
(title: "Fix exception check.")

[commit-bot: I haz the power, 2016-03-09 21:41:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1773883002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1773883002/120001

[Dirk Pranke, 2016-03-09 22:57:07]

still lgtm. It doesn't sound like MB is quite doing what I want it to, but I'll
look at that later.

Thanks again!

[commit-bot: I haz the power, 2016-03-09 23:06:50]

Committed patchset #4 (id:120001)

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

Description was changed from

==========
Enable all Android builders to use 'chrome_with_codecs'.

Now that we have a unified media pipeline on Android, we need to
set these flags to ensure proprietary codec tests and such continue
to work properly.

This does *NOT* enable 'chrome_with_codecs' for the builder which
archives the publicly advertised Chrome for Android builds at:

https://storage.googleapis.com/chromium-browser-continuous/index.html

BUG=570762
TEST=none
==========

to

==========
Enable all Android builders to use 'chrome_with_codecs'.

Now that we have a unified media pipeline on Android, we need to
set these flags to ensure proprietary codec tests and such continue
to work properly.

This does *NOT* enable 'chrome_with_codecs' for the builder which
archives the publicly advertised Chrome for Android builds at:

https://storage.googleapis.com/chromium-browser-continuous/index.html

BUG=570762
TEST=none

Committed: https://crrev.com/56fd27e36643e9e47c1ea9c07115a89a16523724
Cr-Commit-Position: refs/heads/master@{#380254}
==========

[commit-bot: I haz the power, 2016-03-09 23:08:20]

Patchset 4 (id:??) landed as
https://crrev.com/56fd27e36643e9e47c1ea9c07115a89a16523724
Cr-Commit-Position: refs/heads/master@{#380254}

[kjellander_chromium, 2016-03-15 03:54:58]

kjellander@chromium.org changed reviewers:
+ kjellander@chromium.org

[kjellander_chromium, 2016-03-15 03:54:59]

https://codereview.chromium.org/1773883002/diff/120001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/1773883002/diff/120001/tools/mb/mb.py#newcode311
tools/mb/mb.py:311: errs.append('Missing "chromium" master. Please update this
proprietary '
In WebRTC, we have own mb_config.pyl for the standalone build:
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/build/mb_co...

This check causes an error when we run the mb.py validation on our config. Would
it be possible to just remove the error here?
It feels wrong that we would just add a fake 'chromium' master to our config.

[jbudorick, 2016-03-15 03:55:49]

On 2016/03/15 03:54:59, kjellander (chromium) wrote:
> https://codereview.chromium.org/1773883002/diff/120001/tools/mb/mb.py
> File tools/mb/mb.py (right):
> 
>
https://codereview.chromium.org/1773883002/diff/120001/tools/mb/mb.py#newcode311
> tools/mb/mb.py:311: errs.append('Missing "chromium" master. Please update this
> proprietary '
> In WebRTC, we have own mb_config.pyl for the standalone build:
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/build/mb_co...
> 
> This check causes an error when we run the mb.py validation on our config.
Would
> it be possible to just remove the error here?
> It feels wrong that we would just add a fake 'chromium' master to our config.

We hit this internally as well and wound up doing exactly that.

[kjellander_chromium, 2016-03-15 05:15:35]

On 2016/03/15 03:55:49, jbudorick wrote:
> On 2016/03/15 03:54:59, kjellander (chromium) wrote:
> > https://codereview.chromium.org/1773883002/diff/120001/tools/mb/mb.py
> > File tools/mb/mb.py (right):
> > 
> >
>
https://codereview.chromium.org/1773883002/diff/120001/tools/mb/mb.py#newcode311
> > tools/mb/mb.py:311: errs.append('Missing "chromium" master. Please update
this
> > proprietary '
> > In WebRTC, we have own mb_config.pyl for the standalone build:
> >
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/build/mb_co...
> > 
> > This check causes an error when we run the mb.py validation on our config.
> Would
> > it be possible to just remove the error here?
> > It feels wrong that we would just add a fake 'chromium' master to our
config.
> 
> We hit this internally as well and wound up doing exactly that.

OK, but wouldn't it make more sense to remove the error here? I don't see what
value it adds.

[Dirk Pranke, 2016-03-15 15:36:12]

On 2016/03/15 05:15:35, kjellander (chromium) wrote:
> On 2016/03/15 03:55:49, jbudorick wrote:
> > On 2016/03/15 03:54:59, kjellander (chromium) wrote:
> > > https://codereview.chromium.org/1773883002/diff/120001/tools/mb/mb.py
> > > File tools/mb/mb.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1773883002/diff/120001/tools/mb/mb.py#newcode311
> > > tools/mb/mb.py:311: errs.append('Missing "chromium" master. Please update
> this
> > > proprietary '
> > > In WebRTC, we have own mb_config.pyl for the standalone build:
> > >
> >
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/build/mb_co...
> > > 
> > > This check causes an error when we run the mb.py validation on our config.
> > Would
> > > it be possible to just remove the error here?
> > > It feels wrong that we would just add a fake 'chromium' master to our
> config.
> > 
> > We hit this internally as well and wound up doing exactly that.
> 
> OK, but wouldn't it make more sense to remove the error here? I don't see what
> value it adds.

Ah, whoops. Yes, we should only do this check if using the default mb config and
the chromium master exists; you shouldn't have to add a fake master.

[DaleCurtis, 2016-03-15 17:19:25]

Whoops, sorry for the trouble. I didn't realize this was a general purpose tool.
Happy to put out a fix to restrict this to only the "default config", but not
sure how that's checked. If the suggestion is to limit it to only when
'chromium' exists, we run the risk of a rename losing this check. Any pointers?

[Dirk Pranke, 2016-03-15 17:54:52]

On 2016/03/15 17:19:25, DaleCurtis wrote:
> Whoops, sorry for the trouble. I didn't realize this was a general purpose
tool.
> Happy to put out a fix to restrict this to only the "default config", but not
> sure how that's checked. If the suggestion is to limit it to only when
> 'chromium' exists, we run the risk of a rename losing this check. Any
pointers?

I'll put up a patch and fix the thing I was referring to in comment #16 as well.

[DaleCurtis, 2016-03-15 17:56:34]

Thanks Dirk!

[Dirk Pranke, 2016-03-16 03:01:43]

Fix posted to https://codereview.chromium.org/1803293002/ .