Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2905353003:
Enable multidex for ChromePublicTests app (Closed)
DescriptionUse multidex capable application for chrome_public_test_apk.
https://codereview.chromium.org/2839983002 enabled multidex build to avoid the max limit of methods. But the tests app was not multidex aware on L and below devices (M and above working fine due to support in Android). Tests on waterfall were failing to find the needed classes.
BUG=726516
Review-Url: https://codereview.chromium.org/2905353003
Cr-Commit-Position: refs/heads/master@{#480537}
Committed: https://chromium.googlesource.com/chromium/src/+/7ce5ca5baaa4efaa739ce6b5fe2444e010a5eb06
Patch Set 1 #
Total comments: 1
Patch Set 2 : Always turn on multidex to avoid MultiDexApplication not being included in apk when in debug mode #
Total comments: 4
Patch Set 3 : Addressed John's comments. #Patch Set 4 : Patch using MainDex #
Messages
Total messages: 26 (10 generated)
Description was changed from ========== Enable multidex for ChromePublicTests BUG=726516 ========== to ========== Enable multidex for ChromePublicTests app https://codereview.chromium.org/2839983002 enabled multidex build to avoid the max limit of methods. But the tests app was not multidex aware on L and below devices (M and above working fine due to support in Android). Tests on waterfall were failing to find the needed classes. BUG=726516 ==========
aluo@chromium.org changed reviewers: + dtrainor@chromium.org, jbudorick@chromium.org
ping
Were the failing tests limited to only those that use the Activities contained in that application? BaseChromiumInstrumentationTestRunner should handle multidex installation for the normal case... https://codereview.chromium.org/2905353003/diff/1/chrome/android/javatests/An... File chrome/android/javatests/AndroidManifest.xml (right): https://codereview.chromium.org/2905353003/diff/1/chrome/android/javatests/An... chrome/android/javatests/AndroidManifest.xml:18: android:name="android.support.multidex.MultiDexApplication" This should likely be BaseChromiumApplication instead.
On 2017/05/30 19:44:26, jbudorick wrote: > Were the failing tests limited to only those that use the Activities contained > in that application? BaseChromiumInstrumentationTestRunner should handle > multidex installation for the normal case... I believe that didn't work because the provider was needed as the manifest was being parsed: E/AndroidRuntime(21433): java.lang.RuntimeException: Unable to get provider org.chromium.chrome.test.TestContentProvider: java.lang.ClassNotFoundException: Didn't find class "org.chromium.chrome.test.TestContentProvider" on path: DexPathList[[zip file "/system/framework/android.test.runner.jar", zip file "/data/app/org.chromium.chrome.tests-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome.tests-1, /vendor/lib, /system/lib]] > > https://codereview.chromium.org/2905353003/diff/1/chrome/android/javatests/An... > File chrome/android/javatests/AndroidManifest.xml (right): > > https://codereview.chromium.org/2905353003/diff/1/chrome/android/javatests/An... > chrome/android/javatests/AndroidManifest.xml:18: > android:name="android.support.multidex.MultiDexApplication" > This should likely be BaseChromiumApplication instead. yes that should work too since it overrode the attachBaseContext method, however it didn't work when I tried it, no ideas why yet.
Description was changed from ========== Enable multidex for ChromePublicTests app https://codereview.chromium.org/2839983002 enabled multidex build to avoid the max limit of methods. But the tests app was not multidex aware on L and below devices (M and above working fine due to support in Android). Tests on waterfall were failing to find the needed classes. BUG=726516 ========== to ========== Use multidex capable application for chrome_public_test_apk. https://codereview.chromium.org/2839983002 enabled multidex build to avoid the max limit of methods. But the tests app was not multidex aware on L and below devices (M and above working fine due to support in Android). Tests on waterfall were failing to find the needed classes. BUG=726516 ==========
On 2017/05/30 23:52:35, aluo wrote: > On 2017/05/30 19:44:26, jbudorick wrote: > > Were the failing tests limited to only those that use the Activities contained > > in that application? BaseChromiumInstrumentationTestRunner should handle > > multidex installation for the normal case... > I believe that didn't work because the provider was needed as the manifest was > being parsed: > E/AndroidRuntime(21433): java.lang.RuntimeException: Unable to get provider > org.chromium.chrome.test.TestContentProvider: java.lang.ClassNotFoundException: > Didn't find class "org.chromium.chrome.test.TestContentProvider" on path: > DexPathList[[zip file "/system/framework/android.test.runner.jar", zip file > "/data/app/org.chromium.chrome.tests-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome.tests-1, > /vendor/lib, /system/lib]] > > > > > > > https://codereview.chromium.org/2905353003/diff/1/chrome/android/javatests/An... > > File chrome/android/javatests/AndroidManifest.xml (right): > > > > > https://codereview.chromium.org/2905353003/diff/1/chrome/android/javatests/An... > > chrome/android/javatests/AndroidManifest.xml:18: > > android:name="android.support.multidex.MultiDexApplication" > > This should likely be BaseChromiumApplication instead. > > yes that should work too since it overrode the attachBaseContext method, however > it didn't work when I tried it, no ideas why yet. Using BaseChromiumApplication to do multidex install didn't work because BaseChromiumApplication relies on the generated BuildConfig.java to set the return value of isMultidexEnabled() (from ./base/android/java/templates/BuildConfig.template) and the BuildConfig.java was generated using the chrome_public_apk target in which multidex was only enabled for debug builds. So this causes BaseChromiumApplication to skip the multidex install on non-debug builds (which is the build type we actually need multidex for the test target). By default BuildConfig.java is only generated for non-test targets. If we force generate_buildconfig_java in the chrome_public_test_apk, then we run into problem where 2 versions of the BuildConfig.java classes being used in the apk and this causes build failure: Error: Can't write [/usr/local/google/code/clankium1/src/out-gn/Debug/gen/chrome/android/chrome_public_test_apk__apk/chrome_public_test_apk__apk.proguard.jar] (Can't read [/usr/local/google/code/clankium1/src/out-gn/Debug/gen/chrome/android/chrome_public_apk/chrome_public_apk.jar] (Duplicate zip entry [chrome_public_apk.jar:org/chromium/base/BuildConfig.class])) So sticking with MultiDexApplication is a good way to go, wdyt? uploaded new patch set to take care of debug builds.
Re MultiDexApplication: sgtm Now that you mention it, I remember dealing with the same thing w/ BuildConfig. I tried having a few different ways of implementing a test-specific BuildConfig, but it proved a bit too problematic. https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn... chrome/android/BUILD.gn:918: # the dex limit. Using android.support.multidex.MultiDexApplication This is not the right place for this comment. It'd be fine in the manifest, though. https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn... chrome/android/BUILD.gn:922: enable_multidex = true I don't think this is the right way to ensure MultiDexApplication is included. Instead, I think you could do this by adding //third_party/android_tools:android_support_multidex_java to deps above.
Put comments in correct places. https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn... chrome/android/BUILD.gn:918: # the dex limit. Using android.support.multidex.MultiDexApplication On 2017/06/06 05:44:59, jbudorick wrote: > This is not the right place for this comment. It'd be fine in the manifest, > though. Done. https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn... chrome/android/BUILD.gn:922: enable_multidex = true On 2017/06/06 05:44:59, jbudorick wrote: > I don't think this is the right way to ensure MultiDexApplication is included. > Instead, I think you could do this by adding > //third_party/android_tools:android_support_multidex_java to deps above. That wasn't enough, still couldn't find class in debug mode: android.support.multidex.MultiDexApplication" on path: DexPathList[[zip file "/system/framework/android.test.runner.jar", zip file "/data/app/org.chromium.chrome.tests-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome.tests-1, /vendor/lib, /system/lib]] E/AndroidRuntime( 6139): at android.app.LoadedApk.makeApplication(LoadedApk.java:507) E/AndroidRuntime( 6139): at android.app.ActivityThread.handleBindApplication(ActivityThread.java:4301) E/AndroidRuntime( 6139): at android.app.ActivityThread.access$1500(ActivityThread.java:135) E/AndroidRuntime( 6139): at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1256) E/AndroidRuntime( 6139): at android.os.Handler.dispatchMessage(Handler.java:102) E/AndroidRuntime( 6139): at android.os.Looper.loop(Looper.java:136) E/AndroidRuntime( 6139): at android.app.ActivityThread.main(ActivityThread.java:5001) E/AndroidRuntime( 6139): at java.lang.reflect.Method.invokeNative(Native Method) E/AndroidRuntime( 6139): at java.lang.reflect.Method.invoke(Method.java:515) E/AndroidRuntime( 6139): at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:785) E/AndroidRuntime( 6139): at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:601) E/AndroidRuntime( 6139): at dalvik.system.NativeStart.main(Native Method) E/AndroidRuntime( 6139): Caused by: java.lang.ClassNotFoundException: Didn't find class "android.support.multidex.MultiDexApplication" on path: DexPathList[[zip file "/system/framework/android.test.runner.jar", zip file "/data/app/org.chromium.chrome.tests-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome.tests-1, /vendor/lib, /system/lib]] E/AndroidRuntime( 6139): at dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:56) E/AndroidRuntime( 6139): at java.lang.ClassLoader.loadClass(ClassLoader.java:497) E/AndroidRuntime( 6139): at java.lang.ClassLoader.loadClass(ClassLoader.java:457) E/AndroidRuntime( 6139): at android.app.Instrumentation.newApplication(Instrumentation.java:975) E/AndroidRuntime( 6139): at android.app.LoadedApk.makeApplication(LoadedApk.java:502) E/AndroidRuntime( 6139): ... 11 more enable_multidex does other extra stuff and appears to be needed for the MultiDexApplication to work correctly. Besides, the test target depends on //base:base_java which already depends on this.
On 2017/06/07 00:52:17, aluo wrote: > Put comments in correct places. > > https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn > File chrome/android/BUILD.gn (right): > > https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn... > chrome/android/BUILD.gn:918: # the dex limit. Using > android.support.multidex.MultiDexApplication > On 2017/06/06 05:44:59, jbudorick wrote: > > This is not the right place for this comment. It'd be fine in the manifest, > > though. > > Done. > > https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn... > chrome/android/BUILD.gn:922: enable_multidex = true > On 2017/06/06 05:44:59, jbudorick wrote: > > I don't think this is the right way to ensure MultiDexApplication is included. > > Instead, I think you could do this by adding > > //third_party/android_tools:android_support_multidex_java to deps above. > > That wasn't enough, still couldn't find class in debug mode: > android.support.multidex.MultiDexApplication" on path: DexPathList[[zip file > "/system/framework/android.test.runner.jar", zip file > "/data/app/org.chromium.chrome.tests-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome.tests-1, > /vendor/lib, /system/lib]] > E/AndroidRuntime( 6139): at > android.app.LoadedApk.makeApplication(LoadedApk.java:507) > E/AndroidRuntime( 6139): at > android.app.ActivityThread.handleBindApplication(ActivityThread.java:4301) > E/AndroidRuntime( 6139): at > android.app.ActivityThread.access$1500(ActivityThread.java:135) > E/AndroidRuntime( 6139): at > android.app.ActivityThread$H.handleMessage(ActivityThread.java:1256) > E/AndroidRuntime( 6139): at > android.os.Handler.dispatchMessage(Handler.java:102) > E/AndroidRuntime( 6139): at android.os.Looper.loop(Looper.java:136) > E/AndroidRuntime( 6139): at > android.app.ActivityThread.main(ActivityThread.java:5001) > E/AndroidRuntime( 6139): at java.lang.reflect.Method.invokeNative(Native > Method) > E/AndroidRuntime( 6139): at java.lang.reflect.Method.invoke(Method.java:515) > E/AndroidRuntime( 6139): at > com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:785) > E/AndroidRuntime( 6139): at > com.android.internal.os.ZygoteInit.main(ZygoteInit.java:601) > E/AndroidRuntime( 6139): at dalvik.system.NativeStart.main(Native Method) > E/AndroidRuntime( 6139): Caused by: java.lang.ClassNotFoundException: Didn't > find class "android.support.multidex.MultiDexApplication" on path: > DexPathList[[zip file "/system/framework/android.test.runner.jar", zip file > "/data/app/org.chromium.chrome.tests-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome.tests-1, > /vendor/lib, /system/lib]] > E/AndroidRuntime( 6139): at > dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:56) > E/AndroidRuntime( 6139): at > java.lang.ClassLoader.loadClass(ClassLoader.java:497) > E/AndroidRuntime( 6139): at > java.lang.ClassLoader.loadClass(ClassLoader.java:457) > E/AndroidRuntime( 6139): at > android.app.Instrumentation.newApplication(Instrumentation.java:975) > E/AndroidRuntime( 6139): at > android.app.LoadedApk.makeApplication(LoadedApk.java:502) > E/AndroidRuntime( 6139): ... 11 more > > > enable_multidex does other extra stuff and appears to be needed for the > MultiDexApplication to work correctly. Besides, the test target depends on > //base:base_java which already depends on this.
On 2017/06/07 00:52:17, aluo wrote: > Put comments in correct places. > > https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn > File chrome/android/BUILD.gn (right): > > https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn... > chrome/android/BUILD.gn:918: # the dex limit. Using > android.support.multidex.MultiDexApplication > On 2017/06/06 05:44:59, jbudorick wrote: > > This is not the right place for this comment. It'd be fine in the manifest, > > though. > > Done. > > https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn... > chrome/android/BUILD.gn:922: enable_multidex = true > On 2017/06/06 05:44:59, jbudorick wrote: > > I don't think this is the right way to ensure MultiDexApplication is included. > > Instead, I think you could do this by adding > > //third_party/android_tools:android_support_multidex_java to deps above. > > That wasn't enough, still couldn't find class in debug mode: > android.support.multidex.MultiDexApplication" on path: DexPathList[[zip file > "/system/framework/android.test.runner.jar", zip file > "/data/app/org.chromium.chrome.tests-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome.tests-1, > /vendor/lib, /system/lib]] > E/AndroidRuntime( 6139): at > android.app.LoadedApk.makeApplication(LoadedApk.java:507) > E/AndroidRuntime( 6139): at > android.app.ActivityThread.handleBindApplication(ActivityThread.java:4301) > E/AndroidRuntime( 6139): at > android.app.ActivityThread.access$1500(ActivityThread.java:135) > E/AndroidRuntime( 6139): at > android.app.ActivityThread$H.handleMessage(ActivityThread.java:1256) > E/AndroidRuntime( 6139): at > android.os.Handler.dispatchMessage(Handler.java:102) > E/AndroidRuntime( 6139): at android.os.Looper.loop(Looper.java:136) > E/AndroidRuntime( 6139): at > android.app.ActivityThread.main(ActivityThread.java:5001) > E/AndroidRuntime( 6139): at java.lang.reflect.Method.invokeNative(Native > Method) > E/AndroidRuntime( 6139): at java.lang.reflect.Method.invoke(Method.java:515) > E/AndroidRuntime( 6139): at > com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:785) > E/AndroidRuntime( 6139): at > com.android.internal.os.ZygoteInit.main(ZygoteInit.java:601) > E/AndroidRuntime( 6139): at dalvik.system.NativeStart.main(Native Method) > E/AndroidRuntime( 6139): Caused by: java.lang.ClassNotFoundException: Didn't > find class "android.support.multidex.MultiDexApplication" on path: > DexPathList[[zip file "/system/framework/android.test.runner.jar", zip file > "/data/app/org.chromium.chrome.tests-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome.tests-1, > /vendor/lib, /system/lib]] > E/AndroidRuntime( 6139): at > dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:56) > E/AndroidRuntime( 6139): at > java.lang.ClassLoader.loadClass(ClassLoader.java:497) > E/AndroidRuntime( 6139): at > java.lang.ClassLoader.loadClass(ClassLoader.java:457) > E/AndroidRuntime( 6139): at > android.app.Instrumentation.newApplication(Instrumentation.java:975) > E/AndroidRuntime( 6139): at > android.app.LoadedApk.makeApplication(LoadedApk.java:502) > E/AndroidRuntime( 6139): ... 11 more > > > enable_multidex does other extra stuff and appears to be needed for the > MultiDexApplication to work correctly. Besides, the test target depends on > //base:base_java which already depends on this. I still don't think enabling multidex in debug builds is the right course of action. Let's talk about this tomorrow.
On 2017/06/07 03:10:12, jbudorick wrote: > On 2017/06/07 00:52:17, aluo wrote: > > Put comments in correct places. > > > > https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn > > File chrome/android/BUILD.gn (right): > > > > > https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn... > > chrome/android/BUILD.gn:918: # the dex limit. Using > > android.support.multidex.MultiDexApplication > > On 2017/06/06 05:44:59, jbudorick wrote: > > > This is not the right place for this comment. It'd be fine in the manifest, > > > though. > > > > Done. > > > > > https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn... > > chrome/android/BUILD.gn:922: enable_multidex = true > > On 2017/06/06 05:44:59, jbudorick wrote: > > > I don't think this is the right way to ensure MultiDexApplication is > included. > > > Instead, I think you could do this by adding > > > //third_party/android_tools:android_support_multidex_java to deps above. > > > > That wasn't enough, still couldn't find class in debug mode: > > android.support.multidex.MultiDexApplication" on path: DexPathList[[zip file > > "/system/framework/android.test.runner.jar", zip file > > > "/data/app/org.chromium.chrome.tests-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome.tests-1, > > /vendor/lib, /system/lib]] > > E/AndroidRuntime( 6139): at > > android.app.LoadedApk.makeApplication(LoadedApk.java:507) > > E/AndroidRuntime( 6139): at > > android.app.ActivityThread.handleBindApplication(ActivityThread.java:4301) > > E/AndroidRuntime( 6139): at > > android.app.ActivityThread.access$1500(ActivityThread.java:135) > > E/AndroidRuntime( 6139): at > > android.app.ActivityThread$H.handleMessage(ActivityThread.java:1256) > > E/AndroidRuntime( 6139): at > > android.os.Handler.dispatchMessage(Handler.java:102) > > E/AndroidRuntime( 6139): at android.os.Looper.loop(Looper.java:136) > > E/AndroidRuntime( 6139): at > > android.app.ActivityThread.main(ActivityThread.java:5001) > > E/AndroidRuntime( 6139): at java.lang.reflect.Method.invokeNative(Native > > Method) > > E/AndroidRuntime( 6139): at java.lang.reflect.Method.invoke(Method.java:515) > > E/AndroidRuntime( 6139): at > > > com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:785) > > E/AndroidRuntime( 6139): at > > com.android.internal.os.ZygoteInit.main(ZygoteInit.java:601) > > E/AndroidRuntime( 6139): at dalvik.system.NativeStart.main(Native Method) > > E/AndroidRuntime( 6139): Caused by: java.lang.ClassNotFoundException: Didn't > > find class "android.support.multidex.MultiDexApplication" on path: > > DexPathList[[zip file "/system/framework/android.test.runner.jar", zip file > > > "/data/app/org.chromium.chrome.tests-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome.tests-1, > > /vendor/lib, /system/lib]] > > E/AndroidRuntime( 6139): at > > dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:56) > > E/AndroidRuntime( 6139): at > > java.lang.ClassLoader.loadClass(ClassLoader.java:497) > > E/AndroidRuntime( 6139): at > > java.lang.ClassLoader.loadClass(ClassLoader.java:457) > > E/AndroidRuntime( 6139): at > > android.app.Instrumentation.newApplication(Instrumentation.java:975) > > E/AndroidRuntime( 6139): at > > android.app.LoadedApk.makeApplication(LoadedApk.java:502) > > E/AndroidRuntime( 6139): ... 11 more > > > > > > enable_multidex does other extra stuff and appears to be needed for the > > MultiDexApplication to work correctly. Besides, the test target depends on > > //base:base_java which already depends on this. > > I still don't think enabling multidex in debug builds is the right course of > action. Let's talk about this tomorrow. wasn't able to get to it this week, will work on this next week, thanks.
On 2017/06/10 00:01:59, aluo wrote: > On 2017/06/07 03:10:12, jbudorick wrote: > > On 2017/06/07 00:52:17, aluo wrote: > > > Put comments in correct places. > > > > > > > https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn > > > File chrome/android/BUILD.gn (right): > > > > > > > > > https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn... > > > chrome/android/BUILD.gn:918: # the dex limit. Using > > > android.support.multidex.MultiDexApplication > > > On 2017/06/06 05:44:59, jbudorick wrote: > > > > This is not the right place for this comment. It'd be fine in the > manifest, > > > > though. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2905353003/diff/20001/chrome/android/BUILD.gn... > > > chrome/android/BUILD.gn:922: enable_multidex = true > > > On 2017/06/06 05:44:59, jbudorick wrote: > > > > I don't think this is the right way to ensure MultiDexApplication is > > included. > > > > Instead, I think you could do this by adding > > > > //third_party/android_tools:android_support_multidex_java to deps above. > > > > > > That wasn't enough, still couldn't find class in debug mode: > > > android.support.multidex.MultiDexApplication" on path: DexPathList[[zip file > > > "/system/framework/android.test.runner.jar", zip file > > > > > > "/data/app/org.chromium.chrome.tests-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome.tests-1, > > > /vendor/lib, /system/lib]] > > > E/AndroidRuntime( 6139): at > > > android.app.LoadedApk.makeApplication(LoadedApk.java:507) > > > E/AndroidRuntime( 6139): at > > > android.app.ActivityThread.handleBindApplication(ActivityThread.java:4301) > > > E/AndroidRuntime( 6139): at > > > android.app.ActivityThread.access$1500(ActivityThread.java:135) > > > E/AndroidRuntime( 6139): at > > > android.app.ActivityThread$H.handleMessage(ActivityThread.java:1256) > > > E/AndroidRuntime( 6139): at > > > android.os.Handler.dispatchMessage(Handler.java:102) > > > E/AndroidRuntime( 6139): at android.os.Looper.loop(Looper.java:136) > > > E/AndroidRuntime( 6139): at > > > android.app.ActivityThread.main(ActivityThread.java:5001) > > > E/AndroidRuntime( 6139): at java.lang.reflect.Method.invokeNative(Native > > > Method) > > > E/AndroidRuntime( 6139): at > java.lang.reflect.Method.invoke(Method.java:515) > > > E/AndroidRuntime( 6139): at > > > > > > com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:785) > > > E/AndroidRuntime( 6139): at > > > com.android.internal.os.ZygoteInit.main(ZygoteInit.java:601) > > > E/AndroidRuntime( 6139): at dalvik.system.NativeStart.main(Native Method) > > > E/AndroidRuntime( 6139): Caused by: java.lang.ClassNotFoundException: Didn't > > > find class "android.support.multidex.MultiDexApplication" on path: > > > DexPathList[[zip file "/system/framework/android.test.runner.jar", zip file > > > > > > "/data/app/org.chromium.chrome.tests-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome.tests-1, > > > /vendor/lib, /system/lib]] > > > E/AndroidRuntime( 6139): at > > > dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:56) > > > E/AndroidRuntime( 6139): at > > > java.lang.ClassLoader.loadClass(ClassLoader.java:497) > > > E/AndroidRuntime( 6139): at > > > java.lang.ClassLoader.loadClass(ClassLoader.java:457) > > > E/AndroidRuntime( 6139): at > > > android.app.Instrumentation.newApplication(Instrumentation.java:975) > > > E/AndroidRuntime( 6139): at > > > android.app.LoadedApk.makeApplication(LoadedApk.java:502) > > > E/AndroidRuntime( 6139): ... 11 more > > > > > > > > > enable_multidex does other extra stuff and appears to be needed for the > > > MultiDexApplication to work correctly. Besides, the test target depends on > > > //base:base_java which already depends on this. > > > > I still don't think enabling multidex in debug builds is the right course of > > action. Let's talk about this tomorrow. > > wasn't able to get to it this week, will work on this next week, thanks. As discussed yesterday, marking the test providers with @MainDex also solves the problem. Running through all the tests now to see that they all work, will take a while. We can use this approach if it is indeed preferable to enable_multidex, John?
Patch 3 is using enable_multidex while patch 4 is using MainDex annotation. I'm leaning towards 3 since it avoids issues with future providers also needing to be @MainDex, but there could be other reasons which I'm not aware of for using the MainDex approach so totally fine with either.
On 2017/06/17 00:37:00, aluo wrote: > Patch 3 is using enable_multidex while patch 4 is using MainDex annotation. I'm > leaning towards 3 since it avoids issues with future providers also needing to > be @MainDex, but there could be other reasons which I'm not aware of for using > the MainDex approach so totally fine with either. I'd like all of our multidex installation to be handled by ChromiumMultiDexInstaller. While I don't think there are adverse side-effects to not doing so at the moment, I think it'd be easy to get into such a situation in the future. As such, I'd prefer patchset 4 to patchset 3 for now.
The CQ bit was checked by aluo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/19 15:17:10, jbudorick wrote: > On 2017/06/17 00:37:00, aluo wrote: > > Patch 3 is using enable_multidex while patch 4 is using MainDex annotation. > I'm > > leaning towards 3 since it avoids issues with future providers also needing to > > be @MainDex, but there could be other reasons which I'm not aware of for using > > the MainDex approach so totally fine with either. > > I'd like all of our multidex installation to be handled by > ChromiumMultiDexInstaller. While I don't think there are adverse side-effects to > not doing so at the moment, I think it'd be easy to get into such a situation in > the future. As such, I'd prefer patchset 4 to patchset 3 for now. lgtm? exception in ios-simulator-xcode-clang was due to connection problem: remoteFailed: [Failure instance: Traceback (failure with no frames): <class 'twisted.internet.error.ConnectionLost'>: Connection to the other side was lost in a non-clean fashion.
On 2017/06/19 19:11:19, aluo wrote: > On 2017/06/19 15:17:10, jbudorick wrote: > > On 2017/06/17 00:37:00, aluo wrote: > > > Patch 3 is using enable_multidex while patch 4 is using MainDex annotation. > > I'm > > > leaning towards 3 since it avoids issues with future providers also needing > to > > > be @MainDex, but there could be other reasons which I'm not aware of for > using > > > the MainDex approach so totally fine with either. > > > > I'd like all of our multidex installation to be handled by > > ChromiumMultiDexInstaller. While I don't think there are adverse side-effects > to > > not doing so at the moment, I think it'd be easy to get into such a situation > in > > the future. As such, I'd prefer patchset 4 to patchset 3 for now. > > lgtm? exception in ios-simulator-xcode-clang was due to connection problem: > remoteFailed: [Failure instance: Traceback (failure with no frames): <class > 'twisted.internet.error.ConnectionLost'>: Connection to the other side was lost > in a non-clean fashion. ah, oops. lgtm
The CQ bit was checked by aluo@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497900623145500,
"parent_rev": "c7b66ceeecb9fb5174c6d96b360666b5f1cb73da", "commit_rev":
"7ce5ca5baaa4efaa739ce6b5fe2444e010a5eb06"}
Message was sent while issue was closed.
Description was changed from ========== Use multidex capable application for chrome_public_test_apk. https://codereview.chromium.org/2839983002 enabled multidex build to avoid the max limit of methods. But the tests app was not multidex aware on L and below devices (M and above working fine due to support in Android). Tests on waterfall were failing to find the needed classes. BUG=726516 ========== to ========== Use multidex capable application for chrome_public_test_apk. https://codereview.chromium.org/2839983002 enabled multidex build to avoid the max limit of methods. But the tests app was not multidex aware on L and below devices (M and above working fine due to support in Android). Tests on waterfall were failing to find the needed classes. BUG=726516 Review-Url: https://codereview.chromium.org/2905353003 Cr-Commit-Position: refs/heads/master@{#480537} Committed: https://chromium.googlesource.com/chromium/src/+/7ce5ca5baaa4efaa739ce6b5fe24... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/7ce5ca5baaa4efaa739ce6b5fe24... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
