search

GlowstoneMC / Glowkit-Legacy

A continuation of Bukkit for use with Glowstone
GNU General Public License v3.0
27 stars 12 forks source link

Update to Guava 17 #68

Open deathcap opened 9 years ago

deathcap commented 9 years ago

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).

deathcap commented 9 years ago

Glowstone will also have to be updated to include the com.google.code.findbugs:jsr305:2.0.1 dependency for javax.annotation.Nullable (no longer included in Guava) if this is merged. No PR yet on Glowstone but I can make one if this is accepted (wanted to wait to hear back first).

turt2live commented 9 years ago

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.

Techcable commented 9 years ago

Since spigot now uses guava 17, most plugins are compatible with guava 17. In fact, this PR will probably improve plugin compatiblity.

turt2live commented 9 years ago

Currently we aim to be backwards compatible with Bukkit plugins.

Techcable commented 9 years ago

This would make you compatible with more plugins though. The main api difference between 10 and 17 is caching. Spigotmc's craftbukkit 1.8 no longer shades the minecraft version of guava into net.minecraft.util, opting instead to break compatibility with older plugins. Many active authors have updated their plugins to use guava 17 caching, to become compatible with craftbukkit 1.8 This makes those plugins incompatible with glowstone because glowstone uses guava 10. If glowstone updates to guava 17, you will actually improve compatibility.

turt2live commented 9 years ago

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.

gdude2002 commented 9 years ago

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.

deathcap commented 9 years ago

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: https://github.com/GlowstoneMC/Glowstone/issues/498 Implement Sponge API

Other issues: https://github.com/GlowstoneMC/Glowstone/issues/592 https://github.com/GlowstoneMC/Glowstone/issues/604 https://github.com/GlowstoneMC/Glowstone/issues/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.

socram8888 commented 9 years ago

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.