-
Notifications
You must be signed in to change notification settings - Fork 12
Update to Guava 17 #68
base: master
Are you sure you want to change the base?
Conversation
Glowstone will also have to be updated to include the |
For this to be considered an analysis needs to be done on every single Guava change since v10. Analysis should include which plugin(s) break because of the change, which Glowstone members break, what behaviour changes, and how the change affects future development. Part of this is because Glowstone needs to be compatible with older plugins due to a direction we've decided to take. If we can't be compatible, then we can't move forward. It will take @SpaceManiac's comment to decide the fate of this PR however. I personally do not think it should be accepted without a lengthy and in-depth analysis. |
Since spigot now uses guava 17, most plugins are compatible with guava 17. |
Currently we aim to be backwards compatible with Bukkit plugins. |
This would make you compatible with more plugins though. |
I don't think you understand that we are currently supporting Bukkit plugins, not Spigot plugins. It is best to wait for @SpaceManiac to respond to my earlier note before arguing. |
As much as I'd love to see Guava 17 (no more old pex!), if it breaks plugins then I don't think we should update it. As turtle said, we're supporting Bukkit plugins. Bukkit used Guava 10, so any plugin that requires a different version of Guava needs to include it in their JAR with the required namespace remapping, or they aren't a Bukkit plugin. |
Required for Glowstone compatibility see https://github.com/GlowstoneMC/Glowkit/pull/68
This change does definitely break plugins, written for 1.7 which rely on Guava 10. Would be interesting to determine the scale of this breakage, I think @md-5 has or had an archive of all BukkitDev plugins and a static analysis tool that could help here. The package to search for is com.google.common and javap could be used to dump the method signatures in the API for comparison. With Sponge requiring Guava 17 and using it as part of the API, this will become more of a problem, related: GlowstoneMC/Glowstone#498 Implement Sponge API Other issues: GlowstoneMC/Glowstone#592 GlowstoneMC/Glowstone#604 GlowstoneMC/Glowstone#578 Also worth noting, Guava 18 is out, but no MC-related projects have updated to it yet that I know of, because Minecraft still uses 17. It is possible to include both versions of Guava in the same jar, using https://maven.apache.org/plugins/maven-shade-plugin/examples/class-relocation.html. Bukkit includes a custom class loader, that could be used to perform a similar corresponding relocation when loading plugins. The only problem is then how to select per-plugin which Guava to use (a config option somewhere? what would be the default?). Non-trivial, but should be possible in principle to support both. |
I'd just wanted to point out that I have this thing modified enough to run PEX and Essentials fine using Guava v18, while still retaining all new methods for Guava-v18-aware plugins: https://github.com/socram8888/GuavaCompat10 Note though that I modified it only enough to make those two plugins run, and that I can't promise it will work fine with any other. Adding support for other methods to run any other plugins aren't an issue though. |
Glowkit has inherited a very old version of Guava (10.0.1) from Bukkit; since then, Minecraft, Forge, and Spigot, and Sponge have updated to use Guava 17.0. This PR updates Glowkit's version of Guava accordingly.
A large amount of new API has been added from Guava 10 to 17, so this update in Glowkit should help increase compatibility with code originally written for other platforms built on Minecraft (which must use Guava 17 since Minecraft 1.8 uses it, at least not without hacks to bundle multiple versions of the same library).