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

java_plugin can't depend on android_library #2517

Closed
ronshapiro opened this issue Feb 9, 2017 · 9 comments
Closed

java_plugin can't depend on android_library #2517

ronshapiro opened this issue Feb 9, 2017 · 9 comments
Labels
P2 We'll consider working on this in future. (Assignee optional) stale Issues or PRs that are stale (no activity for 30 days) team-Android Issues for Android team type: feature request

Comments

@ronshapiro
Copy link
Contributor

Please provide the following information. The more we know about your system and use case, the more easily and likely we can help.

Description of the problem / feature request / question:

In Dagger we have an android_library and we would like to have an annotation processor (java_plugin) that validates the usage of the library based on some annotations in the android_library. I wrote a java_library to enclose the annotation processor code and then added that to the deps of the java_plugin, but bazel doesn't seem to like that (though this works internally). If I switch the annotation processor library to an android_library rule, it still complains. Each error is something along the lines of:

ERROR: java/dagger/android/processor/BUILD:44:12: in deps attribute of java_plugin rule
//java/dagger/android/processor:plugin: '//java/dagger/android/processor:processor' does
not have mandatory provider 'link_params' or 'java_common.provider' and android_library
rule '//java/dagger/android/processor:processor' is misplaced here (expected cc_binary,
cc_library, genrule, genproto, java_import, java_library, java_proto_library,
java_lite_proto_library, proto_library, sh_binary or sh_library).
ERROR: Analysis of target '//java/dagger/android/processor:plugin' failed; build aborted

As an aside, I couldn't find any documentation for what link_params or java_common.provider are.

@aj-michael suggested as an interim hack to have a java_import(name = "foo", jars = ["//java/dagger/android/processor:libprocessor.jar"]) to avoid this issue, which is working but feels like a hack.

If possible, provide a minimal example to reproduce the problem:

Here's the commit that I'd like to push to our repo, but it's failing on travis: google/dagger@303bd11#diff-0afa968a073f33b9f552877b10c01445

Environment info

  • Operating System:
    Linux 14.04

  • Bazel version (output of bazel info release):
    0.4.4

@kchodorow kchodorow added the P2 We'll consider working on this in future. (Assignee optional) label Feb 9, 2017
ronshapiro added a commit to google/dagger that referenced this issue Feb 9, 2017
deps of a java_plugin.

Also fix the return type of AndroidMapKeyValidator#process, which was
depending on an unreleased version of auto-common.

[0]: bazelbuild/bazel#2517

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=147052219
@aj-michael
Copy link
Contributor

So I think that java_plugin not being able to depend on android_library is intentional. The only reason that this works internally is to support robolectric, which is its own special case.

@aj-michael
Copy link
Contributor

I think that the ideal solution here is to expose android.jar to the java rules. This is a little hairy, because android.jar from the Android SDK doesn't really do much, all the methods throw RuntimeExceptions. Dagger's use case seems to only need some constants, which are present in android.jar.

The best current solution is probably using a java_library that depends on @androidsdk//:platforms/android-25/android.jar instead of an android_library. Unfortunately, this would mean a greater split between internal and external BUILD files.

@ahumesky Do you have any thoughts?

@ahumesky
Copy link
Contributor

ahumesky commented Feb 11, 2017

Yes I think a java_import around @androidsdk//:platforms/android-25/android.jar and depending on that would probably work. Your plugin code though would be compiled against the android 25 jar, which might not be the jar that's being used on the android rule that the plugin is being run on, since that's determined by --android_sdk.

One solution to this is to create config_settings in the android sdk build file for each platform detected in the local android sdk, e.g.

config_setting(
  name = "config_sdk_25",
  values = { "android_sdk": "@androidsdk//:sdk-25" }
)
config_setting(
  name = "config_sdk_24",
  values = { "android_sdk": "@androidsdk//:sdk-24" }
)
config_setting(
  name = "config_sdk-23",
  values = { "android_sdk": "@androidsdk//:sdk-23" }
)
...

and then create an import that selects the right jar:

java_import(
  name = "android_jar",
  jars = select({
    ":config_sdk_25": [":platforms/android-25/android.jar"],
    ":config_sdk_24": [":platforms/android-24/android.jar"],
    ":config_sdk_23": [":platforms/android-23/android.jar"],
    ....
}))

then @androidsdk//:android_jar will be correct with respect to --android_sdk.

However, there is a shortcut, which is to depend on //tools/defaults:android_jar, which is a sort of "synthetic" target that will pick the right android.jar like the above config_setting + select setup. Apparently the rule that generates that synthetic target wasn't updated for some provider validation that's been added, but it should still work if you wrap that in a java_import. For example:

java_binary(
  name = "Foo",
  srcs = ["Foo.java"],
  deps = [":android_jar"],
)

java_import(
  name = "android_jar",
  jars = ["//tools/defaults:android_jar"],
)

Foo.java:

package foo;
import android.app.Activity;
public class Foo {
  public static void main(String[] args) {
    System.out.println(Activity.class.getMethods().length);
  }
}

--android_sdk=@androidsdk//:sdk-24 gives 335 methods, with sdk 23 gives 314.

I think that //tools/defaults:android_jar was added before these sorts of things were possible through config_setting + select so we may want to remove it in favor of config_setting + select.

ronshapiro added a commit to google/dagger that referenced this issue Feb 13, 2017
deps of a java_plugin.

Also fix the return type of AndroidMapKeyValidator#process, which was
depending on an unreleased version of auto-common.

[0]: bazelbuild/bazel#2517

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=147052219
ronshapiro added a commit to google/dagger that referenced this issue Feb 13, 2017
deps of a java_plugin.

Also fix the return type of AndroidMapKeyValidator#process, which was
depending on an unreleased version of auto-common.

[0]: bazelbuild/bazel#2517

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=147052219
@ronshapiro
Copy link
Contributor Author

The issue is not that I want a java_library as the main code for my java_plugin, it's that I want to write a java_library which uses the Java code from an android_library as the basis for the actual analysis. The annotations for the annotation processor live in an Android library and should be packaged as such. We need to ship them as an .aar

@aj-michael
Copy link
Contributor

Oh I see, I misunderstood. I'm not sure how the Bazel team feels about java rules depending on android rules. The only reason this works internally is because the robolectric test rule is considered a java rule and all the java rules share a common list of allowed dep rules. I suspect if that were redesigned, that would no longer be the case.

IMO, the best way to do this is to explicitly reference the libname.jar output of android_library. That was it is explicit that you only want the classes and do not want any assets, manifests, etc. If the java_import is too verbose, you can also put the jar's directly in the deps of the java_library. E.g.

java_library(
    name = "processor",
    srcs = glob(["*.java"]),
    deps = [
        "//java/dagger/android:libandroid.jar",
        "//java/dagger/android/support:libsupport.jar",
        ...
    ],
)

The Bazel team has talked about allowing rules to depend on each other via declared providers, which may someday happen and would solve this case.

@ittaiz
Copy link
Member

ittaiz commented Feb 14, 2017 via email

@ahumesky
Copy link
Contributor

I would say that right thing to do would be to put the annotations in their own java_library and have the annotation processor code and the android code depend on the common library, but aar's do not contain their transitive dependencies, so that doesn't help you here. As aj-michael said, and as you've implemented, the best way forward is to depend on the jar output of the android_library.

@ronshapiro
Copy link
Contributor Author

That is all true, as well as the fact that the Annotations referenced android types (e.g. Class<? extends Activity>) so at some point it would have to have some connection to Android

@jin jin added the team-Android Issues for Android team label Aug 11, 2018
copybara-service bot pushed a commit to google/dagger that referenced this issue May 26, 2021
…l artifact.

Due mainly to BasicAnnotationProcessor.ProcessingStep, the dagger-android-processor required a dependency on the dagger-android annotation classes. However, due to restrictions on java_plugin depending on an android_library, mentioned in bazelbuild/bazel#2517, we had to create a java_import to work around this issue. However, soon even the java_import workaround will no longer work.

This CL fixes the above issues by switching to the new BasicAnnotaitonProcessor.Step, which allows processing annotations via String instead of Class, which allows us to remove the processors dependency on the dagger-android API altogether.

RELNOTES=the Dagger artifact, "dagger-android-jarimpl", has been removed. This was an internal-only artifact, so its removal shouldn't affect users.
PiperOrigin-RevId: 375386035
copybara-service bot pushed a commit to google/dagger that referenced this issue May 26, 2021
…l artifact.

Due mainly to BasicAnnotationProcessor.ProcessingStep, the dagger-android-processor required a dependency on the dagger-android annotation classes. However, due to restrictions on java_plugin depending on an android_library, mentioned in bazelbuild/bazel#2517, we had to create a java_import to work around this issue. However, soon even the java_import workaround will no longer work.

This CL fixes the above issues by switching to the new BasicAnnotaitonProcessor.Step, which allows processing annotations via String instead of Class, which allows us to remove the processors dependency on the dagger-android API altogether.

RELNOTES=the Dagger artifact, "dagger-android-jarimpl", has been removed. This was an internal-only artifact, so its removal shouldn't affect users.
PiperOrigin-RevId: 375386035
copybara-service bot pushed a commit to google/dagger that referenced this issue May 26, 2021
…l artifact.

Due mainly to BasicAnnotationProcessor.ProcessingStep, the dagger-android-processor required a dependency on the dagger-android annotation classes. However, due to restrictions on java_plugin depending on an android_library, mentioned in bazelbuild/bazel#2517, we had to create a java_import to work around this issue. However, soon even the java_import workaround will no longer work.

This CL fixes the above issues by switching to the new BasicAnnotaitonProcessor.Step, which allows processing annotations via String instead of Class, which allows us to remove the processors dependency on the dagger-android API altogether.

RELNOTES=the Dagger artifact, "dagger-android-jarimpl", has been removed. This was an internal-only artifact, so its removal shouldn't affect users.
PiperOrigin-RevId: 375962581
@sgowroji sgowroji added the stale Issues or PRs that are stale (no activity for 30 days) label Feb 3, 2023
@sgowroji
Copy link
Member

Hi there! We're doing a clean up of old issues and will be closing this one. Please reopen if you’d like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) stale Issues or PRs that are stale (no activity for 30 days) team-Android Issues for Android team type: feature request
Projects
None yet
Development

No branches or pull requests

7 participants