Add support for an additional GN arg that disables Java 8 desugaring |
|||
Issue descriptionThe transition to Java 8 has introduced a post-processing step that desugars Java 8 compiled classes to Java 7. Cronet embedders would like to have an option to skip the desugaring step and use the original Java 8 Cronet jars instead. There are multiple reasons in doing so. One of them is that the embedders may have their own desugaring libraries that are not compatible with the ones used in Chromium. Using an incompatible library may cause runtime errors. Packaging the desugaring support classes with Cronet classes is also not optimal since it increases the size of the Jars and still may cause error due to class name conflicts.
,
Aug 25 2017
Hmm, this change already went in to address this: https://chromium-review.googlesource.com/c/chromium/src/+/620987 Is there more that's required?
,
Aug 25 2017
Yes. Cronet code is also distributed in "cronet_impl_common_java.jar", "cronet_impl_platform_java.jar" and "cronet_api.jar". In particular, "cronet_impl_platform_java.jar" currently contains dependency on "com/google/devtools/build/android/desugar/runtime/ThrowableExtension" class. We could copy the jars from the pre-processing location (i.e. _compile_java.javac.jars) and rename them but having a switch seems a better option. We would also like to test the jars that we ship.
,
Aug 25 2017
Adding a GN arg sounds good to me then :).
,
Aug 25 2017
,
Aug 25 2017
Thanks, zpeng@. Really appreciated the help. Once your CL lands, I will enable the new GN flag on Cronet buildbots.
,
Aug 25 2017
Sorry - I'm not sure I took a close enough look at this before. If you disable desugaring globally, how will you be able to "test the jars that you ship"? Ninja will not produce an APK dex files that work on pre-O devices without desugaring. There's also been a recent change to do additional chromium-specific byte-code rewriting, which causes all android_library targets to have an implicit dependency on //build/android/build_hooks_android_java. You probably don't want this either for Cronet. I think the best option actually is to just distribute the .javc.jar files rather than make any changes here.
,
Aug 25 2017
Is it possible to add the GN arg and assign the value to it depending on the Android version of the device connected to the bot? E.g., if the test is going to run on 'O' then set skip_desugaring=true; but for the pre-O devices set skip_desugaring=false? This way we will achieve better coverage.
,
Aug 25 2017
We could add such an arg, but I think it's very rare for apps to deploy minSdkVersion apks, and doesn't really address the problem in this bug.
,
Aug 25 2017
#8: that is definitely not something we want to do, though we could do something similar. We typically limit the devices on which a particular bot triggers test to a single configuration, so we'd instead configure a bot that triggered O tests w/ skip_desugaring (or what have you) and bots that triggered pre-O tests w/o it.
,
Aug 25 2017
I have uploaded a CL with a fix to copy Java 8 jars to the Cronet distribution package: https://chromium-review.googlesource.com/c/chromium/src/+/636623
,
Aug 28 2017
,
Aug 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/87cec1cd9c3d2cf845e299ffd29f2231f80130cc commit 87cec1cd9c3d2cf845e299ffd29f2231f80130cc Author: kapishnikov <kapishnikov@chromium.org> Date: Mon Aug 28 14:50:34 2017 Copy pre-desugared Cronet JARs to the distribution package BUG= 759000 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Change-Id: Ic11afd6b1fcafabc4fee043c7e0a679afee7ed74 Reviewed-on: https://chromium-review.googlesource.com/636623 Reviewed-by: Andrew Grieve <agrieve@chromium.org> Reviewed-by: Helen Li <xunjieli@chromium.org> Commit-Queue: Andrei Kapishnikov <kapishnikov@chromium.org> Cr-Commit-Position: refs/heads/master@{#497760} [modify] https://crrev.com/87cec1cd9c3d2cf845e299ffd29f2231f80130cc/components/cronet/android/BUILD.gn
,
Aug 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/73a893b065730fc797d8cee8ea4153f09f9e5435 commit 73a893b065730fc797d8cee8ea4153f09f9e5435 Author: kapishnikov <kapishnikov@chromium.org> Date: Tue Aug 29 22:49:33 2017 Don't rename cronet_api.jar while copying pre-desugared Java8 files BUG= 759000 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Change-Id: I3958d7578e6e725f767963e69f15c58d7d548a0c Reviewed-on: https://chromium-review.googlesource.com/641659 Reviewed-by: Helen Li <xunjieli@chromium.org> Reviewed-by: Paul Jensen <pauljensen@chromium.org> Commit-Queue: Andrei Kapishnikov <kapishnikov@chromium.org> Cr-Commit-Position: refs/heads/master@{#498266} [modify] https://crrev.com/73a893b065730fc797d8cee8ea4153f09f9e5435/components/cronet/android/BUILD.gn |
|||
►
Sign in to add a comment |
|||
Comment 1 by jbudorick@chromium.org
, Aug 25 2017Components: -Infra>Client>Android Build
Status: Available (was: Untriaged)