Reland: Make separate net and url GN targets with and without ICU

Reland: Make separate net and url GN targets with and without ICU

The net and url GYP files were modified in crrev.com/933293003
so Cronet could be built side-by-side with Chrome. However GN
files are not modified. This CL keeps the GN files in sync
with the GYP files.

TBR=brettw@chromium.org
BUG=522096
Committed: https://crrev.com/4c8c6921ca6739d16f6551635328faf164848f9d
Cr-Commit-Position: refs/heads/master@{#345891}

Committed: https://crrev.com/905496a6338fa535b5d242fda1d3eb0cab1d01ae
Cr-Commit-Position: refs/heads/master@{#346392}

[xunjieli, 2015-08-19 19:39:30]

Patchset #1 (id:1) has been deleted

[xunjieli, 2015-08-19 19:39:36]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2015-08-19 21:04:32]

xunjieli@chromium.org changed reviewers:
+ mmenke@chromium.org

[xunjieli, 2015-08-19 21:04:33]

Hi Matt, PTAL. I will send it out to url OWNER if you think this is the way to
go. Strangely I can compile targets fine locally, but try jobs fail on
android_chromium_gn_compile_dbg and linux_chromium_gn_dbg. If you know what be
happening, please let me know.

[xunjieli, 2015-08-19 21:35:26]

On 2015/08/19 21:04:33, xunjieli wrote:
> Hi Matt, PTAL. I will send it out to url OWNER if you think this is the way to
> go. Strangely I can compile targets fine locally, but try jobs fail on
> android_chromium_gn_compile_dbg and linux_chromium_gn_dbg. If you know what be
> happening, please let me know.

I am able to reproduce the compile error by enabling component build. I will try
to see what I am missing. Will update this thread.

[mmenke, 2015-08-19 21:37:28]

I looking through the CL, nothing immediately occurs to me as a possible cause
for the linker errors you're seeing.

https://codereview.chromium.org/1287893005/diff/40001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/40001/net/BUILD.gn#newcode517
net/BUILD.gn:517: "base/filename_util.h",
Don't need the header file, it's included in the common sources.

[mmenke, 2015-08-19 22:28:26]

https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode533
net/BUILD.gn:533: 
I think you may need NET_IMPLEMENTATION here, too.

[xunjieli, 2015-08-19 22:32:41]

On 2015/08/19 22:28:26, mmenke wrote:
> https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> File net/BUILD.gn (right):
> 
> https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode533
> net/BUILD.gn:533: 
> I think you may need NET_IMPLEMENTATION here, too.

That is exactly the cause. I figured it out a little before you sent the email
and tested it locally, and was waiting for the try job to finish just to be
sure. Thanks, Matt!

[xunjieli, 2015-08-19 22:37:03]

Thanks for the help, Matt! I think the try bots should be passing now. The Cl is
ready for review.

https://codereview.chromium.org/1287893005/diff/40001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/40001/net/BUILD.gn#newcode517
net/BUILD.gn:517: "base/filename_util.h",
On 2015/08/19 21:37:27, mmenke wrote:
> Don't need the header file, it's included in the common sources.

Done.

[mmenke, 2015-08-19 22:56:47]

On 2015/08/19 22:32:41, xunjieli wrote:
> On 2015/08/19 22:28:26, mmenke wrote:
> > https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> > File net/BUILD.gn (right):
> > 
> >
https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode533
> > net/BUILD.gn:533: 
> > I think you may need NET_IMPLEMENTATION here, too.
> 
> That is exactly the cause. I figured it out a little before you sent the email
> and tested it locally, and was waiting for the try job to finish just to be
> sure. Thanks, Matt!

I didn't figure out the cause, I just noticed you'd updated the CL, and then
found that you should probably add the NET_IMPLEMENTATION in one more spot.

Great job figuring out the problem!

[xunjieli, 2015-08-20 14:28:46]

https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode533
net/BUILD.gn:533: 
On 2015/08/19 22:28:26, mmenke wrote:
> I think you may need NET_IMPLEMENTATION here, too.

net_string_util_icu_alternatives_android.h does not have any NET_EXPORT macros,
so I did not add NET_IMPLEMENTATION here. Let me know if you'd like me to do
that :)

[mmenke, 2015-08-20 15:37:47]

https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode65
net/BUILD.gn:65: source_set("net_common") {
Should we restrict visibility to the two new source sets?

visibility = [":net", ":net_small"]

I believe

https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode526
net/BUILD.gn:526: # same as net, but with ICU dependency stripped.
nit:  same -> Same

https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode528
net/BUILD.gn:528: defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
We weren't defining this before, were we?  Do we need to add it in this CL?

https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode533
net/BUILD.gn:533: 
On 2015/08/20 14:28:45, xunjieli wrote:
> On 2015/08/19 22:28:26, mmenke wrote:
> > I think you may need NET_IMPLEMENTATION here, too.
> 
> net_string_util_icu_alternatives_android.h does not have any NET_EXPORT
macros,
> so I did not add NET_IMPLEMENTATION here. Let me know if you'd like me to do
> that :)

Looks like you're right - it's not needed.  This is also a net implementation,
though, and could legitimately have implementations of such functions, so I
think we should probably just include it, anyways.

[xunjieli, 2015-08-20 17:55:35]

https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode528
net/BUILD.gn:528: defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
On 2015/08/20 15:37:47, mmenke wrote:
> We weren't defining this before, were we?  Do we need to add it in this CL?

We had this define in url/BUILD.gn: 

config("url_icu_config") {
  if (use_icu_alternatives_on_android) {
    defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
  }
}

USE_ICU_ALTERNATIVES_ON_ANDROID is used in net/android/net_jni_registrar.cc.

On second thought, I think this CL won't work.
source_set(net_common) does not know the defines we have in net_small, unlike
how gyp's "includes:net_common.gpyi" works. So we can't set
USE_ICU_ALTERNATIVES_ON_ANDROID, DISABLE_FILE_SUPPORT, or DISABLE_FTP_SUPPORT in
net_small and expect these will be used in compiling net_common's sources.

Do you see a way to make this work other than duplicating the original net
target in its entirety?

[mmenke, 2015-08-20 18:00:39]

On 2015/08/20 17:55:35, xunjieli wrote:
> https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> File net/BUILD.gn (right):
> 
> https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode528
> net/BUILD.gn:528: defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> On 2015/08/20 15:37:47, mmenke wrote:
> > We weren't defining this before, were we?  Do we need to add it in this CL?
> 
> We had this define in url/BUILD.gn: 
> 
> config("url_icu_config") {
>   if (use_icu_alternatives_on_android) {
>     defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
>   }
> }
> 
> USE_ICU_ALTERNATIVES_ON_ANDROID is used in net/android/net_jni_registrar.cc.
> 
> On second thought, I think this CL won't work.
> source_set(net_common) does not know the defines we have in net_small, unlike
> how gyp's "includes:net_common.gpyi" works. So we can't set
> USE_ICU_ALTERNATIVES_ON_ANDROID, DISABLE_FILE_SUPPORT, or DISABLE_FTP_SUPPORT
in
> net_small and expect these will be used in compiling net_common's sources.
> 
> Do you see a way to make this work other than duplicating the original net
> target in its entirety?

So how do we do it in the gyp files?

[xunjieli, 2015-08-20 18:04:33]

On 2015/08/20 18:00:39, mmenke wrote:
> On 2015/08/20 17:55:35, xunjieli wrote:
> > https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> > File net/BUILD.gn (right):
> > 
> >
https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode528
> > net/BUILD.gn:528: defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > On 2015/08/20 15:37:47, mmenke wrote:
> > > We weren't defining this before, were we?  Do we need to add it in this
CL?
> > 
> > We had this define in url/BUILD.gn: 
> > 
> > config("url_icu_config") {
> >   if (use_icu_alternatives_on_android) {
> >     defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> >   }
> > }
> > 
> > USE_ICU_ALTERNATIVES_ON_ANDROID is used in net/android/net_jni_registrar.cc.
> > 
> > On second thought, I think this CL won't work.
> > source_set(net_common) does not know the defines we have in net_small,
unlike
> > how gyp's "includes:net_common.gpyi" works. So we can't set
> > USE_ICU_ALTERNATIVES_ON_ANDROID, DISABLE_FILE_SUPPORT, or
DISABLE_FTP_SUPPORT
> in
> > net_small and expect these will be used in compiling net_common's sources.
> > 
> > Do you see a way to make this work other than duplicating the original net
> > target in its entirety?
> 
> So how do we do it in the gyp files?

We factored out the common parts in to a gypi file and we include it in the gyp
targets (see "'includes': [ 'cronet/cronet_static.gypi' ],"). This approach is
recommended by torne@ and is used in webview (see
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ap...).
But webview does not have a counterpart in GN.. I also didn't find a way to do
the same thing in GN..

[mmenke, 2015-08-20 18:10:03]

On 2015/08/20 18:04:33, xunjieli wrote:
> On 2015/08/20 18:00:39, mmenke wrote:
> > On 2015/08/20 17:55:35, xunjieli wrote:
> > > https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> > > File net/BUILD.gn (right):
> > > 
> > >
> https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode528
> > > net/BUILD.gn:528: defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > > On 2015/08/20 15:37:47, mmenke wrote:
> > > > We weren't defining this before, were we?  Do we need to add it in this
> CL?
> > > 
> > > We had this define in url/BUILD.gn: 
> > > 
> > > config("url_icu_config") {
> > >   if (use_icu_alternatives_on_android) {
> > >     defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > >   }
> > > }
> > > 
> > > USE_ICU_ALTERNATIVES_ON_ANDROID is used in
net/android/net_jni_registrar.cc.
> > > 
> > > On second thought, I think this CL won't work.
> > > source_set(net_common) does not know the defines we have in net_small,
> unlike
> > > how gyp's "includes:net_common.gpyi" works. So we can't set
> > > USE_ICU_ALTERNATIVES_ON_ANDROID, DISABLE_FILE_SUPPORT, or
> DISABLE_FTP_SUPPORT
> > in
> > > net_small and expect these will be used in compiling net_common's sources.
> > > 
> > > Do you see a way to make this work other than duplicating the original net
> > > target in its entirety?
> > 
> > So how do we do it in the gyp files?
> 
> We factored out the common parts in to a gypi file and we include it in the
gyp
> targets (see "'includes': [ 'cronet/cronet_static.gypi' ],"). This approach is
> recommended by torne@ and is used in webview (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ap...).
> But webview does not have a counterpart in GN.. I also didn't find a way to do
> the same thing in GN..

GN also has similar syntax.

e.g. sources += gypi_values.net_linux_test_sources

[xunjieli, 2015-08-20 18:17:26]

On 2015/08/20 18:10:03, mmenke wrote:
> On 2015/08/20 18:04:33, xunjieli wrote:
> > On 2015/08/20 18:00:39, mmenke wrote:
> > > On 2015/08/20 17:55:35, xunjieli wrote:
> > > > https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> > > > File net/BUILD.gn (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode528
> > > > net/BUILD.gn:528: defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > > > On 2015/08/20 15:37:47, mmenke wrote:
> > > > > We weren't defining this before, were we?  Do we need to add it in
this
> > CL?
> > > > 
> > > > We had this define in url/BUILD.gn: 
> > > > 
> > > > config("url_icu_config") {
> > > >   if (use_icu_alternatives_on_android) {
> > > >     defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > > >   }
> > > > }
> > > > 
> > > > USE_ICU_ALTERNATIVES_ON_ANDROID is used in
> net/android/net_jni_registrar.cc.
> > > > 
> > > > On second thought, I think this CL won't work.
> > > > source_set(net_common) does not know the defines we have in net_small,
> > unlike
> > > > how gyp's "includes:net_common.gpyi" works. So we can't set
> > > > USE_ICU_ALTERNATIVES_ON_ANDROID, DISABLE_FILE_SUPPORT, or
> > DISABLE_FTP_SUPPORT
> > > in
> > > > net_small and expect these will be used in compiling net_common's
sources.
> > > > 
> > > > Do you see a way to make this work other than duplicating the original
net
> > > > target in its entirety?
> > > 
> > > So how do we do it in the gyp files?
> > 
> > We factored out the common parts in to a gypi file and we include it in the
> gyp
> > targets (see "'includes': [ 'cronet/cronet_static.gypi' ],"). This approach
is
> > recommended by torne@ and is used in webview (see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ap...).
> > But webview does not have a counterpart in GN.. I also didn't find a way to
do
> > the same thing in GN..
> 
> GN also has similar syntax.
> 
> e.g. sources += gypi_values.net_linux_test_sources

Yep, but that syntax is only for an array of sources (whereas
cronet/cronet_static.gypi contains more than just the sources). If you have any
conditional, you can't simply do that to reuse logic. In BUILD.gn net target,
there is a huge block of code where we add/subtract sources depend on variables.
We can factor out the sources, but that will be extremely tedious, since there
are a bunch of them, and we have to create sources for every single one of them.

[xunjieli, 2015-08-20 18:20:03]

On 2015/08/20 18:17:26, xunjieli wrote:
> On 2015/08/20 18:10:03, mmenke wrote:
> > On 2015/08/20 18:04:33, xunjieli wrote:
> > > On 2015/08/20 18:00:39, mmenke wrote:
> > > > On 2015/08/20 17:55:35, xunjieli wrote:
> > > > > https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> > > > > File net/BUILD.gn (right):
> > > > > 
> > > > >
> > >
> https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode528
> > > > > net/BUILD.gn:528: defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > > > > On 2015/08/20 15:37:47, mmenke wrote:
> > > > > > We weren't defining this before, were we?  Do we need to add it in
> this
> > > CL?
> > > > > 
> > > > > We had this define in url/BUILD.gn: 
> > > > > 
> > > > > config("url_icu_config") {
> > > > >   if (use_icu_alternatives_on_android) {
> > > > >     defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > > > >   }
> > > > > }
> > > > > 
> > > > > USE_ICU_ALTERNATIVES_ON_ANDROID is used in
> > net/android/net_jni_registrar.cc.
> > > > > 
> > > > > On second thought, I think this CL won't work.
> > > > > source_set(net_common) does not know the defines we have in net_small,
> > > unlike
> > > > > how gyp's "includes:net_common.gpyi" works. So we can't set
> > > > > USE_ICU_ALTERNATIVES_ON_ANDROID, DISABLE_FILE_SUPPORT, or
> > > DISABLE_FTP_SUPPORT
> > > > in
> > > > > net_small and expect these will be used in compiling net_common's
> sources.
> > > > > 
> > > > > Do you see a way to make this work other than duplicating the original
> net
> > > > > target in its entirety?
> > > > 
> > > > So how do we do it in the gyp files?
> > > 
> > > We factored out the common parts in to a gypi file and we include it in
the
> > gyp
> > > targets (see "'includes': [ 'cronet/cronet_static.gypi' ],"). This
approach
> is
> > > recommended by torne@ and is used in webview (see
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ap...).
> > > But webview does not have a counterpart in GN.. I also didn't find a way
to
> do
> > > the same thing in GN..
> > 
> > GN also has similar syntax.
> > 
> > e.g. sources += gypi_values.net_linux_test_sources
> 
> Yep, but that syntax is only for an array of sources (whereas
> cronet/cronet_static.gypi contains more than just the sources). If you have
any
> conditional, you can't simply do that to reuse logic. In BUILD.gn net target,
> there is a huge block of code where we add/subtract sources depend on
variables.
> We can factor out the sources, but that will be extremely tedious, since there
> are a bunch of them, and we have to create sources for every single one of
them.

net/net_common.gypi is a better example on what we'd like to achieve here. You
can see it has quite a lot of complexity to factor out if we just use sources +=
some_sources.

[mmenke, 2015-08-20 18:38:57]

On 2015/08/20 18:20:03, xunjieli wrote:
> On 2015/08/20 18:17:26, xunjieli wrote:
> > On 2015/08/20 18:10:03, mmenke wrote:
> > > On 2015/08/20 18:04:33, xunjieli wrote:
> > > > On 2015/08/20 18:00:39, mmenke wrote:
> > > > > On 2015/08/20 17:55:35, xunjieli wrote:
> > > > > > https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> > > > > > File net/BUILD.gn (right):
> > > > > > 
> > > > > >
> > > >
> >
https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode528
> > > > > > net/BUILD.gn:528: defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > > > > > On 2015/08/20 15:37:47, mmenke wrote:
> > > > > > > We weren't defining this before, were we?  Do we need to add it in
> > this
> > > > CL?
> > > > > > 
> > > > > > We had this define in url/BUILD.gn: 
> > > > > > 
> > > > > > config("url_icu_config") {
> > > > > >   if (use_icu_alternatives_on_android) {
> > > > > >     defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > > > > >   }
> > > > > > }
> > > > > > 
> > > > > > USE_ICU_ALTERNATIVES_ON_ANDROID is used in
> > > net/android/net_jni_registrar.cc.
> > > > > > 
> > > > > > On second thought, I think this CL won't work.
> > > > > > source_set(net_common) does not know the defines we have in
net_small,
> > > > unlike
> > > > > > how gyp's "includes:net_common.gpyi" works. So we can't set
> > > > > > USE_ICU_ALTERNATIVES_ON_ANDROID, DISABLE_FILE_SUPPORT, or
> > > > DISABLE_FTP_SUPPORT
> > > > > in
> > > > > > net_small and expect these will be used in compiling net_common's
> > sources.
> > > > > > 
> > > > > > Do you see a way to make this work other than duplicating the
original
> > net
> > > > > > target in its entirety?
> > > > > 
> > > > > So how do we do it in the gyp files?
> > > > 
> > > > We factored out the common parts in to a gypi file and we include it in
> the
> > > gyp
> > > > targets (see "'includes': [ 'cronet/cronet_static.gypi' ],"). This
> approach
> > is
> > > > recommended by torne@ and is used in webview (see
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ap...).
> > > > But webview does not have a counterpart in GN.. I also didn't find a way
> to
> > do
> > > > the same thing in GN..
> > > 
> > > GN also has similar syntax.
> > > 
> > > e.g. sources += gypi_values.net_linux_test_sources
> > 
> > Yep, but that syntax is only for an array of sources (whereas
> > cronet/cronet_static.gypi contains more than just the sources). If you have
> any
> > conditional, you can't simply do that to reuse logic. In BUILD.gn net
target,
> > there is a huge block of code where we add/subtract sources depend on
> variables.
> > We can factor out the sources, but that will be extremely tedious, since
there
> > are a bunch of them, and we have to create sources for every single one of
> them.
> 
> net/net_common.gypi is a better example on what we'd like to achieve here. You
> can see it has quite a lot of complexity to factor out if we just use sources
+=
> some_sources.

Can GN configs do that sort of thing?

[xunjieli, 2015-08-20 19:03:18]

On 2015/08/20 18:38:57, mmenke wrote:
> On 2015/08/20 18:20:03, xunjieli wrote:
> > On 2015/08/20 18:17:26, xunjieli wrote:
> > > On 2015/08/20 18:10:03, mmenke wrote:
> > > > On 2015/08/20 18:04:33, xunjieli wrote:
> > > > > On 2015/08/20 18:00:39, mmenke wrote:
> > > > > > On 2015/08/20 17:55:35, xunjieli wrote:
> > > > > > > https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> > > > > > > File net/BUILD.gn (right):
> > > > > > > 
> > > > > > >
> > > > >
> > >
> https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode528
> > > > > > > net/BUILD.gn:528: defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1"
]
> > > > > > > On 2015/08/20 15:37:47, mmenke wrote:
> > > > > > > > We weren't defining this before, were we?  Do we need to add it
in
> > > this
> > > > > CL?
> > > > > > > 
> > > > > > > We had this define in url/BUILD.gn: 
> > > > > > > 
> > > > > > > config("url_icu_config") {
> > > > > > >   if (use_icu_alternatives_on_android) {
> > > > > > >     defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > > > > > >   }
> > > > > > > }
> > > > > > > 
> > > > > > > USE_ICU_ALTERNATIVES_ON_ANDROID is used in
> > > > net/android/net_jni_registrar.cc.
> > > > > > > 
> > > > > > > On second thought, I think this CL won't work.
> > > > > > > source_set(net_common) does not know the defines we have in
> net_small,
> > > > > unlike
> > > > > > > how gyp's "includes:net_common.gpyi" works. So we can't set
> > > > > > > USE_ICU_ALTERNATIVES_ON_ANDROID, DISABLE_FILE_SUPPORT, or
> > > > > DISABLE_FTP_SUPPORT
> > > > > > in
> > > > > > > net_small and expect these will be used in compiling net_common's
> > > sources.
> > > > > > > 
> > > > > > > Do you see a way to make this work other than duplicating the
> original
> > > net
> > > > > > > target in its entirety?
> > > > > > 
> > > > > > So how do we do it in the gyp files?
> > > > > 
> > > > > We factored out the common parts in to a gypi file and we include it
in
> > the
> > > > gyp
> > > > > targets (see "'includes': [ 'cronet/cronet_static.gypi' ],"). This
> > approach
> > > is
> > > > > recommended by torne@ and is used in webview (see
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ap...).
> > > > > But webview does not have a counterpart in GN.. I also didn't find a
way
> > to
> > > do
> > > > > the same thing in GN..
> > > > 
> > > > GN also has similar syntax.
> > > > 
> > > > e.g. sources += gypi_values.net_linux_test_sources
> > > 
> > > Yep, but that syntax is only for an array of sources (whereas
> > > cronet/cronet_static.gypi contains more than just the sources). If you
have
> > any
> > > conditional, you can't simply do that to reuse logic. In BUILD.gn net
> target,
> > > there is a huge block of code where we add/subtract sources depend on
> > variables.
> > > We can factor out the sources, but that will be extremely tedious, since
> there
> > > are a bunch of them, and we have to create sources for every single one of
> > them.
> > 
> > net/net_common.gypi is a better example on what we'd like to achieve here.
You
> > can see it has quite a lot of complexity to factor out if we just use
sources
> +=
> > some_sources.
> 
> Can GN configs do that sort of thing?

I just tried making net_common as a config, but i seems that config does not
recognize "sources" as special variables:

ERROR at //net/BUILD.gn:495:5: Assignment had no effect.
    sources += [
    ^-----------
You set the variable "sources" here and it was unused before it went
out of scope.

[mmenke, 2015-08-20 19:04:36]

On 2015/08/20 19:03:18, xunjieli wrote:
> On 2015/08/20 18:38:57, mmenke wrote:
> > On 2015/08/20 18:20:03, xunjieli wrote:
> > > On 2015/08/20 18:17:26, xunjieli wrote:
> > > > On 2015/08/20 18:10:03, mmenke wrote:
> > > > > On 2015/08/20 18:04:33, xunjieli wrote:
> > > > > > On 2015/08/20 18:00:39, mmenke wrote:
> > > > > > > On 2015/08/20 17:55:35, xunjieli wrote:
> > > > > > > >
https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> > > > > > > > File net/BUILD.gn (right):
> > > > > > > > 
> > > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode528
> > > > > > > > net/BUILD.gn:528: defines = [
"USE_ICU_ALTERNATIVES_ON_ANDROID=1"
> ]
> > > > > > > > On 2015/08/20 15:37:47, mmenke wrote:
> > > > > > > > > We weren't defining this before, were we?  Do we need to add
it
> in
> > > > this
> > > > > > CL?
> > > > > > > > 
> > > > > > > > We had this define in url/BUILD.gn: 
> > > > > > > > 
> > > > > > > > config("url_icu_config") {
> > > > > > > >   if (use_icu_alternatives_on_android) {
> > > > > > > >     defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > > > > > > >   }
> > > > > > > > }
> > > > > > > > 
> > > > > > > > USE_ICU_ALTERNATIVES_ON_ANDROID is used in
> > > > > net/android/net_jni_registrar.cc.
> > > > > > > > 
> > > > > > > > On second thought, I think this CL won't work.
> > > > > > > > source_set(net_common) does not know the defines we have in
> > net_small,
> > > > > > unlike
> > > > > > > > how gyp's "includes:net_common.gpyi" works. So we can't set
> > > > > > > > USE_ICU_ALTERNATIVES_ON_ANDROID, DISABLE_FILE_SUPPORT, or
> > > > > > DISABLE_FTP_SUPPORT
> > > > > > > in
> > > > > > > > net_small and expect these will be used in compiling
net_common's
> > > > sources.
> > > > > > > > 
> > > > > > > > Do you see a way to make this work other than duplicating the
> > original
> > > > net
> > > > > > > > target in its entirety?
> > > > > > > 
> > > > > > > So how do we do it in the gyp files?
> > > > > > 
> > > > > > We factored out the common parts in to a gypi file and we include it
> in
> > > the
> > > > > gyp
> > > > > > targets (see "'includes': [ 'cronet/cronet_static.gypi' ],"). This
> > > approach
> > > > is
> > > > > > recommended by torne@ and is used in webview (see
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ap...).
> > > > > > But webview does not have a counterpart in GN.. I also didn't find a
> way
> > > to
> > > > do
> > > > > > the same thing in GN..
> > > > > 
> > > > > GN also has similar syntax.
> > > > > 
> > > > > e.g. sources += gypi_values.net_linux_test_sources
> > > > 
> > > > Yep, but that syntax is only for an array of sources (whereas
> > > > cronet/cronet_static.gypi contains more than just the sources). If you
> have
> > > any
> > > > conditional, you can't simply do that to reuse logic. In BUILD.gn net
> > target,
> > > > there is a huge block of code where we add/subtract sources depend on
> > > variables.
> > > > We can factor out the sources, but that will be extremely tedious, since
> > there
> > > > are a bunch of them, and we have to create sources for every single one
of
> > > them.
> > > 
> > > net/net_common.gypi is a better example on what we'd like to achieve here.
> You
> > > can see it has quite a lot of complexity to factor out if we just use
> sources
> > +=
> > > some_sources.
> > 
> > Can GN configs do that sort of thing?
> 
> I just tried making net_common as a config, but i seems that config does not
> recognize "sources" as special variables:
> 
> ERROR at //net/BUILD.gn:495:5: Assignment had no effect.
>     sources += [
>     ^-----------
> You set the variable "sources" here and it was unused before it went
> out of scope.

I assume using "sources =" doesn't fix the problem.  :(

[xunjieli, 2015-08-20 19:06:50]

On 2015/08/20 19:04:36, mmenke wrote:
> On 2015/08/20 19:03:18, xunjieli wrote:
> > On 2015/08/20 18:38:57, mmenke wrote:
> > > On 2015/08/20 18:20:03, xunjieli wrote:
> > > > On 2015/08/20 18:17:26, xunjieli wrote:
> > > > > On 2015/08/20 18:10:03, mmenke wrote:
> > > > > > On 2015/08/20 18:04:33, xunjieli wrote:
> > > > > > > On 2015/08/20 18:00:39, mmenke wrote:
> > > > > > > > On 2015/08/20 17:55:35, xunjieli wrote:
> > > > > > > > >
> https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> > > > > > > > > File net/BUILD.gn (right):
> > > > > > > > > 
> > > > > > > > >
> > > > > > >
> > > > >
> > >
> https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode528
> > > > > > > > > net/BUILD.gn:528: defines = [
> "USE_ICU_ALTERNATIVES_ON_ANDROID=1"
> > ]
> > > > > > > > > On 2015/08/20 15:37:47, mmenke wrote:
> > > > > > > > > > We weren't defining this before, were we?  Do we need to add
> it
> > in
> > > > > this
> > > > > > > CL?
> > > > > > > > > 
> > > > > > > > > We had this define in url/BUILD.gn: 
> > > > > > > > > 
> > > > > > > > > config("url_icu_config") {
> > > > > > > > >   if (use_icu_alternatives_on_android) {
> > > > > > > > >     defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > > > > > > > >   }
> > > > > > > > > }
> > > > > > > > > 
> > > > > > > > > USE_ICU_ALTERNATIVES_ON_ANDROID is used in
> > > > > > net/android/net_jni_registrar.cc.
> > > > > > > > > 
> > > > > > > > > On second thought, I think this CL won't work.
> > > > > > > > > source_set(net_common) does not know the defines we have in
> > > net_small,
> > > > > > > unlike
> > > > > > > > > how gyp's "includes:net_common.gpyi" works. So we can't set
> > > > > > > > > USE_ICU_ALTERNATIVES_ON_ANDROID, DISABLE_FILE_SUPPORT, or
> > > > > > > DISABLE_FTP_SUPPORT
> > > > > > > > in
> > > > > > > > > net_small and expect these will be used in compiling
> net_common's
> > > > > sources.
> > > > > > > > > 
> > > > > > > > > Do you see a way to make this work other than duplicating the
> > > original
> > > > > net
> > > > > > > > > target in its entirety?
> > > > > > > > 
> > > > > > > > So how do we do it in the gyp files?
> > > > > > > 
> > > > > > > We factored out the common parts in to a gypi file and we include
it
> > in
> > > > the
> > > > > > gyp
> > > > > > > targets (see "'includes': [ 'cronet/cronet_static.gypi' ],"). This
> > > > approach
> > > > > is
> > > > > > > recommended by torne@ and is used in webview (see
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ap...).
> > > > > > > But webview does not have a counterpart in GN.. I also didn't find
a
> > way
> > > > to
> > > > > do
> > > > > > > the same thing in GN..
> > > > > > 
> > > > > > GN also has similar syntax.
> > > > > > 
> > > > > > e.g. sources += gypi_values.net_linux_test_sources
> > > > > 
> > > > > Yep, but that syntax is only for an array of sources (whereas
> > > > > cronet/cronet_static.gypi contains more than just the sources). If you
> > have
> > > > any
> > > > > conditional, you can't simply do that to reuse logic. In BUILD.gn net
> > > target,
> > > > > there is a huge block of code where we add/subtract sources depend on
> > > > variables.
> > > > > We can factor out the sources, but that will be extremely tedious,
since
> > > there
> > > > > are a bunch of them, and we have to create sources for every single
one
> of
> > > > them.
> > > > 
> > > > net/net_common.gypi is a better example on what we'd like to achieve
here.
> > You
> > > > can see it has quite a lot of complexity to factor out if we just use
> > sources
> > > +=
> > > > some_sources.
> > > 
> > > Can GN configs do that sort of thing?
> > 
> > I just tried making net_common as a config, but i seems that config does not
> > recognize "sources" as special variables:
> > 
> > ERROR at //net/BUILD.gn:495:5: Assignment had no effect.
> >     sources += [
> >     ^-----------
> > You set the variable "sources" here and it was unused before it went
> > out of scope.
> 
> I assume using "sources =" doesn't fix the problem.  :(

There is a "sources = " at the start of net_common. I think GN just think it is
a local variable.

[mmenke, 2015-08-20 19:13:01]

On 2015/08/20 19:06:50, xunjieli wrote:
> On 2015/08/20 19:04:36, mmenke wrote:
> > On 2015/08/20 19:03:18, xunjieli wrote:
> > > On 2015/08/20 18:38:57, mmenke wrote:
> > > > On 2015/08/20 18:20:03, xunjieli wrote:
> > > > > On 2015/08/20 18:17:26, xunjieli wrote:
> > > > > > On 2015/08/20 18:10:03, mmenke wrote:
> > > > > > > On 2015/08/20 18:04:33, xunjieli wrote:
> > > > > > > > On 2015/08/20 18:00:39, mmenke wrote:
> > > > > > > > > On 2015/08/20 17:55:35, xunjieli wrote:
> > > > > > > > > >
> > https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> > > > > > > > > > File net/BUILD.gn (right):
> > > > > > > > > > 
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode528
> > > > > > > > > > net/BUILD.gn:528: defines = [
> > "USE_ICU_ALTERNATIVES_ON_ANDROID=1"
> > > ]
> > > > > > > > > > On 2015/08/20 15:37:47, mmenke wrote:
> > > > > > > > > > > We weren't defining this before, were we?  Do we need to
add
> > it
> > > in
> > > > > > this
> > > > > > > > CL?
> > > > > > > > > > 
> > > > > > > > > > We had this define in url/BUILD.gn: 
> > > > > > > > > > 
> > > > > > > > > > config("url_icu_config") {
> > > > > > > > > >   if (use_icu_alternatives_on_android) {
> > > > > > > > > >     defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > > > > > > > > >   }
> > > > > > > > > > }
> > > > > > > > > > 
> > > > > > > > > > USE_ICU_ALTERNATIVES_ON_ANDROID is used in
> > > > > > > net/android/net_jni_registrar.cc.
> > > > > > > > > > 
> > > > > > > > > > On second thought, I think this CL won't work.
> > > > > > > > > > source_set(net_common) does not know the defines we have in
> > > > net_small,
> > > > > > > > unlike
> > > > > > > > > > how gyp's "includes:net_common.gpyi" works. So we can't set
> > > > > > > > > > USE_ICU_ALTERNATIVES_ON_ANDROID, DISABLE_FILE_SUPPORT, or
> > > > > > > > DISABLE_FTP_SUPPORT
> > > > > > > > > in
> > > > > > > > > > net_small and expect these will be used in compiling
> > net_common's
> > > > > > sources.
> > > > > > > > > > 
> > > > > > > > > > Do you see a way to make this work other than duplicating
the
> > > > original
> > > > > > net
> > > > > > > > > > target in its entirety?
> > > > > > > > > 
> > > > > > > > > So how do we do it in the gyp files?
> > > > > > > > 
> > > > > > > > We factored out the common parts in to a gypi file and we
include
> it
> > > in
> > > > > the
> > > > > > > gyp
> > > > > > > > targets (see "'includes': [ 'cronet/cronet_static.gypi' ],").
This
> > > > > approach
> > > > > > is
> > > > > > > > recommended by torne@ and is used in webview (see
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ap...).
> > > > > > > > But webview does not have a counterpart in GN.. I also didn't
find
> a
> > > way
> > > > > to
> > > > > > do
> > > > > > > > the same thing in GN..
> > > > > > > 
> > > > > > > GN also has similar syntax.
> > > > > > > 
> > > > > > > e.g. sources += gypi_values.net_linux_test_sources
> > > > > > 
> > > > > > Yep, but that syntax is only for an array of sources (whereas
> > > > > > cronet/cronet_static.gypi contains more than just the sources). If
you
> > > have
> > > > > any
> > > > > > conditional, you can't simply do that to reuse logic. In BUILD.gn
net
> > > > target,
> > > > > > there is a huge block of code where we add/subtract sources depend
on
> > > > > variables.
> > > > > > We can factor out the sources, but that will be extremely tedious,
> since
> > > > there
> > > > > > are a bunch of them, and we have to create sources for every single
> one
> > of
> > > > > them.
> > > > > 
> > > > > net/net_common.gypi is a better example on what we'd like to achieve
> here.
> > > You
> > > > > can see it has quite a lot of complexity to factor out if we just use
> > > sources
> > > > +=
> > > > > some_sources.
> > > > 
> > > > Can GN configs do that sort of thing?
> > > 
> > > I just tried making net_common as a config, but i seems that config does
not
> > > recognize "sources" as special variables:
> > > 
> > > ERROR at //net/BUILD.gn:495:5: Assignment had no effect.
> > >     sources += [
> > >     ^-----------
> > > You set the variable "sources" here and it was unused before it went
> > > out of scope.
> > 
> > I assume using "sources =" doesn't fix the problem.  :(
> 
> There is a "sources = " at the start of net_common. I think GN just think it
is
> a local variable.

Right, but configs aren't the same as implicitly including all the lines of the
config in the target - they have their own variables, which at the end are added
to the including source...  At least that's how it looks like they work to me. 
I see no GN config that uses +=, except to something it already created with an
"=" line earlier.

[mmenke, 2015-08-20 19:14:57]

On 2015/08/20 19:13:01, mmenke wrote:
> On 2015/08/20 19:06:50, xunjieli wrote:
> > On 2015/08/20 19:04:36, mmenke wrote:
> > > On 2015/08/20 19:03:18, xunjieli wrote:
> > > > On 2015/08/20 18:38:57, mmenke wrote:
> > > > > On 2015/08/20 18:20:03, xunjieli wrote:
> > > > > > On 2015/08/20 18:17:26, xunjieli wrote:
> > > > > > > On 2015/08/20 18:10:03, mmenke wrote:
> > > > > > > > On 2015/08/20 18:04:33, xunjieli wrote:
> > > > > > > > > On 2015/08/20 18:00:39, mmenke wrote:
> > > > > > > > > > On 2015/08/20 17:55:35, xunjieli wrote:
> > > > > > > > > > >
> > > https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> > > > > > > > > > > File net/BUILD.gn (right):
> > > > > > > > > > > 
> > > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > >
> > >
> https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode528
> > > > > > > > > > > net/BUILD.gn:528: defines = [
> > > "USE_ICU_ALTERNATIVES_ON_ANDROID=1"
> > > > ]
> > > > > > > > > > > On 2015/08/20 15:37:47, mmenke wrote:
> > > > > > > > > > > > We weren't defining this before, were we?  Do we need to
> add
> > > it
> > > > in
> > > > > > > this
> > > > > > > > > CL?
> > > > > > > > > > > 
> > > > > > > > > > > We had this define in url/BUILD.gn: 
> > > > > > > > > > > 
> > > > > > > > > > > config("url_icu_config") {
> > > > > > > > > > >   if (use_icu_alternatives_on_android) {
> > > > > > > > > > >     defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > > > > > > > > > >   }
> > > > > > > > > > > }
> > > > > > > > > > > 
> > > > > > > > > > > USE_ICU_ALTERNATIVES_ON_ANDROID is used in
> > > > > > > > net/android/net_jni_registrar.cc.
> > > > > > > > > > > 
> > > > > > > > > > > On second thought, I think this CL won't work.
> > > > > > > > > > > source_set(net_common) does not know the defines we have
in
> > > > > net_small,
> > > > > > > > > unlike
> > > > > > > > > > > how gyp's "includes:net_common.gpyi" works. So we can't
set
> > > > > > > > > > > USE_ICU_ALTERNATIVES_ON_ANDROID, DISABLE_FILE_SUPPORT, or
> > > > > > > > > DISABLE_FTP_SUPPORT
> > > > > > > > > > in
> > > > > > > > > > > net_small and expect these will be used in compiling
> > > net_common's
> > > > > > > sources.
> > > > > > > > > > > 
> > > > > > > > > > > Do you see a way to make this work other than duplicating
> the
> > > > > original
> > > > > > > net
> > > > > > > > > > > target in its entirety?
> > > > > > > > > > 
> > > > > > > > > > So how do we do it in the gyp files?
> > > > > > > > > 
> > > > > > > > > We factored out the common parts in to a gypi file and we
> include
> > it
> > > > in
> > > > > > the
> > > > > > > > gyp
> > > > > > > > > targets (see "'includes': [ 'cronet/cronet_static.gypi' ],").
> This
> > > > > > approach
> > > > > > > is
> > > > > > > > > recommended by torne@ and is used in webview (see
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ap...).
> > > > > > > > > But webview does not have a counterpart in GN.. I also didn't
> find
> > a
> > > > way
> > > > > > to
> > > > > > > do
> > > > > > > > > the same thing in GN..
> > > > > > > > 
> > > > > > > > GN also has similar syntax.
> > > > > > > > 
> > > > > > > > e.g. sources += gypi_values.net_linux_test_sources
> > > > > > > 
> > > > > > > Yep, but that syntax is only for an array of sources (whereas
> > > > > > > cronet/cronet_static.gypi contains more than just the sources). If
> you
> > > > have
> > > > > > any
> > > > > > > conditional, you can't simply do that to reuse logic. In BUILD.gn
> net
> > > > > target,
> > > > > > > there is a huge block of code where we add/subtract sources depend
> on
> > > > > > variables.
> > > > > > > We can factor out the sources, but that will be extremely tedious,
> > since
> > > > > there
> > > > > > > are a bunch of them, and we have to create sources for every
single
> > one
> > > of
> > > > > > them.
> > > > > > 
> > > > > > net/net_common.gypi is a better example on what we'd like to achieve
> > here.
> > > > You
> > > > > > can see it has quite a lot of complexity to factor out if we just
use
> > > > sources
> > > > > +=
> > > > > > some_sources.
> > > > > 
> > > > > Can GN configs do that sort of thing?
> > > > 
> > > > I just tried making net_common as a config, but i seems that config does
> not
> > > > recognize "sources" as special variables:
> > > > 
> > > > ERROR at //net/BUILD.gn:495:5: Assignment had no effect.
> > > >     sources += [
> > > >     ^-----------
> > > > You set the variable "sources" here and it was unused before it went
> > > > out of scope.
> > > 
> > > I assume using "sources =" doesn't fix the problem.  :(
> > 
> > There is a "sources = " at the start of net_common. I think GN just think it
> is
> > a local variable.
> 
> Right, but configs aren't the same as implicitly including all the lines of
the
> config in the target - they have their own variables, which at the end are
added
> to the including source...  At least that's how it looks like they work to me.

> I see no GN config that uses +=, except to something it already created with
an
> "=" line earlier.

See, for example:

https://code.google.com/p/chromium/codesearch#chromium/src/build/config/mac/B...
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD...

(Both mac_dynamic_flags and mac_executable_flags use ldflags=, but they are
combined in some projects)

[xunjieli, 2015-08-20 19:47:00]

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

[xunjieli, 2015-08-20 19:47:42]

On 2015/08/20 19:14:57, mmenke wrote:
> On 2015/08/20 19:13:01, mmenke wrote:
> > On 2015/08/20 19:06:50, xunjieli wrote:
> > > On 2015/08/20 19:04:36, mmenke wrote:
> > > > On 2015/08/20 19:03:18, xunjieli wrote:
> > > > > On 2015/08/20 18:38:57, mmenke wrote:
> > > > > > On 2015/08/20 18:20:03, xunjieli wrote:
> > > > > > > On 2015/08/20 18:17:26, xunjieli wrote:
> > > > > > > > On 2015/08/20 18:10:03, mmenke wrote:
> > > > > > > > > On 2015/08/20 18:04:33, xunjieli wrote:
> > > > > > > > > > On 2015/08/20 18:00:39, mmenke wrote:
> > > > > > > > > > > On 2015/08/20 17:55:35, xunjieli wrote:
> > > > > > > > > > > >
> > > > https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> > > > > > > > > > > > File net/BUILD.gn (right):
> > > > > > > > > > > > 
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode528
> > > > > > > > > > > > net/BUILD.gn:528: defines = [
> > > > "USE_ICU_ALTERNATIVES_ON_ANDROID=1"
> > > > > ]
> > > > > > > > > > > > On 2015/08/20 15:37:47, mmenke wrote:
> > > > > > > > > > > > > We weren't defining this before, were we?  Do we need
to
> > add
> > > > it
> > > > > in
> > > > > > > > this
> > > > > > > > > > CL?
> > > > > > > > > > > > 
> > > > > > > > > > > > We had this define in url/BUILD.gn: 
> > > > > > > > > > > > 
> > > > > > > > > > > > config("url_icu_config") {
> > > > > > > > > > > >   if (use_icu_alternatives_on_android) {
> > > > > > > > > > > >     defines = [ "USE_ICU_ALTERNATIVES_ON_ANDROID=1" ]
> > > > > > > > > > > >   }
> > > > > > > > > > > > }
> > > > > > > > > > > > 
> > > > > > > > > > > > USE_ICU_ALTERNATIVES_ON_ANDROID is used in
> > > > > > > > > net/android/net_jni_registrar.cc.
> > > > > > > > > > > > 
> > > > > > > > > > > > On second thought, I think this CL won't work.
> > > > > > > > > > > > source_set(net_common) does not know the defines we have
> in
> > > > > > net_small,
> > > > > > > > > > unlike
> > > > > > > > > > > > how gyp's "includes:net_common.gpyi" works. So we can't
> set
> > > > > > > > > > > > USE_ICU_ALTERNATIVES_ON_ANDROID, DISABLE_FILE_SUPPORT,
or
> > > > > > > > > > DISABLE_FTP_SUPPORT
> > > > > > > > > > > in
> > > > > > > > > > > > net_small and expect these will be used in compiling
> > > > net_common's
> > > > > > > > sources.
> > > > > > > > > > > > 
> > > > > > > > > > > > Do you see a way to make this work other than
duplicating
> > the
> > > > > > original
> > > > > > > > net
> > > > > > > > > > > > target in its entirety?
> > > > > > > > > > > 
> > > > > > > > > > > So how do we do it in the gyp files?
> > > > > > > > > > 
> > > > > > > > > > We factored out the common parts in to a gypi file and we
> > include
> > > it
> > > > > in
> > > > > > > the
> > > > > > > > > gyp
> > > > > > > > > > targets (see "'includes': [ 'cronet/cronet_static.gypi'
],").
> > This
> > > > > > > approach
> > > > > > > > is
> > > > > > > > > > recommended by torne@ and is used in webview (see
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ap...).
> > > > > > > > > > But webview does not have a counterpart in GN.. I also
didn't
> > find
> > > a
> > > > > way
> > > > > > > to
> > > > > > > > do
> > > > > > > > > > the same thing in GN..
> > > > > > > > > 
> > > > > > > > > GN also has similar syntax.
> > > > > > > > > 
> > > > > > > > > e.g. sources += gypi_values.net_linux_test_sources
> > > > > > > > 
> > > > > > > > Yep, but that syntax is only for an array of sources (whereas
> > > > > > > > cronet/cronet_static.gypi contains more than just the sources).
If
> > you
> > > > > have
> > > > > > > any
> > > > > > > > conditional, you can't simply do that to reuse logic. In
BUILD.gn
> > net
> > > > > > target,
> > > > > > > > there is a huge block of code where we add/subtract sources
depend
> > on
> > > > > > > variables.
> > > > > > > > We can factor out the sources, but that will be extremely
tedious,
> > > since
> > > > > > there
> > > > > > > > are a bunch of them, and we have to create sources for every
> single
> > > one
> > > > of
> > > > > > > them.
> > > > > > > 
> > > > > > > net/net_common.gypi is a better example on what we'd like to
achieve
> > > here.
> > > > > You
> > > > > > > can see it has quite a lot of complexity to factor out if we just
> use
> > > > > sources
> > > > > > +=
> > > > > > > some_sources.
> > > > > > 
> > > > > > Can GN configs do that sort of thing?
> > > > > 
> > > > > I just tried making net_common as a config, but i seems that config
does
> > not
> > > > > recognize "sources" as special variables:
> > > > > 
> > > > > ERROR at //net/BUILD.gn:495:5: Assignment had no effect.
> > > > >     sources += [
> > > > >     ^-----------
> > > > > You set the variable "sources" here and it was unused before it went
> > > > > out of scope.
> > > > 
> > > > I assume using "sources =" doesn't fix the problem.  :(
> > > 
> > > There is a "sources = " at the start of net_common. I think GN just think
it
> > is
> > > a local variable.
> > 
> > Right, but configs aren't the same as implicitly including all the lines of
> the
> > config in the target - they have their own variables, which at the end are
> added
> > to the including source...  At least that's how it looks like they work to
me.
> 
> > I see no GN config that uses +=, except to something it already created with
> an
> > "=" line earlier.
> 
> See, for example:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/mac/B...
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD...
> 
> (Both mac_dynamic_flags and mac_executable_flags use ldflags=, but they are
> combined in some projects)

+ brettw,

Hi Brett,

Misha (mef@) a while ago made new net and url targets without ICU support in
https://codereview.chromium.org/933293003. I am trying to do the same for our GN
files here. But my approach does not work quite right. I was trying to use
"source_set" to share code between net and net_small. However, the prepocessor
defines in net_small won't get included when building the source_set,
net_common. I also tried GN's "config", which does not seem to serve the purpose
of sharing sources.

Do you know a way to sharing code between net and net_small in GN?

[brettw, 2015-08-21 05:08:23]

I'm assuming with the following answer that you want to have *both* net and
net_small compiled at the same time, and you need have different #defines for
each one (such that each file needs to be compiled twice).

The easiest way to do this is to define the target twice but precompute
everything you need before doing so:

net_shared_sources = [
  "foo.cc",
  ...
]
 ...all conditions on net sources go here...
net_shared_deps = [
  "//url"
  ...
]

component("net") {
  sources = net_shared_sources
  deps = net_shared_deps
  ... other stuff you computed ...
}

component("net_small") {
  sources = net_shared_sources
  deps = net_shared_deps
  ... other stuff you computed ...
  ...tweak defines...
}

[xunjieli, 2015-08-21 16:38:15]

Patchset #3 (id:80001) has been deleted

[xunjieli, 2015-08-21 16:40:21]

Patchset #3 (id:100001) has been deleted

[xunjieli, 2015-08-21 19:17:25]

Thanks, Brett! that's very smart! I modified my CL. I think the new approach
works. 

Matt and Brett, PTAL.

https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode526
net/BUILD.gn:526: # same as net, but with ICU dependency stripped.
On 2015/08/20 15:37:47, mmenke wrote:
> nit:  same -> Same

Done.

[mmenke, 2015-08-21 19:32:49]

On 2015/08/21 19:17:25, xunjieli wrote:
> Thanks, Brett! that's very smart! I modified my CL. I think the new approach
> works. 
> 
> Matt and Brett, PTAL.
> 
> https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn
> File net/BUILD.gn (right):
> 
> https://codereview.chromium.org/1287893005/diff/60001/net/BUILD.gn#newcode526
> net/BUILD.gn:526: # same as net, but with ICU dependency stripped.
> On 2015/08/20 15:37:47, mmenke wrote:
> > nit:  same -> Same
> 
> Done.

I'll get to this on Monday

[brettw, 2015-08-24 20:51:37]

https://codereview.chromium.org/1287893005/diff/140001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/140001/net/BUILD.gn#newcode63
net/BUILD.gn:63: }
Here, can you add another config:
  config("net_internal_config") {
  }
and move all of the shared defines and include dirs from below into it? Then you
can just reference this config from the shared_configs variable once.

https://codereview.chromium.org/1287893005/diff/140001/net/BUILD.gn#newcode66
net/BUILD.gn:66: _net_shared_sources =
I'd delete the leading underscores. They don't actually do anything in this
context. Leading underscores are used in .gni files to indicate temporaries that
shouldn't be exported, but that's all.

https://codereview.chromium.org/1287893005/diff/140001/net/BUILD.gn#newcode69
net/BUILD.gn:69: _net_shared_public_deps = [
CAn you move this to right before the _deps?

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

https://codereview.chromium.org/1287893005/diff/140001/net/net.gypi#newcode1870
net/net.gypi:1870: 'net_file_support_sources': [
If you want to do this, remove the corresponding files from the main list of
sources, and include the *_sources in both the GN and GYP builds in the opposite
conditions that they're currently excluded. Listing each file only once rather
than adding it and then removing it makes it much easier to track what's going
on.

The GN build used the exclusions to match the GYP build, but when we clean
things up to share the lists, the additions are much easier to follow.

https://codereview.chromium.org/1287893005/diff/140001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/140001/url/BUILD.gn#newcode12
url/BUILD.gn:12: url_srcs = exec_script("//build/gypi_to_gn.py",
There's no reason to shave the file lists here. Can you put them back to the old
way and remove the change in .gn? We should only be sharing file lists for long
and frequently updated things, otherwise it just makes everything slower and
more difficult to understand.

https://codereview.chromium.org/1287893005/diff/140001/url/BUILD.gn#newcode49
url/BUILD.gn:49: if (is_android) {
Can you combine this with the previous block?

https://codereview.chromium.org/1287893005/diff/140001/url/BUILD.gn#newcode73
url/BUILD.gn:73: ":url_jni_headers",
This goes first.

[xunjieli, 2015-08-25 22:15:02]

Patchset #5 (id:160001) has been deleted

[xunjieli, 2015-08-25 22:18:01]

Patchset #5 (id:180001) has been deleted

[xunjieli, 2015-08-25 22:46:25]

Patchset #5 (id:200001) has been deleted

[xunjieli, 2015-08-26 13:55:17]

Patchset #5 (id:220001) has been deleted

[xunjieli, 2015-08-26 13:58:10]

Thanks a lot for the reviews! PTAL.

https://codereview.chromium.org/1287893005/diff/140001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/140001/net/BUILD.gn#newcode63
net/BUILD.gn:63: }
On 2015/08/24 20:51:37, brettw wrote:
> Here, can you add another config:
>   config("net_internal_config") {
>   }
> and move all of the shared defines and include dirs from below into it? Then
you
> can just reference this config from the shared_configs variable once.

Done.

https://codereview.chromium.org/1287893005/diff/140001/net/BUILD.gn#newcode66
net/BUILD.gn:66: _net_shared_sources =
On 2015/08/24 20:51:37, brettw wrote:
> I'd delete the leading underscores. They don't actually do anything in this
> context. Leading underscores are used in .gni files to indicate temporaries
that
> shouldn't be exported, but that's all.

Done.

https://codereview.chromium.org/1287893005/diff/140001/net/BUILD.gn#newcode69
net/BUILD.gn:69: _net_shared_public_deps = [
On 2015/08/24 20:51:37, brettw wrote:
> CAn you move this to right before the _deps?

Done.

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

https://codereview.chromium.org/1287893005/diff/140001/net/net.gypi#newcode1870
net/net.gypi:1870: 'net_file_support_sources': [
On 2015/08/24 20:51:37, brettw wrote:
> If you want to do this, remove the corresponding files from the main list of
> sources, and include the *_sources in both the GN and GYP builds in the
opposite
> conditions that they're currently excluded. Listing each file only once rather
> than adding it and then removing it makes it much easier to track what's going
> on.
> 
> The GN build used the exclusions to match the GYP build, but when we clean
> things up to share the lists, the additions are much easier to follow.

Done.

https://codereview.chromium.org/1287893005/diff/140001/url/BUILD.gn
File url/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/140001/url/BUILD.gn#newcode12
url/BUILD.gn:12: url_srcs = exec_script("//build/gypi_to_gn.py",
On 2015/08/24 20:51:37, brettw wrote:
> There's no reason to shave the file lists here. Can you put them back to the
old
> way and remove the change in .gn? We should only be sharing file lists for
long
> and frequently updated things, otherwise it just makes everything slower and
> more difficult to understand.

Done. Good to know. thanks!

https://codereview.chromium.org/1287893005/diff/140001/url/BUILD.gn#newcode49
url/BUILD.gn:49: if (is_android) {
On 2015/08/24 20:51:37, brettw wrote:
> Can you combine this with the previous block?

Done.

https://codereview.chromium.org/1287893005/diff/140001/url/BUILD.gn#newcode73
url/BUILD.gn:73: ":url_jni_headers",
On 2015/08/24 20:51:37, brettw wrote:
> This goes first.

Done.

[mmenke, 2015-08-26 17:53:33]

https://codereview.chromium.org/1287893005/diff/240001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/240001/net/BUILD.gn#newcode366
net/BUILD.gn:366: set_sources_assignment_filter(sources_assignment_filter)
You're duplicating most of this block below.  I believe this either needs to be
inlined in the component, or you need to add a source list
(net_shared_unfiltered_sources?  override_filter_sources?)

https://codereview.chromium.org/1287893005/diff/240001/net/BUILD.gn#newcode430
net/BUILD.gn:430: ]
Most of this block appears in both the shared scope and in this component.

[brettw, 2015-08-26 22:09:03]

GN files LGTM. I'm not reviewing the actual logic.

https://codereview.chromium.org/1287893005/diff/280001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/280001/net/BUILD.gn#newcode414
net/BUILD.gn:414: public_deps = net_shared_public_deps + [ "//url:url" ]
Just write "//url" here.

[xunjieli, 2015-08-27 13:04:42]

Thanks for the review! Matt, PTAL.

https://codereview.chromium.org/1287893005/diff/240001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/240001/net/BUILD.gn#newcode366
net/BUILD.gn:366: set_sources_assignment_filter(sources_assignment_filter)
On 2015/08/26 17:53:33, mmenke wrote:
> You're duplicating most of this block below.  I believe this either needs to
be
> inlined in the component, or you need to add a source list
> (net_shared_unfiltered_sources?  override_filter_sources?)

Done.

https://codereview.chromium.org/1287893005/diff/240001/net/BUILD.gn#newcode430
net/BUILD.gn:430: ]
On 2015/08/26 17:53:33, mmenke wrote:
> Most of this block appears in both the shared scope and in this component.

Done.

https://codereview.chromium.org/1287893005/diff/280001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1287893005/diff/280001/net/BUILD.gn#newcode414
net/BUILD.gn:414: public_deps = net_shared_public_deps + [ "//url:url" ]
On 2015/08/26 22:09:03, brettw wrote:
> Just write "//url" here.

Done.

[mmenke, 2015-08-27 15:53:37]

LGTM!

[xunjieli, 2015-08-27 15:57:23]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2015-08-27 15:57:23]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1287893005/#ps300001
(title: "Use //url instead of //url:url")

[commit-bot: I haz the power, 2015-08-27 15:57:32]

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

[commit-bot: I haz the power, 2015-08-27 16:02:50]

Committed patchset #8 (id:300001)

[commit-bot: I haz the power, 2015-08-27 16:03:38]

Patchset 8 (id:??) landed as
https://crrev.com/4c8c6921ca6739d16f6551635328faf164848f9d
Cr-Commit-Position: refs/heads/master@{#345891}

[xunjieli, 2015-08-27 17:11:31]

A revert of this CL (patchset #8 id:300001) has been created in
https://codereview.chromium.org/1312583006/ by xunjieli@chromium.org.

The reason for reverting is: This CL broke a GN check on an internal bot:
https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20GN/b...

Running ['/b/build/slave/Android_GN/build/src/buildtools/linux64/gn',
'--root=/b/build/slave/Android_GN/build/src', 'check', '//out/Release']
ERROR at //url/android/url_jni_registrar.cc:8:11: Include not allowed.
#include "url/url_canon_icu_alternatives_android.h"
          ^---------------------------------------
It is not in any dependency of
  //url:url
The include file is in the target(s):
  //url:url_lib_use_icu_alternatives_on_android
which should somehow be reachable.
Command ['/b/build/slave/Android_GN/build/src/buildtools/linux64/gn',
'--root=/b/build/slave/Android_GN/build/src', 'check', '//out/Release'] returned
exit code 1
.

[dewittj, 2015-08-27 17:13:16]

A revert of this CL (patchset #8 id:300001) has been created in
https://codereview.chromium.org/1320933002/ by dewittj@chromium.org.

The reason for reverting is:
http://build.chromium.org/p/chromium.linux/builders/Android%20GN/builds/29757...

Likely broke build GN check.

[xunjieli, 2015-08-28 14:15:42]

Matt and Brett, could you take another look? This fixes the GN check.

[mmenke, 2015-08-28 15:03:09]

On 2015/08/28 14:15:42, xunjieli wrote:
> Matt and Brett, could you take another look? This fixes the GN check.

LGTM

[xunjieli, 2015-08-31 14:23:47]

Patchset #10 (id:340001) has been deleted

[xunjieli, 2015-08-31 14:41:29]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2015-08-31 14:41:29]

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

[commit-bot: I haz the power, 2015-08-31 14:41:45]

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

[commit-bot: I haz the power, 2015-08-31 15:51:27]

Committed patchset #10 (id:360001)

[commit-bot: I haz the power, 2015-08-31 15:52:02]

Patchset 10 (id:??) landed as
https://crrev.com/905496a6338fa535b5d242fda1d3eb0cab1d01ae
Cr-Commit-Position: refs/heads/master@{#346392}