GN: Add //ui/base/ime, fix up printing on mac

GN: Add //ui/base/ime, fix up printing on mac

This adds the //ui/base/ime target to the GN build, which is used
everywhere except for ios. In the gyp build this isn't a real target,
it's a set of properties that are included into the ui_base target which
is the best you can do in GYP since you can't link a static library into
a component without dropping some symbols. In GN, we can define this as
a source_set with its own dependencies and settings.

This also fixes up some miscellaneous mac issues.

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

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

[brettw, 2014-07-14 18:23:54]

lgtm

[Ben Goodger (Google), 2014-07-14 19:13:25]

lgtm

[jamesr, 2014-07-14 19:19:25]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-07-14 19:21:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/389083002/20001

[commit-bot: I haz the power, 2014-07-14 21:46:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-07-14 21:57:00]

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

[commit-bot: I haz the power, 2014-07-14 21:57:01]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[jamesr, 2014-07-23 06:33:30]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-07-23 06:34:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/389083002/80001

[commit-bot: I haz the power, 2014-07-23 09:05:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-07-23 11:20:19]

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

[commit-bot: I haz the power, 2014-07-23 11:20:20]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[jamesr, 2014-07-23 17:33:12]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-07-23 17:35:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/389083002/80001

[commit-bot: I haz the power, 2014-07-23 17:45:55]

Change committed as 284981

[tfarina, 2014-07-24 00:06:09]

I would love to have being copied before on this patch!

To get an email or something.

https://codereview.chromium.org/389083002/diff/80001/ui/base/ime/BUILD.gn
File ui/base/ime/BUILD.gn (right):

https://codereview.chromium.org/389083002/diff/80001/ui/base/ime/BUILD.gn#new...
ui/base/ime/BUILD.gn:8: # GYP version: ui/base/ime/ime.gypi, which is included
into ui_base
Why? I have said in the gn-dev snippets I was working on this.

I don't think we should do this way. It is not a separate target on GYP.

[tfarina, 2014-07-24 00:06:56]

https://codereview.chromium.org/389083002/diff/80001/ui/base/ime/BUILD.gn
File ui/base/ime/BUILD.gn (right):

https://codereview.chromium.org/389083002/diff/80001/ui/base/ime/BUILD.gn#new...
ui/base/ime/BUILD.gn:8: # GYP version: ui/base/ime/ime.gypi, which is included
into ui_base
ime.gypi is gone. I did all the work to remove it in preparation for the GN
patch.

[jamesr, 2014-07-24 00:27:24]

On 2014/07/24 00:06:09, tfarina wrote:
> I would love to have being copied before on this patch!
> 
> To get an email or something.
> 
> https://codereview.chromium.org/389083002/diff/80001/ui/base/ime/BUILD.gn
> File ui/base/ime/BUILD.gn (right):
> 
>
https://codereview.chromium.org/389083002/diff/80001/ui/base/ime/BUILD.gn#new...
> ui/base/ime/BUILD.gn:8: # GYP version: ui/base/ime/ime.gypi, which is included
> into ui_base
> Why? I have said in the gn-dev snippets I was working on this.
> 
> I don't think we should do this way. It is not a separate target on GYP.

It should be.  It can't in GYP because there's no equivalent of source_set, but
GN has it.

[jamesr, 2014-07-24 00:27:35]

On 2014/07/24 00:06:56, tfarina wrote:
> https://codereview.chromium.org/389083002/diff/80001/ui/base/ime/BUILD.gn
> File ui/base/ime/BUILD.gn (right):
> 
>
https://codereview.chromium.org/389083002/diff/80001/ui/base/ime/BUILD.gn#new...
> ui/base/ime/BUILD.gn:8: # GYP version: ui/base/ime/ime.gypi, which is included
> into ui_base
> ime.gypi is gone. I did all the work to remove it in preparation for the GN
> patch.

Thanks - I'll fix the comment.

[tfarina, 2014-07-24 00:29:33]

On Wed, Jul 23, 2014 at 9:27 PM, <jamesr@chromium.org> wrote:

> On 2014/07/24 00:06:56, tfarina wrote:
>
>> https://codereview.chromium.org/389083002/diff/80001/ui/base/ime/BUILD.gn
>> File ui/base/ime/BUILD.gn (right):
>>
>
>
> https://codereview.chromium.org/389083002/diff/80001/ui/
> base/ime/BUILD.gn#newcode8
>
>> ui/base/ime/BUILD.gn:8: # GYP version: ui/base/ime/ime.gypi, which is
>> included
>> into ui_base
>> ime.gypi is gone. I did all the work to remove it in preparation for the
>> GN
>> patch.
>>
>
> Thanks - I'll fix the comment.
>
> I will go back with the ime files to the ui/base/BUILD.gn list. That is
the right thing to do. That is the way it is in GYP.


> https://codereview.chromium.org/389083002/
>



-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-07-24 00:31:23]

No, it's not.  These are separate files with separate DEPS from the rest of
ui/base/.  The build file should live with the code where possible (which
it is here, but only in GN).


On Wed, Jul 23, 2014 at 5:29 PM, Thiago Farina <tfarina@chromium.org> wrote:

>
>
>
> On Wed, Jul 23, 2014 at 9:27 PM, <jamesr@chromium.org> wrote:
>
>> On 2014/07/24 00:06:56, tfarina wrote:
>>
>>> https://codereview.chromium.org/389083002/diff/80001/ui/
>>> base/ime/BUILD.gn
>>> File ui/base/ime/BUILD.gn (right):
>>>
>>
>>
>> https://codereview.chromium.org/389083002/diff/80001/ui/
>> base/ime/BUILD.gn#newcode8
>>
>>> ui/base/ime/BUILD.gn:8: # GYP version: ui/base/ime/ime.gypi, which is
>>> included
>>> into ui_base
>>> ime.gypi is gone. I did all the work to remove it in preparation for the
>>> GN
>>> patch.
>>>
>>
>> Thanks - I'll fix the comment.
>>
>> I will go back with the ime files to the ui/base/BUILD.gn list. That is
> the right thing to do. That is the way it is in GYP.
>
>
>> https://codereview.chromium.org/389083002/
>>
>
>
>
> --
> Thiago Farina
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2014-07-24 00:34:08]

On Wed, Jul 23, 2014 at 9:31 PM, James Robinson <jamesr@google.com> wrote:

> No, it's not.  These are separate files with separate DEPS from the rest
> of ui/base/.
>
They are not separate from ui/base. If they were they would be in ui/ime.
So it is not separate at this moment.

The ime files are listed withing sources ui_base.gyp sources list.
https://chromium.googlesource.com/chromium/src/+/master/ui/base/ui_base.gyp#180

I haven't work on this for months for nothing.

-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-07-24 01:14:20]

On Wed, Jul 23, 2014 at 5:34 PM, Thiago Farina <tfarina@chromium.org> wrote:

>
>
>
> On Wed, Jul 23, 2014 at 9:31 PM, James Robinson <jamesr@google.com> wrote:
>
>> No, it's not.  These are separate files with separate DEPS from the rest
>> of ui/base/.
>>
> They are not separate from ui/base. If they were they would be in ui/ime.
> So it is not separate at this moment.
>

That's irrelevant.  As a general principle, all build files should be
located as close to the source as possible.  I.e. the build file for
a/b/c/foo.cc should be in a/b/c/BUILD.gn.  The same applies here.  GYP has
restrictions that make it harder to do this, so we do suboptimal things
like define build rules in parent directories and have .gypi's that are
included into parents, but that's a restriction of GYP that GN does not
share.


>
> The ime files are listed withing sources ui_base.gyp sources list.
>
>
https://chromium.googlesource.com/chromium/src/+/master/ui/base/ui_base.gyp#180
>

That's because ui_base is a component and in gyp the only way to make sure
sources within a component end up in the final library is to have them
listed directly in the sources list of the component meaning either listing
them in the sources list directly or (as this code used to do) using a gypi
to 'inject' into the sources list.

In GN we invented source_set to get around this.  We can define a
source_set in the proper location and depend on it from a component and the
right thing works.  This is far better since it means that the build file
is close to the source and we can express dependencies more precisely.

>
>
> I haven't work on this for months for nothing.
>

I haven't been following what has been going on in ui/base/ carefully, I
just wrote GN files for the tree as it currently exists.  This is the
proper set up for GN.  I don't know if folding the ime gypi into
ui/base/ui_base.gyp is an improvement or not.

- James

>
> --
> Thiago Farina
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2014-07-24 01:36:59]

On Wednesday, July 23, 2014, James Robinson <jamesr@google.com> wrote:

>
>
>
> On Wed, Jul 23, 2014 at 5:34 PM, Thiago Farina <tfarina@chromium.org
> <javascript:_e(%7B%7D,'cvml','tfarina@chromium.org');>> wrote:
>
>>
>>
>>
>> On Wed, Jul 23, 2014 at 9:31 PM, James Robinson <jamesr@google.com
>> <javascript:_e(%7B%7D,'cvml','jamesr@google.com');>> wrote:
>>
>>> No, it's not.  These are separate files with separate DEPS from the rest
>>> of ui/base/.
>>>
>> They are not separate from ui/base. If they were they would be in ui/ime.
>> So it is not separate at this moment.
>>
>
> That's irrelevant.  As a general principle, all build files should be
> located as close to the source as possible.  I.e. the build file for
> a/b/c/foo.cc should be in a/b/c/BUILD.gn.  The same applies here.  GYP has
> restrictions that make it harder to do this, so we do suboptimal things
> like define build rules in parent directories and have .gypi's that are
> included into parents, but that's a restriction of GYP that GN does not
> share.
>
>
>>
>> The ime files are listed withing sources ui_base.gyp sources list.
>>
>>
https://chromium.googlesource.com/chromium/src/+/master/ui/base/ui_base.gyp#180
>>
>
> That's because ui_base is a component and in gyp the only way to make sure
> sources within a component end up in the final library is to have them
> listed directly in the sources list of the component meaning either listing
> them in the sources list directly or (as this code used to do) using a gypi
> to 'inject' into the sources list.
>
> In GN we invented source_set to get around this.  We can define a
> source_set in the proper location and depend on it from a component and the
> right thing works.  This is far better since it means that the build file
> is close to the source and we can express dependencies more precisely.
>
It is close to the source. OK. But ui/base/ime was never a separate target
and you are trying to imply that as if it was (I think just because you saw
the gypi).

Sorry, I just worked hard on this, that I feel I should have been consulted
before.

I see you want build files close to the source, but I don't see the benefit
to have part of the tree in one way (ui/base/ime/BUILD.gn) while *all* the
other ui/base files are being built by ui/base/BUILD.gn.



-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.