bazelbuild / bazel

a fast, scalable, multi-language and extensible build system
https://bazel.build
Apache License 2.0
23k stars 4.03k forks source link

Android Mobile Install chooses platform name instead of correct abi for native_lib when --incompatible_enable_android_toolchain_resolution is set #15837

Open erikmchut opened 2 years ago

erikmchut commented 2 years ago

When --incompatible_enable_android_toolchain_resolution is set, bazel uses the --android_platforms=xxx flag to chose target architecture. The bazel mobile-install method is currently using this platform name as the ABI name and then failing to install libraries when this name doesn't match an Android ABI [armeabi-v7a, arm64-v8a, x86, x86_64].

Example

# Assume the following is defined in //bzl:BUILD
platform(
    name = "android_arm64",
    constraint_values = [
        "@platforms//os:android",
        "@platforms//cpu:arm64",
    ],
)

Running the following command:

bazel mobile-install --incompatible_enable_android_toolchain_resolution --android_platforms=//bzl:android_arm64 
 //bzl/examples/android:hello-android 

will result in the following build warnings:

W0707 09:54:55.281064 4334355840 incremental_install.py:541] No native libs for device ABI 'arm64-v8a'. App has native libs for ABIs: android_arm64
I0707 09:54:55.412683 4334355840 incremental_install.py:600] Updating 0 native libs...

I believe this occurs due to the logic here, which takes the NativeLib key as the abi name, which in the case of platforms is the platform target base name. https://github.com/bazelbuild/bazel/blob/c7460e084687bdb70a826ad9078b046e7ff9f2f0/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinaryMobileInstall.java#L426

A limited workaround is to define alias with a name that matches an android ABI, for example:

# Temporary workaround be defining an alias matching the Android abi name defined in //bzl:BUILD
alias(
    name = "arm64-v8a",
    actual = ":android_arm64",
)
# The following will properly install.
bazel mobile-install --incompatible_enable_android_toolchain_resolution --android_platforms=//bzl:arm64-v8a
 //bzl/examples/android:hello-android 
ahumesky commented 1 year ago

cc @katre

katre commented 1 year ago

Is there a list of allowed names?

Is there a reason to not just use these names as the platform names?

colinrgodsey commented 1 year ago

any update here? also running into this

erikmchut commented 6 months ago

Is there a list of allowed names?

Is there a reason to not just use these names as the platform names?

Yes, I have custom toolchains and platforms with names like //mytools:android_arm64, used in numerous places throughout the codebase. These platform names have the OS and CPU in them, and it isn't feasible to rename them to match Android ABI names.

This problem arose again following upgrade to Bazel 7, which has enabled --incompatible_enable_android_toolchain_resolution by default. Fortunately it can still be disabled with --noincompatible_enable_android_toolchain_resolution while this issue persists.

The relevant Bazel code with this logic can now be found here: https://github.com/bazelbuild/bazel/blob/ad2113058b49fae469cd1a65d4c98df9b8fbed87/src/main/java/com/google/devtools/build/lib/rules/android/NativeLibs.java#L50

jdcormie commented 4 months ago

This is affecting us in grpc as well. We upgraded to Bazel 7 & Android Platforms in https://github.com/grpc/grpc/pull/36116 and named our platforms android_$ARCH as suggested by the blog's official announcement But now targets like //examples/android/binder/java/io/grpc/binder/cpp/exampleserver:app generate an APK that cannot be installed ([INSTALL_FAILED_NO_MATCHING_ABIS: Failed to extract native libraries, res=-113]) because the native libs aren't where Android expects to find them (lib/android_arm64/libapp.so instead of the correct lib/arm64-v8a/libapp.so, for example).

The problem isn't limited to mobile-install either.

We can certainly rename our platforms to work around this, but the docs really do suggest that you can name your platform whatever you want. Seems to me this should be labelled a bug rather than a support issue.

katre commented 4 months ago

Argh, I should update the blog post.

The best way to name your android platforms is to have the actual name be the same as the CPU architecture. So, instead of platform(name = android_arm64), useplatform(name = "arm64-v8a")`.

This is fairly restrictive, I understand. Since all of this is being migrated to Starlark, I suggest also making an issue at https://github.com/bazelbuild/rules_android so that they can tackle this for the future.

katre commented 4 months ago

For the rules_android owners: instead of using the platform name, we should define a specific key in the exec_properties dict to use. The platform would then be:

platform(
    name = "anything",
    constraint_values = ...,
    exec_properties = {
        "android_cpu_identifier": "arm64-v8a",
    },
)

But that's a more complex change than I can pick up now, and needs to be done in both native and Starlark rules.

(This is also why I want to rename exec_properties to properties, but attribute renames are hard).

jdcormie commented 4 months ago

Thanks for confirming katre!

Since these lib/ directory names are a fixed part of android and the same for everyone, what would you think of an alternative where the native & Starlark rules looked at the cpu constraint and did the right thing without any special platform configuration?

katre commented 4 months ago

That might be possible: it depends on exactly where the information is needed (and how the constraint names are mapped to the Android identifiers).

Also complicating is that the implementations will be entirely different for native vs Starlark rules.