Port cronet build rules to GN part 1

Port cronet build rules to GN part 1

BUG=462737
Committed: https://crrev.com/92e2e231a1e99fa168220fe543653512383ed34a
Cr-Commit-Position: refs/heads/master@{#365357}

[pkotwicz, 2015-11-30 01:56:15]

pkotwicz@chromium.org changed reviewers:
+ agrieve@chromium.org

[pkotwicz, 2015-11-30 01:56:15]

agrieve@ can you please take a look?

https://codereview.chromium.org/1483843002/diff/1/components/pref_registry/BU...
File components/pref_registry/BUILD.gn (left):

https://codereview.chromium.org/1483843002/diff/1/components/pref_registry/BU...
components/pref_registry/BUILD.gn:15: "//ui/base",
This change is intentional.

In order for //components/data_reduction_proxy/core/browser:browser_small to be
able to depend on "pref_registry", "pref_registry" cannot depend on //ui/base
because //ui/base depends on //url

[agrieve, 2015-11-30 16:05:32]

On 2015/11/30 01:56:15, pkotwicz wrote:
> agrieve@ can you please take a look?
> 
>
https://codereview.chromium.org/1483843002/diff/1/components/pref_registry/BU...
> File components/pref_registry/BUILD.gn (left):
> 
>
https://codereview.chromium.org/1483843002/diff/1/components/pref_registry/BU...
> components/pref_registry/BUILD.gn:15: "//ui/base",
> This change is intentional.
> 
> In order for //components/data_reduction_proxy/core/browser:browser_small to
be
> able to depend on "pref_registry", "pref_registry" cannot depend on //ui/base
> because //ui/base depends on //url

Wow, great work on such a monster-sided porting!

[agrieve, 2015-11-30 16:05:39]

https://codereview.chromium.org/1483843002/diff/1/build/config/android/rules.gni
File build/config/android/rules.gni (left):

https://codereview.chromium.org/1483843002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1888: run_findbugs = false
Intentional? Might be better as a separate change?

https://codereview.chromium.org/1483843002/diff/1/components/cronet/android/B...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1483843002/diff/1/components/cronet/android/B...
components/cronet/android/BUILD.gn:13: enable_data_reduction_proxy_support =
false
nit: maybe prefix with cronet_?

https://codereview.chromium.org/1483843002/diff/1/components/cronet/android/B...
components/cronet/android/BUILD.gn:97: static_library(target_name) {
Can this use source_set() instead? If not, add a comment saying why.

https://codereview.chromium.org/1483843002/diff/1/components/cronet/android/B...
components/cronet/android/BUILD.gn:406: sources_rel =
exec_script("//build/android/gyp/find.py",
Rather than add another exec_script exception, I think let's either:
1. List out all the paths explicitly, or
2. Use asset_dir for now, and once https://codereview.chromium.org/1444113003/
lands, we can change it so sources just has the test/ directory in it.

https://codereview.chromium.org/1483843002/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/common/BUILD.gn (right):

https://codereview.chromium.org/1483843002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/BUILD.gn:8:
static_library(target_name) {
Can this be source_set?

[pkotwicz, 2015-12-01 02:11:27]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2015-12-01 02:12:42]

Andrew, can you please take another look? I think I have addressed all of your
concerns

[agrieve, 2015-12-01 13:33:31]

On 2015/12/01 02:12:42, pkotwicz wrote:
> Andrew, can you please take another look? I think I have addressed all of your
> concerns

lgtm

[pkotwicz, 2015-12-01 15:48:12]

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

[pkotwicz, 2015-12-01 15:48:13]

mef@ can you please take a look?

[mef, 2015-12-02 16:57:49]

Looks pretty good, mostly questions as I'm not very familiar with GN.

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:75: zip("cronet_version_srcjar") {
what does this do?

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:144: "-DLOGGING=1",
Interesting. Are these flags different from regular android build?

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:172: # ICU alternatives which requires file
and ftp support be disabled.
file and ftp support are disabled primarily for size reduction.

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:217: DEPRECATED_java_in_dir = "api/src"
If new way is to use java_files section, then we can use that here as well.

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:223: srcjar_deps = [
Nice, are those added to src jar?

[pkotwicz, 2015-12-03 19:24:19]

mef@ can you please take another look?

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:75: zip("cronet_version_srcjar") {
This packages up the generated Version.java in a .jar so that it can be use in
compiling |cronet_api|

The reason that this is different from GYP is because GN does not have a 1:1
equivalent to direct_dependent_settings.

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:144: "-DLOGGING=1",
These flags are taken right out of cronet_static.gypi

This block in particular:
  'cflags': [
    '-DLOGGING=1',
    '-fdata-sections',
    '-ffunction-sections',
    '-fno-rtti',
    '-fvisibility=hidden',
    '-fvisibility-inlines-hidden',
    '-Wno-sign-promo',
    '-Wno-missing-field-initializers',
  ],

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:172: # ICU alternatives which requires file
and ftp support be disabled.
I copied the comment from cronet.gypi. Should I modify the comment in both GYP
and GN? What should I modify the comment to?

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:217: DEPRECATED_java_in_dir = "api/src"
DEPRECATED_java_in_dir is a misnomer. It is not deprecated. One day we will
replace DEPRECATED_java_in_dir with java_files but that day is not near

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:223: srcjar_deps = [
I do not really understand your question.

The contents of cronet_api.jar when cronet_api.jar is generated via both GYP and
GN are the same (I checked this by comparing output of zipinfo)

The srcjar_deps are included in the output jar (cronet_api.jar). For instance,
NetworkQualityObservationSource.class is present in cronet_api.jar

GN does not have built-in functionality to generate src jars for the
"cronet_package" target. I have ported the package step to GN in
https://codereview.chromium.org/1488383002/

[pkotwicz, 2015-12-07 16:02:30]

mef@ Ping!

[mef, 2015-12-07 20:52:07]

lgtm

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:144: "-DLOGGING=1",
On 2015/12/03 19:24:19, pkotwicz wrote:
> These flags are taken right out of cronet_static.gypi
> 
> This block in particular:
>   'cflags': [
>     '-DLOGGING=1',
>     '-fdata-sections',
>     '-ffunction-sections',
>     '-fno-rtti',
>     '-fvisibility=hidden',
>     '-fvisibility-inlines-hidden',
>     '-Wno-sign-promo',
>     '-Wno-missing-field-initializers',
>   ],

Right, and given that you don't list the entire block here suggests to me that
most of the flags are inherited from base/default android configuration. I'm a
bit surprised that cronet has these options set differently.

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:172: # ICU alternatives which requires file
and ftp support be disabled.
On 2015/12/03 19:24:19, pkotwicz wrote:
> I copied the comment from cronet.gypi. Should I modify the comment in both GYP
> and GN? What should I modify the comment to?

No, that's fine.

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:217: DEPRECATED_java_in_dir = "api/src"
On 2015/12/03 19:24:19, pkotwicz wrote:
> DEPRECATED_java_in_dir is a misnomer. It is not deprecated. One day we will
> replace DEPRECATED_java_in_dir with java_files but that day is not near

Acknowledged.

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:223: srcjar_deps = [
On 2015/12/03 19:24:18, pkotwicz wrote:
> I do not really understand your question.
> 
> The contents of cronet_api.jar when cronet_api.jar is generated via both GYP
and
> GN are the same (I checked this by comparing output of zipinfo)
> 
> The srcjar_deps are included in the output jar (cronet_api.jar). For instance,
> NetworkQualityObservationSource.class is present in cronet_api.jar
> 
> GN does not have built-in functionality to generate src jars for the
> "cronet_package" target. I have ported the package step to GN in
> https://codereview.chromium.org/1488383002/

Hm, I thought that srcjars generated by GYP don't include java classes generated
from templates, and thought that GN fixes that.

[pkotwicz, 2015-12-08 17:17:57]

pkotwicz@chromium.org changed reviewers:
+ megjablon@chromium.org

[pkotwicz, 2015-12-08 17:17:57]

megjablon@ can you please take a look at the changes in
components/data_reduction_proxy ?

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:144: "-DLOGGING=1",
It looks like these flags have been around since
https://codereview.chromium.org/145213003

I don't know how to track the cronet code further back in time

The DLOGGING C flag seems to not have ever been in common.gypi (git log -S
DLOGGING build/common.gypi returns no results)

[megjablon, 2015-12-08 20:03:31]

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:13:
cronet_enable_data_reduction_proxy_support = false
when should this be true?

https://codereview.chromium.org/1483843002/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1483843002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/BUILD.gn:75: "data_store_impl.h",
Why not add these two to browser_sources?

[pkotwicz, 2015-12-08 20:42:56]

pkotwicz@chromium.org changed reviewers:
+ bengr@chromium.org

[pkotwicz, 2015-12-08 20:42:56]

+bengr@ who can better answer megjablon@'s question than I can

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:13:
cronet_enable_data_reduction_proxy_support = false
When a user specifies this as true in gn gen --args=''

The flag was added in GYP in https://codereview.chromium.org/1112513003 by
bengr@ in July

https://codereview.chromium.org/1483843002/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1483843002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/BUILD.gn:75: "data_store_impl.h",
Because browser_sources are sources for both "browser_small" and "browser" but
data_store_impl.* is not a source for "browser_small".

data_store_impl.cc was added to the GYP file in
https://codereview.chromium.org/1173343009 I am stupidly porting the GYP rules
to GN. I do not know why it is this way.

[bengr, 2015-12-09 21:52:43]

components/d_r_p lgtm

[pkotwicz, 2015-12-09 22:09:13]

pkotwicz@chromium.org changed reviewers:
+ bauerb@chromium.org, dpranke@chromium.org

[pkotwicz, 2015-12-09 22:09:13]

bauerb@ for change to components/pref_registry (This change is necessary because
components/data_reduction_proxy/core/browser:browser_small depends on
pref_registry but cannot depend on //url

dpranke@ for change to BUILD.gn

[Dirk Pranke, 2015-12-09 22:28:26]

lgtm

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/1483843002/diff/40001/components/cronet/andro...
components/cronet/android/BUILD.gn:96: template("cronet_static_tmpl") {
templates should have comments describing their input parameters (in this case,
defines and deps).

https://codereview.chromium.org/1483843002/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/common/BUILD.gn (right):

https://codereview.chromium.org/1483843002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/BUILD.gn:7: template("common_tmpl")
{
same comment.

[bengr, 2015-12-10 00:19:45]

https://codereview.chromium.org/1483843002/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1483843002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/BUILD.gn:75: "data_store_impl.h",
On 2015/12/08 20:42:56, pkotwicz wrote:
> Because browser_sources are sources for both "browser_small" and "browser" but
> data_store_impl.* is not a source for "browser_small".
> 
> data_store_impl.cc was added to the GYP file in
> https://codereview.chromium.org/1173343009 I am stupidly porting the GYP rules
> to GN. I do not know why it is this way.

This is correct. Cronet and other consumers that use browser_small will not have
a persistent data store.

[Bernhard Bauer, 2015-12-10 13:22:25]

LGTM

[pkotwicz, 2015-12-10 22:10:20]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-12-10 22:10:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, agrieve@chromium.org, bengr@chromium.org,
mef@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1483843002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-12-10 22:12:28]

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

[commit-bot: I haz the power, 2015-12-10 23:09:32]

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

[commit-bot: I haz the power, 2015-12-10 23:09:33]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[pkotwicz, 2015-12-11 04:05:31]

dpranke@ can you please take a look at the changes in google_apis/BUILD.gn (No
specified OWNERs for this file)

[Dirk Pranke, 2015-12-11 06:05:03]

lgtm

[pkotwicz, 2015-12-15 16:55:55]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-12-15 16:55:55]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
agrieve@chromium.org, bengr@chromium.org, mef@chromium.org, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1483843002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-12-15 16:56:17]

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

[commit-bot: I haz the power, 2015-12-15 17:08:26]

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

[commit-bot: I haz the power, 2015-12-15 17:08:27]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[pkotwicz, 2015-12-15 20:43:25]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-12-15 20:43:25]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
dpranke@chromium.org, agrieve@chromium.org, bengr@chromium.org, mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1483843002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-12-15 20:43:49]

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

[commit-bot: I haz the power, 2015-12-15 22:29:53]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-12-15 22:30:35]

Description was changed from

==========
Port cronet build rules to GN part 1

BUG=462737
==========

to

==========
Port cronet build rules to GN part 1

BUG=462737

Committed: https://crrev.com/92e2e231a1e99fa168220fe543653512383ed34a
Cr-Commit-Position: refs/heads/master@{#365357}
==========

[commit-bot: I haz the power, 2015-12-15 22:30:36]

Patchset 7 (id:??) landed as
https://crrev.com/92e2e231a1e99fa168220fe543653512383ed34a
Cr-Commit-Position: refs/heads/master@{#365357}