Update to modern guava #247
Comments
|
@zhangkun83 would shading guava make sense here? None of the guava classes appear in public APIs |
|
@bootstraponline, why do you think this plugin is the cause of issue? The plugin should work fine with newer versions of Guava, and Gradle should resolve the newer of the versions necessary. As long as plugins work with newer versions of Guava, "things are fine." Is there any reason to believe that's not the case? @zpencer, bumping the Gradle version is easy. Why not do so? I came across https://discuss.gradle.org/t/guava-in-gradles-classpath/24861/9 which led me to gradle/gradle#3698 . I don't think the jib issue is caused by this plugin; I think it may be caused by Gradle itself. |
|
I built a local snapshot of |
|
@bootstraponline, thanks for the investigation. I agree that updating is a good idea, it just didn't seem like the current state should cause breakages. |
|
Actually current state does create breakages - at the moment I can not use protobuf-gradle-plugin alongside with spotbugs gradle plugin, since latter depends on newer guava version but somehow older version from protobuf plugin gets on the way. It would be nice to either update guava library or better to repackage it, so it won't create any problems in the future. |
|
Also there is a PR #269 - that would be even better. |
This appears to work on Gradle 5.6. But please fix the bug. |
|
Hi @ejona86 Any chance this could be fixed, either by upgrading guava or shading it? |
|
I was fine with bumping the version of guava, "just because." But I don't think the peanut gallery's view of the situation is entirely accurate as to what is going on. While upgrading the guava dependency might fix this specific issue just this specific time, I don't think that's the whole story. Something more fundamental is going on for a guava version to be downgraded. Gradle should use the newest of any dependencies. If it is not, this problem is ripe to repeat continually as new versions of dependencies are released and used. When this problem is fixed I want it to be fixed for the future, not just for today. But I'll also point out there is an easy workaround (in addition to excluding the dependency as seen above). Note that this shows that gradle does choose the newest version amongst plugins. Spotbugs claims not to use guava (spotbugs/spotbugs-gradle-plugin#119 (comment)), and that's what it looks like from my perspective. I see no Guava dependency in Looking at AbstractInjectedPropertyHandler.getInjectedServices, I see: So spotbugs is just using an API in Gradle that is broken by the guava version. Figuring out what version of Guava Gradle is using was a bit strange, since I see lots of guava dependencies throughout the codebase, many of them v18.0. But based on its name, It seems we should upgrade our version of Guava to 27.1-android to match Gradle and file a bug against Gradle. |
|
Aaand upgrading to 27.1-android breaks Android builds (ugly output because I'm pasting directly from the test xml output). And this is probably because the Guava -android doesn't have CacheBuilder: That was with Just that change by itself breaks some tests. There are some guava hacks in the tests already, but a cursory attempt to disable them didn't yield better results. |
|
After gathering all the context and pain points of leaked Guava dependency by protobuf-gradle-plugin. Shading guava-18.0 would be a better solution than bumping the version (we could bump it to 27.0.1-jre as well). This would fix the problem for the future as new versions of dependencies are released and used. People might still get the issue for Gradle versions earlier than 5.6 (by Gradle itself gradle/gradle#3698), but they would be aware that's not caused by protobuf-gradle-plugin. @ejona86 |
|
@voidzcy, we should bump the version of Guava. Guava 18 is really old. If we also want to shade, fine. But we do need to file a bug against Gradle. #3698 is not what we are seeing, as the fix was in Gradle 5.6 and I still saw this issue with Gradle 5.6. The current bug would mean we shouldn't use any dependencies that Gradle itself uses, which is not appropriate. In other words, this is not a "Guava problem." This feels more like a run-of-the-mill, "something has a broken ClassLoader" problem. |
|
agree with the above ^^ Upgrading guava would only help to reduce the pain/friction when using gradle. Shading would to the same but there's an underlying problem there that should be solved at some point. Thanks for looking into this folks |
|
Seems it's not easy to bump Guava version. It looks like Gradle, Gradle Android Plugin and Guava are twisting together. Since we are supporting different versions of Gradle Android Plugin, each of them has different dependency requirement for Guava. Guava keeps deprecating APIs as it evolves. Here are a couple examples:
This means we did have reason to depend on Guava 18.0 even though it is ancient (we do have hacks in android tests for newer version of Gradle Android Plugin). Gradle Android Plugin has requirements for version compatibilities with Gradle. Seems the same applies for Guava. So the protobuf-gradle-plugin cannot easily upgrade Guava version. But all this matters for Android users only. For normal Java users, you can still force to depend on a newer version of Guava. For dependency leaking, we can shade Guava in protobuf-gradle-plugin to make the problem never come from the plugin. If Gradle fixes its #3698, then the leaking issue should be completely fixed. We might consider getting rid of Guava dependency in protobuf-gradle-plugin as the Android compatibility issue is subtle and usage of Guava is minor. |
|
I just realized CharMatcher.JAVA_LETTER_OR_DIGIT was always com.android.tools.build:gradle:3.4.0 used Guava 26.0-jre, which seems new enough to "fix" the current problem. So that would be a worst-case: we upgrade to Guava 26.0-jre; Android users would need to use android plugin version 3.4.0 or later. Maybe 3.3.0 or earlier happens to work with newer versions of Guava (it was using Guava 22, which is too old for newer Gradles). As I mentioned earlier, gradle/gradle#3698 was fixed in Gradle 5.6 and I still saw this problem happening in 5.6. I don't mind dropping Guava, but we should do it for the right reasons. If the ecosystem is broken, we should make sure there are tracking bugs for them to fix themselves. The Gradle problem is not a Guava problem; if you say "drop Guava to make the pain go away" you are really saying "don't use any libraries that Gradle may choose to use" which is not acceptable. And for the Android plugin, if they are using |
|
agree with the above, but it seems that it will be considered in planning to be removed from Beta -> google/guava#3591 (comment) |
|
@voidzcy pointed me to spotbugs/spotbugs-gradle-plugin#166 and had trouble reproducing this issue without spotbugs, so it does seem it is a spotbugs bug and not a Gradle bug. I'm very grateful to those who identified the root cause. There's been a reasonable amount of time spent on this behind-the-scenes. We were dealing with a bit of a perfect storm of issues, independent of spotbugs, that were preventing us from testing properly. It took three of us, but we think we've found the root cause for enough of the issues to have a path forward. |
We like Guava, but the plugin has little need for it. As a side-benefit dropping it avoids systems-not-handling-classpaths-correctly issues like google#247. We wouldn't drop Guava just for that reason (as the ecosystem should fix its bugs), but instead of just removing it from the API surface, let's remove it entirely. Similarly, we could continue using it in our tests, but we really don't need it. Fixes google#572
We like Guava, but the plugin has little need for it. As a side-benefit dropping it avoids systems-not-handling-classpaths-correctly issues like #247. We wouldn't drop Guava just for that reason (as the ecosystem should fix its bugs), but instead of just removing it from the API surface, let's remove it entirely. Similarly, we could continue using it in our tests, but we really don't need it. Fixes #572
compile 'com.google.guava:guava:18.0'is ancient and causes method missing errors with more modern Java software (for example GoogleContainerTools/jib#591). This was highlighted recently in a Google cloud blog post:It'd be really nice if the Google Java projects played well together and used modern dependencies. Guava 18 was released on Aug 25, 2014. That's four years ago!
The text was updated successfully, but these errors were encountered: