Remove hotword installation code at compile time if hotwording disabled.

Remove hotword installation code at compile time if hotwording disabled.

If enable_hotwording is false, the code to download/install the hotword
shared module is compiled out.

(This should not change behaviour; it was already disabled at run-time,
this just removes the installation code from the build.)

(Reland after r335843 was reverted.)

BUG=491435, 504235, 546098

[Matt Giuca, 2015-06-23 08:45:58]

mgiuca@chromium.org changed reviewers:
+ benwells@chromium.org

[benwells, 2015-06-23 23:44:27]

lgtm

[Matt Giuca, 2015-06-24 00:05:41]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2015-06-24 00:10:03]

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

[commit-bot: I haz the power, 2015-06-24 01:25:21]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-06-24 01:26:49]

Patchset 1 (id:??) landed as
https://crrev.com/a38832be9fef269ae1f3a191a9b69432aaf680c4
Cr-Commit-Position: refs/heads/master@{#335843}

[Matt Giuca, 2015-06-24 03:53:01]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1199343003/ by mgiuca@chromium.org.

The reason for reverting is: This CL added a compile-time check for
ENABLE_HOTWORDING in a file where ENABLE_HOTWORDING is never defined, thus the
hotword shared module would never be downloaded..

[Matt Giuca, 2015-06-24 06:02:05]

mgiuca@chromium.org changed reviewers:
+ thakis@chromium.org

[Matt Giuca, 2015-06-24 06:02:05]

thakis@chromium.org: Please review changes in *.gn, *.gni, *.gypi. (Had to move
the define up to the top level so it can be accessed from c/b/extensions.)

[Nico, 2015-06-24 19:34:05]

https://codereview.chromium.org/1201163002/diff/40001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1201163002/diff/40001/build/config/BUILD.gn#n...
build/config/BUILD.gn:40: # define, but anything that doesn't depend on X11
doesn't see it.
Have you seen this comment? Is it possible to do something like it suggests for
your flag?

[Matt Giuca, 2015-06-25 03:13:18]

mgiuca@chromium.org changed reviewers:
+ brettw@chromium.org

[Matt Giuca, 2015-06-25 03:13:18]

+brettw for comment on BUILD.gn:40.

https://codereview.chromium.org/1201163002/diff/40001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1201163002/diff/40001/build/config/BUILD.gn#n...
build/config/BUILD.gn:40: # define, but anything that doesn't depend on X11
doesn't see it.
No I didn't see that before, sorry.

This sounds like a dangerous rule. I've already broken the build twice now
(https://codereview.chromium.org/1199343003/ and http://crbug.com/503963) due to
adding #if defined(ENABLE_HOTWORDING) in files where that flag was not
available. We can't test for this, since it's based on a compile-time flag (the
tests have to pass whether the flag is on or off, and we can't test both code
paths on the main builders).

I'd prefer to keep this here since it will already be used in three places
(chrome/browser/extensions, chrome/browser/search and chrome/browser/search
unit_tests). If you feel strongly about it, I'll try to make the flag available
in just those three places but I think this is asking for more bugs.

Note: Is this just a GN issue or does it also have to be fixed for GYP?

[brettw, 2015-06-25 22:39:43]

Since it's global in GYP, generally it's best for the GN build to match. So this
should be fine.

[Matt Giuca, 2015-06-26 03:36:24]

On 2015/06/25 22:39:43, brettw wrote:
> Since it's global in GYP, generally it's best for the GN build to match. So
this
> should be fine.

Well to be fair, it's not global in GYP yet (this CL is making it global in both
GYP and GN). The question is whether we should have (in both GYP and GN):
a) Global define, or
b) Specifically define in 3 places (chrome/browser, chrome/browser/extensions
and chrome/browser unit tests). (Which I argue is dangerous as we may
inadvertently add a dependency on this in a fourth place without realising that
it is not defined there.)

[Nico, 2015-06-26 04:30:39]

On 2015/06/26 03:36:24, Matt Giuca wrote:
> On 2015/06/25 22:39:43, brettw wrote:
> > Since it's global in GYP, generally it's best for the GN build to match. So
> this
> > should be fine.
> 
> Well to be fair, it's not global in GYP yet (this CL is making it global in
both
> GYP and GN). The question is whether we should have (in both GYP and GN):
> a) Global define, or
> b) Specifically define in 3 places (chrome/browser, chrome/browser/extensions
> and chrome/browser unit tests). (Which I argue is dangerous as we may
> inadvertently add a dependency on this in a fourth place without realising
that
> it is not defined there.)

Can you define it in the target that needs it and then push it to deps via
all_dependent_settings?

[Matt Giuca, 2015-10-25 23:18:01]

Description was changed from

==========
Remove hotword installation code at compile time if hotwording disabled.

If enable_hotwording is false, the code to download/install the hotword
shared module is compiled out.

(This should not change behaviour; it was already disabled at run-time,
this just removes the installation code from the build.)

(Reland after r335843 was reverted.)

BUG=491435,504235
==========

to

==========
Remove hotword installation code at compile time if hotwording disabled.

If enable_hotwording is false, the code to download/install the hotword
shared module is compiled out.

(This should not change behaviour; it was already disabled at run-time,
this just removes the installation code from the build.)

(Reland after r335843 was reverted.)

BUG=491435,504235,546098
==========

[Matt Giuca, 2015-10-26 04:26:02]

Sorry I didn't get to this sooner... I lost touch with this CL until Evan
brought it up this morning.

On 2015/06/26 04:30:39, Nico (vacation Fri Oct 23) wrote:
> On 2015/06/26 03:36:24, Matt Giuca wrote:
> > On 2015/06/25 22:39:43, brettw wrote:
> > > Since it's global in GYP, generally it's best for the GN build to match.
So
> > this
> > > should be fine.
> > 
> > Well to be fair, it's not global in GYP yet (this CL is making it global in
> both
> > GYP and GN). The question is whether we should have (in both GYP and GN):
> > a) Global define, or
> > b) Specifically define in 3 places (chrome/browser,
chrome/browser/extensions
> > and chrome/browser unit tests). (Which I argue is dangerous as we may
> > inadvertently add a dependency on this in a fourth place without realising
> that
> > it is not defined there.)
> 
> Can you define it in the target that needs it and then push it to deps via
> all_dependent_settings?

I don't know what this means (I have never encountered all_dependent_settings
before; I see it is used in a lot of build files but there is no documentation,
it isn't clear what it does, or what the GN equivalent of it is). Are you
suggesting a way of making this constant available globally, or in specific
targets?

My point from (b) is that we don't *know* what targets need it. Right now, it is
chrome_browser as well as unit_tests, but we may (and probably do -- see
http://crbug.com/546098) want to add more dependencies on it in the future, from
other targets. Since the failure mode if you forget to add a dependency
somewhere is silent failure, this could result in inadvertently compiling the
wrong code path. I'd much rather make this available globally.

(Brett, maybe you can answer this since Nico appears to be on vacation.)

[brettw, 2015-10-26 09:15:01]

These days, it looks like it's used in exactly one file. Did we remove
hotwording? I suspect this is moot now.

[Matt Giuca, 2015-10-26 23:59:26]

On 2015/10/26 09:15:01, brettw wrote:
> These days, it looks like it's used in exactly one file. Did we remove
> hotwording? I suspect this is moot now.

No, we removed it on some platforms (Win/Mac/Linux) and not others (ChromeOS).
ENABLE_HOTWORDING is a new flag that was added to help us selectively compile
out hotwording code, which I want to make more use of, but can't because it's
trapped within one target. http://crbug.com/546098.

[Nico, 2015-10-27 00:05:06]

all_dependent_settings get applied to all targets (transitively) depending on
the target that contains that. So if target A has an all_dependent settings and
B depends on A and C on B

  A <- B <- C

then both B and C get the stuff from A's all_dependent_settings.

[Matt Giuca, 2015-10-27 07:26:57]

On 2015/10/27 00:05:06, Nico wrote:
> all_dependent_settings get applied to all targets (transitively) depending on
> the target that contains that. So if target A has an all_dependent settings
and
> B depends on A and C on B
> 
>   A <- B <- C
> 
> then both B and C get the stuff from A's all_dependent_settings.

It's not clear what you want me to do here.

1. It is currently defined in the browser target; I need it in at least
"extensions", "unit_tests" and possibly several other targets (that aren't
currently using ENABLE_HOTWORDING). Most importantly, I want future-proofing, so
that I don't inadvertently add "#if defined(ENABLE_HOTWORDING)" in a target that
doesn't see this define, and get the wrong behaviour. What would "A" be in this
case such that all the other targets inherit this setting? (Are you suggesting
that I create a new target just for hotwording?)

2. I am trying to make a *macro* (ENABLE_HOTWORDING) visible in all of these
places, not a *setting* (enable_hotwording). It doesn't look like
all_dependent_settings will propagate a macro.

I still think the simplest thing to do is make ENABLE_HOTWORDING visible to all
code (like dozens and dozens of other compile-time enables are -- and btw I
think that's helpful for configuring a build to have all of those options in one
place). But if you think I need to make it available more selectively, I am not
understanding how you want me to do this.

[brettw, 2015-11-17 20:29:36]

I'm working on a proposal to make this better, will post in a bit.

[benzh, 2016-07-29 21:54:59]

On 2015/11/17 20:29:36, brettw (ping on IM after 24h) wrote:
> I'm working on a proposal to make this better, will post in a bit.

Hi Brett,

We hit the same issue again. Hotwording on ChromeOS got silently broken due to
ifdef guard added to files where the flag isn't defined:
https://bugs.chromium.org/p/chromium/issues/detail?id=616203#c9

Any suggestions on how to handle the flag better? Or should we land this CL to
make the flag global to avoid future breaks?

Thanks

[brettw, 2016-08-01 18:06:27]

You should use the new buildflag system which will cause compiler errors if you
screw up. See build/buildflag_header.gni.

The only disadvantage is that the error messages when you screw up can be a bit
inscrutable (blame the preprocessor).

[benzh, 2016-08-02 01:42:42]

On 2016/08/01 18:06:27, brettw (ping on IM after 24h) wrote:
> You should use the new buildflag system which will cause compiler errors if
you
> screw up. See build/buildflag_header.gni.
> 
> The only disadvantage is that the error messages when you screw up can be a
bit
> inscrutable (blame the preprocessor).

Thanks, I'm working on a patch to switch to buildflag for ENABLE_HOTWORDING.
https://codereview.chromium.org/1469383005 seems like a good example to follow.

[Matt Giuca, 2016-08-02 01:43:28]

On 2016/08/02 01:42:42, benzh wrote:
> On 2016/08/01 18:06:27, brettw (ping on IM after 24h) wrote:
> > You should use the new buildflag system which will cause compiler errors if
> you
> > screw up. See build/buildflag_header.gni.
> > 
> > The only disadvantage is that the error messages when you screw up can be a
> bit
> > inscrutable (blame the preprocessor).
> 
> Thanks, I'm working on a patch to switch to buildflag for ENABLE_HOTWORDING.
> https://codereview.chromium.org/1469383005 seems like a good example to
follow.

benzh@: Great! Does that mean this CL will be obsolete? (If so I'll close it.)

[benzh, 2016-08-02 01:52:22]

On 2016/08/02 01:43:28, Matt Giuca wrote:
> On 2016/08/02 01:42:42, benzh wrote:
> > On 2016/08/01 18:06:27, brettw (ping on IM after 24h) wrote:
> > > You should use the new buildflag system which will cause compiler errors
if
> > you
> > > screw up. See build/buildflag_header.gni.
> > > 
> > > The only disadvantage is that the error messages when you screw up can be
a
> > bit
> > > inscrutable (blame the preprocessor).
> > 
> > Thanks, I'm working on a patch to switch to buildflag for ENABLE_HOTWORDING.
> > https://codereview.chromium.org/1469383005 seems like a good example to
> follow.
> 
> benzh@: Great! Does that mean this CL will be obsolete? (If so I'll close it.)

Yeah, I think we can close it. The new patch will use BUILDFLAG() to fix the
unittest and ExternalComponentLoader.

[Matt Giuca, 2016-08-02 03:02:19]

Great, closed!