Add GN targets for mojom generator

Add GN targets for mojom generator.

BUG=None
Test=None

R=brettw@chromium.org, viettrungluu@chromium.org

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

[yzshen1, 2014-04-28 19:39:34]

Hi, Brett and Trung

Would you please take a look?

viettrungluu@chromium.org: OWNER and general idea.
brettw@chromium.org: everything.

[brettw, 2014-04-28 19:46:23]

lgtm

https://codereview.chromium.org/259823005/diff/40001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/259823005/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/mojom.gni:77: include_dirs = [ root_gen_dir ]
This should be implicit for everybody, you should be able to remove this line. I
think this means that you can just delete this whole config.

[yzshen1, 2014-04-28 20:40:29]

Thanks Brett.

https://codereview.chromium.org/259823005/diff/40001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/259823005/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/mojom.gni:77: include_dirs = [ root_gen_dir ]
On 2014/04/28 19:46:24, brettw wrote:
> This should be implicit for everybody, you should be able to remove this line.
I
> think this means that you can just delete this whole config.

Done.

[viettrungluu, 2014-04-28 21:25:50]

https://codereview.chromium.org/259823005/diff/60001/mojo/BUILD.gn
File mojo/BUILD.gn (right):

https://codereview.chromium.org/259823005/diff/60001/mojo/BUILD.gn#newcode11
mojo/BUILD.gn:11: 
Nooooooooooooooooooooooooooooooooooooooooooooooo. Extra blank line. :)

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

https://codereview.chromium.org/259823005/diff/60001/mojo/public/BUILD.gn#new...
mojo/public/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
This file probably should be in the cpp subdirectory. Probably in cpp/bindings,
even.

https://codereview.chromium.org/259823005/diff/60001/mojo/public/BUILD.gn#new...
mojo/public/BUILD.gn:44: "js/bindings/constants.cc",
These files don't exist.

https://codereview.chromium.org/259823005/diff/60001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/259823005/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/mojom.gni:36:
"$generator_root/pylib/mojom/generate/__init__.py",
This is missing .../mojom/__init__.py and .../mojom/error.py, but so is the
.gypi (oops). Could you please add these files, and also to the .gypi?

https://codereview.chromium.org/259823005/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/mojom.gni:69: "-d", rebase_path("//",
root_build_dir),
Please also add a "--use_chromium_bundled_pylibs" argument (like the current
.gypi has).

[yzshen1, 2014-04-28 22:57:19]

Thanks Trung!

https://codereview.chromium.org/259823005/diff/60001/mojo/BUILD.gn
File mojo/BUILD.gn (right):

https://codereview.chromium.org/259823005/diff/60001/mojo/BUILD.gn#newcode11
mojo/BUILD.gn:11: 
On 2014/04/28 21:25:50, viettrungluu wrote:
> Nooooooooooooooooooooooooooooooooooooooooooooooo. Extra blank line. :)

It shouldn't be the most terrible mistake that I have ever made! :)

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

https://codereview.chromium.org/259823005/diff/60001/mojo/public/BUILD.gn#new...
mojo/public/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
On 2014/04/28 21:25:50, viettrungluu wrote:
> This file probably should be in the cpp subdirectory. Probably in
cpp/bindings,
> even.

I put it here because of the js/bindings/constants.{cc,h} files. Do you think it
is better to split them into two targets? I feel that it will be easier for
maintenance if the targets have roughly 1:1 mapping between gyp and gn.
(Please also see the reply to the next comment.)

https://codereview.chromium.org/259823005/diff/60001/mojo/public/BUILD.gn#new...
mojo/public/BUILD.gn:44: "js/bindings/constants.cc",
On 2014/04/28 21:25:50, viettrungluu wrote:
> These files don't exist.

Sorry I probably haven't understood:
It seems they do exist:
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/js/bin...

Or do you mean they shouldn't belong to this target?

https://codereview.chromium.org/259823005/diff/60001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/259823005/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/mojom.gni:36:
"$generator_root/pylib/mojom/generate/__init__.py",
On 2014/04/28 21:25:50, viettrungluu wrote:
> This is missing .../mojom/__init__.py and .../mojom/error.py, but so is the
> .gypi (oops). Could you please add these files, and also to the .gypi?

Added both and also enum_traits.tmpl.

It seems hard to keep this list up-to-date. People will forget to update it...:/

https://codereview.chromium.org/259823005/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/mojom.gni:69: "-d", rebase_path("//",
root_build_dir),
On 2014/04/28 21:25:50, viettrungluu wrote:
> Please also add a "--use_chromium_bundled_pylibs" argument (like the current
> .gypi has).

Done.

[viettrungluu, 2014-04-28 23:32:47]

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

https://codereview.chromium.org/259823005/diff/60001/mojo/public/BUILD.gn#new...
mojo/public/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
On 2014/04/28 22:57:20, yzshen1 wrote:
> On 2014/04/28 21:25:50, viettrungluu wrote:
> > This file probably should be in the cpp subdirectory. Probably in
> cpp/bindings,
> > even.
> 
> I put it here because of the js/bindings/constants.{cc,h} files. Do you think
it
> is better to split them into two targets?

Yes.

> I feel that it will be easier for
> maintenance if the targets have roughly 1:1 mapping between gyp and gn.
> (Please also see the reply to the next comment.)

https://codereview.chromium.org/259823005/diff/60001/mojo/public/BUILD.gn#new...
mojo/public/BUILD.gn:44: "js/bindings/constants.cc",
On 2014/04/28 22:57:20, yzshen1 wrote:
> On 2014/04/28 21:25:50, viettrungluu wrote:
> > These files don't exist.
> 
> Sorry I probably haven't understood:
> It seems they do exist:
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/js/bin...
> 
> Or do you mean they shouldn't belong to this target?

Oops, sorry, I misread the filenames.

They don't belong in this target. They don't belong in the directory that
they're in actually.

[yzshen1, 2014-04-29 03:58:12]

Thanks, Trung.

Please take another look.

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

https://codereview.chromium.org/259823005/diff/60001/mojo/public/BUILD.gn#new...
mojo/public/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
On 2014/04/28 23:32:47, viettrungluu wrote:
> On 2014/04/28 22:57:20, yzshen1 wrote:
> > On 2014/04/28 21:25:50, viettrungluu wrote:
> > > This file probably should be in the cpp subdirectory. Probably in
> > cpp/bindings,
> > > even.
> > 
> > I put it here because of the js/bindings/constants.{cc,h} files. Do you
think
> it
> > is better to split them into two targets?
> 
> Yes.
> 
> > I feel that it will be easier for
> > maintenance if the targets have roughly 1:1 mapping between gyp and gn.
> > (Please also see the reply to the next comment.)

Done.

https://codereview.chromium.org/259823005/diff/60001/mojo/public/BUILD.gn#new...
mojo/public/BUILD.gn:44: "js/bindings/constants.cc",
On 2014/04/28 23:32:47, viettrungluu wrote:
> On 2014/04/28 22:57:20, yzshen1 wrote:
> > On 2014/04/28 21:25:50, viettrungluu wrote:
> > > These files don't exist.
> > 
> > Sorry I probably haven't understood:
> > It seems they do exist:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/js/bin...
> > 
> > Or do you mean they shouldn't belong to this target?
> 
> Oops, sorry, I misread the filenames.
> 
> They don't belong in this target. They don't belong in the directory that
> they're in actually.

Done.

[viettrungluu, 2014-04-29 21:59:22]

lgtm

https://codereview.chromium.org/259823005/diff/100001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/259823005/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:7: "allocation_scope.h",
I wonder if we should separate the interface headers from the implementation
(lib). We can worry about that later.

[yzshen1, 2014-04-29 22:01:12]

https://codereview.chromium.org/259823005/diff/100001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/259823005/diff/100001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:7: "allocation_scope.h",
On 2014/04/29 21:59:22, viettrungluu wrote:
> I wonder if we should separate the interface headers from the implementation
> (lib). We can worry about that later.

I will bear that in mind.

[yzshen1, 2014-04-29 22:01:17]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-04-29 22:03:52]

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

[commit-bot: I haz the power, 2014-04-29 23:20:27]

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

[commit-bot: I haz the power, 2014-04-29 23:20:27]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium

[yzshen1, 2014-04-30 00:24:30]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-04-30 00:26:29]

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

[commit-bot: I haz the power, 2014-04-30 01:41:01]

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

[commit-bot: I haz the power, 2014-04-30 01:41:01]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium

[yzshen1, 2014-04-30 05:27:09]

Committed patchset #6 manually as r267086 (presubmit successful).