|
|
Created:
8 years, 7 months ago by nilesh Modified:
8 years, 7 months ago Reviewers:
sky, Ryan Sleevi, jam, Mark Mentovai, Yaron, darin (slow to review), jar (doing other things) CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cbentzel+watch_chromium.org, jochen+watch-content_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd APK targets for content_unittests and net_unittests.
- Add a gyp template to simplify adding new targets.
- Add support for multiple jars in the test APK.
BUG=125059
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139106
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : Created a template and used it in all the build rules. #Patch Set 4 : #
Total comments: 17
Patch Set 5 : Addressed comments. #Patch Set 6 : Addressed nit #
Total comments: 8
Patch Set 7 : Added dependency on input jars, other minor issues fixed. #
Total comments: 2
Patch Set 8 : #Patch Set 9 : Check for no jars specified in generate_native_test.py #
Total comments: 6
Patch Set 10 : Fixed order of dependencies. #Patch Set 11 : Rebase #
Created: 8 years, 7 months ago
Messages
Total messages: 22 (0 generated)
http://codereview.chromium.org/10399126/diff/2001/build/android/gtest_filter/... File build/android/gtest_filter/content_unittests_disabled (right): http://codereview.chromium.org/10399126/diff/2001/build/android/gtest_filter/... build/android/gtest_filter/content_unittests_disabled:45: ChromeAppCacheServiceTest.* Have you looked into why these fail? http://codereview.chromium.org/10399126/diff/2001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10399126/diff/2001/net/net.gyp#newcode1849 net/net.gyp:1849: # net_unittests into an android apk for execution. So can we templatize these now like in build/java.gypi? Define a build/apk_test.gypi (I'm not sure we can share a template between a regular apk and a test apk given that the test apks rely on a python script to wrap/generate the apk). The reason I'd like to do this now as it's also causing me separate issues trying to get these to work with ninja.
http://codereview.chromium.org/10399126/diff/2001/build/android/gtest_filter/... File build/android/gtest_filter/content_unittests_disabled (right): http://codereview.chromium.org/10399126/diff/2001/build/android/gtest_filter/... build/android/gtest_filter/content_unittests_disabled:45: ChromeAppCacheServiceTest.* On 2012/05/22 17:35:55, Yaron wrote: > Have you looked into why these fail? There are issues with reading the sql databases. Will handle these in a separate CL. Removing these suppressions for now. http://codereview.chromium.org/10399126/diff/2001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10399126/diff/2001/net/net.gyp#newcode1849 net/net.gyp:1849: # net_unittests into an android apk for execution. On 2012/05/22 17:35:55, Yaron wrote: > So can we templatize these now like in build/java.gypi? Define a > build/apk_test.gypi (I'm not sure we can share a template between a regular apk > and a test apk given that the test apks rely on a python script to wrap/generate > the apk). > > The reason I'd like to do this now as it's also causing me separate issues > trying to get these to work with ninja. Done. Thanks, this is much cleaner now.
lgtm but adding Ryan to comment on this. It's a little strange that content_unittests and net_unittests currently specify a single jar_name (especially since it's base's jar). This seems like a fine incremental step though and I think you should fulfill the TODO in build/apk_test.gypi next
yaron: Why would you do that to poor nilesh? :-) GYP comments below... http://codereview.chromium.org/10399126/diff/5002/build/apk_test.gypi File build/apk_test.gypi (right): http://codereview.chromium.org/10399126/diff/5002/build/apk_test.gypi#newcode27 build/apk_test.gypi:27: 'actions': [ I feel like you should be checking 'target_conditions': [ ['_toolset == "target"', { 'conditions': [ ['OS == "android" and _component == "shared_library"', { 'actions': [ ... ], }], ], # conditions }], ], # target_conditions http://codereview.chromium.org/10399126/diff/5002/build/apk_test.gypi#newcode33 build/apk_test.gypi:33: '../testing/android/generate_native_test.py', I'm pretty sure these two (line 32, 33) should be using <(DEPTH). That's because, IIRC, file relativization happens post-GYPI merging, which means the '../' will be relative to the including .gyp. Might want to double check the GYP sources. http://codereview.chromium.org/10399126/diff/5002/build/apk_test.gypi#newcode35 build/apk_test.gypi:35: '<(PRODUCT_DIR)/lib.java/<(jar_name)', I think I'd feel better if the caller was required to fully qualify the path to the inputs, rather than making this .gypi do it. That way, it's clear at the 'include' time what the input/output dependencies are. http://codereview.chromium.org/10399126/diff/5002/build/apk_test.gypi#newcode38 build/apk_test.gypi:38: '<(PRODUCT_DIR)/<(test_suite_name)_apk/<(test_suite_name)-debug.apk', Should this be "->(_configuration).apk" instead of hardcoding "-debug" here? Note, >() because this is a target-expansion that needs to happen after this has been merged (see http://code.google.com/p/gyp/wiki/InputFormatReference#Variable_Expansions_(<... ) http://codereview.chromium.org/10399126/diff/5002/build/apk_test.gypi#newcode43 build/apk_test.gypi:43: '<(PRODUCT_DIR)/lib.target/lib<(test_suite_name).so', '/lib.target/<(SHARED_LIB_PREFIX)<(test_suite_name)<(SHARED_LIB_SUFFIX)' http://codereview.chromium.org/10399126/diff/5002/build/apk_test.gypi#newcode45 build/apk_test.gypi:45: '<(PRODUCT_DIR)/lib.java/chromium_base.jar', Is this right? Did you mean to do '<(PRODUCT_DIR)/lib.java/<(jar_name)' instead? If not, I'm of the opinion that the caller should explicitly specify their dependencies as arguments. You should also be ensuring those dependencies are added to the list of 'inputs' on line 31. You should define the instances (line 34, 35, 43, 45) as variables, so that it's clear what was intended. For that matter, I think lines 38/47 would also be served as variables. Note, the variables should be suffixed appropriately (_file/_path - see http://code.google.com/p/gyp/wiki/InputFormatReference#Pathname_Relativization ) Note, for specifying dependencies, you could use list expansion ( http://code.google.com/p/gyp/wiki/InputFormatReference#Variable_Expansions_(<... ), such like '--jar', '<@(required_jar_paths)' becomes --jar foo1.jar foo2.jar foo3.jar [etc] I'm guessing this would require updating the generate_native_test.py script first http://codereview.chromium.org/10399126/diff/5002/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/10399126/diff/5002/content/content_tests.gypi#... content/content_tests.gypi:399: ['OS=="android" and "<(gtest_target_type)"=="shared_library"', { Test _component (rather than "<(gtest_target_type)") here, since that's really what we're talking about. nit: Also, whitespace between the operators, as on line 390 http://codereview.chromium.org/10399126/diff/5002/content/content_tests.gypi#... content/content_tests.gypi:557: 'jar_name': 'chromium_base.jar', You probably want to rename jar_name to something that more meaningfully indicates that it's a required dependency. As written, it's not clear if this is input or output.
Thanks for the detailed review. PTAL. http://codereview.chromium.org/10399126/diff/5002/build/apk_test.gypi File build/apk_test.gypi (right): http://codereview.chromium.org/10399126/diff/5002/build/apk_test.gypi#newcode27 build/apk_test.gypi:27: 'actions': [ On 2012/05/23 01:00:45, Ryan Sleevi wrote: > I feel like you should be checking > > 'target_conditions': [ > ['_toolset == "target"', { > 'conditions': [ > ['OS == "android" and _component == "shared_library"', { > 'actions': [ ... ], > }], > ], # conditions > }], > ], # target_conditions Done. http://codereview.chromium.org/10399126/diff/5002/build/apk_test.gypi#newcode33 build/apk_test.gypi:33: '../testing/android/generate_native_test.py', On 2012/05/23 01:00:45, Ryan Sleevi wrote: > I'm pretty sure these two (line 32, 33) should be using <(DEPTH). That's > because, IIRC, file relativization happens post-GYPI merging, which means the > '../' will be relative to the including .gyp. > > Might want to double check the GYP sources. Done. http://codereview.chromium.org/10399126/diff/5002/build/apk_test.gypi#newcode35 build/apk_test.gypi:35: '<(PRODUCT_DIR)/lib.java/<(jar_name)', On 2012/05/23 01:00:45, Ryan Sleevi wrote: > I think I'd feel better if the caller was required to fully qualify the path to > the inputs, rather than making this .gypi do it. > > That way, it's clear at the 'include' time what the input/output dependencies > are. Done. http://codereview.chromium.org/10399126/diff/5002/build/apk_test.gypi#newcode38 build/apk_test.gypi:38: '<(PRODUCT_DIR)/<(test_suite_name)_apk/<(test_suite_name)-debug.apk', On 2012/05/23 01:00:45, Ryan Sleevi wrote: > Should this be "->(_configuration).apk" instead of hardcoding "-debug" here? > > Note, >() because this is a target-expansion that needs to happen after this has > been merged (see > http://code.google.com/p/gyp/wiki/InputFormatReference#Variable_Expansions_%2...) > ) Currently, we always generate *-debug.apk (even for BUILDTYPE=Release). There are some issues (for e.g. signing the apk) with trying to generate non-debug APK using ant. http://codereview.chromium.org/10399126/diff/5002/build/apk_test.gypi#newcode43 build/apk_test.gypi:43: '<(PRODUCT_DIR)/lib.target/lib<(test_suite_name).so', On 2012/05/23 01:00:45, Ryan Sleevi wrote: > '/lib.target/<(SHARED_LIB_PREFIX)<(test_suite_name)<(SHARED_LIB_SUFFIX)' Done. http://codereview.chromium.org/10399126/diff/5002/build/apk_test.gypi#newcode45 build/apk_test.gypi:45: '<(PRODUCT_DIR)/lib.java/chromium_base.jar', On 2012/05/23 01:00:45, Ryan Sleevi wrote: > Is this right? > > Did you mean to do '<(PRODUCT_DIR)/lib.java/<(jar_name)' instead? Yes. Thanks for catching. > > If not, I'm of the opinion that the caller should explicitly specify their > dependencies as arguments. You should also be ensuring those dependencies are > added to the list of 'inputs' on line 31. > > You should define the instances (line 34, 35, 43, 45) as variables, so that it's > clear what was intended. Done > > For that matter, I think lines 38/47 would also be served as variables. The output-dir and output filename are hardcodeded. We don't want the caller to modify these. Out test runner scripts rely on these APKs to be present here. > > Note, the variables should be suffixed appropriately (_file/_path - see > http://code.google.com/p/gyp/wiki/InputFormatReference#Pathname_Relativization ) > > > Note, for specifying dependencies, you could use list expansion ( > http://code.google.com/p/gyp/wiki/InputFormatReference#Variable_Expansions_%2...) > ), such like > > '--jar', '<@(required_jar_paths)' > > becomes > > --jar foo1.jar foo2.jar foo3.jar [etc] > > I'm guessing this would require updating the generate_native_test.py script > first Done http://codereview.chromium.org/10399126/diff/5002/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/10399126/diff/5002/content/content_tests.gypi#... content/content_tests.gypi:399: ['OS=="android" and "<(gtest_target_type)"=="shared_library"', { On 2012/05/23 01:00:45, Ryan Sleevi wrote: > Test _component (rather than "<(gtest_target_type)") here, since that's really > what we're talking about. If I understand correctly, using component would require us to set component=shared_library in our GYP_DEFINES and it will have other side effects (other targets may change behavior based on component value). We only want to change gtest target types. > nit: Also, whitespace between the operators, as on line 390 Done http://codereview.chromium.org/10399126/diff/5002/content/content_tests.gypi#... content/content_tests.gypi:557: 'jar_name': 'chromium_base.jar', On 2012/05/23 01:00:45, Ryan Sleevi wrote: > You probably want to rename jar_name to something that more meaningfully > indicates that it's a required dependency. As written, it's not clear if this is > input or output. Done.
You probably want to double check this works before committing it, but I think you should be good here. Hopefully I haven't lied about any GYP syntax :) LGTM with some nits below. I think you'll be fine to make them without another review, but if you need me to double check, I'm more than happy to. http://codereview.chromium.org/10399126/diff/5002/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/10399126/diff/5002/content/content_tests.gypi#... content/content_tests.gypi:399: ['OS=="android" and "<(gtest_target_type)"=="shared_library"', { On 2012/05/23 22:45:20, nileshagrawal1 wrote: > On 2012/05/23 01:00:45, Ryan Sleevi wrote: > > Test _component (rather than "<(gtest_target_type)") here, since that's really > > what we're talking about. > > If I understand correctly, using component would require us to set > component=shared_library in our GYP_DEFINES and it will have other side effects > (other targets may change behavior based on component value). We only want to > change gtest target types. Sorry, I had meant _type (the automatic variable for 'type'), rather than component (the explicit shared/static build type). However, using _type requires that it be within a target_conditions block, since you want late evaluation of _type (eg: after it has been merged into the final target dict). Since target_conditions has additional requirements on what keys you can adjust, due to the late phase evaluation, this is fine. Do see my comment about "<(gtest_target_type)" vs gtest_target_type though > > > > nit: Also, whitespace between the operators, as on line 390 > > Done http://codereview.chromium.org/10399126/diff/17001/base/base.gyp File base/base.gyp (right): http://codereview.chromium.org/10399126/diff/17001/base/base.gyp#newcode315 base/base.gyp:315: ['"<(gtest_target_type)" == "shared_library"', { You don't need to treat it in var form (that is, <(gtest_target_type) within conditions, as variables are automatically replaced, as part of the syntax described at http://code.google.com/p/gyp/wiki/InputFormatReference#conditions ['gtest_target_type == "shared_library"', { http://codereview.chromium.org/10399126/diff/17001/base/base.gyp#newcode658 base/base.gyp:658: 'base', # So that android/java/java.gyp:base_java is built I can't recall - is there a reason there's a dependency on 'base' here, rather than android/java/java.gyp:base_java ? http://codereview.chromium.org/10399126/diff/17001/base/base.gyp#newcode664 base/base.gyp:664: 'input_jars': ['<(PRODUCT_DIR)/lib.java/chromium_base.jar',], I believe you may want these variables to be sufficed as _path/etc, so that the paths are appropriately relativized to the .gyp, rather than to the .gypi I think the current behaviour is generator-defined, which is why it "just works" with Make on Linux. http://codereview.chromium.org/10399126/diff/17001/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/10399126/diff/17001/content/content_tests.gypi... content/content_tests.gypi:553: 'content_unittests', Since you're directly depending on the build products of certain targets (base & content), you should also list them as dependencies. This would apply to the other targets as well - that is, in addition to the '_unittests' target, depend on the jar-generating target explicitly.
Adding OWNERS now. Thanks for the review again. I have verified this works locally and will test on the bots too (only android_test try bot builds these targets). http://codereview.chromium.org/10399126/diff/17001/base/base.gyp File base/base.gyp (right): http://codereview.chromium.org/10399126/diff/17001/base/base.gyp#newcode315 base/base.gyp:315: ['"<(gtest_target_type)" == "shared_library"', { On 2012/05/23 23:09:24, Ryan Sleevi wrote: > You don't need to treat it in var form (that is, <(gtest_target_type) within > conditions, as variables are automatically replaced, as part of the syntax > described at http://code.google.com/p/gyp/wiki/InputFormatReference#conditions > > ['gtest_target_type == "shared_library"', { Done. http://codereview.chromium.org/10399126/diff/17001/base/base.gyp#newcode658 base/base.gyp:658: 'base', # So that android/java/java.gyp:base_java is built On 2012/05/23 23:09:24, Ryan Sleevi wrote: > I can't recall - is there a reason there's a dependency on 'base' here, rather > than android/java/java.gyp:base_java ? It should depend on base_java to generate chromium_base.jar. The original comment looks obsolete. http://codereview.chromium.org/10399126/diff/17001/base/base.gyp#newcode664 base/base.gyp:664: 'input_jars': ['<(PRODUCT_DIR)/lib.java/chromium_base.jar',], On 2012/05/23 23:09:24, Ryan Sleevi wrote: > I believe you may want these variables to be sufficed as _path/etc, so that the > paths are appropriately relativized to the .gyp, rather than to the .gypi > > I think the current behaviour is generator-defined, which is why it "just works" > with Make on Linux. Done. http://codereview.chromium.org/10399126/diff/17001/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/10399126/diff/17001/content/content_tests.gypi... content/content_tests.gypi:553: 'content_unittests', On 2012/05/23 23:09:24, Ryan Sleevi wrote: > Since you're directly depending on the build products of certain targets (base & > content), you should also list them as dependencies. > > This would apply to the other targets as well - that is, in addition to the > '_unittests' target, depend on the jar-generating target explicitly. I should have thought of this. Done. Thanks.
http://codereview.chromium.org/10399126/diff/12003/build/apk_test.gypi File build/apk_test.gypi (right): http://codereview.chromium.org/10399126/diff/12003/build/apk_test.gypi#newcode25 build/apk_test.gypi:25: ['OS == "android" and "<(gtest_target_type)" == "shared_library"', { Looks like there's still one place here (and line 51)
http://codereview.chromium.org/10399126/diff/12003/build/apk_test.gypi File build/apk_test.gypi (right): http://codereview.chromium.org/10399126/diff/12003/build/apk_test.gypi#newcode25 build/apk_test.gypi:25: ['OS == "android" and "<(gtest_target_type)" == "shared_library"', { On 2012/05/23 23:58:33, Ryan Sleevi wrote: > Looks like there's still one place here (and line 51) Done.
On 2012/05/24 00:05:34, nileshagrawal1 wrote: > http://codereview.chromium.org/10399126/diff/12003/build/apk_test.gypi > File build/apk_test.gypi (right): > > http://codereview.chromium.org/10399126/diff/12003/build/apk_test.gypi#newcode25 > build/apk_test.gypi:25: ['OS == "android" and "<(gtest_target_type)" == > "shared_library"', { > On 2012/05/23 23:58:33, Ryan Sleevi wrote: > > Looks like there's still one place here (and line 51) > > Done. This looks much better. Glad to see all the jar dependencies listed too. Thanks for your help, Ryan!
You may want to mention to the OWNERS that you were waiting review ;-) That said, a few nits, but nothing exciting. http://codereview.chromium.org/10399126/diff/29001/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/10399126/diff/29001/content/content_tests.gypi... content/content_tests.gypi:556: 'content_java', nit: sort http://codereview.chromium.org/10399126/diff/29001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10399126/diff/29001/net/net.gyp#newcode1859 net/net.gyp:1859: 'net_java', nit: sort http://codereview.chromium.org/10399126/diff/29001/ui/ui_unittests.gypi File ui/ui_unittests.gypi (right): http://codereview.chromium.org/10399126/diff/29001/ui/ui_unittests.gypi#newco... ui/ui_unittests.gypi:222: '../base/base.gyp:base_java', nit: sort ('../' should appear first)
Darin/Scott, Please take a look. I need your approval for OWNERS. Thanks. http://codereview.chromium.org/10399126/diff/29001/content/content_tests.gypi File content/content_tests.gypi (right): http://codereview.chromium.org/10399126/diff/29001/content/content_tests.gypi... content/content_tests.gypi:556: 'content_java', On 2012/05/24 10:11:15, Ryan Sleevi wrote: > nit: sort Done. http://codereview.chromium.org/10399126/diff/29001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10399126/diff/29001/net/net.gyp#newcode1859 net/net.gyp:1859: 'net_java', On 2012/05/24 10:11:15, Ryan Sleevi wrote: > nit: sort Done. http://codereview.chromium.org/10399126/diff/29001/ui/ui_unittests.gypi File ui/ui_unittests.gypi (right): http://codereview.chromium.org/10399126/diff/29001/ui/ui_unittests.gypi#newco... ui/ui_unittests.gypi:222: '../base/base.gyp:base_java', On 2012/05/24 10:11:15, Ryan Sleevi wrote: > nit: sort ('../' should appear first) Done.
LGTM
@John: Can you please look at the content/ and ipc/ gyp changes. @jar: Can you please look at base/ Thanks.
On 2012/05/24 21:10:44, nileshagrawal1 wrote: > @John: Can you please look at the content/ and ipc/ gyp changes. lgtm
Mark Mentovai would be a much better reviewer for the gyp changes (to base) than I am... so I put him on the list instead.
LGTM for base OWNERS approval.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/10399126/35001
Change committed as 139106 |