Don't set fastbuild=1 for Mac ASan trybots.

Don't set fastbuild=1 for Mac ASan trybots.
ASan bots require debug info in order to print meaningful reports.

BUG=444835
R=phajdan@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=293900

[Alexander Potapenko, 2015-01-27 13:42:50]

Please take a look.

[Paweł Hajdan Jr., 2015-01-27 13:57:19]

phajdan.jr@chromium.org changed reviewers:
+ phajdan.jr@chromium.org
- phajdan@chromium.org

[Paweł Hajdan Jr., 2015-01-27 13:57:19]

https://codereview.chromium.org/878923002/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/chromium/config.py (right):

https://codereview.chromium.org/878923002/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/chromium/config.py:457:
c.gyp_env.GYP_DEFINES.get('asan', 0)):
This is potentially fragile. Expectations show us that it seems to behave
correctly for now, but AFAICT nothing prevents people from e.g. accidentally
calling fastbuild after this, or adding asan config after trybot_flavor.

In other words, this depends on order of applying configs, which I'd prefer to
avoid. This seems to be the only place where we call GYP_DEFINES.get in a
config.py file, and I'd like to avoid introducing the first case.

Robbie, Aaron: what would be your recommendations? I'm quite concerned the
recipe configs in their current shape may make maintainability harder.

[Paweł Hajdan Jr., 2015-01-27 13:57:41]

phajdan.jr@chromium.org changed reviewers:
+ agable@chromium.org, iannucci@chromium.org

[Paweł Hajdan Jr., 2015-01-27 13:57:41]

Actually adding Robbie and Aaron.

[Alexander Potapenko, 2015-01-27 14:07:59]

Agreed.
Initially I've tried setting fastbuild=0 in the ASan Mac buildbot config that
mac_chromium_asan_rel_ng builds upon, but turned out trybot_flavor() overrode
that.

[Alexander Potapenko, 2015-01-29 16:41:57]

On 2015/01/27 14:07:59, Alexander Potapenko wrote:
> Agreed.
> Initially I've tried setting fastbuild=0 in the ASan Mac buildbot config that
> mac_chromium_asan_rel_ng builds upon, but turned out trybot_flavor() overrode
> that.

Robbie, Aaron, ping?

[agable, 2015-01-29 23:54:42]

On 2015/01/29 16:41:57, Alexander Potapenko wrote:
> On 2015/01/27 14:07:59, Alexander Potapenko wrote:
> > Agreed.
> > Initially I've tried setting fastbuild=0 in the ASan Mac buildbot config
that
> > mac_chromium_asan_rel_ng builds upon, but turned out trybot_flavor()
overrode
> > that.
> 
> Robbie, Aaron, ping?

Robbie is the config guy and probably has better opinions on this than I do.

[iannucci, 2015-01-30 01:21:00]

https://chromiumcodereview.appspot.com/878923002/diff/1/scripts/slave/recipe_...
File scripts/slave/recipe_modules/chromium/config.py (right):

https://chromiumcodereview.appspot.com/878923002/diff/1/scripts/slave/recipe_...
scripts/slave/recipe_modules/chromium/config.py:457:
c.gyp_env.GYP_DEFINES.get('asan', 0)):
On 2015/01/27 at 13:57:19, Paweł Hajdan Jr. wrote:
> This is potentially fragile. Expectations show us that it seems to behave
correctly for now, but AFAICT nothing prevents people from e.g. accidentally
calling fastbuild after this, or adding asan config after trybot_flavor.
> 
> In other words, this depends on order of applying configs, which I'd prefer to
avoid. This seems to be the only place where we call GYP_DEFINES.get in a
config.py file, and I'd like to avoid introducing the first case.
> 
> Robbie, Aaron: what would be your recommendations? I'm quite concerned the
recipe configs in their current shape may make maintainability harder.

Configs are definitely allowed to introspect themselves, the .get is fine.

The config (that is: `c`) actually stores a list of config functions which have
already been applied, and prevents you from applying the same config function
from being applied twice. The 'optional=True' here is actually specifying that
'fastbuild' NOT be added to this internal list.

What's the intended behavior here? IIUC you want to conditionally apply it, and
then ban it from ever being applied to this config. You could do this by
removing the optional=True, and then making an else case which adds it to the
banlist (don't remember how this is done off the top of my head, it may be an
internal api that we have to make public).

[Alexander Potapenko, 2015-01-30 10:29:00]

https://chromiumcodereview.appspot.com/878923002/diff/1/scripts/slave/recipe_...
File scripts/slave/recipe_modules/chromium/config.py (right):

https://chromiumcodereview.appspot.com/878923002/diff/1/scripts/slave/recipe_...
scripts/slave/recipe_modules/chromium/config.py:457:
c.gyp_env.GYP_DEFINES.get('asan', 0)):
On 2015/01/30 01:21:00, iannucci wrote:
> On 2015/01/27 at 13:57:19, Paweł Hajdan Jr. wrote:
> > This is potentially fragile. Expectations show us that it seems to behave
> correctly for now, but AFAICT nothing prevents people from e.g. accidentally
> calling fastbuild after this, or adding asan config after trybot_flavor.
> > 
> > In other words, this depends on order of applying configs, which I'd prefer
to
> avoid. This seems to be the only place where we call GYP_DEFINES.get in a
> config.py file, and I'd like to avoid introducing the first case.
> > 
> > Robbie, Aaron: what would be your recommendations? I'm quite concerned the
> recipe configs in their current shape may make maintainability harder.
> 
> Configs are definitely allowed to introspect themselves, the .get is fine.
> 
> The config (that is: `c`) actually stores a list of config functions which
have
> already been applied, and prevents you from applying the same config function
> from being applied twice. The 'optional=True' here is actually specifying that
> 'fastbuild' NOT be added to this internal list.
That's nice to know, thank you.

> What's the intended behavior here? IIUC you want to conditionally apply it,
and
> then ban it from ever being applied to this config.
Well, my intention was to at least remove fastbuild=1 from the ASan Mac trybot
configs.
This CL solves that particular task, but I agree that other configs may still
change the fastbuild setting.

> You could do this by
> removing the optional=True, and then making an else case which adds it to the
> banlist (don't remember how this is done off the top of my head, it may be an
> internal api that we have to make public).
What if I just add fastbuild=0 in the else case? That'll be the same "fastbuild"
config, so nobody'll change the fastbuild setting on top of that.

[Alexander Potapenko, 2015-01-30 14:18:38]

On 2015/01/30 10:29:00, Alexander Potapenko wrote:
>
https://chromiumcodereview.appspot.com/878923002/diff/1/scripts/slave/recipe_...
> File scripts/slave/recipe_modules/chromium/config.py (right):
> 
>
https://chromiumcodereview.appspot.com/878923002/diff/1/scripts/slave/recipe_...
> scripts/slave/recipe_modules/chromium/config.py:457:
> c.gyp_env.GYP_DEFINES.get('asan', 0)):
> On 2015/01/30 01:21:00, iannucci wrote:
> > On 2015/01/27 at 13:57:19, Paweł Hajdan Jr. wrote:
> > > This is potentially fragile. Expectations show us that it seems to behave
> > correctly for now, but AFAICT nothing prevents people from e.g. accidentally
> > calling fastbuild after this, or adding asan config after trybot_flavor.
> > > 
> > > In other words, this depends on order of applying configs, which I'd
prefer
> to
> > avoid. This seems to be the only place where we call GYP_DEFINES.get in a
> > config.py file, and I'd like to avoid introducing the first case.
> > > 
> > > Robbie, Aaron: what would be your recommendations? I'm quite concerned the
> > recipe configs in their current shape may make maintainability harder.
> > 
> > Configs are definitely allowed to introspect themselves, the .get is fine.
> > 
> > The config (that is: `c`) actually stores a list of config functions which
> have
> > already been applied, and prevents you from applying the same config
function
> > from being applied twice. The 'optional=True' here is actually specifying
that
> > 'fastbuild' NOT be added to this internal list.
> That's nice to know, thank you.
> 
> > What's the intended behavior here? IIUC you want to conditionally apply it,
> and
> > then ban it from ever being applied to this config.
> Well, my intention was to at least remove fastbuild=1 from the ASan Mac trybot
> configs.
> This CL solves that particular task, but I agree that other configs may still
> change the fastbuild setting.
> 
> > You could do this by
> > removing the optional=True, and then making an else case which adds it to
the
> > banlist (don't remember how this is done off the top of my head, it may be
an
> > internal api that we have to make public).
> What if I just add fastbuild=0 in the else case? That'll be the same
"fastbuild"
> config, so nobody'll change the fastbuild setting on top of that.

I've changed trybot_flavor() to look as follows:

454 @config_ctx()
455 def trybot_flavor(c):
456   if not (c.TARGET_PLATFORM == 'mac' and
457           c.gyp_env.GYP_DEFINES.get('asan', 0)):
458     fastbuild(c, optional=True)
459   else:
460     # Unconditionally set fastbuild=0 for Mac ASan trybots and prevent other
461     # configs from changing it.
462     fastbuild(c, invert=True) 
463   dcheck(c, optional=True)

and am getting a PRESUBMIT message that the dcheck() call isn't covered:

Name                                                            Stmts   Miss 
Cover   Missing
---------------------------------------------------------------------------------------------
scripts/slave/recipe_modules/android/config                        15      0  
100%   
scripts/slave/recipe_modules/chromium/config                      323      1   
99%   462

, which doesn't make sense at all.

[Alexander Potapenko, 2015-01-30 14:59:04]

That's not dcheck(), but the second fastbuild() call.
Looks like this happens because the trybot_flavor() config is applied before our
trybot knows it's an ASan one.

[Alexander Potapenko, 2015-01-30 15:21:33]

On 2015/01/30 14:59:04, Alexander Potapenko wrote:
> That's not dcheck(), but the second fastbuild() call.
> Looks like this happens because the trybot_flavor() config is applied before
our
> trybot knows it's an ASan one.

Ok, I've changed the patch so that we now set fastbuild=0 directly in the asan()
config using optional=False, so that it can't be overridden by other configs.
Can you please take another look?

[Paweł Hajdan Jr., 2015-01-30 21:23:33]

LGTM

[Alexander Potapenko, 2015-02-02 10:48:55]

The CQ bit was checked by glider@chromium.org

[commit-bot: I haz the power, 2015-02-02 10:49:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/878923002/40001

[commit-bot: I haz the power, 2015-02-02 10:51:02]

Committed patchset #3 (id:40001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=293900