Add APK targets for content_unittests and net_unittests.

Add 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

[Yaron, 2012-05-22 17:35:55]

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.

[nilesh, 2012-05-22 21:01:01]

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.

[Yaron, 2012-05-23 00:17:55]

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

[Ryan Sleevi, 2012-05-23 01:00:45]

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.

[nilesh, 2012-05-23 22:45:20]

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.

[Ryan Sleevi, 2012-05-23 23:09:24]

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.

[nilesh, 2012-05-23 23:55:40]

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.

[Ryan Sleevi, 2012-05-23 23:58:33]

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)

[nilesh, 2012-05-24 00:05:34]

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.

[Yaron, 2012-05-24 01:15:32]

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!

[Ryan Sleevi, 2012-05-24 10:11:15]

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)

[nilesh, 2012-05-24 16:56:22]

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.

[sky, 2012-05-24 17:34:04]

LGTM

[nilesh, 2012-05-24 21:10:44]

@John: Can you please look at the content/ and ipc/ gyp changes.
@jar: Can you please look at base/

Thanks.

[jam, 2012-05-24 21:51:27]

On 2012/05/24 21:10:44, nileshagrawal1 wrote:
> @John: Can you please look at the content/ and ipc/ gyp changes.
lgtm

[jar (doing other things), 2012-05-25 17:26:12]

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.

[Mark Mentovai, 2012-05-25 17:44:00]

LGTM for base OWNERS approval.

[commit-bot: I haz the power, 2012-05-25 17:50:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/10399126/35001

[commit-bot: I haz the power, 2012-05-25 20:17:52]

Change committed as 139106