Closed Bencodes closed 2 years ago
Bencodes
commented
2 years ago cc: @ted-xie There may still be issues with this PR https://github.com/bazelbuild/bazel/commit/ae247145b897d0f41cdf2b317f5c7b856845e303#diff-ac45a531edff6b8e6694f669184fc3625c869c84710142175e2caae08e159a9dR301-R308.
Happy to test against our codebase if you have any suggestions.
ted-xie
commented
2 years ago I'll try to repro this. It looks like the easiest path to repro would be generating an artifical case with >2^16 classes and >2^16 fields.
ahumesky
commented
2 years ago Thanks for letting us know you're running into this -- we updated to the latest version of d8 and that broke our multidex integration, so multidex is effectively always off. We're running into this for an internal use case as well. I'm working on a fix, hope to have that in by the end of the week.
Could you file a separate issue for Expected stack map table for method with non-linear control flow?
Bencodes
commented
2 years ago @ahumesky opened up a new issue for the stack map able warnings here https://github.com/bazelbuild/bazel/issues/15751
ahumesky
commented
2 years ago As a workaround, you can try adding --experimental_use_dex_splitter_for_incremental_dexing which should avoid the codepaths that are failing here. We've had that enabled internally for several years already, so we might just default it to be enabled and simplify the code.
Bencodes
commented
2 years ago
experimental_use_dex_splitter_for_incremental_dexing
I'll give this a try internally and report back.
Bencodes
commented
2 years ago Still seeing the same error with experimental_use_dex_splitter_for_incremental_dexing enabled. We don't have incremental dexing enabled internally, so maybe that's why.
ahumesky
commented
2 years ago Ah, yeah that only applies to incremental dexing, which is enabled by default. Non-incremental dexing ("monolithic") is yet a different set of actions / code paths.
Bencodes
commented
2 years ago It still fails with this configuration. Are you using something different internally (flipping incremental_dexing to false also fails)?
# Enable experimental d8 merger with incremental dexing
build \
--define=android_dexmerger_tool=d8_dexmerger \
--define=android_incremental_dexing_tool=d8_dexbuilder \
--define=android_standalone_dexing_tool=d8_compat_dx \
--experimental_use_dex_splitter_for_incremental_dexing \
--incremental_dexing=truehmmm it seems to work for my (admitedly simple) test app:
$ ~/downloads/bazel-6.0.0-pre.20220526.1-linux-x86_64 build java/com/app:app --define=android_dexmerger_tool=d8_dexmerger \
--define=android_incremental_dexing_tool=d8_dexbuilder \
--define=android_standalone_dexing_tool=d8_compat_dx \
--experimental_use_dex_splitter_for_incremental_dexing \
--incremental_dexing=true
INFO: Build option --define has changed, discarding analysis cache.
INFO: Analyzed target //java/com/app:app (0 packages loaded, 7188 targets configured).
INFO: Found 1 target...
Target //java/com/app:app up-to-date:
bazel-bin/java/com/app/app_deploy.jar
bazel-bin/java/com/app/app_unsigned.apk
bazel-bin/java/com/app/app.apk
INFO: Elapsed time: 0.689s, Critical Path: 0.01s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total actionjava/com/app/BUILD:
android_binary(
name = "app",
manifest = "AndroidManifest.xml",
deps = [
":applib",
],
multidex = "native",
)
android_library(
name = "applib",
srcs = [
"MainActivity.java",
],
manifest = "AndroidManifest.xml",
deps = [
":lib1",
":lib2",
]
)
android_library(
name = "lib1",
srcs = ["Lib1.java"],
)
android_library(
name = "lib2",
srcs = ["Lib2.java"],
)java/com/app/AndroidManifest.xml:
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="com.app"
android:versionCode="1"
android:versionName="1.0" >
<uses-sdk
android:minSdkVersion="21"
android:targetSdkVersion="21" />
<application android:label="Test App" >
<activity
android:name="app.MainActivity"
android:label="App" >
<intent-filter>
<action android:name="android.intent.action.MAIN" />
<category android:name="android.intent.category.LAUNCHER" />
</intent-filter>
</activity>
</application>
</manifest>java/com/app/MainActivity.java:
package com.app;
import android.app.Activity;
public class MainActivity extends Activity {
}And then Lib1.java and Lib2.java were something like:
echo "package com.app;" > Lib1.java
echo "public class Lib1 {" >> Lib1.java
for i in $(seq 35000); do echo "public int foo_$i() { return $i ; }" >> Lib1.java; done
echo "}" >> Lib1.javaand then similar for Lib2.java
Bencodes
commented
2 years ago I see the difference. For debug builds that aren't meant to be distributed to the play store, we pass dex_shards. Commenting this out and only using multidex = "native" works fine with the above bazelrc.
The other case that fails is our release builds where we do not provide dex_shards, but we do provide proguard specs. Passing either of these two values will fail.
ahumesky
commented
2 years ago Are you setting dex_shards for use with mobile-install? From a build perspective, incremental dexing obviates dex_shards. I still need to add a proguard spec to verify that that case works with d8+multidex -- sounds like that's broken too
Bencodes
commented
2 years ago Yes we are as per the mobile install docs which might be outdated at this point.
I may have misspoken about dex_shards breaking builds, but passing proguard_specs definitely does when using the above bazelrc.
ahumesky
commented
2 years ago The mobile-install docs are still correct with respect to dex_shards, I just mean that for non-mobile-install builds, incremental dexing is better than dex_shards (that is, both dex_shards and incremental dexing are ways to speed up the build by reducing the amount of duplicate dexing. it just so happens that mobile-install is wired to dex_shards, and not to the outputs of incremental dexing)
ahumesky
commented
2 years ago I can repro the failure with proguard with this config:
-keep class com.app.**
-keep class com.app.Lib1 {
public int foo*();
}
-keep class com.app.Lib2 {
public int bar*();
}Try adding these flags:
--experimental_incremental_dexing_after_proguard_by_default
--experimental_incremental_dexing_after_proguard=50New crash now for release builds using the above flags in combination with the above bazelrc.
java.util.concurrent.ExecutionException: com.android.dx.cf.code.SimException: ERROR in com.google.common.collect.e.a:(IILjava/util/function/IntFunction;Ljava/util/Comparator;)Ljava/util/Spliterator;: invoking a static interface method java.util.stream.IntStream.range:(II)Ljava/util/stream/IntStream; strictly requires --min-sdk-version >= 24 (blocked at current API level 13)
at java.base/java.util.concurrent.FutureTask.report(FutureTask.java:122)
at java.base/java.util.concurrent.FutureTask.get(FutureTask.java:191)
at com.google.devtools.build.android.dexer.DexBuilder.produceDexArchive(DexBuilder.java:256)
at com.google.devtools.build.android.dexer.DexBuilder.processRequest(DexBuilder.java:221)
at com.google.devtools.build.android.dexer.DexBuilder.runPersistentWorker(DexBuilder.java:173)
at com.google.devtools.build.android.dexer.DexBuilder.main(DexBuilder.java:121)
Caused by: com.android.dx.cf.code.SimException: ERROR in com.google.common.collect.e.a:(IILjava/util/function/IntFunction;Ljava/util/Comparator;)Ljava/util/Spliterator;: invoking a static interface method java.util.stream.IntStream.range:(II)Ljava/util/stream/IntStream; strictly requires --min-sdk-version >= 24 (blocked at current API level 13)
at com.android.dx.cf.code.Simulator.fail(Simulator.java:947)
at com.android.dx.cf.code.Simulator.checkInvokeInterfaceSupported(Simulator.java:917)
at com.android.dx.cf.code.Simulator.access$500(Simulator.java:43)
at com.android.dx.cf.code.Simulator$SimVisitor.visitConstant(Simulator.java:687)
at com.android.dx.cf.code.BytecodeArray.parseInstruction(BytecodeArray.java:764)
at com.android.dx.cf.code.Simulator.simulate(Simulator.java:117)
at com.android.dx.cf.code.Ropper.processBlock(Ropper.java:789)
at com.android.dx.cf.code.Ropper.doit(Ropper.java:744)
at com.android.dx.cf.code.Ropper.convert(Ropper.java:349)
at com.android.dx.dex.cf.CfTranslator.processMethods(CfTranslator.java:309)
at com.android.dx.dex.cf.CfTranslator.translate0(CfTranslator.java:150)
at com.android.dx.dex.cf.CfTranslator.translate(CfTranslator.java:102)
at com.google.devtools.build.android.dexer.Dexing.addToDexFile(Dexing.java:219)
at com.google.devtools.build.android.dexer.DexConverter.toDexFile(DexConverter.java:31)
at com.google.devtools.build.android.dexer.DexConversionEnqueuer$ClassToDex.call(DexConversionEnqueuer.java:173)
at com.google.devtools.build.android.dexer.DexConversionEnqueuer$ClassToDex.call(DexConversionEnqueuer.java:156)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
at java.base/java.lang.Thread.run(Thread.java:829)Also breaks when it hits compose class files:
java.util.concurrent.ExecutionException: com.android.dx.cf.code.SimException: ERROR in androidx.compose.ui.text.android.b.clone:()Ljava/lang/Object;: invoking a default interface method java.text.CharacterIterator.clone:()Ljava/lang/Object; strictly requires --min-sdk-version >= 24 (blocked at current API level 13)
at java.base/java.util.concurrent.FutureTask.report(FutureTask.java:122)
at java.base/java.util.concurrent.FutureTask.get(FutureTask.java:191)
at com.google.devtools.build.android.dexer.DexBuilder.produceDexArchive(DexBuilder.java:256)
at com.google.devtools.build.android.dexer.DexBuilder.processRequest(DexBuilder.java:221)
at com.google.devtools.build.android.dexer.DexBuilder.runPersistentWorker(DexBuilder.java:173)
at com.google.devtools.build.android.dexer.DexBuilder.main(DexBuilder.java:121)
Caused by: com.android.dx.cf.code.SimException: ERROR in androidx.compose.ui.text.android.b.clone:()Ljava/lang/Object;: invoking a default interface method java.text.CharacterIterator.clone:()Ljava/lang/Object; strictly requires --min-sdk-version >= 24 (blocked at current API level 13)
at com.android.dx.cf.code.Simulator.fail(Simulator.java:947)
at com.android.dx.cf.code.Simulator.checkInvokeInterfaceSupported(Simulator.java:917)
at com.android.dx.cf.code.Simulator.access$500(Simulator.java:43)
at com.android.dx.cf.code.Simulator$SimVisitor.visitConstant(Simulator.java:687)
at com.android.dx.cf.code.BytecodeArray.parseInstruction(BytecodeArray.java:764)
at com.android.dx.cf.code.Simulator.simulate(Simulator.java:117)
at com.android.dx.cf.code.Ropper.processBlock(Ropper.java:789)
at com.android.dx.cf.code.Ropper.doit(Ropper.java:744)
at com.android.dx.cf.code.Ropper.convert(Ropper.java:349)
at com.android.dx.dex.cf.CfTranslator.processMethods(CfTranslator.java:309)
at com.android.dx.dex.cf.CfTranslator.translate0(CfTranslator.java:150)
at com.android.dx.dex.cf.CfTranslator.translate(CfTranslator.java:102)
at com.google.devtools.build.android.dexer.Dexing.addToDexFile(Dexing.java:219)
at com.google.devtools.build.android.dexer.DexConverter.toDexFile(DexConverter.java:31)
at com.google.devtools.build.android.dexer.DexConversionEnqueuer$ClassToDex.call(DexConversionEnqueuer.java:173)
at com.google.devtools.build.android.dexer.DexConversionEnqueuer$ClassToDex.call(DexConversionEnqueuer.java:156)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
at java.base/java.lang.Thread.run(Thread.java:829)That looks like an error from dx, as opposed to d8. Are you running dx with that build? Also looks like a problem with java 8 and desugaring ("invoking a default interface"), do you have desugaring off? (e.g., targeting recent devices?)
Bencodes
commented
2 years ago The presence of proguard_specs in combination with --experimental_incremental_dexing_after_proguard_by_default=true pushes the build back into dx. Once you flip --experimental_incremental_dexing_after_proguard_by_default back to false the build gets kicked back into using d8.
ahumesky
commented
2 years ago Seems this is the reason: https://cs.opensource.google/bazel/bazel/+/master:tools/android/BUILD.tools;l=75-78;drc=e0f4f159a4ab60af10df3de81932da43061a2ac0
Unfortunately there's no flag to adjust that dexbuilder_after_proguard target in the tools BUILD file. So we'll need to change it to something like this:
config_setting(
name = "dx_standalone_dexer",
values = {"define": "android_standalone_dexing_tool=dx_compat_dx"},
)
alias(
name = "dexbuilder_after_proguard",
actual = select({
":dx_standalone_dexer": "//src/tools/android/java/com/google/devtools/build/android/dexer:DexBuilder",
"//conditions:default": ":d8_compat_dx",
}),
)
java_binary(
name = "d8_compat_dx",
main_class = "com.google.devtools.build.android.r8.CompatDx",
runtime_deps = [
"//src/tools/android/java/com/google/devtools/build/android/r8",
],
)com.google.devtools.build.android.r8.CompatDx is what we use internally for dexbuilder_after_proguard, however that's not updated to use workers, so that needs --nouse_workers_with_dexbuilder.
But that reveals the next problem:
Exception in thread "main" com.google.devtools.build.android.r8.CompatDxCompilationError: File does not exist: @bazel-out/k8-fastbuild/bin/java/com/app/_dx/app/shard1.jar.dex.zip-0.paramsSo more investigation is needed.
ahumesky
commented
2 years ago Actually I followed the wrong set of selects internally, it's not CompatDx but com.google.devtools.build.android.r8.CompatDexBuilder. There's already a java_binary in the tools BUILD file for that, so all that's needed is
# Matches dx_standalone_dexer in android_sdk_repository_template.bzl. Not using the one there to avoid
# a dependency on the android sdk.
config_setting(
name = "dx_standalone_dexer",
values = {"define": "android_standalone_dexing_tool=dx_compat_dx"},
)
alias(
name = "dexbuilder_after_proguard",
actual = select({
":dx_standalone_dexer": "//src/tools/android/java/com/google/devtools/build/android/dexer:DexBuilder",
"//conditions:default": ":d8_dexbuilder",
}),
)So looks like we should update the tools BUILD file as above, and set the above mentioned flags:
--experimental_use_dex_splitter_for_incremental_dexing--experimental_incremental_dexing_after_proguard_by_default--experimental_incremental_dexing_after_proguard=50Really it would be nice to just delete the flags and simplify the code, but we're currently setting those flags internally in our rc files, so we unfortunately can't delete them in one go.
Bencodes
commented
2 years ago @ahumesky I confirmed that the above alias select does in fact work for our release builds. Happy to open up a PR for this if needed.
ahumesky
commented
2 years ago Thanks for confirming, I have a change out that should be submitted early next week.
mauriciogg
commented
2 years ago Im getting
_bazel_root/1557532aa844cabca730c318accb6923/external/androidsdk/BUILD.bazel:13:25 Extracting interface @androidsdk//:dx_jar_import failed: 1 input file(s) do not existWith bazel 5.xx, latest build-tools version 33.0.0 and changes in this file. The dx jar is not longer distributed with the latest build tools. I'm I missing other changes available at bazel head?
mauriciogg
commented
2 years ago Im getting
_bazel_root/1557532aa844cabca730c318accb6923/external/androidsdk/BUILD.bazel:13:25 Extracting interface @androidsdk//:dx_jar_import failed: 1 input file(s) do not existWith bazel 5.xx, latest build-tools version
33.0.0and changes in this file. The dx jar is not longer distributed with the latest build tools. I'm I missing other changes available at bazel head?
Looks like --experimental_use_dex_splitter_for_incremental_dexing creates an implicit dependency on dx.jar via //src/tools/android/java/com/google/devtools/build/android/dexer:DexFileSplitter and not using this function results in Error: Cannot fit requested classes in a single dex file (# methods: 1513902 > 65536 ; # fields: 889322 > 65536). Try supplying a main-dex list, so atm bazel does not support build tools >30.x.x. Is this a correct assessment or am I missing something?
ahumesky
commented
2 years ago Hmm, the changes in https://github.com/bazelbuild/bazel/issues/15742#issuecomment-1170639928 should break that dependency. If you patched that code in and rebuilt bazel, do you happen to have --define=android_standalone_dexing_tool=dx_compat_dx? That's the only way I could think of that dexer:DexFileSplitter would still be invoked
ahumesky
commented
2 years ago And for what it's worth, I tested these changes with build tools version 33.0.0
mauriciogg
commented
2 years ago Oh I see what was going with my code. I have the condition if (dexArchives.size() == 1 || !multidex) { and at master is if (dexArchives.size() == 1) { only so i was triggering the dexsplitter.
ahumesky
commented
2 years ago Thanks for following up Mauricio.
I had hoped to submit the change to fix this early this week (i.e. mostly https://github.com/bazelbuild/bazel/issues/15742#issuecomment-1170639928), however there are a number of failing tests because they rely on the current code paths. I have most of those fixed, but there are a few other tests that need to be updated, and apparently @bazel_tools//src/tools/android/java/com/google/devtools/build/android/dexer:dexer is somehow breaking. Since that's the tool we no longer want to use, it would be ideal to just remove it, however it's not clear how much work it is to delete all that code. If I can fix all that tomorrow, I may get this submitted then or (again) early next week.
ahumesky
commented
2 years ago I've asked for the fix to be cherrypicked into 5.3.0: https://github.com/bazelbuild/bazel/pull/15917
ahumesky
commented
2 years ago It doesn't look like 5.2.0 actually has this problem because it still defaults to dx, only some of the recent rolling releases have this issue, so it doesn't look like we need to cherrypick this (plus, this change depends on several other changes that will be tricky to get into 5.3.0)
mauriciogg
commented
2 years ago @Bencodes did you test this change with proguard disabled? I think there are still some issues when running unproguarded builds.
Bencodes
commented
2 years ago @mauriciogg I believe so. I can go back and check against master if needed to be sure. What issues are you seeing and is it worth re-opening this issue to do some further investigation?
ahumesky
commented
2 years ago For what it's worth, there are tests here for both the proguard and non-proguard cases: https://github.com/bazelbuild/bazel/commit/ce55639c3ef2b9bd703d64026c40df0b7485b6a5#diff-247298e5ca47519afadcf45341d3089048f4476152c78f046cd651305a49b4c2R202
mauriciogg
commented
2 years ago the dx.jar file does not exist in latest build tools. I dont see a way to avoid that dependency when going through the else branch. Are dexArchives expected to be == 1 at this point?
if (dexArchives.size() == 1) {
checkState(inclusionFilterJar == null);
createDexMergerAction(ruleContext, "minimal", dexArchives, classesDex, mainDexList, dexopts);
} else {
SpecialArtifact shardsToMerge =
createSharderAction(ruleContext, dexArchives, mainDexList, dexopts, inclusionFilterJar);
Artifact multidexShards = createTemplatedMergerActions(ruleContext, shardsToMerge, dexopts);
// TODO(b/69431301): avoid this action and give the files to apk build action directly
createZipMergeAction(ruleContext, multidexShards, classesDex);
}
}alias(
name = "dexsharder",
actual = "//src/tools/android/java/com/google/devtools/build/android/dexer:DexFileSplitter",
)java_binary(
name = "DexFileSplitter",
main_class = "com.google.devtools.build.android.dexer.DexFileSplitter",
visibility = ["//tools/android:__subpackages__"],
runtime_deps = [":dexer"],
)this is the output or running native_multidex_test with tools version 33.0.0 (I deleted all other tests, there was also a failuer with missing mainDexClasses file)
mgalindo@C02G37A4MD6R bazel % /tmp/bazel test src/test/shell/bazel/android:android_integration_test --define=android_dexmerger_tool=d8_dexmerger --define=android_incremental_dexing_tool=d8_dexbuilder --define=android_standalone_dexing_tool=d8_compat_dx --experimental_use_dex_splitter_for_incremental_dexing=true
Starting local Bazel server and connecting to it...
DEBUG: /private/var/tmp/_bazel_mgalindo/f01e3e6b36dade5f901c06f92cd379f2/external/build_bazel_rules_nodejs/index.bzl:122:10: WARNING: check_rules_nodejs_version has been removed. This is a no-op, please remove the call.
INFO: Analyzed target //src/test/shell/bazel/android:android_integration_test (416 packages loaded, 10293 targets configured).
INFO: Found 1 test target...
INFO: Deleting stale sandbox base /private/var/tmp/_bazel_mgalindo/f01e3e6b36dade5f901c06f92cd379f2/sandbox
ERROR: /Users/mgalindo/Snapchat/Dev/bazel/src/test/shell/bazel/android/BUILD:31:16: Middleman _middlemen/src_Stest_Sshell_Sbazel_Sandroid_Sandroid_Uintegration_Utest-runfiles failed: missing input file '@androidsdk//:build-tools/33.0.0/lib/dx.jar'
ERROR: /Users/mgalindo/Snapchat/Dev/bazel/src/test/shell/bazel/android/BUILD:31:16: Middleman _middlemen/src_Stest_Sshell_Sbazel_Sandroid_Sandroid_Uintegration_Utest-runfiles failed: 1 input file(s) do not exist
Target //src/test/shell/bazel/android:android_integration_test failed to build
Use --verbose_failures to see the command lines of failed build steps.
ERROR: /Users/mgalindo/Snapchat/Dev/bazel/src/test/shell/bazel/android/BUILD:31:16 Middleman _middlemen/src_Stest_Sshell_Sbazel_Sandroid_Sandroid_Uintegration_Utest-runfiles failed: 1 input file(s) do not exist
INFO: Elapsed time: 10.929s, Critical Path: 1.15s
INFO: 1 process: 1 internal.
FAILED: Build did NOT complete successfully
//src/test/shell/bazel/android:android_integration_test FAILED TO BUILDDexFileSplitter is still based on dx, unfortunately both internally and with bazel, so a new tool (i.e. wrapper around d8) has to be written (vs just open sourcing what we have internally). I see there isn't an issue filed for this, so I'll create one and we can continue there. We're aiming to get rid of all dependencies on dx for Bazel 6.0.
mauriciogg
commented
2 years ago @ahumesky do you have an idea of what would be the ETA for the D8 based wrapper?
ahumesky
commented
2 years ago Filed this: https://github.com/bazelbuild/bazel/issues/15957
The earliest we could see this is probably a few weeks from now, but ideally well in enough time to get this into Bazel 6.0
Description of the bug:
After updating to rolling release
6.0.0-pre.20220526.1,CompatDxproduces a massive amount of warnings and eventually fails with the following error:What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.
You might need to have a classpath large enough to exceed a single dex file to reproduce this.
We are using the following dexing configuration with
multidexmode set tonative:Reverting this PR resolves the issue. More specifically just reverting the changes in
tools/android/android_sdk_repository_template.bzlis enough to build successfully again.Which operating system are you running Bazel on?
Mac OS
What is the output of
bazel info release?6.0.0-pre.20220526.1
If
bazel info releasereturnsdevelopment versionor(@non-git), tell us how you built Bazel.No response
What's the output of
git remote get-url origin; git rev-parse master; git rev-parse HEAD?No response
Have you found anything relevant by searching the web?
No response
Any other information, logs, or outputs that you want to share?
No response