Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mobile-install fails with dependencies from rules_jvm_external #232

Open
fhilgers opened this issue May 18, 2024 · 7 comments
Open

mobile-install fails with dependencies from rules_jvm_external #232

fhilgers opened this issue May 18, 2024 · 7 comments

Comments

@fhilgers
Copy link

fhilgers commented May 18, 2024

When trying to use mobile-install on the example/basicapp I am getting the following stacktrace after launching the application: stacktrace.txt

I am using rules_jvm_external with an overridden rules_android to the version from rules_android git repository.

This is a patch to apply to the example:

diff --git a/examples/basicapp/MODULE.bazel b/examples/basicapp/MODULE.bazel
index aac81cc..37a891e 100644
--- a/examples/basicapp/MODULE.bazel
+++ b/examples/basicapp/MODULE.bazel
@@ -4,6 +4,14 @@ module(
 
 bazel_dep(name = "rules_java", version = "7.4.0")
 bazel_dep(name = "bazel_skylib", version = "1.3.0")
+bazel_dep(name = "rules_jvm_external", version = "6.1")
+local_path_override(
+    module_name = "rules_jvm_external",
+    path = "../rules_jvm_external/"
+)
+
+remote_android_extensions = use_extension("@rules_android//bzlmod_extensions:android_extensions.bzl", "remote_android_tools_extensions")
+use_repo(remote_android_extensions, "android_gmaven_r8", "android_tools")
 
 bazel_dep(
     name = "rules_android",
@@ -15,7 +23,21 @@ local_path_override(
     path = "../../",
 )
 
-register_toolchains(
-    "@rules_android//toolchains/android:android_default_toolchain",
-    "@rules_android//toolchains/android_sdk:android_sdk_tools",
+maven = use_extension("@rules_jvm_external//:extensions.bzl", "maven")
+
+maven.install(
+    aar_import_bzl_label = "@rules_android//android:rules.bzl",
+    artifacts = [
+       "androidx.appcompat:appcompat:1.6.1",
+       "androidx.appcompat:appcompat-resources:1.6.1",
+    ],
+    fail_if_repin_required = True,
+    lock_file = "//:manifest_install.json",
+    repositories = [
+        "https://maven.google.com",
+        "https://repo1.maven.org/maven2",
+    ],
+    resolver = "maven",
+    use_starlark_android_rules = True,
 )
+use_repo(maven, "maven")
diff --git a/examples/basicapp/WORKSPACE.bzlmod b/examples/basicapp/WORKSPACE.bzlmod
index a69bd79..589a040 100644
--- a/examples/basicapp/WORKSPACE.bzlmod
+++ b/examples/basicapp/WORKSPACE.bzlmod
@@ -1,4 +1,10 @@
 load("@rules_android//rules:rules.bzl", "android_sdk_repository")
 android_sdk_repository(
     name = "androidsdk",
-)
\ No newline at end of file
+)
+
+new_local_repository(
+    name = "androidstudio",
+    path = "android-studio",
+    build_file = "//:androidstudio.BUILD",
+)
diff --git a/examples/basicapp/java/com/basicapp/BUILD b/examples/basicapp/java/com/basicapp/BUILD
index a8fb99d..80b401c 100644
--- a/examples/basicapp/java/com/basicapp/BUILD
+++ b/examples/basicapp/java/com/basicapp/BUILD
@@ -11,4 +11,7 @@ android_library(
     srcs = ["BasicActivity.java"],
     manifest = "AndroidManifest.xml",
     resource_files = glob(["res/**"]),
+    deps = [
+      "@maven//:androidx_appcompat_appcompat"
+    ]
 )
diff --git a/examples/basicapp/java/com/basicapp/BasicActivity.java b/examples/basicapp/java/com/basicapp/BasicActivity.java
index 03c9aef..98f4824 100644
--- a/examples/basicapp/java/com/basicapp/BasicActivity.java
+++ b/examples/basicapp/java/com/basicapp/BasicActivity.java
@@ -21,10 +21,12 @@ import android.view.View;
 import android.widget.Button;
 import android.widget.TextView;
 
+import androidx.appcompat.app.AppCompatActivity;
+
 /**
  * The main activity of the Basic Sample App.
  */
-public class BasicActivity extends Activity {
+public class BasicActivity extends AppCompatActivity {
 
   @Override
   protected void onCreate(Bundle savedInstanceState) {
diff --git a/mobile_install/tools.bzl b/mobile_install/tools.bzl
index 72dd5b6..c71fd58 100644
--- a/mobile_install/tools.bzl
+++ b/mobile_install/tools.bzl
@@ -37,7 +37,7 @@ TOOL_ATTRS = dict(
         ),
     ),
     _studio_deployer = attr.label(
-        default = "//tools/android:gen_fail", # TODO(#119): Studio deployer jar to be released
+        default = "@@androidstudio//:deployer_deploy.jar", # TODO(#119): Studio deployer jar to be released
         allow_single_file = True,
         cfg = "exec",
         executable = True,

Edit: I forgot to mention that building and manually installing the apk works fine.

@fhilgers
Copy link
Author

fhilgers commented May 18, 2024

As far as I understood by looking over the implementation, mobile-install mainly uses the AndroidIdeInfo provider which has a merged R.java:

In the extract function from mobile-install/adapters/android_binary:

resource_src_jar = target[AndroidIdeInfo].resource_jar.source_jar,  # This is the R with real ids.

I am not really familiar with android but maybe this is helpful, as in the stacktrace the error is that the specific androidx/startup/R$string could not be found.

When looking at all the split apks that mobile install produces and extracting the dex files there is also only com/example/basicapp/R.class with all the merged resources.

@fhilgers
Copy link
Author

Okay there are actually two reasons why it fails:

In mobile_install/adapters/android_binary.bzl all the resources files are filtered:

    return dict(
        debug_key = utils.only(ctx.rule.files.debug_key, allow_empty = True),
        debug_signing_keys = ctx.rule.files.debug_signing_keys,
        debug_signing_lineage_file = utils.only(ctx.rule.files.debug_signing_lineage_file, allow_empty = True),
        key_rotation_min_sdk = ctx.rule.attr.key_rotation_min_sdk,
        merged_manifest = target[AndroidIdeInfo].generated_manifest,
        native_libs = target[AndroidIdeInfo].native_libs,
        package = target[AndroidIdeInfo].java_package,
        resource_apk = target[AndroidIdeInfo].resource_apk,
        resource_src_jar = target[AndroidIdeInfo].resource_jar.source_jar,  # This is the R with real ids.
        aar_native_libs_info = MIAndroidAarNativeLibsInfo(
            transitive_native_libs = transitive_native_libs,
        ),
        android_dex_info = providers.make_mi_android_dex_info(
            dex_shards = dex(
                ctx,
                target[JavaInfo].runtime_output_jars +
                #filter_jars(
                #    ctx.label.name + "_resources.jar",
                #    target[JavaInfo].runtime_output_jars,
                #) +
                (
                    [
                        extension_registry_class_jar,
                    ] if extension_registry_class_jar else []
                ),
                target[JavaInfo].transitive_compile_time_jars,
            ),
            deps = providers.collect(MIAndroidDexInfo, ctx.rule.attr.deps),
        ),
        # TODO(djwhang): It wasteful to collect packages in
        # android_resources_info, rather we should be looking to pull them
        # from the resources_v3_info.
        android_resources_info = providers.make_mi_android_resources_info(
            package = target[AndroidIdeInfo].java_package,
            deps = providers.collect(
                MIAndroidResourcesInfo,
                ctx.rule.attr.deps,
            ),
        ),
        java_resources_info = providers.make_mi_java_resources_info(
            deps = providers.collect(
                MIJavaResourcesInfo,
                ctx.rule.attr.deps,
            ),
        ),
        apk = target[AndroidIdeInfo].signed_apk,
    )

Commenting that filtering out makes another problem apparent:

The rules_jvm_external ruleset uses jvm_import instead of java_import for importing jar files. I have not yet looked into the difference but changing that to java_import leads to everything working with mobile-install.

What is the reason all resources files are filtered out? Is it important or was it just overlooked that it filters out external resources. Should they be added somewhere else?

@fhilgers
Copy link
Author

The reason why jvm_import does not work is that it has to be added to the adapters in the mobile-install aspects.

@fhilgers
Copy link
Author

So the problem was threefold:

  1. No aspect for jvm_import which is used by rules_jvm_external instead of java import -> The aspect for java_import can be reused for that
  2. Package name is inferred by directory structure or the explicit attribute on aar_import. This is not set by rules_jvm_external. There is alread a Todo label to instead retrieve the package name from AndroidManifest.xml -> I have implemented the package name retrieval from manifest and that works fine.
  3. The aar_import aspect ignores StarlarkAndroidResourcesInfo -> Try to use that if available

Let me know if this makes sense and whether these proposed fixes are appropriate, so that I can open a pr.

@ted-xie
Copy link
Contributor

ted-xie commented May 23, 2024

Hi, thanks for filing this issue!

  1. I think jvm_import only exists in rules_jvm_external. I believe they had their own reasons to write their own rule for dealing with imported jars rather than re-using java_import.
  2. Usually package name inferred by directory structure works well enough. Can you elaborate why this doesn't work for your case? I also don't see where you've implemented the package name retrieval in the patch you shared.
  3. I think we will end up implementing this in the near future, as we finish up the Starlark migration effort for rules_android and fully delete both native Android rules and native Android providers from Bazel.

@fhilgers
Copy link
Author

rules_jvm_external is not setting the package field in aar_import and as the BUILD file with all imports is in the root of the generated repository the package name is empty. My usecase was just importing any library from maven but because the package name is unset the resources are missing or in the wrong location for them to be found. The patch I shared was before I found out the problems, it was just importing packages from maven via rules_jvm_external where the problem happened. I can clean up a bit and make a pr so it can be looked at.

@ajsinclair
Copy link
Contributor

The reason why jvm_import does not work is that it has to be added to the adapters in the mobile-install aspects.

This commit should resolve that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants