Bug 95690 – webkit_unit_tests-debug.apk does not include chromium_net.jar.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Philippe Liard]

Although webkit_unit_tests-debug.apk indirectly depends on 'net', its classes.dex file does not include classes from 'net_java' which can make some tests fail (see http://codereview.chromium.org/10916056/ which can be submitted once this issue is solved). Making webkit_support explicitly depend on 'net_java' does not solve the issue (i.e. the APK still does not include Java classes from net_java although net_java is built). The issue seems to come from the way we generate the APK.

From WebKitUnitTests.gyp:
                'target_name': 'webkit_unit_tests_apk',
                'type': 'none',
                'dependencies': [
                    '<(chromium_src_dir)/base/base.gyp:base_java',
                    'webkit_unit_tests',
                ],  
                'variables': {
                    'input_shlib_path': '<(SHARED_LIB_DIR)/<(SHARED_LIB_PREFIX)webkit_unit_tests<(SHARED_LIB_SUFFIX)',
                    'input_jars_paths': [
                        '<(PRODUCT_DIR)/lib.java/chromium_base.jar',                                                                                                                                                                                                                                                           
                    ],  
                    'conditions': [
                        ['inside_chromium_build==1', {
                            'ant_build_to_chromium_src': '<(ant_build_out)/../../',
                        }, {
                            'ant_build_to_chromium_src': '<(ant_build_out)/../../Source/WebKit/chromium',
                        }], 
                    ],  
                },
                 # Part of the following was copied from <(chromium_src_dir)/build/apk_test.gpyi.
                # Not including it because gyp include doesn't support variable in path or under
                # conditions. And we also have some different requirements.
                'actions': [{
                    'action_name': 'apk_webkit_unit_tests',
                    'message': 'Building webkit_unit_tests test apk.',
                    'inputs': [
                        '<(chromium_src_dir)/testing/android/AndroidManifest.xml',
                        '<(chromium_src_dir)/testing/android/generate_native_test.py',
                        '<(input_shlib_path)',
                        '<@(input_jars_paths)',
                    ],  
                    'outputs': [
                        '<(PRODUCT_DIR)/webkit_unit_tests_apk/webkit_unit_tests-debug.apk',
                    ],  
                    'action': [
                        '<(chromium_src_dir)/testing/android/generate_native_test.py',
                        '--native_library',
                        '<(input_shlib_path)',
                        '--jars',
                        '"<@(input_jars_paths)"',
                        '--output',
                        '<(PRODUCT_DIR)/webkit_unit_tests_apk',
                        '--strip-binary=<(android_strip)',
                        '--ant-args',
                        '-DANDROID_SDK=<(android_sdk)',
                        '--ant-args',
                        '-DANDROID_SDK_ROOT=<(android_sdk_root)',
                        '--ant-args',
                        '-DANDROID_SDK_TOOLS=<(android_sdk_tools)',
                        '--ant-args',
                        '-DANDROID_SDK_VERSION=<(android_sdk_version)',
                        '--ant-args',
                        '-DANDROID_TOOLCHAIN=<(android_toolchain)',
                        '--ant-args',
                        '-DPRODUCT_DIR=<(ant_build_out)',
                        '--ant-args',
                        '-DCHROMIUM_SRC=<(ant_build_to_chromium_src)',
                        '--sdk-build=<(sdk_build)',
                        '--app_abi',
                        '<(android_app_abi)',
                    ],  
                }], 


As you can see the dependency JARs that webkit_unit_tests-debug.apk includes are listed in 'input_jar_paths'. Right now only 'chromium_base.jar' is in the list although we also need 'chromium_net.jar'. Note that in the internal version of Chrome for Android (as opposed to upstream), the resulting APK does contain chromium_net.jar magically although it's not in the list mentioned earlier either. I didn't manage to figure out how it ends up in the APK.

What is the right way to make webkit_unit_tests-debug.apk include chromium_net.jar? Should it be listed in |input_jars_paths|, next to chromium_base.jar (which would be consistent at least)? I confirm that adding it there solves the issue upstream (i.e. classes.dex contains Java classes from 'net_java').

[Peter Beverloo]

Yes, we should explicitly include chromium_net.jar in the input_jars_paths list if we depend on it. Other WebKit binaries, such as DumpRenderTree and (less likely) TestWebKitAPI, may need to be updated too.

Our dependencies are a complete mess, and we need to be much more explicit about this. The right way to fix this would be:

1) net should depend on net_java (just like base should depend on base_java for target builds, etc).
2) libraries should explicitly depend on the jar files they need.

As such, adding the jar to input_jars_paths is fine. While you're at it, could you please change uses of the input_jars_paths list (on lines 175 and 185) to be a late expansion (i.e. change "<" to ">")? That brings us in line with Chromium's apk_test.gypi again.

[Philippe Liard]

Created attachment 163146 [details]
Patch

[Philippe Liard]

This patch depends on Chromium r>=155737. Will there be a Chromium roll soon?

[Peter Beverloo]

Comment on attachment 163146 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163146&action=review

> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:456
> +                    '<(chromium_src_dir)/media/media.gyp:media',

media does not depend on media_java yet, so this would actually make us loose JNI bindings. Is that intentional?

[Peter Beverloo]

(In reply to comment #3)
> This patch depends on Chromium r>=155737. Will there be a Chromium roll soon?

I rolled WebKit to Chromium r155883. Going further is blocked on more compositor failures, I asked James to take a look. Nonetheless, it's >155737.

[Philippe Liard]

Created attachment 163344 [details]
Patch

[Peter Beverloo]

Comment on attachment 163344 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163344&action=review

Looks good to me, thanks Philippe! Adam, mind doing the formal review?

> Source/WebKit/chromium/ChangeLog:1
> [phone]  Philippe Liard  <[e-mail]>

nit: You may want to update your git configuration to have your @chromium.org e-mail address.

> Source/WebKit/chromium/ChangeLog:3
> +        Depend on {base,net} GYP targets rather than {base,net}_java.

nit: For future reference: If your patch only touches Chromium code (moreso: files only used by Chromium), it's generally a good idea to include a [Chromium] prefix in your title.

[Adam Barth]

Comment on attachment 163344 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163344&action=review

ok

> Source/WebKit/chromium/WebKitUnitTests.gyp:176
> -                        '<@(input_jars_paths)',
> +                        '>@(input_jars_paths)',

I don't fully understand why you've reversed the < here.

[Philippe Liard]

Comment on attachment 163344 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163344&action=review

>> Source/WebKit/chromium/WebKitUnitTests.gyp:176
>> +                        '>@(input_jars_paths)',
> 
> I don't fully understand why you've reversed the < here.

I did this change following Peter's suggestion which was that we should be consistent with apk_test.gypi (which this code comes from).

[Philippe Liard]

From Peter in his first comment:
'While you're at it, could you please change uses of the input_jars_paths list (on lines 175 and 185) to be a late expansion (i.e. change "<" to ">")? That brings us in line with Chromium's apk_test.gypi again.'

[Peter Beverloo]

Comment on attachment 163344 [details]
Patch

Setting cq+.

(In reply to comment #8)
> (From update of attachment 163344 [details])
> > Source/WebKit/chromium/WebKitUnitTests.gyp:176
> > -                        '<@(input_jars_paths)',
> > +                        '>@(input_jars_paths)',
> 
> I don't fully understand why you've reversed the < here.

To keep in sync with the template we're trying to immitate here. We should work towards being able to use the template directly, which would resolve quite dependency issues requiring three-way patches. I filed bug 96493 for that purpose.

[WebKit Review Bot]

Comment on attachment 163344 [details]
Patch

Rejecting attachment 163344 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13811912

[Peter Beverloo]

(In reply to comment #12)
> (From update of attachment 163344 [details])
> Rejecting attachment 163344 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1
> 
> ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> 
> Full output: http://queues.webkit.org/results/13811912

Again?.. Philippe, could you please upload a new patch with the common ChangeLog format? :).

[Philippe Liard]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 163344 [details] [details])
> > Rejecting attachment 163344 [details] [details] from commit-queue.
> > 
> > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1
> > 
> > ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> > 
> > Full output: http://queues.webkit.org/results/13811912
> 
> Again?.. Philippe, could you please upload a new patch with the common ChangeLog format? :).

Sure, I will upload a new patch :) Sorry for the inconvenience. What is the exact workflow I should adopt? Systematically upload a new patch at the end of the review adding the 'Reviewed by...'?

[Philippe Liard]

Created attachment 163595 [details]
Patch

[Peter Beverloo]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (From update of attachment 163344 [details] [details] [details])
> > > Rejecting attachment 163344 [details] [details] [details] from commit-queue.
> > > 
> > > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1
> > > 
> > > ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> > > 
> > > Full output: http://queues.webkit.org/results/13811912
> > 
> > Again?.. Philippe, could you please upload a new patch with the common ChangeLog format? :).
> 
> Sure, I will upload a new patch :) Sorry for the inconvenience. What is the exact workflow I should adopt? Systematically upload a new patch at the end of the review adding the 'Reviewed by...'?

The "Reviewed by (OOPS!)." line is part of the standard ChangeLog message, so it's unclear to me why it isn't there in the first place (unless you removed it manually). There's no need to remove the (OOPS!), the commit queue or webkit-patch will fill it in for you automatically.

An easy workflow which I've adopted is:
1) make changes
2) webkit-patch upload --request-commit

The "webkit-patch upload" command will open a bug for you, set the right title/url, prepare your ChangeLog contents, show you a diff before uploading and then uploading the patch itself, requesting the patch to be committed as well.

[Philippe Liard]

(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > (From update of attachment 163344 [details] [details] [details] [details])
> > > > Rejecting attachment 163344 [details] [details] [details] [details] from commit-queue.
> > > > 
> > > > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1
> > > > 
> > > > ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> > > > 
> > > > Full output: http://queues.webkit.org/results/13811912
> > > 
> > > Again?.. Philippe, could you please upload a new patch with the common ChangeLog format? :).
> > 
> > Sure, I will upload a new patch :) Sorry for the inconvenience. What is the exact workflow I should adopt? Systematically upload a new patch at the end of the review adding the 'Reviewed by...'?
> 
> The "Reviewed by (OOPS!)." line is part of the standard ChangeLog message, so it's unclear to me why it isn't there in the first place (unless you removed it manually). There's no need to remove the (OOPS!), the commit queue or webkit-patch will fill it in for you automatically.
> 
> An easy workflow which I've adopted is:
> 1) make changes
> 2) webkit-patch upload --request-commit
> 
> The "webkit-patch upload" command will open a bug for you, set the right title/url, prepare your ChangeLog contents, show you a diff before uploading and then uploading the patch itself, requesting the patch to be committed as well.

I see thanks. I confirm that I was removing this line manually. I didn't know that it was automatically processed.

[WebKit Review Bot]

Comment on attachment 163595 [details]
Patch

Clearing flags on attachment: 163595

Committed r128299: <http://trac.webkit.org/changeset/128299>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.