New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 759000 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Last visit > 30 days ago
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Add support for an additional GN arg that disables Java 8 desugaring

Project Member Reported by kapishnikov@chromium.org, Aug 25 2017

Issue description

The 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.
 
Cc: zpeng@chromium.org jbudorick@chromium.org agrieve@chromium.org
Components: -Infra>Client>Android Build
Status: Available (was: Untriaged)
Hmm, this change already went in to address this:
https://chromium-review.googlesource.com/c/chromium/src/+/620987

Is there more that's required?
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.
Adding a GN arg sounds good to me then :). 

Comment 5 by zpeng@chromium.org, Aug 25 2017

Owner: zpeng@chromium.org
Status: Started (was: Available)
Thanks, zpeng@. Really appreciated the help. Once your CL lands, I will enable the new GN flag on Cronet buildbots.
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.
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.
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.
#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.
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
Status: WontFix (was: Started)
Project Member

Comment 13 by bugdroid1@chromium.org, 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

Project Member

Comment 14 by bugdroid1@chromium.org, 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