GN configs for ipc fuzzer.

GN configs for ipc fuzzer.

Also removes an unneeded dep from message_lib.gyp.

BUG=605926
Committed: https://crrev.com/cc11d0efd2608da10eb729187f4c3618787e5a5a
Cr-Commit-Position: refs/heads/master@{#389861}

[Oliver Chang, 2016-04-22 20:13:06]

Description was changed from

==========
GN configs for ipc fuzzer.
==========

to

==========
GN configs for ipc fuzzer.

Also removes an unneeded dep from message_lib.gyp.
==========

[Oliver Chang, 2016-04-22 20:19:24]

Description was changed from

==========
GN configs for ipc fuzzer.

Also removes an unneeded dep from message_lib.gyp.
==========

to

==========
GN configs for ipc fuzzer.

Also removes an unneeded dep from message_lib.gyp.

BUG=605926
==========

[Oliver Chang, 2016-04-22 20:25:30]

ochang@chromium.org changed reviewers:
+ dpranke@chromium.org, esprehn@chromium.org, mbarbella@chromium.org

[Oliver Chang, 2016-04-22 20:31:40]

ochang@chromium.org changed reviewers:
- dpranke@chromium.org, esprehn@chromium.org, mbarbella@chromium.org

[Oliver Chang, 2016-04-22 20:51:01]

ochang@chromium.org changed reviewers:
+ dpranke@chromium.org, esprehn@chromium.org, mbarbella@chromium.org

[Oliver Chang, 2016-04-22 20:53:02]

PTAL.

Note that it looks like building ASan Chrome with GN is broken (unrelated to
this patch): https://crbug.com/586586

[Martin Barbella, 2016-04-22 21:00:33]

lgtm

https://codereview.chromium.org/1916493003/diff/60001/docs/ipc_fuzzer.md
File docs/ipc_fuzzer.md (right):

https://codereview.chromium.org/1916493003/diff/60001/docs/ipc_fuzzer.md#newc...
docs/ipc_fuzzer.md:16: *   add `enable_ipc_fuzzer=1` to `GYP_DEFINES` (or add
`enable_ipc_fuzzer=true`
Let's actually switch the order of this to mention GN first, and remove the
parens.

[Oliver Chang, 2016-04-22 21:10:22]

Patchset #5 (id:80001) has been deleted

[Oliver Chang, 2016-04-22 21:14:44]

https://codereview.chromium.org/1916493003/diff/60001/docs/ipc_fuzzer.md
File docs/ipc_fuzzer.md (right):

https://codereview.chromium.org/1916493003/diff/60001/docs/ipc_fuzzer.md#newc...
docs/ipc_fuzzer.md:16: *   add `enable_ipc_fuzzer=1` to `GYP_DEFINES` (or add
`enable_ipc_fuzzer=true`
On 2016/04/22 21:00:33, Martin Barbella wrote:
> Let's actually switch the order of this to mention GN first, and remove the
> parens.

Done. Also reworded some the GN instructions a little to make it clearer.

[Nico, 2016-04-22 21:17:53]

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

[Nico, 2016-04-22 21:17:53]

https://codereview.chromium.org/1916493003/diff/100001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1916493003/diff/100001/build/config/features....
build/config/features.gni:120: # Don't build IPC fuzzer by default.
can you remind me again why this is off by default?

[Oliver Chang, 2016-04-22 21:30:32]

https://codereview.chromium.org/1916493003/diff/100001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1916493003/diff/100001/build/config/features....
build/config/features.gni:120: # Don't build IPC fuzzer by default.
On 2016/04/22 21:17:53, Nico wrote:
> can you remind me again why this is off by default?

It changes some behaviour (based on the ENABLE_IPC_FUZZER define), and breaks
component builds.

That said, we do want to have a better way of preventing breakages for this
config. If possible we'd like to look into adding builders to the main waterfall
and CQ for this configuration.

[Dirk Pranke, 2016-04-22 23:18:11]

https://codereview.chromium.org/1916493003/diff/100001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1916493003/diff/100001/BUILD.gn#newcode598
BUILD.gn:598: deps += [ "//tools/ipc_fuzzer:ipc_fuzzer_all" ]
A convention we often follow here is to unconditionally  depend on
:ipc_fuzzer_all, but when we define ipc_fuzzer_all, define it to be an empty
group when you don't want it to do anything.

https://codereview.chromium.org/1916493003/diff/100001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1916493003/diff/100001/build/config/features....
build/config/features.gni:121: enable_ipc_fuzzer = false
In GN we don't want to add arguments globally if they're not needed globally
(i.e. if we don't need to check them all over the place).

It looks like the ENABLE_IPC_FUZZER define is only used in a few places, so
instead of adding it here, you should create a separate ipc_fuzzer.gni file that
declares this arg and a config that adds the define, and then only include that
.gni file from the files that need it (ipc and a couple of places that take
command line switches, I think)?

https://codereview.chromium.org/1916493003/diff/100001/tools/ipc_fuzzer/BUILD.gn
File tools/ipc_fuzzer/BUILD.gn (right):

https://codereview.chromium.org/1916493003/diff/100001/tools/ipc_fuzzer/BUILD...
tools/ipc_fuzzer/BUILD.gn:10: group("ipc_fuzzer_all") {
as per the other comment, consider making this just be an empty group if
!(is_linux || is_mac || is_win).

[Oliver Chang, 2016-04-25 17:11:05]

Patchset #9 (id:180001) has been deleted

[Oliver Chang, 2016-04-25 18:16:06]

https://codereview.chromium.org/1916493003/diff/100001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1916493003/diff/100001/BUILD.gn#newcode598
BUILD.gn:598: deps += [ "//tools/ipc_fuzzer:ipc_fuzzer_all" ]
On 2016/04/22 23:18:11, Dirk Pranke wrote:
> A convention we often follow here is to unconditionally  depend on
> :ipc_fuzzer_all, but when we define ipc_fuzzer_all, define it to be an empty
> group when you don't want it to do anything.

Done.

https://codereview.chromium.org/1916493003/diff/100001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1916493003/diff/100001/build/config/features....
build/config/features.gni:121: enable_ipc_fuzzer = false
On 2016/04/22 23:18:11, Dirk Pranke wrote:
> In GN we don't want to add arguments globally if they're not needed globally
> (i.e. if we don't need to check them all over the place).
> 
> It looks like the ENABLE_IPC_FUZZER define is only used in a few places, so
> instead of adding it here, you should create a separate ipc_fuzzer.gni file
that
> declares this arg and a config that adds the define, and then only include
that
> .gni file from the files that need it (ipc and a couple of places that take
> command line switches, I think)?

OK, done. I'm a little concerned about header files that use ENABLE_IPC_FUZZER
which are included by other targets. It seems to build, but should the the
configs += [ "..." ] in the various BUILD.gn files be public_configs instead (or
even all_dependent_configs)?

https://codereview.chromium.org/1916493003/diff/100001/tools/ipc_fuzzer/BUILD.gn
File tools/ipc_fuzzer/BUILD.gn (right):

https://codereview.chromium.org/1916493003/diff/100001/tools/ipc_fuzzer/BUILD...
tools/ipc_fuzzer/BUILD.gn:10: group("ipc_fuzzer_all") {
On 2016/04/22 23:18:11, Dirk Pranke wrote:
> as per the other comment, consider making this just be an empty group if
> !(is_linux || is_mac || is_win).

Done.

[Dirk Pranke, 2016-04-25 18:48:59]

https://codereview.chromium.org/1916493003/diff/100001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1916493003/diff/100001/build/config/features....
build/config/features.gni:121: enable_ipc_fuzzer = false
On 2016/04/25 18:16:06, Oliver Chang wrote:
> On 2016/04/22 23:18:11, Dirk Pranke wrote:
> > In GN we don't want to add arguments globally if they're not needed globally
> > (i.e. if we don't need to check them all over the place).
> > 
> > It looks like the ENABLE_IPC_FUZZER define is only used in a few places, so
> > instead of adding it here, you should create a separate ipc_fuzzer.gni file
> that
> > declares this arg and a config that adds the define, and then only include
> that
> > .gni file from the files that need it (ipc and a couple of places that take
> > command line switches, I think)?
> 
> OK, done. I'm a little concerned about header files that use ENABLE_IPC_FUZZER
> which are included by other targets. It seems to build, but should the the
> configs += [ "..." ] in the various BUILD.gn files be public_configs instead
(or
> even all_dependent_configs)?

If the defines are needed in public headers (i.e., headers which are included by
other components),
then you should use public_configs instead of configs.

It is rare to really need all_dependent_configs.

I don't know enough about the structure of the ipc #includes to know what the
right thing to do really is, but if that guidance isn't enough, let me know and
we can work through it together.

https://codereview.chromium.org/1916493003/diff/200001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1916493003/diff/200001/BUILD.gn#newcode597
BUILD.gn:597: deps += [ "//tools/ipc_fuzzer:ipc_fuzzer_all" ]
since this is now unconditionally added, you should just list this in the main
block of deps up on line 102.

https://codereview.chromium.org/1916493003/diff/200001/tools/ipc_fuzzer/BUILD.gn
File tools/ipc_fuzzer/BUILD.gn (right):

https://codereview.chromium.org/1916493003/diff/200001/tools/ipc_fuzzer/BUILD...
tools/ipc_fuzzer/BUILD.gn:14: "USE_CUPS",
I'm curious, why is USE_CUPS needed here?

[Oliver Chang, 2016-04-25 19:25:50]

https://codereview.chromium.org/1916493003/diff/100001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/1916493003/diff/100001/build/config/features....
build/config/features.gni:121: enable_ipc_fuzzer = false
On 2016/04/25 18:48:58, Dirk Pranke wrote:
> On 2016/04/25 18:16:06, Oliver Chang wrote:
> > On 2016/04/22 23:18:11, Dirk Pranke wrote:
> > > In GN we don't want to add arguments globally if they're not needed
globally
> > > (i.e. if we don't need to check them all over the place).
> > > 
> > > It looks like the ENABLE_IPC_FUZZER define is only used in a few places,
so
> > > instead of adding it here, you should create a separate ipc_fuzzer.gni
file
> > that
> > > declares this arg and a config that adds the define, and then only include
> > that
> > > .gni file from the files that need it (ipc and a couple of places that
take
> > > command line switches, I think)?
> > 
> > OK, done. I'm a little concerned about header files that use
ENABLE_IPC_FUZZER
> > which are included by other targets. It seems to build, but should the the
> > configs += [ "..." ] in the various BUILD.gn files be public_configs instead
> (or
> > even all_dependent_configs)?
> 
> If the defines are needed in public headers (i.e., headers which are included
by
> other components),
> then you should use public_configs instead of configs.
> 
> It is rare to really need all_dependent_configs.
> 
> I don't know enough about the structure of the ipc #includes to know what the
> right thing to do really is, but if that guidance isn't enough, let me know
and
> we can work through it together.

Thanks, I've changed ipc/BUILD.gn to use public_configs.

https://codereview.chromium.org/1916493003/diff/200001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1916493003/diff/200001/BUILD.gn#newcode597
BUILD.gn:597: deps += [ "//tools/ipc_fuzzer:ipc_fuzzer_all" ]
On 2016/04/25 18:48:58, Dirk Pranke wrote:
> since this is now unconditionally added, you should just list this in the main
> block of deps up on line 102.

Done.

https://codereview.chromium.org/1916493003/diff/200001/tools/ipc_fuzzer/BUILD.gn
File tools/ipc_fuzzer/BUILD.gn (right):

https://codereview.chromium.org/1916493003/diff/200001/tools/ipc_fuzzer/BUILD...
tools/ipc_fuzzer/BUILD.gn:14: "USE_CUPS",
On 2016/04/25 18:48:59, Dirk Pranke wrote:
> I'm curious, why is USE_CUPS needed here?

I'm afraid I don't know, I just ported these from the GYP.

[Dirk Pranke, 2016-04-25 21:17:56]

lgtm

https://codereview.chromium.org/1916493003/diff/240001/tools/ipc_fuzzer/BUILD.gn
File tools/ipc_fuzzer/BUILD.gn (right):

https://codereview.chromium.org/1916493003/diff/240001/tools/ipc_fuzzer/BUILD...
tools/ipc_fuzzer/BUILD.gn:14: "USE_CUPS",
my guess is that USE_CUPS just got cargo-culted in and you don't actually need
it. Maybe try removing it in a follow-up CL?

[Oliver Chang, 2016-04-25 21:29:59]

ochang@chromium.org changed reviewers:
+ tsepez@chromium.org

[Oliver Chang, 2016-04-25 21:30:00]

Thanks Dirk for the review and guidance :)

Nico, could you please take a look at the chrome/ changes as an OWNER?

esprehn, could you please do the same for content/child? 

+Tom for ipc/ change.

https://codereview.chromium.org/1916493003/diff/240001/tools/ipc_fuzzer/BUILD.gn
File tools/ipc_fuzzer/BUILD.gn (right):

https://codereview.chromium.org/1916493003/diff/240001/tools/ipc_fuzzer/BUILD...
tools/ipc_fuzzer/BUILD.gn:14: "USE_CUPS",
On 2016/04/25 21:17:55, Dirk Pranke wrote:
> my guess is that USE_CUPS just got cargo-culted in and you don't actually need
> it. Maybe try removing it in a follow-up CL?

Sure.

[Tom Sepez, 2016-04-25 22:30:25]

lgtm

[esprehn, 2016-04-25 22:55:30]

esprehn@chromium.org changed reviewers:
- esprehn@chromium.org

[esprehn, 2016-04-25 22:56:01]

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

[esprehn, 2016-04-25 22:56:02]

lgtm

[Nico, 2016-04-26 19:39:35]

lgtm (sorry, i meant to lgtm with my initial question and didn't realize i
didn't)

[Oliver Chang, 2016-04-26 19:48:28]

The CQ bit was checked by ochang@chromium.org

[Oliver Chang, 2016-04-26 19:48:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
mbarbella@chromium.org
Link to the patchset: https://codereview.chromium.org/1916493003/#ps240001
(title: "public_configs")

[commit-bot: I haz the power, 2016-04-26 19:48:54]

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

[commit-bot: I haz the power, 2016-04-26 19:58:39]

Description was changed from

==========
GN configs for ipc fuzzer.

Also removes an unneeded dep from message_lib.gyp.

BUG=605926
==========

to

==========
GN configs for ipc fuzzer.

Also removes an unneeded dep from message_lib.gyp.

BUG=605926
==========

[commit-bot: I haz the power, 2016-04-26 19:58:40]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2016-04-26 20:00:02]

Description was changed from

==========
GN configs for ipc fuzzer.

Also removes an unneeded dep from message_lib.gyp.

BUG=605926
==========

to

==========
GN configs for ipc fuzzer.

Also removes an unneeded dep from message_lib.gyp.

BUG=605926

Committed: https://crrev.com/cc11d0efd2608da10eb729187f4c3618787e5a5a
Cr-Commit-Position: refs/heads/master@{#389861}
==========

[commit-bot: I haz the power, 2016-04-26 20:00:03]

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