com.google.j2objc:j2objc-annotations compileOnly dependency not supported on Android

[mkubiczek]

Guava Version

33.3.0

Description

After updating guava to 33.3.0 I am getting following error when building for Android

Execution failed for task ':app:preDebugBuild'.

The following Android dependencies are set to compileOnly which is not supported:
-> com.google.j2objc:j2objc-annotations:3.0.0

Most likely introduced by this change: #6606

Example

-

Expected Behavior

Compilation passes

Actual Behavior

Compilation fails

Packages

No response

Platforms

Android

Checklist

• I agree to follow the code of conduct.

• I can reproduce the bug with the latest version of Guava available.

[cpovirk]

Huh, thanks. It looks like AGP has probably just started discouraging compileOnly dependencies, and indeed, we have j2objc-annotations in ApiElements but not RuntimeElements.

AGP's decision to discourage compileOnly is probably the right move, since it is so often misused.

And I was planning to go on to say: In this particular case, compileOnly should be safe because j2objc-annotations contains exclusively source-retention annotations. But that turns out to be untrue! While @J2ObjCIncompatible is source-retention, some of the other annotations are not. (And we use everything four other annotations that I will look at now.) I'm not 100% sure how intentional the choices of retention were, especially given that @WeakOuter is source-retention while the very similar @Weak is not. (It seems that both have been that way since their creation over a decade ago.) @RetainedWith and @ReflectionSupport are also class-retention. (@tomball in case he has any immediate thoughts before he disappears)

I'm a little surprised that AGP wouldn't have complained already for any builds that depended on Guava classes that used those annotations, since it presumably wouldn't find the annotations on the runtime classpath. I guess that it might not care if the build is configured to not include usages of those annotations in the final jar? Or maybe it doesn't use the runtime classpath even for the final optimizations? I'm not sure.

Anyway, I should put j2objc-annotations into the runtime configuration after all. Thanks for reporting this. I see similar reports on some other projects, so I'll comment on those, as well.

[cpovirk]

Ehhhh, I guess there's an interesting question as to whether even class-retention annotations (which is what we're talking about here) should be on the runtime classpath: Since they're class-retention, they can't be read by runtime reflection, so I actually could see still omitting them.

This may also explain why AGP didn't already complain about missing annotation definitions (as I commented on above): Since it knows that the annotations can't be read at runtime, it's presumably not including them in the dex, so there's no need to have references.

Still, I could see an argument that in general (even if not so much in the Android case) we should have class-retention annotations on the runtime classpath, since the runtime jar might be rewrittten by an agent. If nothing else, that's the safer default.

I see now that I'd made reference to this in #6603, but I'd since forgotten.

Theory aside, it seems clear that adding the dependency will eliminate the error that AGP users are seeing. That's probably enough reason to do it—and enough reason to avoid making further deps compileOnly in the future.

[tomball]

My (limited) understanding is that CLASS retention annotations are for compilers’ and annotation processors’ use, while RUNTIME annotations are for apps using reflection. If everyone agrees with that, I’ll be happy to review J2ObjC’s annotations for correctness. I suspect Chris is right that all should have SOURCE retention going forward. We can modify any mistakes safely because the j2objc-annotations library is versioned using Maven. El El lun, sept 23, 2024 a la(s) 8:41 a.m., Chris Povirk < ***@***.***> escribió:

…

Ehhhh, I guess there's an interesting question as to whether even *class*-retention annotations (which is what we're talking about here) should be on the runtime classpath: Since they're class-retention, they can't be read by runtime reflection, so I actually could see still omitting them. This may also explain why AGP didn't already complain about missing annotation definitions (as I commented on above): Since it knows that the annotations can't be read at runtime, it's presumably not including them in the dex, so there's no need to have references. *Still*, I could see an argument that *in general* (even if not so much in the *Android* case) we should have class-retention annotations on the runtime classpath, since the runtime jar might be rewrittten by an agent. If nothing else, that's the safer default. I see now that I'd made reference to this in #6603 <#6603>, but I'd since forgotten. Theory aside, it seems clear that adding the dependency will eliminate the error that AGP users are seeing. That's probably enough reason to do it—and enough reason to avoid making further deps compileOnly <#2824> in the future. — Reply to this email directly, view it on GitHub <#7397 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAW2JL24I4VYD3KZH3OIQZLZYASDFAVCNFSM6AAAAABOWDWDWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRYGQ4TENJUGQ> . You are receiving this because you were mentioned.Message ID: <google/guava/issues/7397/2368492544 ***@***.***>

[cpovirk]

Thanks, Tom.

As a quick test, I just tried to run the monorepo's tests with a CL that changes all the J2ObjC annotations to source-retention. That test went poorly, but the main reason for that is a bad common-library build target that accidentally bundles the J2ObjC annotations into itself, letting downstream users use the J2ObjC annotations without declaring a dependency. That bundling incidentally stops happening when I change the annotations to source-retention, so I end up with a bunch of errors for missing deps in Java compilation... :) I may try to clean that up if it doesn't look too hard, but for now, the result is that I don't have a trivial way to test the retention change.

After thinking about this some more, I do find it at least believable that J2ObjC might want to see annotations from not just the code isn't currently compiling but from its dependencies, too. (Maybe we'd at least want this during the cycle tests that still exist? But I didn't see any failures in them during my testing.) If so, that would require class retention. But I think I've managed to avoid acquiring a deep enough understanding of the transpilation process to say whether that is actually the case. I can say that, if you look at the testing sample results for my CL 677811765, you can see a number of errors in Objective-C compilation. Maybe that's a sign that the class retention is actually needed for the annotations, or maybe it's a sign of another build-system quirk like the one that's affecting the Java compilation step above.

I am reasonably happy to just include the J2ObjC annotations on the classpath regardless, since including them supports an AGP initiative that I am thinking is a good idea. So we don't need to figure out the retention change for Guava's purposes, and it doesn't need to take up any of your remaining time unless you think it would justify it for other reasons.