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.

[mkubiczek]

Thanks @cpovirk @tomball for the prompt response and swift resolution of the issue. Can I expect patch release of guava anytime soon?

[cpovirk]

I just mailed out the preparatory change for a release, and I'm hoping to get the release published later today.

[cpovirk]

Fix released in 33.3.1.

[chadlwilson]

It's a shame that it seems we've had to walk back on basically everything from #3683 / #6603 now. I don't know what the solution is (maybe there is not one) but it's a shame regardless that there isn't a way to resolve these issues without pulling in irrelevant stuff at runtime for the vast majority of cases.

[cpovirk]

Yes :(

If it's any consolation, we are approaching the point at which we can replace our two (medium-to-largish) artifacts of nullness annotations with just one small one. And it may still be possible to find some way of addressing the J2ObjC dependency, whether it's to essentially shade those annotations (bringing some of the usual pros and cons of that approach), to get the retention changed upstream (and change the dependency to optional/provided? But see my initial concerning test results above, which have remained after I fixed the bad build target I'd mentioned), or to move them out of the code entirely (into comments or a separate branch if J2ObjC users could work with that?).

The J2ObjC ideas are all half-baked, and they aren't likely to become a priority over things like the nullness work. So basically I acknowledge my change yesterday is a step back in that respect, and I don't know that we'll do better anytime soon, unfortunately.

[tomball]

FWIW, the j2objc compiler now supports external annotations, where a separate file can define annotations for various symbols in a build target. So all of the j2objc annotations for Guava can be moved from its Java source files into a separate file, which would then be only used when building for iOS. I don’t know if cycle_finder supports external annotation files (I’m on a bus in the Mexican highlands now), but it should and wouldn’t be hard to add that ability if it doesn’t. El El mar, sept 24, 2024 a la(s) 7:30 a.m., Chris Povirk < ***@***.***> escribió:

…

Yes :( If it's any consolation, we are approaching the point at which we can replace our two (medium-to-largish) artifacts of nullness annotations with just one small one. And it may still be possible to find some way of addressing the J2ObjC dependency, whether it's to essentially shade those annotations (bringing some of the usual pros and cons of that approach), to get the retention changed upstream (and change the dependency to optional/ provided? But see my initial concerning test results above <#7397 (comment)>, which have remained after I fixed the bad build target I'd mentioned), or to move them out of the code entirely (into comments or a separate branch if J2ObjC users could work with that?). The J2ObjC ideas are all half-baked, and they aren't likely to become a priority over things like the nullness work. So basically I acknowledge my change yesterday is a step back in that respect, and I don't know that we'll do better anytime soon, unfortunately. — Reply to this email directly, view it on GitHub <#7397 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAW2JLZKMT57KEXJ4LO5P4DZYFSQLAVCNFSM6AAAAABOWDWDWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNZRGI4DIMRSHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[cpovirk]

It looks like AGP has probably just started discouraging compileOnly dependencies

This may be academic at this point, but: I'm not sure that's actually true. I see a compileOnly check dating back to before this commit 7+ years ago.

That commit says "We need to detect cases where the dependency is present in both graphs but just with a different version," so maybe what happened is that Guava and some other project started depending on different versions of j2objc-annotations? Guava went from using 2.8 in 33.0.0 to using 3.0.0 in 33.1.0, so maybe another project is using 2.8 in a non-compileOnly configuration?

Then again, both firebase/firebase-android-sdk#6232 and androidx/media#1700 report errors that refer to 2.8 as being used with compileOnly. So those users would have to be seeing a conflict between 2.8 (from an old version of Guava or from some other compileOnly user?) and some other version from elsewhere.

It might just be that "something" changed elsewhere in Gradle/AGP.

I still think that the change I made to Guava should reduce Guava's contribution to the problem. But maybe it's even possible for it to trigger the problem in some cases, since now Guava's "full" dependency on j2objc-annotations might conflict with some preexisting compileOnly dependency from another project?

I hope that we'll find that we're in good shape, but I want to have this written down in case not.