bergerkiller / BKCommonLib

An extensive library I use in all of my plugins
38 stars 38 forks source link

ObjectArrayConverter exceptions with Cauldron 1.7.2 #23

Open gixmy opened 10 years ago

gixmy commented 10 years ago

I'm seeing exceptions when BKCommonLib is loaded by Cauldron (formerly MCPC+). See http://pastebin.com/T3Mas1P0

There are other exceptions later from related plugins such as TrainCarts but the ObjectArrayConverter ones in BKCommonLib are the first. The later exceptions may well be a result of the earlier ObjectArrayConverter exceptions.

This issue stops any plugins that depends on BKCommonLib from working with Cauldron.

Stab in the dark but a commit from last September at https://github.com/bergerkiller/BKCommonLib/commit/11e063666ea2876a3f8e2b0519b96de42ec3c86a#diff-5594f991573e6e5663f33696a0b21bed might have something to do with this.

I'm raising a similar ticket for Cauldron too as this could be a Cauldron issue or a combination of Cauldron and BKCommonLib.

gixmy commented 10 years ago

I have updated to the latest version of Cauldron. The previous exceptions have gone away. However I'm now getting a reflection issue - see http://pastebin.com/ZeeDJaBH

Encouragingly the TrainCarts plugin now loads with no exceptions although it gets disabled because BKCommonLib failed to enable.

vico93 commented 10 years ago

Seems like BKCommonLib 1.7.2 is incompatible with Cauldron

gixmy commented 10 years ago

Copied from the cauldron issue... I've been looking at the code that appears to be the source of the latest issue - see /src/main/java/com/bergerkiller/bukkit/common/server/CraftBukkitServer.java in the BKCommonLib source code:

    public void postInit() {
            try {
                    // Obtain MinecraftServer instance from server
                    Class<?> server = Class.forName(CB_ROOT_VERSIONED + ".CraftServer");
                    MethodAccessor<Object> getServer = new SafeMethod<Object>(server, "getServer");
                    Object minecraftServerInstance = getServer.invoke(Bukkit.getServer());

                    // Use MinecraftServer instance to obtain the version
                    Class<?> mcServer = minecraftServerInstance.getClass();
                    MethodAccessor<String> getVersion = new SafeMethod<String>(mcServer, "getVersion");
                    MC_VERSION = getVersion.invoke(minecraftServerInstance);
            } catch (Throwable t) {
                    t.printStackTrace();
            }
    }

Judging by the crash report, it's getting as far as the line:

                    MethodAccessor<String> getVersion = new SafeMethod<String>(mcServer, "getVersion");

I'm not familiar with Java or this code but to me this suggests an issue with the mcServer not having a getVersion method which in turn suggests mcServer itself is not the right class. Thoughts anyone?

aadnk commented 10 years ago

I'm pretty sure this hard-coded substring check is the actual culprit:

if (!super.init() || !Bukkit.getServer().getVersion().contains("MCPC-Plus")) {
    return false;
}

The only significant change between MCPC and Cauldron for plugins is the server name and version, so to preserve compatibility you only have to replace MCPC-Plus or MCPC with Cauldron in the source code. Or - add it as an optional check.

Now, Cauldron could have avoided all this by simply adding MCPC to its version string (MCPC-Plus Compatible, perhaps). Of course, I can understand if people don't want to go down the same path as the HTTP user agent header ...

gixmy commented 10 years ago

Thanks aadnk - that's an excellent point. I checked over at https://github.com/MinecraftPortCentral/Cauldron/issues/1187 and bloodmc confirmed the change.

So if I have my logic correct the following trivial change should fix this issue at least:

 if (!super.init() || (!Bukkit.getServer().getVersion().contains("MCPC-Plus") && !Bukkit.getServer().getVersion().contains("Cauldron"))) {
       return false;
 }
vico93 commented 10 years ago

There is another issue on the same file @aadnk mentioned, but on line 50:

    @Override
    public String getServerName() {
        return "MCPC+";
    }

This could need to be changed too...

lenis0012 commented 10 years ago

Ah, easy name checking logics i see. What is the accact server name of a Cauldron server?

aadnk commented 10 years ago

It's similar to Spigot:

public class ExampleMod extends JavaPlugin {
    @Override
    public void onEnable() {
        getLogger().info("Version: " + getServer().getVersion());
    }
}

Which prints the following:

[00:31:56 INFO]: [ExamplePlugin] Version: git-Cauldron-1.7.2-1.1112.04.1616 (MC:
 1.7.2)
gixmy commented 10 years ago

The server name was modified to return "Cauldron". See https://github.com/MinecraftPortCentral/Cauldron/issues/1187

However if you go to the bottom of that page bloodmc has now modified it again to add legacy support for plugins:

"Added legacy support for plugins detecting MCPC. Fixes #1272, #1187"

Therefore BKCommonLib should work now with no further changes. I'll try it with Cauldron later today.

gixmy commented 10 years ago

I've now tested BKCommonLib with the latest version of Cauldron.

BKCommonLib at first enabled successfully using the 1.57 version. The later versions would not enable due to a version mismatch.

I tried to spawn a cart but that generated an exception. Now BKCommonLib is throwing an exception on Cauldron startup.

[BKCommonLib] [Reflection] Field 'note' does not exist in class file org.bukkit.craftbukkit.v1_7_R1.block.CraftNoteBlock (Update BKCommonLib?)

Whatever I do I cannot get BKCommonLib to enable successfully now.

lenis0012 commented 10 years ago

I will look into Cauldron suppoort in this summer vacation. But my finals are starting this week

gixmy commented 10 years ago

Thanks lenis0012 and good luck with the finals.