Add GN build targets for the rest of mojo/public

Add GN build targets for the rest of mojo/public

BUG=369643
TEST='gn gen out/Debug && ninja -C out/Debug mojo

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283490

[Chris Masone, 2014-07-14 19:43:42]

Obviously not done yet, but this is the tricky bit and I was hoping for expert
advice :-)

https://codereview.chromium.org/388333003/diff/1/mojo/public/c/system/BUILD.gn
File mojo/public/c/system/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/1/mojo/public/c/system/BUILD.g...
mojo/public/c/system/BUILD.gn:6: ldflags = [ "-Wl,--exclude-libs=ALL" ]
I _think_ this is the right way to translate the clause where this argument is
filtered out of ldflags in the GYP mojo_system target, but I'm not sure.

https://codereview.chromium.org/388333003/diff/1/mojo/public/c/system/BUILD.g...
mojo/public/c/system/BUILD.gn:20:
"../../../public/platform/native/system_thunks.h",
I'm also not sure this is the right way to reference these files, which are
outside of the directory where this file lives. Should I instead create a
BUILD.gn in that directory with a source_set including just these files, and
then reference that here?

[yzshen1, 2014-07-14 19:53:12]

https://codereview.chromium.org/388333003/diff/1/mojo/public/c/system/BUILD.gn
File mojo/public/c/system/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/1/mojo/public/c/system/BUILD.g...
mojo/public/c/system/BUILD.gn:20:
"../../../public/platform/native/system_thunks.h",
I think it should be fine because I have seen this pattern elsewhere.
On 2014/07/14 19:43:41, Chris Masone wrote:
> I'm also not sure this is the right way to reference these files, which are
> outside of the directory where this file lives. Should I instead create a
> BUILD.gn in that directory with a source_set including just these files, and
> then reference that here?

https://codereview.chromium.org/388333003/diff/1/mojo/public/c/system/BUILD.g...
mojo/public/c/system/BUILD.gn:25: all_dependent_configs -= [
":no_exclude_all_libs" ]
Mismatch name?
(I assume you want to use exclude_all_libs?)

IIUC, this won't work because "exclude_all_libs" is not a config that already
exists in the list of configs.

[Chris Masone, 2014-07-14 19:58:54]

https://codereview.chromium.org/388333003/diff/1/mojo/public/c/system/BUILD.gn
File mojo/public/c/system/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/1/mojo/public/c/system/BUILD.g...
mojo/public/c/system/BUILD.gn:20:
"../../../public/platform/native/system_thunks.h",
On 2014/07/14 19:53:11, yzshen1 wrote:
> I think it should be fine because I have seen this pattern elsewhere.
> On 2014/07/14 19:43:41, Chris Masone wrote:
> > I'm also not sure this is the right way to reference these files, which are
> > outside of the directory where this file lives. Should I instead create a
> > BUILD.gn in that directory with a source_set including just these files, and
> > then reference that here?
> 

Acknowledged.

https://codereview.chromium.org/388333003/diff/1/mojo/public/c/system/BUILD.g...
mojo/public/c/system/BUILD.gn:25: all_dependent_configs -= [
":no_exclude_all_libs" ]
On 2014/07/14 19:53:11, yzshen1 wrote:
> Mismatch name?
> (I assume you want to use exclude_all_libs?)
> 
> IIUC, this won't work because "exclude_all_libs" is not a config that already
> exists in the list of configs.

That's what I was concerned about. So I should go to the place where this flag
is originally added to ldflags and instead break it out into a separate config?
And then make sure it gets pulled in by default under whatever conditions this
flag is currently being set, right?

[yzshen1, 2014-07-14 20:02:01]

On 2014/07/14 19:58:54, Chris Masone wrote:
> https://codereview.chromium.org/388333003/diff/1/mojo/public/c/system/BUILD.gn
> File mojo/public/c/system/BUILD.gn (right):
> 
>
https://codereview.chromium.org/388333003/diff/1/mojo/public/c/system/BUILD.g...
> mojo/public/c/system/BUILD.gn:20:
> "../../../public/platform/native/system_thunks.h",
> On 2014/07/14 19:53:11, yzshen1 wrote:
> > I think it should be fine because I have seen this pattern elsewhere.
> > On 2014/07/14 19:43:41, Chris Masone wrote:
> > > I'm also not sure this is the right way to reference these files, which
are
> > > outside of the directory where this file lives. Should I instead create a
> > > BUILD.gn in that directory with a source_set including just these files,
and
> > > then reference that here?
> > 
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/388333003/diff/1/mojo/public/c/system/BUILD.g...
> mojo/public/c/system/BUILD.gn:25: all_dependent_configs -= [
> ":no_exclude_all_libs" ]
> On 2014/07/14 19:53:11, yzshen1 wrote:
> > Mismatch name?
> > (I assume you want to use exclude_all_libs?)
> > 
> > IIUC, this won't work because "exclude_all_libs" is not a config that
already
> > exists in the list of configs.
> 
> That's what I was concerned about. So I should go to the place where this flag
> is originally added to ldflags and instead break it out into a separate
config?
> And then make sure it gets pulled in by default under whatever conditions this
> flag is currently being set, right?

That is my understanding. :)

[Chris Masone, 2014-07-14 21:09:57]

brettw@, can you look at the mods I made to all the stuff under build/config?
Context:

When building mojo/public/c/system/BUILD.gn for android, we need to screen out
an ldflags entry, fortunately one that's only ever added for android builds. I
made a separate config for that and then added it in BUILDCONFIG.gn, so that I
could conditionally screen it out later.

[yzshen1, 2014-07-14 21:19:40]

https://codereview.chromium.org/388333003/diff/20001/mojo/public/c/system/BUI...
File mojo/public/c/system/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/20001/mojo/public/c/system/BUI...
mojo/public/c/system/BUILD.gn:21: all_dependent_configs -= [
":no_export_static_lib_symbols" ]
I think this should have the path to the config?

https://codereview.chromium.org/388333003/diff/20001/mojo/public/cpp/utility/...
File mojo/public/cpp/utility/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/20001/mojo/public/cpp/utility/...
mojo/public/cpp/utility/BUILD.gn:22: "mutex.h",
wrong indent.

[Chris Masone, 2014-07-14 21:24:54]

Hm, there seems to be something wrong with my use of all_dependent_configs as
well. Brett?

https://codereview.chromium.org/388333003/diff/20001/mojo/public/c/system/BUI...
File mojo/public/c/system/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/20001/mojo/public/c/system/BUI...
mojo/public/c/system/BUILD.gn:21: all_dependent_configs -= [
":no_export_static_lib_symbols" ]
On 2014/07/14 21:19:39, yzshen1 wrote:
> I think this should have the path to the config?

Good catch. I think there may also be something else wrong, though.

[Chris Masone, 2014-07-14 21:38:37]

https://codereview.chromium.org/388333003/diff/20001/mojo/public/cpp/utility/...
File mojo/public/cpp/utility/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/20001/mojo/public/cpp/utility/...
mojo/public/cpp/utility/BUILD.gn:22: "mutex.h",
On 2014/07/14 21:19:39, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/388333003/diff/40001/mojo/public/c/system/BUI...
File mojo/public/c/system/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/40001/mojo/public/c/system/BUI...
mojo/public/c/system/BUILD.gn:21: all_dependent_configs -= [
Per the Dependent settings section of
https://code.google.com/p/chromium/wiki/GNCookbook, it seems like I should be
able to just refer to this variable here, but I get the following:

ERROR at //mojo/public/c/system/BUILD.gn:21:5: Undefined variable for -=.
    all_dependent_configs -= [
    ^--------------------
I don't have something with this name in scope now.

(from
http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)

What'd I do wrong?

[Chris Masone, 2014-07-14 21:48:23]

https://codereview.chromium.org/388333003/diff/40001/mojo/public/c/system/BUI...
File mojo/public/c/system/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/40001/mojo/public/c/system/BUI...
mojo/public/c/system/BUILD.gn:21: all_dependent_configs -= [
On 2014/07/14 21:38:37, Chris Masone wrote:
> Per the Dependent settings section of
> https://code.google.com/p/chromium/wiki/GNCookbook, it seems like I should be
> able to just refer to this variable here, but I get the following:
> 
> ERROR at //mojo/public/c/system/BUILD.gn:21:5: Undefined variable for -=.
>     all_dependent_configs -= [
>     ^--------------------
> I don't have something with this name in scope now.
> 
> (from
>
http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
> 
> What'd I do wrong?



Hm. I think the problem is that, essentially, this target needs to dictate that
a config option be removed from all dependent targets, and that there's not
really a way to express that? I can remove the config from _this_ target, but
that's not enough. I can _add_ a config to all dependent targets, but that isn't
exactly what I want to do either...unless there's a way that a config can
specify the removal of an ldflags entry.

[Chris Masone, 2014-07-15 20:39:22]

https://codereview.chromium.org/388333003/diff/40001/mojo/public/c/system/BUI...
File mojo/public/c/system/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/40001/mojo/public/c/system/BUI...
mojo/public/c/system/BUILD.gn:21: all_dependent_configs -= [
On 2014/07/14 21:48:22, Chris Masone wrote:
> On 2014/07/14 21:38:37, Chris Masone wrote:
> > Per the Dependent settings section of
> > https://code.google.com/p/chromium/wiki/GNCookbook, it seems like I should
be
> > able to just refer to this variable here, but I get the following:
> > 
> > ERROR at //mojo/public/c/system/BUILD.gn:21:5: Undefined variable for -=.
> >     all_dependent_configs -= [
> >     ^--------------------
> > I don't have something with this name in scope now.
> > 
> > (from
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
> > 
> > What'd I do wrong?
> 
> 
> 
> Hm. I think the problem is that, essentially, this target needs to dictate
that
> a config option be removed from all dependent targets, and that there's not
> really a way to express that? I can remove the config from _this_ target, but
> that's not enough. I can _add_ a config to all dependent targets, but that
isn't
> exactly what I want to do either...unless there's a way that a config can
> specify the removal of an ldflags entry.

Discussed with Brett offline, and he points out that this flag only matters with
static libraries. Since we're using a source_set here, we no longer need to
worry about this :-)

[yzshen1, 2014-07-15 21:00:00]

A few more nits.

Thanks!

https://codereview.chromium.org/388333003/diff/80001/mojo/BUILD.gn
File mojo/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/80001/mojo/BUILD.gn#newcode7
mojo/BUILD.gn:7: "//mojo/public:mojo_public",
Maybe we could consider naming the target "public", and using //mojo/public
here.

https://codereview.chromium.org/388333003/diff/80001/mojo/bindings/java/BUILD.gn
File mojo/bindings/java/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/80001/mojo/bindings/java/BUILD...
mojo/bindings/java/BUILD.gn:7: android_library("mojo_public_java") {
The style guide suggests using short (possibly non-unique) names that make
writing dependencies clearer, "mojo_" is probably not needed.

Besides, why we use "public_" in the name?

https://codereview.chromium.org/388333003/diff/80001/mojo/public/BUILD.gn
File mojo/public/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/80001/mojo/public/BUILD.gn#new...
mojo/public/BUILD.gn:5: group("mojo_public") {
Please consider "public", you could notice that all other GN targets don't have
"mojo_" prefix in their names.

https://codereview.chromium.org/388333003/diff/80001/mojo/public/c/system/BUI...
File mojo/public/c/system/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/80001/mojo/public/c/system/BUI...
mojo/public/c/system/BUILD.gn:15: "../../platform/native/system_thunks.cc",
nit: Usually, paths started with ../ go to the top of the list.

https://codereview.chromium.org/388333003/diff/80001/mojo/public/c/system/BUI...
mojo/public/c/system/BUILD.gn:21: # static library.  When building for Android,
BUILDCONFIG.gn adds
nit: GYP doesn't use BUILDCONFIG.gn.

https://codereview.chromium.org/388333003/diff/80001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/80001/mojo/public/cpp/applicat...
mojo/public/cpp/application/BUILD.gn:20:
"//mojo/public/interfaces/service_provider:service_provider",
nit: ":service_provider" is not needed.

https://codereview.chromium.org/388333003/diff/80001/mojo/public/cpp/utility/...
File mojo/public/cpp/utility/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/80001/mojo/public/cpp/utility/...
mojo/public/cpp/utility/BUILD.gn:21: sources += [
it should be -=

https://codereview.chromium.org/388333003/diff/80001/mojo/public/java/BUILD.gn
File mojo/public/java/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/80001/mojo/public/java/BUILD.g...
mojo/public/java/BUILD.gn:7: android_library("mojo_public_java") {
Please consider renaming this target: maybe it could be "java" and other targets
can refer to it as "//mojo/public/java".

[Chris Masone, 2014-07-15 21:11:07]

https://codereview.chromium.org/388333003/diff/80001/mojo/BUILD.gn
File mojo/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/80001/mojo/BUILD.gn#newcode7
mojo/BUILD.gn:7: "//mojo/public:mojo_public",
On 2014/07/15 20:59:59, yzshen1 wrote:
> Maybe we could consider naming the target "public", and using //mojo/public
> here.

Done.

https://codereview.chromium.org/388333003/diff/80001/mojo/bindings/java/BUILD.gn
File mojo/bindings/java/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/80001/mojo/bindings/java/BUILD...
mojo/bindings/java/BUILD.gn:7: android_library("mojo_public_java") {
On 2014/07/15 20:59:59, yzshen1 wrote:
> The style guide suggests using short (possibly non-unique) names that make
> writing dependencies clearer, "mojo_" is probably not needed.
> 
> Besides, why we use "public_" in the name?

Copy-paste error! Didn't know that about the style guide, I'll follow that
convention.

https://codereview.chromium.org/388333003/diff/80001/mojo/public/BUILD.gn
File mojo/public/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/80001/mojo/public/BUILD.gn#new...
mojo/public/BUILD.gn:5: group("mojo_public") {
On 2014/07/15 20:59:59, yzshen1 wrote:
> Please consider "public", you could notice that all other GN targets don't
have
> "mojo_" prefix in their names.

Done.

https://codereview.chromium.org/388333003/diff/80001/mojo/public/c/system/BUI...
File mojo/public/c/system/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/80001/mojo/public/c/system/BUI...
mojo/public/c/system/BUILD.gn:15: "../../platform/native/system_thunks.cc",
On 2014/07/15 20:59:59, yzshen1 wrote:
> nit: Usually, paths started with ../ go to the top of the list.

Done.

https://codereview.chromium.org/388333003/diff/80001/mojo/public/c/system/BUI...
mojo/public/c/system/BUILD.gn:21: # static library.  When building for Android,
BUILDCONFIG.gn adds
On 2014/07/15 21:00:00, yzshen1 wrote:
> nit: GYP doesn't use BUILDCONFIG.gn.

reworded for clarity

https://codereview.chromium.org/388333003/diff/80001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/80001/mojo/public/cpp/applicat...
mojo/public/cpp/application/BUILD.gn:20:
"//mojo/public/interfaces/service_provider:service_provider",
On 2014/07/15 21:00:00, yzshen1 wrote:
> nit: ":service_provider" is not needed.

Done.

https://codereview.chromium.org/388333003/diff/80001/mojo/public/cpp/utility/...
File mojo/public/cpp/utility/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/80001/mojo/public/cpp/utility/...
mojo/public/cpp/utility/BUILD.gn:21: sources += [
On 2014/07/15 21:00:00, yzshen1 wrote:
> it should be -=

Done.

https://codereview.chromium.org/388333003/diff/80001/mojo/public/java/BUILD.gn
File mojo/public/java/BUILD.gn (right):

https://codereview.chromium.org/388333003/diff/80001/mojo/public/java/BUILD.g...
mojo/public/java/BUILD.gn:7: android_library("mojo_public_java") {
On 2014/07/15 21:00:00, yzshen1 wrote:
> Please consider renaming this target: maybe it could be "java" and other
targets
> can refer to it as "//mojo/public/java".

Done.

[yzshen1, 2014-07-15 21:19:07]

LGTM

Thanks!

[Chris Masone, 2014-07-16 02:21:56]

The CQ bit was checked by cmasone@chromium.org

[commit-bot: I haz the power, 2014-07-16 02:24:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cmasone@chromium.org/388333003/100001

[commit-bot: I haz the power, 2014-07-16 20:35:30]

Change committed as 283490