Configs Blimp Engine official build to use non-debug build with dcheck_always_on

Configs Blimp Engine official build to use non-debug build with dcheck_always_on

WebP encoding is much slower on debug build. Switches to release build with dcheck on.

BUG=603643
Committed: https://crrev.com/335a22ae39ad32f0a75f1c7accfc6119e5f00df8
Cr-Commit-Position: refs/heads/master@{#392150}

[haibinlu, 2016-04-29 18:06:28]

haibinlu@chromium.org changed reviewers:
+ dpranke@chromium.org, maniscalco@chromium.org

[maniscalco, 2016-04-29 18:32:28]

https://codereview.chromium.org/1927323003/diff/1/build/args/bots/official.de...
File build/args/bots/official.desktop/blimp-engine.gn (right):

https://codereview.chromium.org/1927323003/diff/1/build/args/bots/official.de...
build/args/bots/official.desktop/blimp-engine.gn:3: dcheck_always_on = true
Why change the flags here instead of in build/args/blimp_engine.gn ?  What's the
rationale?

[haibinlu, 2016-04-29 22:51:34]

https://codereview.chromium.org/1927323003/diff/1/build/args/bots/official.de...
File build/args/bots/official.desktop/blimp-engine.gn (right):

https://codereview.chromium.org/1927323003/diff/1/build/args/bots/official.de...
build/args/bots/official.desktop/blimp-engine.gn:3: dcheck_always_on = true
On 2016/04/29 18:32:28, maniscalco wrote:
> Why change the flags here instead of in build/args/blimp_engine.gn ?  What's
the
> rationale?

in local development case, debug build is desirable.

[maniscalco, 2016-04-29 22:58:57]

https://codereview.chromium.org/1927323003/diff/1/build/args/bots/official.de...
File build/args/bots/official.desktop/blimp-engine.gn (right):

https://codereview.chromium.org/1927323003/diff/1/build/args/bots/official.de...
build/args/bots/official.desktop/blimp-engine.gn:3: dcheck_always_on = true
On 2016/04/29 22:51:34, haibinlu wrote:
> On 2016/04/29 18:32:28, maniscalco wrote:
> > Why change the flags here instead of in build/args/blimp_engine.gn ?  What's
> the
> > rationale?
> 
> in local development case, debug build is desirable. 

For local builds we always have the option of just adding is_debug = true to our
args.gn file (just like we do with use_goma = true).

Dirk, WDYT?  Should we be adding the new args to
//build/args/bots/official.desktop/blimp-engine.gn or
//build/args/blimp_engine.gn ?

[Dirk Pranke, 2016-04-30 00:04:32]

https://codereview.chromium.org/1927323003/diff/1/build/args/bots/official.de...
File build/args/bots/official.desktop/blimp-engine.gn (right):

https://codereview.chromium.org/1927323003/diff/1/build/args/bots/official.de...
build/args/bots/official.desktop/blimp-engine.gn:3: dcheck_always_on = true
On 2016/04/29 22:58:57, maniscalco wrote:
> On 2016/04/29 22:51:34, haibinlu wrote:
> > On 2016/04/29 18:32:28, maniscalco wrote:
> > > Why change the flags here instead of in build/args/blimp_engine.gn ? 
What's
> > the
> > > rationale?
> > 
> > in local development case, debug build is desirable. 
> 
> For local builds we always have the option of just adding is_debug = true to
our
> args.gn file (just like we do with use_goma = true).
> 
> Dirk, WDYT?  Should we be adding the new args to
> //build/args/bots/official.desktop/blimp-engine.gn or
> //build/args/blimp_engine.gn ?

Well, the //build/args/bots file needs to transitively include the
flags that the builder needs. However, if you just stuff them 
all in //build/args/blimp_engine.gn, then blimp_engine isn't
really all that different from the .../bots file and so you
might as well get rid of it.

So, that suggests that args/blimp_engine should be different
in at least some way. One approach would be to only have 
that file contain the flags that will be the same for *any*
engine build, so that would ban is_debug, 
is_component_build, etc. And then devs would add stuff 
as needed to customize for default, release, shared, etc.

Another approach would be to do what is done here, 
and leave args/blimp_engine as just the default dev 
build (in this case, debug, maybe also should be shared 
and not static?).

A third would be to create the 'base blimp mixin' file, 
and then additional templates as needed
for a dev debug build, a dev release build, the actual 
bot build, etc.

This is basically the same question I wrestled with
in the MB config (//tools/mb/mb_config.pyl). 
I don't really know what the right way to expose this 
tuff is, so I'd suggest you pick whatever makes 
your team happiest.

[haibinlu, 2016-05-02 18:31:47]

Nick, what do you think?

[maniscalco, 2016-05-02 19:13:00]

https://codereview.chromium.org/1927323003/diff/1/build/args/bots/official.de...
File build/args/bots/official.desktop/blimp-engine.gn (right):

https://codereview.chromium.org/1927323003/diff/1/build/args/bots/official.de...
build/args/bots/official.desktop/blimp-engine.gn:3: dcheck_always_on = true
On 2016/04/30 00:04:32, Dirk Pranke wrote:
> On 2016/04/29 22:58:57, maniscalco wrote:
> > On 2016/04/29 22:51:34, haibinlu wrote:
> > > On 2016/04/29 18:32:28, maniscalco wrote:
> > > > Why change the flags here instead of in build/args/blimp_engine.gn ? 
> What's
> > > the
> > > > rationale?
> > > 
> > > in local development case, debug build is desirable. 
> > 
> > For local builds we always have the option of just adding is_debug = true to
> our
> > args.gn file (just like we do with use_goma = true).
> > 
> > Dirk, WDYT?  Should we be adding the new args to
> > //build/args/bots/official.desktop/blimp-engine.gn or
> > //build/args/blimp_engine.gn ?
> 
> Well, the //build/args/bots file needs to transitively include the
> flags that the builder needs. However, if you just stuff them 
> all in //build/args/blimp_engine.gn, then blimp_engine isn't
> really all that different from the .../bots file and so you
> might as well get rid of it.
> 
> So, that suggests that args/blimp_engine should be different
> in at least some way. One approach would be to only have 
> that file contain the flags that will be the same for *any*
> engine build, so that would ban is_debug, 
> is_component_build, etc. And then devs would add stuff 
> as needed to customize for default, release, shared, etc.
> 
> Another approach would be to do what is done here, 
> and leave args/blimp_engine as just the default dev 
> build (in this case, debug, maybe also should be shared 
> and not static?).
> 
> A third would be to create the 'base blimp mixin' file, 
> and then additional templates as needed
> for a dev debug build, a dev release build, the actual 
> bot build, etc.
> 
> This is basically the same question I wrestled with
> in the MB config (//tools/mb/mb_config.pyl). 
> I don't really know what the right way to expose this 
> tuff is, so I'd suggest you pick whatever makes 
> your team happiest.

Dirk, that's helpful thanks.

SGTM.  I can see value in having the two different configurations
(//build/args/bots and //build/args/blimp_engine.gn).

I'm OK with putting is_debug and dcheck_always_on in the bots file.

Haibin, can you add a comment to //build/args/blimp_engine.gn that tells the
reader which build args belong in the file?  E.g.

# This file contains Blimp engine build args common to both
# official builds and personal development builds.

Also, can you add a comment in the bots file about why we want is_debug = false,
but DCHECKs on (improve engine performance, but notice bugs).

[haibinlu, 2016-05-05 00:36:45]

PTAL

[maniscalco, 2016-05-05 14:33:45]

lgtm

[haibinlu, 2016-05-05 18:08:48]

The CQ bit was checked by haibinlu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-05 18:09:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1927323003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1927323003/20001

[commit-bot: I haz the power, 2016-05-05 19:59:54]

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

[commit-bot: I haz the power, 2016-05-05 19:59:55]

Dry run: This issue passed the CQ dry run.

[haibinlu, 2016-05-06 18:05:43]

Dirk, PTAL, Thx!

[Dirk Pranke, 2016-05-06 19:22:07]

lgtm

[haibinlu, 2016-05-06 19:52:09]

The CQ bit was checked by haibinlu@chromium.org

[commit-bot: I haz the power, 2016-05-06 19:52:34]

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

[commit-bot: I haz the power, 2016-05-06 20:56:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-06 20:59:18]

Description was changed from

==========
Configs Blimp Engine official build to use non-debug build with dcheck_always_on

WebP encoding is much slower on debug build. Switches to release build with
dcheck on.

BUG=603643
==========

to

==========
Configs Blimp Engine official build to use non-debug build with dcheck_always_on

WebP encoding is much slower on debug build. Switches to release build with
dcheck on.

BUG=603643

Committed: https://crrev.com/335a22ae39ad32f0a75f1c7accfc6119e5f00df8
Cr-Commit-Position: refs/heads/master@{#392150}
==========

[commit-bot: I haz the power, 2016-05-06 20:59:19]

Patchset 2 (id:??) landed as
https://crrev.com/335a22ae39ad32f0a75f1c7accfc6119e5f00df8
Cr-Commit-Position: refs/heads/master@{#392150}