Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1488383002:
Port cronet_package GYP target to GN (Closed)
DescriptionPort cronet_package GYP target to GN
BUG=462737
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 7
Messages
Total messages: 25 (3 generated)
pkotwicz@chromium.org changed reviewers: + agrieve@chromium.org
Andrew, can you please take a look?
lgtm https://codereview.chromium.org/1488383002/diff/1/components/cronet/android/B... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/1488383002/diff/1/components/cronet/android/B... components/cronet/android/BUILD.gn:457: _extract_cronet_jars_dir = "$root_gen_dir/cronet_jar_extract" nit: since this one is temporary, can you use target_gen_dir for it? https://codereview.chromium.org/1488383002/diff/1/components/cronet/android/B... components/cronet/android/BUILD.gn:470: depfile, unfortunate that it can't list any of its outputs (although it's clear why). What they should have done here (imo) is written a "create_cronet_jar.py" that both extracts and repackages.
pkotwicz@chromium.org changed reviewers: + mef@chromium.org
mef@ can you please take a look? https://codereview.chromium.org/1488383002/diff/1/components/cronet/android/B... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/1488383002/diff/1/components/cronet/android/B... components/cronet/android/BUILD.gn:457: _extract_cronet_jars_dir = "$root_gen_dir/cronet_jar_extract" On 2015/12/02 15:03:13, agrieve wrote: > nit: since this one is temporary, can you use target_gen_dir for it? Done.
On 2015/12/10 22:16:08, pkotwicz wrote: > mef@ can you please take a look? > > https://codereview.chromium.org/1488383002/diff/1/components/cronet/android/B... > File components/cronet/android/BUILD.gn (right): > > https://codereview.chromium.org/1488383002/diff/1/components/cronet/android/B... > components/cronet/android/BUILD.gn:457: _extract_cronet_jars_dir = > "$root_gen_dir/cronet_jar_extract" > On 2015/12/02 15:03:13, agrieve wrote: > > nit: since this one is temporary, can you use target_gen_dir for it? > > Done. lgtm, what's a good way to try it out?
A good way of trying this CL out is to compile the cronet_package target with GN (After having applied this CL and https://codereview.chromium.org/1483843002/ locally) You can do the equivalent of "gclient runhooks" for gen via "gn gen --args='target_os="android"' Let me know if you have any other questions :)
A good way of trying this CL out is to compile the cronet_package target with GN (After having applied this CL and https://codereview.chromium.org/1483843002/ locally) You can do the equivalent of "gclient runhooks" for gen via "gn gen --args='target_os="android"' Let me know if you have any other questions :)
A good way of trying this CL out is to compile the cronet_package target with GN (After having applied this CL and https://codereview.chromium.org/1483843002/ locally) You can do the equivalent of "gclient runhooks" for GN via "gn gen --args='target_os="android"' Let me know if you have any other questions :)
A good way of trying this CL out is to compile the cronet_package target with GN (After having applied this CL and https://codereview.chromium.org/1483843002/ locally) You can do the equivalent of "gclient runhooks" for GN via "gn gen --args='target_os="android"' Let me know if you have any other questions :)
pkotwicz@chromium.org changed reviewers: + brettw@chromium.org, dpranke@chromium.org
brettw@ for url/ OWNERS dpranke@ for BUILD.gn OWNERS
https://codereview.chromium.org/1488383002/diff/20001/components/cronet/andro... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/1488383002/diff/20001/components/cronet/andro... components/cronet/android/BUILD.gn:518: depfile = "$target_gen_dir/$target_name.d" It looks like this script gets pointed at a directory, walks the directory to find the list of source files, and then writes them into a .d file . This kinda works, but I believe entries in *.d files are allowed to be missing, which means that we won't trigger rebuilds when files get removed from the tree. Brett, do we have other alternatives here other than explicitly listing all of the java files in the BUILD.gn file? Do we pervasively do these sorts of things in other java targets on Android, or is this specific to cronet?
https://codereview.chromium.org/1488383002/diff/20001/components/cronet/andro... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/1488383002/diff/20001/components/cronet/andro... components/cronet/android/BUILD.gn:518: depfile = "$target_gen_dir/$target_name.d" jar_src.py is only used by cronet. We take directories as inputs in several places in Android. A few examples: - "resource_dirs" parameter for "android_resources" template - "resources" parameter in "mojo_native_application" template - used in components/resource_provider:apptests - "DEPRECATED_java_in_dir" parameter in "android_library" template
dpranke@, brettw@ Ping!
On 2015/12/21 16:05:06, pkotwicz wrote: > dpranke@, brettw@ Ping! I've tried doing ["gn gen out/gn_rel --args='target_os="android"'] and unfortunately the result does not lgtm. 1. cronet/symbols should contain unstripped libcronet.so, but contains stripped version instead. 2. cronet/libs should contain stripped libcronet.so, but is missing all together. 3. stripped libcronet.so is considerably bigger (3,382,840 bytes) than with gyp (2,684,520 bytes). We need to understand why and fix that. 4. The cronet_api.jar/HttpUrlConnectionUrlRequest.class is somewhat smaller (13,837 bytes) than with gyp (14,015 bytes). We need to understand why. I have a theory about #3 (we use these GYP_DEFINES: "OS=android enable_websockets=0 disable_file_support=1 disable_ftp_support=1" so I need to try with those).
https://codereview.chromium.org/1488383002/diff/20001/components/cronet/andro... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/1488383002/diff/20001/components/cronet/andro... components/cronet/android/BUILD.gn:463: depfile = "$root_gen_dir/$target_name.d" This should probably go in the target gen dir. https://codereview.chromium.org/1488383002/diff/20001/components/cronet/andro... components/cronet/android/BUILD.gn:518: depfile = "$target_gen_dir/$target_name.d" I think this will work because extract_from_jars deletes the directory before extracting anything. I think this is worth mentioning explicitly in the extract action above since this is subtle and will break if it gets screwed up. https://codereview.chromium.org/1488383002/diff/20001/components/cronet/tools... File components/cronet/tools/extract_from_jars.py (right): https://codereview.chromium.org/1488383002/diff/20001/components/cronet/tools... components/cronet/tools/extract_from_jars.py:45: for root, _, filenames in os.walk(options.classes_dir): It looks like this is writing the names of the extracted files to the dep file. This is backwards, the dep file lists the inputs. I think doing this will cause some confusion. Just write the python dependencies to this file so things will get redone when the scripts change. https://codereview.chromium.org/1488383002/diff/20001/url/BUILD.gn File url/BUILD.gn (right): https://codereview.chromium.org/1488383002/diff/20001/url/BUILD.gn#newcode76 url/BUILD.gn:76: DEPRECATED_java_in_dir = "android/java/src" As a higher-level-comment, I'm pretty confused about this variable. It's called deprecated but we continue to add it all over the Android build. Something that's deprecated should have a clear alternative so we can stop adding new uses.
On 2015/12/21 20:23:44, mef wrote: > On 2015/12/21 16:05:06, pkotwicz wrote: > > dpranke@, brettw@ Ping! > > I've tried doing ["gn gen out/gn_rel --args='target_os="android"'] and > unfortunately the result does not lgtm. > > 1. cronet/symbols should contain unstripped libcronet.so, but contains stripped > version instead. > 2. cronet/libs should contain stripped libcronet.so, but is missing all > together. > 3. stripped libcronet.so is considerably bigger (3,382,840 bytes) than with gyp > (2,684,520 bytes). We need to understand why and fix that. > 4. The cronet_api.jar/HttpUrlConnectionUrlRequest.class is somewhat smaller > (13,837 bytes) than with gyp (14,015 bytes). We need to understand why. > > I have a theory about #3 (we use these GYP_DEFINES: "OS=android > enable_websockets=0 disable_file_support=1 disable_ftp_support=1" so I need to > try with those). I've tried this command line: gn gen out/rel_gn --args='target_os="android" enable_websockets=false disable_file_support=true disable_ftp_support=true use_goma=true' But the resulting binaries were the same size as with just target_os="android". I've also got this warning despite enable_websockets being used in net/BUILD.gn (https://code.google.com/p/chromium/codesearch#chromium/src/net/BUILD.gn&l=41). ERROR at the command-line "--args":1:39: Build argument has no effect. target_os="android" enable_websockets=false disable_file_support=true disable_ftp_support=true use_goma=true ^---- The variable "enable_websockets" was set as a build argument but never appeared in a declare_args() block in any buildfile.
(3) is because you did a debug build instead of a release build with GN (debug is default) To do a release build: gn gen --args out/gn_rel --args='is_debug=false target_os="android"'
(3) is because you did a debug build instead of a release build with GN (debug is default) To do a release build: gn gen --args out/gn_rel --args='is_debug=false target_os="android"' I'll look at your other comments once I get back from vacation on Jan 11
On 2015/12/22 20:16:13, pkotwicz wrote: > (3) is because you did a debug build instead of a release build with GN (debug > is default) > To do a release build: > gn gen --args out/gn_rel --args='is_debug=false target_os="android"' > > I'll look at your other comments once I get back from vacation on Jan 11 Here's a patch to make disable_ftp_support and enable_websockets settable via gn args: https://codereview.chromium.org/1542633004 Thanks for taking a look at this mef! It's one of the last hold-outs to fully switching to GN! I'll hopefully have a look at the other things you pointed out next week while Peter is out.
On 2015/12/22 20:45:16, agrieve wrote: > On 2015/12/22 20:16:13, pkotwicz wrote: > > (3) is because you did a debug build instead of a release build with GN (debug > > is default) > > To do a release build: > > gn gen --args out/gn_rel --args='is_debug=false target_os="android"' > > > > I'll look at your other comments once I get back from vacation on Jan 11 > > Here's a patch to make disable_ftp_support and enable_websockets settable via gn > args: https://codereview.chromium.org/1542633004 > > Thanks for taking a look at this mef! It's one of the last hold-outs to fully > switching to GN! I'll hopefully have a look at the other things you pointed out > next week while Peter is out. Sounds good, thank you for doing this!
The top-level BUILD and //build changes look fine. I defer to brettw and mef otherwise.
https://codereview.chromium.org/1488383002/diff/20001/url/BUILD.gn File url/BUILD.gn (right): https://codereview.chromium.org/1488383002/diff/20001/url/BUILD.gn#newcode76 url/BUILD.gn:76: DEPRECATED_java_in_dir = "android/java/src" On 2015/12/21 20:25:02, brettw wrote: > As a higher-level-comment, I'm pretty confused about this variable. It's called > deprecated but we continue to add it all over the Android build. Something > that's deprecated should have a clear alternative so we can stop adding new > uses. https://code.google.com/p/chromium/issues/detail?id=484854 tldr - The replacement is java_files, but we don't want to switch to it until we stop having GYP be the primary target (or else people will forget to update the GN version of the rule)
On 2015/12/30 20:17:40, agrieve wrote: > https://codereview.chromium.org/1488383002/diff/20001/url/BUILD.gn > File url/BUILD.gn (right): > > https://codereview.chromium.org/1488383002/diff/20001/url/BUILD.gn#newcode76 > url/BUILD.gn:76: DEPRECATED_java_in_dir = "android/java/src" > On 2015/12/21 20:25:02, brettw wrote: > > As a higher-level-comment, I'm pretty confused about this variable. It's > called > > deprecated but we continue to add it all over the Android build. Something > > that's deprecated should have a clear alternative so we can stop adding new > > uses. > > https://code.google.com/p/chromium/issues/detail?id=484854 > > tldr - The replacement is java_files, but we don't want to switch to it until we > stop having GYP be the primary target (or else people will forget to update the > GN version of the rule) Took this over and addressed all comments: https://codereview.chromium.org/1553623003/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
