[Cronet] Make Cronet buildable on regular Android bots.

Cronet depends on url/ and net/ compiled with special flag 'USE_ICU_ALTERNATIVES_ON_ANDROID'.

That requires either a separate set of build bots, OR making separate targets for url/ and net/ that use that flag.

This change adds such targets (extracting net target into separate net_common.gypi).

BUG=430500
Committed: https://crrev.com/dc8e94bd41b5a9f19a2160510c74edacce2fa1c7
Cr-Commit-Position: refs/heads/master@{#320126}

[mef, 2015-02-24 18:23:03]

mef@chromium.org changed reviewers:
+ agable@chromium.org, mmenke@chromium.org, pauljensen@chromium.org,
torne@chromium.org, ttuttle@chromium.org, xunjieli@chromium.org

[mef, 2015-02-24 18:23:31]

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

[mef, 2015-02-24 18:26:51]

Hi,

this change would let us build Cronet with regular Chromium/Android checkout,
without any special flags, so we would not have to create a new set of official
buildbots specifically for Cronet.

brettw: url/url.gyp
ttuttle: net/net.gyp
everybody: WDYT?

I think this is a reasonable price to pay as GYP complexity is not doubled.

[brettw, 2015-02-24 18:30:54]

https://codereview.chromium.org/933293003/diff/40001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/933293003/diff/40001/url/url.gyp#newcode128
url/url.gyp:128: # Same as url_lib but using ICU alternatives on Android.
This doesn't make any sense because url_lib is defined above and has a condition
based on icu_alternatives. Did you mean to delete those conditions and make
url_lib always link ICU?

[mef, 2015-02-24 18:39:15]

https://codereview.chromium.org/933293003/diff/40001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/933293003/diff/40001/url/url.gyp#newcode128
url/url.gyp:128: # Same as url_lib but using ICU alternatives on Android.
On 2015/02/24 18:30:54, brettw wrote:
> This doesn't make any sense because url_lib is defined above and has a
condition
> based on icu_alternatives. Did you mean to delete those conditions and make
> url_lib always link ICU?

Sorry, yes. Done.

[brettw, 2015-02-24 19:06:55]

lgtm

[Torne, 2015-02-25 13:27:43]

Not an owner for any of these files but it was my suggestion to go with this
approach, so for the record this LGTM :)

[xunjieli, 2015-02-25 15:06:56]

https://codereview.chromium.org/933293003/diff/140001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/933293003/diff/140001/components/cronet.gypi#...
components/cronet.gypi:102: 'target_name': 'cronet_static_small',
How about adding a comment here? along the line that cronet_static_small has
ICU, FILE support stripped, and it is used in production. cronet_static is only
used in tests.

I am thinking maybe naming them "cronet_static" and "cronet_static_tests" might
be clearer as to when they are used.

https://codereview.chromium.org/933293003/diff/140001/components/cronet.gypi#...
components/cronet.gypi:118: 'target_name': 'cronet_static',
Should "cronet_static" have DISABLE_FTP_SUPPORT=1 as well?

[mmenke, 2015-02-25 16:34:54]

https://codereview.chromium.org/933293003/diff/140001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/933293003/diff/140001/net/net.gyp#newcode1201
net/net.gyp:1201: 'target_name': 'net_small',
I'm confused.  How does this actually work in terms of output directories? 
Looks to me like both this project and net will be writing to the same output
directories...which works fine as far as the "variables" are concerned, but does
not work for different defines, as those affect object files.

https://codereview.chromium.org/933293003/diff/140001/net/net.gyp#newcode1213
net/net.gyp:1213: ],
Rather than exclude stuff here, think it makes more sense to include stuff in
"net".

https://codereview.chromium.org/933293003/diff/140001/net/net_common.gypi
File net/net_common.gypi (right):

https://codereview.chromium.org/933293003/diff/140001/net/net_common.gypi#new...
net/net_common.gypi:1: # Copyright 2013 The Chromium Authors. All rights
reserved.
2015?  Not really sure.

https://codereview.chromium.org/933293003/diff/140001/net/net_common.gypi#new...
net/net_common.gypi:6: # This target is included into both 'net' and
'net_small'.
into -> in

https://codereview.chromium.org/933293003/diff/140001/net/net_common.gypi#new...
net/net_common.gypi:17: '../third_party/icu/icu.gyp:icuuc',
Can we move these two dependencies, and the base_i18n one, into net instead? 
And the move the icu and non-icu includes into net and net_small as well?  Can
then also get rid of the "use_icu_alternatives_on_android" variable.

[Torne, 2015-02-25 16:46:06]

https://codereview.chromium.org/933293003/diff/140001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/933293003/diff/140001/net/net.gyp#newcode1201
net/net.gyp:1201: 'target_name': 'net_small',
On 2015/02/25 16:34:54, mmenke wrote:
> I'm confused.  How does this actually work in terms of output directories? 
> Looks to me like both this project and net will be writing to the same output
> directories...which works fine as far as the "variables" are concerned, but
does
> not work for different defines, as those affect object files.

The object files are either in a directory named after the target, or have the
name of the target included in the filename, depending which gyp backend is
being used. There's nothing in our build system stopping us from compiling the
same sources multiple times in different targets AFAIK.

[mmenke, 2015-02-25 16:48:03]

https://codereview.chromium.org/933293003/diff/140001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/933293003/diff/140001/net/net.gyp#newcode1201
net/net.gyp:1201: 'target_name': 'net_small',
On 2015/02/25 16:46:06, Torne wrote:
> On 2015/02/25 16:34:54, mmenke wrote:
> > I'm confused.  How does this actually work in terms of output directories? 
> > Looks to me like both this project and net will be writing to the same
output
> > directories...which works fine as far as the "variables" are concerned, but
> does
> > not work for different defines, as those affect object files.
> 
> The object files are either in a directory named after the target, or have the
> name of the target included in the filename, depending which gyp backend is
> being used. There's nothing in our build system stopping us from compiling the
> same sources multiple times in different targets AFAIK.

Ahh...you're right.  Didn't notice the object file names had the target name in
them.

[mef, 2015-02-26 21:22:51]

Thanks, PTAL.

https://codereview.chromium.org/933293003/diff/140001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/933293003/diff/140001/components/cronet.gypi#...
components/cronet.gypi:102: 'target_name': 'cronet_static_small',
On 2015/02/25 15:06:56, xunjieli wrote:
> How about adding a comment here? along the line that cronet_static_small has
> ICU, FILE support stripped, and it is used in production. cronet_static is
only
> used in tests.
Done.

> I am thinking maybe naming them "cronet_static" and "cronet_static_tests"
might
> be clearer as to when they are used.

Well, integration of Cronet into Webview will use full cronet_static, so I'd
rather not call it 'cronet_static_tests'.

I'm contemplating renaming cronet_static_small into cronet_static_lean, but I
don't have strong preference.

https://codereview.chromium.org/933293003/diff/140001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/933293003/diff/140001/net/net.gyp#newcode1213
net/net.gyp:1213: ],
On 2015/02/25 16:34:54, mmenke wrote:
> Rather than exclude stuff here, think it makes more sense to include stuff in
> "net".

Done. I've also moved items that depend on use_icu_alternatives_on_android here.

https://codereview.chromium.org/933293003/diff/140001/net/net_common.gypi
File net/net_common.gypi (right):

https://codereview.chromium.org/933293003/diff/140001/net/net_common.gypi#new...
net/net_common.gypi:1: # Copyright 2013 The Chromium Authors. All rights
reserved.
On 2015/02/25 16:34:54, mmenke wrote:
> 2015?  Not really sure.

Done.

https://codereview.chromium.org/933293003/diff/140001/net/net_common.gypi#new...
net/net_common.gypi:6: # This target is included into both 'net' and
'net_small'.
On 2015/02/25 16:34:54, mmenke wrote:
> into -> in

Done.

https://codereview.chromium.org/933293003/diff/140001/net/net_common.gypi#new...
net/net_common.gypi:17: '../third_party/icu/icu.gyp:icuuc',
On 2015/02/25 16:34:54, mmenke wrote:
> Can we move these two dependencies, and the base_i18n one, into net instead? 
> And the move the icu and non-icu includes into net and net_small as well?  Can
> then also get rid of the "use_icu_alternatives_on_android" variable.

Done.

[xunjieli, 2015-02-26 21:43:04]

On Thu, Feb 26, 2015 at 4:22 PM, <mef@chromium.org> wrote:

> Thanks, PTAL.
>
>
> https://codereview.chromium.org/933293003/diff/140001/
> components/cronet.gypi
> File components/cronet.gypi (right):
>
> https://codereview.chromium.org/933293003/diff/140001/
> components/cronet.gypi#newcode102
> components/cronet.gypi:102: 'target_name': 'cronet_static_small',
> On 2015/02/25 15:06:56, xunjieli wrote:
>
>> How about adding a comment here? along the line that
>>
> cronet_static_small has
>
>> ICU, FILE support stripped, and it is used in production.
>>
> cronet_static is only
>
>> used in tests.
>>
> Done.
>
>  I am thinking maybe naming them "cronet_static" and
>>
> "cronet_static_tests" might
>
>> be clearer as to when they are used.
>>
>
> Well, integration of Cronet into Webview will use full cronet_static, so
> I'd rather not call it 'cronet_static_tests'.
>
> Ah. i see. that's what I am missing. Thanks!


> I'm contemplating renaming cronet_static_small into cronet_static_lean,
> but I don't have strong preference.
>
https://codereview.chromium.org/933293003/diff/140001/net/net.gyp
> File net/net.gyp (right):
>
> https://codereview.chromium.org/933293003/diff/140001/net/
> net.gyp#newcode1213
> net/net.gyp:1213: ],
> On 2015/02/25 16:34:54, mmenke wrote:
>
>> Rather than exclude stuff here, think it makes more sense to include
>>
> stuff in
>
>> "net".
>>
>
> Done. I've also moved items that depend on
> use_icu_alternatives_on_android here.
>
> https://codereview.chromium.org/933293003/diff/140001/net/net_common.gypi
> File net/net_common.gypi (right):
>
> https://codereview.chromium.org/933293003/diff/140001/net/
> net_common.gypi#newcode1
> net/net_common.gypi:1: # Copyright 2013 The Chromium Authors. All rights
> reserved.
> On 2015/02/25 16:34:54, mmenke wrote:
>
>> 2015?  Not really sure.
>>
>
> Done.
>
> https://codereview.chromium.org/933293003/diff/140001/net/
> net_common.gypi#newcode6
> net/net_common.gypi:6: # This target is included into both 'net' and
> 'net_small'.
> On 2015/02/25 16:34:54, mmenke wrote:
>
>> into -> in
>>
>
> Done.
>
> https://codereview.chromium.org/933293003/diff/140001/net/
> net_common.gypi#newcode17
> net/net_common.gypi:17: '../third_party/icu/icu.gyp:icuuc',
> On 2015/02/25 16:34:54, mmenke wrote:
>
>> Can we move these two dependencies, and the base_i18n one, into net
>>
> instead?
>
>> And the move the icu and non-icu includes into net and net_small as
>>
> well?  Can
>
>> then also get rid of the "use_icu_alternatives_on_android" variable.
>>
>
> Done.
>
> https://codereview.chromium.org/933293003/
>

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

[mmenke, 2015-02-27 21:32:44]

https://codereview.chromium.org/933293003/diff/160001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/933293003/diff/160001/net/BUILD.gn#newcode523
net/BUILD.gn:523: } else {
Shouldn't we be making parallel projects here, too?  Also, should change the
deps-= to deps+=.

https://codereview.chromium.org/933293003/diff/160001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/933293003/diff/160001/net/net.gyp#newcode1217
net/net.gyp:1217: 'use_icu_alternatives_on_android': 1,
No longer needed.

https://codereview.chromium.org/933293003/diff/160001/net/net.gyp#newcode1223
net/net.gyp:1223: 'USE_ICU_ALTERNATIVES_ON_ANDROID=1',
Hrm...This is only used in net/ and Cronet/...  I think that now that Cronet
can't be built with ICU, we can remove the uses of it in cronet, though that
still leaves the one url/...  Should probably just leave it here and use it in
url/ until the GN transition, though that may be ages away.

[mef, 2015-02-27 22:03:45]

Thanks, PTAL.

https://codereview.chromium.org/933293003/diff/160001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/933293003/diff/160001/net/BUILD.gn#newcode523
net/BUILD.gn:523: } else {
On 2015/02/27 21:32:43, mmenke wrote:
> Shouldn't we be making parallel projects here, too?  
Possibly, but we don't have BUILD.gn for Cronet, so there is no real reason to
create net_small in this CL. Entered
https://code.google.com/p/chromium/issues/detail?id=462737 for GN support.

> Also, should change the deps-= to deps+=.
Done.

https://codereview.chromium.org/933293003/diff/160001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/933293003/diff/160001/net/net.gyp#newcode1217
net/net.gyp:1217: 'use_icu_alternatives_on_android': 1,
On 2015/02/27 21:32:44, mmenke wrote:
> No longer needed.

Done.

https://codereview.chromium.org/933293003/diff/160001/net/net.gyp#newcode1223
net/net.gyp:1223: 'USE_ICU_ALTERNATIVES_ON_ANDROID=1',
On 2015/02/27 21:32:44, mmenke wrote:
> Hrm...This is only used in net/ and Cronet/...  I think that now that Cronet
> can't be built with ICU, we can remove the uses of it in cronet, though that
> still leaves the one url/...  Should probably just leave it here and use it in
> url/ until the GN transition, though that may be ages away.

Acknowledged.

[mmenke, 2015-03-02 17:50:04]

https://codereview.chromium.org/933293003/diff/160001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/933293003/diff/160001/net/net.gyp#newcode1223
net/net.gyp:1223: 'USE_ICU_ALTERNATIVES_ON_ANDROID=1',
On 2015/02/27 22:03:45, mef wrote:
> On 2015/02/27 21:32:44, mmenke wrote:
> > Hrm...This is only used in net/ and Cronet/...  I think that now that Cronet
> > can't be built with ICU, we can remove the uses of it in cronet, though that
> > still leaves the one url/...  Should probably just leave it here and use it
in
> > url/ until the GN transition, though that may be ages away.
> 
> Acknowledged.

For the record:  Misha pointed out I was wrong, we still have to build Cronet
with ICU for tests (I skipped over reviewing those files, intended to get back
to them later...and then forgot about it).

He also pointed out we also need to do it if we're building in the context and
Android WebView (Though then we can probably depend on the WebView to init ICU,
anyways, I guess?)

https://codereview.chromium.org/933293003/diff/180001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/933293003/diff/180001/components/cronet.gypi#...
components/cronet.gypi:106: 'USE_ICU_ALTERNATIVES_ON_ANDROID=1',
Seems more than a little unfortunate that we're not testing anything with the
ICU alternatives.

https://codereview.chromium.org/933293003/diff/180001/components/cronet.gypi#...
components/cronet.gypi:120: # of file and ftp support and using ICU
alternatives.
This comment is a little confusing, since cronet_static *also* disables file and
FTP support.

https://codereview.chromium.org/933293003/diff/180001/components/cronet.gypi#...
components/cronet.gypi:124: 'DISABLE_FTP_SUPPORT=1',
Wait...aren't we including a version of net/ build without these flags?

https://codereview.chromium.org/933293003/diff/180001/components/cronet.gypi#...
components/cronet.gypi:126: 'includes': [ 'cronet/cronet_static.gypi' ],
I think we're missing a dependencies section.

[mef, 2015-03-03 17:37:48]

Thanks, PTAL.

https://codereview.chromium.org/933293003/diff/180001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/933293003/diff/180001/components/cronet.gypi#...
components/cronet.gypi:106: 'USE_ICU_ALTERNATIVES_ON_ANDROID=1',
On 2015/03/02 17:50:04, mmenke wrote:
> Seems more than a little unfortunate that we're not testing anything with the
> ICU alternatives.

Yeah, I'm thinking that maybe Cronet bots should stay and test with ICU
alternative?

https://codereview.chromium.org/933293003/diff/180001/components/cronet.gypi#...
components/cronet.gypi:120: # of file and ftp support and using ICU
alternatives.
On 2015/03/02 17:50:04, mmenke wrote:
> This comment is a little confusing, since cronet_static *also* disables file
and
> FTP support.

Oops, copy-paste gone awry. Done.

https://codereview.chromium.org/933293003/diff/180001/components/cronet.gypi#...
components/cronet.gypi:124: 'DISABLE_FTP_SUPPORT=1',
On 2015/03/02 17:50:04, mmenke wrote:
> Wait...aren't we including a version of net/ build without these flags?

Done.

https://codereview.chromium.org/933293003/diff/180001/components/cronet.gypi#...
components/cronet.gypi:126: 'includes': [ 'cronet/cronet_static.gypi' ],
On 2015/03/02 17:50:04, mmenke wrote:
> I think we're missing a dependencies section.
It is in cronet_static.gypi

[mmenke, 2015-03-03 18:03:47]

https://codereview.chromium.org/933293003/diff/220001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/933293003/diff/220001/components/cronet.gypi#...
components/cronet.gypi:115: ],
Instead of an exclusion, could we remove that dep from cronet_static.gypi, and
add it to the cronet_static project?  In general, I think excluding stuff is a
little ugly.

https://codereview.chromium.org/933293003/diff/220001/components/cronet/crone...
File components/cronet/cronet_static.gypi (right):

https://codereview.chromium.org/933293003/diff/220001/components/cronet/crone...
components/cronet/cronet_static.gypi:12: ],
These should be removed (They're wrong for cronet_static).

[mef, 2015-03-10 16:50:39]

Sorry for taking so long to address your comments.

https://codereview.chromium.org/933293003/diff/220001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/933293003/diff/220001/components/cronet.gypi#...
components/cronet.gypi:115: ],
On 2015/03/03 18:03:47, mmenke wrote:
> Instead of an exclusion, could we remove that dep from cronet_static.gypi, and
> add it to the cronet_static project?  In general, I think excluding stuff is a
> little ugly.

Done.

https://codereview.chromium.org/933293003/diff/220001/components/cronet/crone...
File components/cronet/cronet_static.gypi (right):

https://codereview.chromium.org/933293003/diff/220001/components/cronet/crone...
components/cronet/cronet_static.gypi:12: ],
On 2015/03/03 18:03:47, mmenke wrote:
> These should be removed (They're wrong for cronet_static).

Done.

[mmenke, 2015-03-10 17:07:26]

LGTM, modulo comment, but I wouldn't be surprised if I'm forgetting a
dependency.  :(

https://codereview.chromium.org/933293003/diff/260001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/933293003/diff/260001/components/cronet.gypi#...
components/cronet.gypi:103: # of file and ftp support and using ICU
alternatives.
More accurate to say it has reduced binary size through using ICU alternatives,
which also requires file and ftp support be disabled (Neither of them requires a
huge amount of code, though suppose it adds up)

https://codereview.chromium.org/933293003/diff/260001/components/cronet.gypi#...
components/cronet.gypi:116: # cronet_static target depends on ICU snd includes
file and ftp support.
snd -> and

https://codereview.chromium.org/933293003/diff/260001/components/cronet.gypi#...
components/cronet.gypi:119: '../net/net.gyp:net',
We also depend on base::InitializeICU, which is in ../base/base.gyp:base_i18n, I
believe.

[mef, 2015-03-11 17:58:26]

Thanks! I'm going to commit this unless I hear otherwise.

https://codereview.chromium.org/933293003/diff/260001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/933293003/diff/260001/components/cronet.gypi#...
components/cronet.gypi:103: # of file and ftp support and using ICU
alternatives.
On 2015/03/10 17:07:26, mmenke wrote:
> More accurate to say it has reduced binary size through using ICU
alternatives,
> which also requires file and ftp support be disabled (Neither of them requires
a
> huge amount of code, though suppose it adds up)

Done.

https://codereview.chromium.org/933293003/diff/260001/components/cronet.gypi#...
components/cronet.gypi:116: # cronet_static target depends on ICU snd includes
file and ftp support.
On 2015/03/10 17:07:25, mmenke wrote:
> snd -> and

Done.

https://codereview.chromium.org/933293003/diff/260001/components/cronet.gypi#...
components/cronet.gypi:119: '../net/net.gyp:net',
On 2015/03/10 17:07:25, mmenke wrote:
> We also depend on base::InitializeICU, which is in ../base/base.gyp:base_i18n,
I
> believe.

Done.

[mef, 2015-03-11 18:32:06]

The CQ bit was checked by mef@chromium.org

[mef, 2015-03-11 18:32:06]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
torne@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/933293003/#ps300001
(title: "Sync")

[commit-bot: I haz the power, 2015-03-11 18:32:29]

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

[commit-bot: I haz the power, 2015-03-11 19:49:16]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-03-11 19:50:21]

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