Safeguard Versioning Policy

Discussion in 'Bukkit News' started by EvilSeph, Jan 16, 2013.

Thread Status:
Not open for further replies.
  1. Offline

    EvilSeph Retired Staff

    To protect servers against incompatible plugins that have been developed using the unpredictable and volatile Minecraft code (which we’ll be referring to as “non-Bukkit API plugins” from this point onward), we made the difficult decision of implementing a controversial safeguard into Bukkit. This safeguard forcefully disables non-Bukkit API plugins from running on your server if they have not been verified by their developer to work with the Minecraft version on which your Bukkit server is running. As Bukkit has no control over what changes are made to Minecraft itself, developers should not be relying on its code in any way, shape or form.

    Without this Safeguard in place, running non-Bukkit API plugins on your server had been putting it at risk of instability due to unpredictable behaviour. With the Safeguard in place, developers of non-Bukkit API plugins now have to responsibly check that their plugin works with each Minecraft update, then upload a tested version before the plugin will be able to run on your server.

    When we first implemented the Safeguard, the intention was to forcefully disable incompatible non-Bukkit API plugins with every new Minecraft version, no matter how small the changes. However, since we have the freedom of a flexible release schedule (which Mojang does not), we're usually able to fix bugs faster than Mojang can release Minecraft updates, often removing the need for us to update Bukkit and trigger the Safeguard needlessly (as was the case with Minecraft 1.4.7).

    As such, we're making changes to when we trigger the Safeguard (implementing a versioning policy): instead of triggering the safeguard with every Minecraft update, we will only be triggering it when necessary. This means that non-Bukkit API plugins will only break if a Minecraft update or a rename update has altered the code, instead of with every Minecraft update. As a result, with a Minecraft update like 1.4.7, non-Bukkit API plugins would not break unless we've had to alter the code with a rename update (which we've done for 1.4.7).

    The technical meaning behind this change is: instead of classes being named "name.of.class.v1_4_6", we will now be following a versioning policy of "name.of.class.v1_4_R1" where "R1" is the internal counter for either Minecraft or a rename altering CraftBukkit or Minecraft code.

    This new Safeguard Versioning Policy will allow us to keep up with Minecraft updates without needlessly triggering the safeguard and breaking non-Bukkit API plugins when we've already fixed all the issues the Minecraft update addresses, while still protecting your servers from running unchecked and untested plugins.
     
  2. Offline

    mbaxter ʇıʞʞnq ɐ sɐɥ ı Retired Staff

  3. Offline

    dreadiscool

    mbaxter
    I ask again, could you please tell me how you came up with this statement, seeing as how you're saying it's impossible to make a statistical analysis stating that developers don't like the commit? After you read the 1499 comments on the commit stating they don't like the change, and even people on this thread saying they don't like the change?

    I would like to encourage you to read how statistics are made yourself, before you base your facts on the opinions of one person
     
  4. Offline

    Score_Under

    I don't know about you, but writing one method to get isGrounded() from a player regardless of NMS version is not a great hardship.
    For pretty much every other use-case (e.g. NBT magic, fast block setting, packet instantiation), you're likely to be dealing in obfuscated code which will break when the version number breaks.

    (Aside: Yes, I've had the most basic of packet code break between updates - slight changes in protocol mean type signature changes and field renames)
     
  5. Offline

    mbaxter ʇıʞʞnq ɐ sɐɥ ı Retired Staff

    Statistics is inappropriate for determining how hard something is to achieve. The same post with my statistic lesson outlines why I have come to that conclusion. It's based on additional effort required by the safeguard vs without when dealing with minecraft updates.
     
    Cowboys1919 likes this.
  6. Offline

    dreadiscool

    I used it as an example. I am in no way stating that this was the only thing, otherwise I'd have to write an extremely long list that I don't want to write, and nobody wants to read.

    So in essence, the staff is using this statistic lesson to rationalize you ignoring the community's response?

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: May 30, 2016
  7. Offline

    mbaxter ʇıʞʞnq ɐ sɐɥ ı Retired Staff

    You are making it increasingly clear that you have no read the github commit conversation or even this very topic.
     
  8. Offline

    Score_Under

    Potential API pull-requesters? :p
     
  9. Offline

    agaricus

    There is a possible solution offering the best of both worlds I haven't seen discussed. Specifically, to at least address this problem:

    Indeed, direct references to obfuscated symbols can be very unreliable: whenever Mojang reobfuscates, the names can be shifted around, such that the the old names no longer refers to the same symbols. This is the problem which caused the infamous "disappearing stone" symptom with an outdated plugin circa 1.1, and could corrupt worlds catastrophically. However, in many cases, the field itself is the same, it just goes by a different name. If there is a way to access the same field across versions, this would solve many problems.

    To use a concrete example: Packet23VehicleSpawn's object type field. In 1.4.5, this field was declared as follows:
    Code:
    public int h;
    
    but in 1.4.6 and 1.4.7 it became:
    Code:
    public int j;
    
    .. but its the same type field. A field by any other name is just as sweet. But what is missing is an explicit means to reference "the field for the vehicle's object type", rather than what ephemeral name it calls itself this time around.

    Nonetheless, one could match up each of the symbols, with old names corresponding to new names, for each new version of Minecraft:
    Code:
    net/minecraft/server/Packet23VehicleSpawn/i -> net/minecraft/server/Packet23VehicleSpawn/k
    
    Once the complete "mappings" are created for all classes, methods, and fields, plugins written against the old obfuscations could be programmatically remapped to the latest obfuscated names. Right now this obfuscation update is done by each NMS plugin developer, by hand. But there's no reason why the most tedious part of it could not be automated, as long as thorough and accurate cross-version mappings are available.

    Granted, obfuscation changes are only a fraction of the volatility in NMS. Native Minecraft code is not an API and subject to change at any time. Even without obfuscation, some NMS plugins would have to update depending on how Mojang alters the code. For example, in the 1.5 snapshots (at least 13w02b), the minecart classes were significantly refactored (storage/furnace moved out of the main class, new types, etc.). Not sure about what NMS plugins would be affected (maybe Traincarts, lenis0012 / bergerkiller?), but Railcraft definitely had to update for this change. There were also 1.5 snapshot changes to the World block setting methods. Anything relying on refactored code would of course have to update.

    So cross-version remapping is not a panacea, by any means. And even if the remapped plugin still works across versions, it would undoubtedly be a good idea anyways to test thoroughly and examine the code for any other relevant changes.

    Even then, a large part of cross-version NMS incompatibility could be solved by this technique.

    Out of interest, here's the 1.4.7 "hit rate" for an experiment in matching up symbols across most of the versions available on assets.minecraft.net (yeah I know I could go back further using MC Nostalgia, but there's diminishing returns):

    1.3.1=9612 1.3.2=9626 1.4.2=10793 1.4.4=11076 1.4.5=11076 1.4.6=11185 1.4.7=11185

    This was generated by chaining the mappings through MCP's numeric "srg" names (like "field_73527"), which are intended to be stable across versions. Sure it isn't perfect, but surprisingly many symbols can be matched up reliably this way.


    If anyone wants to further pursue this, the next step is deciding how exactly to make use of these mappings. For example, the org.bukkit.plugin.java.JavaPluginLoader could be enhanced to load NMS-dependent plugins for a given Minecraft version, translating the symbols as necessary on-the-fly. Or plugins could be developed against a "stable" set of symbols, obfuscated to the correct symbols. Or not even reobfuscated (in the world of modding there is a working prototype of "runtime deobfuscation" of Minecraft to do exactly this to reduce the requirement for updating mods with each new version of Minecraft).


    Fortuitously, the version safeguard actually helps this cross-version NMS compatibility concept. Previously, you had no way of knowing for sure what NMS version a plugin was written for. Now each NMS symbol is versioned and can be handled accordingly. In theory, servers could even simultaneously run plugins written for disparate versions, all remapped together in harmony.

    If there is enough need for it, someone could even develop a more complete "compatibility layer" for v1_4_R1 (or whatever), NMS and OBC, providing a wrapper around the current Minecraft version (when a major new version is released), exposing the same old methods but wrapping the new native code, for example, for 1.4.7 EntityMinecart inventory to 1.5 EntityMinecartChest. This would be a lot of work and I'm not planning on doing it, nor would I ask the Bukkit team - since the sole purpose of NMS is to support OBC for supporting the Bukkit API, which is by design intended for safe cross-version compatibility. That's the real solution: further enhance Bukkit API so more plugins can use it as the designed abstraction that it is for any individual Minecraft version, rather than relying on NMS and sophisticated runtime cross-version remapping ASM tricks. But since you asked, dreadiscool, if you want a best of both worlds solution offering safety yet NMS compatibility, this is how it could be done.
     
    NuclearW likes this.
  10. Offline

    bergerkiller

    agaricus
    TrainCarts no longer uses NMS code. I'll show you what this 'safeguard' resulted in:
    A huge amount of reflection that is going to make bug tracking 100x more difficult

    But I warned them about this. I warned them about Objects being passed around instead of NMS/CB types. I warned them about reflection. I warned them about bypassing it. I warned them about people writing their own API/library/layers to dynamically update the package paths. It's now up to them to endure the fun consequences. I am definitely not the only person that is using dynamically generated paths to access NMS. There are even people using byte code generators to replace NMS classes so they can access it.

    BKCommonLib is my new NMS compatibility layer. It uses reflection for pretty much all objects, the NMS classes are accessed dynamically using Class.forName with a small constant declared in Common.java with the root NMS and CB paths.

    Instead of compiler errors I will now see literally nothing when a new CraftBukkit comes out. But I am not responsible for this, I have no other option but to use reflection for everything because I can not pass around NMS types in BKCommonLib-depending plugins. The only way for those plugins to remain compatible without crashing is by redirecting all calls to BKCommonLib.

    Of course, I will try to add proper methods that translate NMS types. I built a huge conversion system to deal with this. It dynamically switches between NMS and Bukkit/wrapper types when accessing fields using the so-called 'TranslatorFieldAccessor'. As well I wrote several utilities to deal with collections that contain NMS types, such as ConvertingList.

    I can deal with it, but the consequences of field changes being unnoticed will become a problem. I will try to obtain a location for a changelog of CraftBukkit so fields can be properly updated without forgetting any. That is, if it is not hidden from plain sight like what happens with MC-dev.

    Don't forget that BKCommonLib does not enable on unknown CraftBukkit NMS versions. This is a restriction I put in place, not CraftBukkit. It is something I could do without package versioning as well, but then without all this reflection. Saying that BKCommonLib is 'dangerous' is not right, because it will not do anything on unknown builds. Simple. It will not try to enable ANYTHING when incompatibility occurs. However, when a new version has to be tested, it will be harder to test it, because field errors are not easy to spot.

    The only benefit I see from this is that I can now support custom CraftBukkit builds as well by detecting them. I can redirect certain methods when, for example, Spigot changes a field name or adds a method. NibbleArrayRef is such an example.

    Final

    I have already managed to build all other possible ways using byte code editing. I made a plugin (NoVerPackage) that can dynamically swap the plugin class loader and redirects the NMS/CB paths using the ASM byte code editing library. I also helped write Libigot, a server implementation that uses this same principle, but in a safer manner. But byte code editors are banned by definition on dev-bukkit, so I had to take the 'reflection route'.
     
    lenis0012 likes this.
  11. Offline

    lenis0012

    Incompatible mc plugins mustly come from a changed field or changed classpath
    There is no reason the code should be obfuscated anyways

    I know that you dont want plugins to interact with CraftBukkit because it is unsafe
    But in some cases its just unfair

    for example:
    You want to send a packet to the client and you use a safe constructor for it
    And you want to send the packet.

    You would have to use some ugly reflection wich is not fast or use a safe code wich breaks every update
    Do you see how many plugins are trying t work around this versioning thing?

    ProtoclLib uses byte code to still repalce CraftBukkit classes, and they use indexing with integers instead of field names because the field names can change.

    Plugins like this would be alot safer and faster if we could directly accest these methods
    We want to use ((CraftPlayer) player).getHandler.playerConnection.sendPacket(packet); without versioned imports

    You can allways put some field checks in the client since you are minecraft developper can't you?
    Right now since we use reflection we have no idea if the field names are correct, simpyl because because we are not going to check all fields if we have tons of them

    We test our plugins but sometimes not everything is getting launched and will then cause errors being reported to CraftBukkit.

    I bet 90% of all server owners would love to at least have a chance for plugins to stay compatible.
    Java is smart enough to point errors to all classes where it got called, wich usualy contains the plugin name.
    Or the name of the developper.

    We write plugins to make things that dont seam possible possible.
    Without using the bukkit api all we can do is use methods that have already been coded for us.
    How is your plugin if you code something that has been done before for like 20 times.

    A ton of simple swearing blocker plugins exist that use event.getMessage().contains("some word")
    But people want to see new stuff, There are a ton of developpers and you cant feed them with a small API.
    PermissionsEx, DisguiseCraft, Citizens and much more huge plugins use NMS

    not because they want their plugin to break with future builds but because they want something original.
    Thats the thing with developping, you can make a game and call it MineCraft+ and make it almost the same as minecraft and nobody is going to play it.

    I do not agree with the fact that you deny all acces on NMS, even some code from CraftBukkit is versioned.
    We have to use java methods that barely get used because there is no open source java software that has classpaths changing every update.

    Minecraft has grown to a huge game being played by 8 million people over all countries of the world.
    It is no longer a simple game with 1 goal and not being extended at all.
    Tons of developpers develop minecraft CraftBukkit plugins because so many people use it, it gives th feeling you do something people appreaciate.
    It gives a warm feeling in your hearth that people use your work, and even donate sometimes.

    We do not code plugins to replace CraftBukkit, we code plugins because we hope people will use it.
    We (I speak for almost all the developpers) want to have acces to CraftBukkit.
    New developpers probably wont interacvt with it anyways, as they dont even got the CraftBukkit jar in their libaries.

    Everyone needs a chgance to develop what they want, and there is no need to force advanced developpers to use supported API code.
    If you deny this because you protect new developpers you are doing it all wrong.
    People will never use a plugin with a feature they have seen before if it is not better.
    Thats how a lot of plugins ended after their first release.

    Many developeprs have left due this 'mistake' and i think it has hirt this comunity enough.

    Sorry if i mae typo's, this was done on a mobile phone
     
  12. Offline

    desht

    Your post would have more merit if NMS access was actually denied; it isn't, of course. You just need to make sure your plugin accesses the correct NMS methods and fields for each MC release which changes the obfuscation.

    I do agree with your point about the pointlessness of obfuscation, though - the only people inconvenienced by that are honest modders. It doesn't stop piracy at all. I guess it could be argued that obfuscation stops other people stealing your ideas, but there are already multiple clean-room reimplementations of Minecraft (or the Minecraft concept) out there now anyway, and I doubt it's hurting Mojang's bottom line much.
     
    Inscrutable likes this.
  13. Offline

    dreadiscool

    lenis0012, MyPictures
    While I do see the benefits of this new system, it's been poorly implemented and poorly backed. The developer community is at an impasse with bukkit staff, and there's no point in arguing with the staff anymore.
     
  14. Offline

    Ne0nx3r0

    Now you're getting it. ; )
     
  15. Offline

    TnT Retired Staff

    Provide a better suggestion to accomplish the same result and we're all ears. That's what we've been asking for since the start of this whole commit. The only good response I've seen was this one, but it involves a great deal of work that no one has signed up for. Lets face it, many have complained about the minimal amount of work this commit brought, so I can't see those same people volunteering for the kind of work this alternative presents. Keep in mind, tackling that would substantially delay a CraftBukkit release when a new MC version comes out. Given that, I am not certain how feasible this would be, but its far better than any other alternative presented. There is an insane amount of intense pressure on the team to put out a new CraftBukkit version whenever Minecraft updates - as such we always try to release as fast as possible while providing a solid product.

    lenis0012
    You can still use NMS in your plugins, no one is stopping you. All you have to do now is ensure your plugins work on a new Minecraft version (realistically, if you just want to update the imports and ignore incompatibilities you can do that - but this should encourage you to check your code). The plugins you use that utilize CraftBukkit internals do so extensively, and would need to be updated anyway. In this sense, its status quo for your development. You say "I bet 90% of all server owners would love to at least have a chance for plugins to stay compatible." but that's a misnomer - those plugins were never compatible with a new MC version. You state yourself you make extensive use of NMS. The more extensive the use, the more incompatible it becomes each new MC version. You know this already, as you've taken the fantastic step to disable BKCommonLib upon detecting an incompatible Minecraft version.
     
  16. Offline

    bergerkiller

    TnT
    You are absolutely right about incompatibility. I also do not disagree on the fact that plugins that access NMS should not work, by default, on all future versions. I've seen first-hand that things will break.

    I agree completely, something needs to be done to make sure plugins disable when the MC version changes.

    But package versioning has the downside that it does not forcibly stop a plugin from working anymore. As I said, I had to include my own disabling code for it. Plugins can still bypass all of the versioning rules in place, simply by using Class.forName to find out what NMS path to use. In the end, there will be plugins that do not 'give up' like the intention was initially, no, they will end up using reflection on the wrong field or method names, or pass in illegal arguments to fields/methods causing exceptions. All of the 'it will stop working because the class doesn't exist' benefits are no longer there.

    But that is not my biggest concern. My biggest concern is that this system is forcing developers to use reflection for all fields, all methods and all classes in net.minecraft.server and org.bukkit.craftbukkit. When fields do not match, it does not result in a compile error, as the field name is represented as a String. It turns into a runtime error, which can happen any time, any moment. In that regard, the 'random crashing of servers' only worsened. You should not underestimate the will of developers to have a stable download, developers will favor producing a plugin that doesn't break every week or month.

    I have already rewritten all of my plugins and BKCommonLib itself to deal with this, I do not care what the end result is. If package versioning is reverted, all I have to do is change a line in Common.java. Also, the disabling code has been in there for several months, ever since BKCommonLib started providing de-obfuscation methods.
     
  17. Offline

    mbaxter ʇıʞʞnq ɐ sɐɥ ı Retired Staff

    bergerkiller Nobody is forced to use reflection to handle the package rename. That is author choice. If your decisions put servers at risk, that is then your fault for creating that situation by not building against specific packages. I build my plugins into the safeguard (without backwards compatibility, too) without risking users.
     
  18. Offline

    TnT Retired Staff

    bergerkiller
    The intention was never to make plugin developers "give up". The intention was to make plugin developers perform a sanity check on their code every new Minecraft version - something which many plugin developers were not doing for whatever reason. Whether this commit existed or not, that sanity check should have always happened.

    That much has been made abundantly clear. Staff have shown you how you could have worked with this commit for the safety of the very servers that run your plugins, even providing backward compatibility. No one forced you to not care, you chose that yourself.
     
  19. Offline

    bergerkiller

    TnT
    mbaxter
    All right, if you do not fear code like this:
    Code:
    Field field = FieldUtil.getField("EntityPlayer", "a");
    Object entityPlayer = FieldUtil.getEP(player);
    FieldUtil.set(field, entityPlayer, 12);
    Or the more commonly used:
    Code:
    Object packet = ...;
    FieldUtil.setField(packet, "a", player.getEntityId());
    FieldUtil.setField(packet, "b", 12);
    Then I can only conclude that we are not on the same level. I have definitely seen the source code of other plugins, and especially when it involves packet listening or sending, the amount of reflection is staggering. I only want to point out an issue that package versioning brought into this world. At least this code actually showed a compiler error if the 'a' or 'b' fields changed names:
    Code:
    Packet8UpdateHealth packet = new Packet8UpdateHealth();
    packet.a = player.getEntityId();
    packet.b = 12;
    But right now, with reflection, no author is going to see any of it. And of course, this is what developers do to themselves, but wasn't the entire issue that developers were doing bad things that started all of these discussions? You want developers to stop doing bad things with NMS, and instead developers are now doing even worse things with it. I honestly try to keep it stable by providing proper names and having a lot of initialization checks, but not even that is perfect. I don't see the benefit in all of this.

    And me using BKCommonLib is unique, most developers do not want to depend on a 400 kb+ library to do just the above. So they will declare a small portion of the reflection in their own code, without someone else updating it for them.

    To reply to what was said: Indeed, I do not care what the end result is, as long as my plugins work. Defining all fields as getters and setters is going to result in a 1 mb large library, still offering less. With the FieldAccessor system I not only allow getting and setting, but also allow translations, transferring and safety systems.

    I am considering a different approach that allows an optional 'field type' parameter as safety system during initialization, but considering how many fields there are right now, it most likely becomes a big mess I want to avoid.
     
    lenis0012 likes this.
  20. Offline

    asofold

    That was a mistake :).
     
  21. Offline

    bergerkiller

    Oh better suggestions? We have said countless of different suggestions.

    1.
    Add a class that extends JavaPlugin called 'CraftJavaPlugin' in CraftBukkit. Plugins that wish to access NMS/CB need to extend this type. We are not altering the API, this is in the CraftBukkit implementation, it makes sense.

    2.
    Alter the Java Plugin loader to keep track of what class references are loaded. This can be done by inspecting the source code using, for example, ASM before it is loaded. JavaPluginLoader is part of Bukkit, so it might be needed to add a 'CraftJavaPluginLoader' which extends that, and is only used by CraftBukkit. If done well, this is only part of the implementation, not the API.

    3.
    If NMS or CB references are detected, require the plugin to extend CraftJavaPlugin. If it does not, it is not enabled. Simple as that.

    4.
    Now we need an NMS/CB build/version number defined somewhere, preferably in CraftJavaPlugin. Here is where the issue lies: it can be bypassed, unless we keep it outside the code. I recommend adding it in the plugin.yml (why not allow custom key/value pairs in the plugin.yml, I use it as well for common properties), but since you rather not have this in the API, you can put it in CraftPlugin for now.

    And if this is now 'ooh that is far too much work', you can decide to skip all of this and ONLY force developers to put the version in the plugin.yml, and include a system for in the API that checks whether a certain plugin is allowed to load a certain class. CraftBukkit can then implement that to provide what classes to 'block' and by what 'requirements'.

    Summary of suggestions that are ignored:
    • Adding a key/value pair in plugin.yml
    • Class loading detection/class inspecting on startup
    • CraftJavaPlugin for CraftBukkit plugins
    And I missed some, which I believe are less useful but are options:
    • Force CraftBukkit-depending plugins to have a version check (provide template)
    • Include this requirement during plugin approval
    • Option for this to be toggled on or off by server admins
     
  22. Offline

    Score_Under

    bergerkiller, are you trolling?
    Your suggestion in the previous post will achieve exactly the same. It will break plugins across version boundaries and require them to be rebuilt. It has exactly the same consequences as the current method, the only practical difference being that it's harder to implement!

    As for your argument about reflection, I honestly think you've gone insane -- why would you use reflection to bypass the version safety system, then go right ahead and make your own version safety system too?

    You seem to be grasping at straws here, and none of your arguments make sense.


    --Edit--
    This is so missing the point it hurts. The whole reason behind this change is to force a compiler error when things change names **even if** another field or method takes over the new name. (e.g. if Packet8UpdateHealth's fields swapped around).
    If the names don't change, then **nothing** will happen to your code that doesn't use reflection - it will continue to work exactly as before.

    Code that uses reflection to access obfuscated fields is so far missing the point that I'm worried you just got on the "hey guys let's hate this commit" hype train and didn't bother to check the consequences out for yourself.
     
  23. Offline

    bergerkiller

    Score_Under
    I believe you did not read correctly what I said previously. This is what I said:

    My point was that these changes did not achieve what was the desired goal, and I provide alternative methods that do not cause the issues as mentioned:
    • No problems with NMS/CB types being lost at compile time
    • No problems with reflection hiding compile-time errors
    The thing is: they do not want let plugins roam free without versioning restrictions put in place. I can understand why they want to do this. They want to force plugins to update themselves. What is happening right now is that plugins are bypassing it in ways that are not safe.

    I rather have people write libraries (be it small) that deal with access to NMS/CB in some way without all this additional reflection.

    I no longer consider it an issue that they want versioned checks on CraftBukkit plugins. There, I said it. People change their minds. But after working 'with' this mindset for a while I noticed that package versioning did not provide an answer to this issue, it only resulted in more unsafe use. Forcing NMS/CB calls to be put 'elsewhere' does help reducing the bugs, as you have all your calls in one place, forcing you to look at them at least once.

    Exactly, that was the whole point, and it is proven that it misses the point. Package versioning did not manage to 'stop' plugins at all. Compile time errors are not safe either. You can not blindly expect plugins to keep working between Minecraft versions.

    The problem I do see, and that is what you mean, is the category of plugins that use NMS/CB calls 'a little bit'. For them, this one call to entityplayer.sendPacket should not result in the plugin being disabled every new version.

    But how do you expect server software to detect these errors in plugins then? Include an NMS and CB field/class changelog in the server and run through all classes in the plugins verifying it? This is an even larger approach that will require far too much time to develop, be it only because of the class loader.

    What you want is 'nothing at all'. Nothing at all works for some plugins, but for others it is not going to work. For some of my plugins I knew it wasn't going to work. But I still (foolishly) left out a version check to auto-disable it. Why? Because why not enjoy the 1/20 chance that it DOES work? I fear that trusting on developers to take care of it is not going to proof fruitful, considering they are forced to update the plugins as fast as possible by 20 people shouting messages in the comments.

    So the question is: can we trust developers that have to cope with full-time updating to not just increment the version and upload it? This is a problem I see as well, and my solution isn't going to fix that either. But neither does the package versioning appear to fix that. It's a mentality issue, and I think fixing this requires fixing the community, not the project. In that sense, you are absolutely right.

    FYI I have been helping lenis the past week to get BKCommonLib up and running. While doing that I ran into several issues, one of them being that I could no longer expose NMS types in the library. This is the result.

    I probably started the entire hype train you are talking about. Check the first pages, check the commit on github, you'll see where it started. Guess why? Because I am a programmer. I know what changes will cause. I know of plugins needing to be rewritten to delegate all NMS/CB calls to the library. I was aware of that from the start. So far, these changes have proven to be more successful than what I had, so in that sense, this commit helped me. That doesn't mean that I can just ignore what is going on because it is going well.

    Visit the commit, but be careful, it MIGHT crash your browser.
     
  24. Offline

    asofold

    At present you see more statements like "hey this must be good since all the changes which will come with 1.5". That could be a train and that could be a hype.
     
  25. Offline

    Score_Under

    My reply got lost. :(
    Asofold: Praise, where unwarranted, can have a negative effect on what is perceived as acceptable. I am not claiming that it's a good thing.

    Bergerkiller: Your method can be bypassed with a loader plugin, so all your points relating to bypassing are moot. As for "losing cb types", that's sort of the point - once you've verified a piece of code works in the new version you replace its package imports with the new version of the package imports. Compile time errors are far better than runtime ones.

    Reflection is not a necessary component. People do not need to use reflection to take advantage of the new system. bkcommonlib appears to have been rewritten partly out of spite.

    Because it's got a pretty high chance of causing chaos too. A plugin replacing the entire map with stone because of renamed NMS calls has actually happened before.

    So you're annoyed that your API now uses wrapper types? I wonder where I've seen APIs with lots of wrapper types before...
     
    bergerkiller likes this.
  26. Offline

    asofold

    Certainly! But imagine keeping a bigger library up to date with backwards compatibility with only like 5-10 % code changes for an update. You now have to maintain a version with 100% code changes (roughly). This is not fun for the developer and it lessens the motivation to do it by... say 90%. In effect people will not be able or not be wanting to support minor version changes due to the safeguard, unless they manage form a team or hire some slaves to do the copy and paste stuff and also propagate each logical fix through all the modules. I do not fancy reflection for this, but for practical reason i can't possibly blame BergerKiller for using it.
     
  27. Offline

    bergerkiller

    Score_Under
    I agree on all your points, and indeed, anything can be bypassed like you said.

    I believe, and correct me if I am wrong, that the goal of this commit was to force developers that build again CraftBukkit to update every new version. As you have pointed out as well, this updating tends to become an annoyance, so people start using reflection to get around it, including me for practical reasons. The downside of all of this is, however, that no compile time errors are shown in the IDE when you use this much reflection. Field renames you will only find out once you reach said field.

    I tried to somewhat fix this by force-loading all reflection on start-up. This resolves 80% of these issues. The other issues are related to 'the same method name is still existing'.

    Anyway, the reason I push for the alternative to package versioning is the impossibleness to expose NMS types. If people can still import and expose NMS types in their library without risking all plugins that depend on it to break every new version, it will reduce the amount of reflection needed.

    For example, sending packets can only be done using a wrapper class in combination of, or, Objects representing packets. The same accounts for entity handles, block handles, chunk handles, world handles, and other stuff. Forcing this upon us results in code that looks like absolute crap sometimes, the only way I try to make it somewhat readable is by appending the type of the object to the name, e.g. entityPlayerHandle.

    Note: All of my plugins have been updates, none of them contain ANY NMS and CB calls anymore. All of that is in BKCommonLib. I am now cleaning up after me fixing reported bugs and similar issues.
     
  28. Offline

    Score_Under

    ... :)

    The issue I have with that is that people can still call methods on those objects, synchronize on them, access their fields, etc. which still ties them to the internal representation of the object, which can basically remove the benefits of using a library for it in the first place.

    If they weren't motivated to check every place they used NMS in the first place, they would have released a buggy/broken plugin.
    Also, My Little Integrated Development Environment: Find And Replace On ".v1_4_R1." Is Magic
     
  29. Offline

    bergerkiller

    Score_Under
    You are right about plugins using methods on NMS types, and that is indeed a problem. Plugins will not rely on a library to perform those methods, because they can 'just call it'. In that sense, providing wrapper class/utility methods does force plugins to be coded better.

    I guess it's more of an user preference issue I am having with this. My inner Java programmer just doesn't want to declare Object types to represent all sorts of things, and especially when multiple Object types are declared, it becomes a bit messy. This is mostly in the library, but also outside when plugins need to pass around, for example, player connections/networkmanagers/packets/IntHashMap/LongHashMap/LongHashSet/etc. I try to provide a wrapper implementation where possible, but for some commonly-used NMS types (ChunkCoordIntPair, ChunkPosition, ChunkCoordinates) it just looks bad, so I end up declaring IntVector2 and IntVector3 in my library to at least have a type I can use.

    This does, unfortunately, result in many additional type conversions (Vector<>Vec3D, and the mentioned IntVectors), which is pretty inefficient for performance-issue code, like TNT explosions. Sometimes the only option is to completely replace all use of an NMS type with an extended version of that type in the library, and this might cause errors/issues in the long run.
     
  30. Offline

    asofold

    Re-check that aspect :) ! That that is not a "happy" option for a library, trying to provide backwards compatibility at least for the last n minor versions of MC. It is not an option.

    Edit: It could be half an option to automatically relocate some module parts for specific versions but that is similarly complicated and "not funny either" to maintain. I am not sure if there are common tools that really improve the situation for this kind of partial-relocation/partial-rewrite thingy.
     
  31. Offline

    agaricus

    Wow, that looks like it's going to be fun to maintain :) To summarize the downsides:

    • Loss of compile-time type safety
    • Loss of compile-time field/method (and even class?) existence checks
    • Still have to check for naming changes on each update, since they could change at any time
    • Slower since reflection has to lookup the symbols dynamically (granted, probably negligible if only reflected once at startup)
    • Requires rolling your own version-checking system for safety
    • If the obfuscation changes in an update without a corresponding change in your code, an erroneous symbol could be easily referenced, possibly with catastrophic consequences
    All for what?

    New Minecraft versions, custom server builds, are all basically the same thing: different implementation details underneath the Bukkit API. There is another approach to supporting multiple implementations besides the reflection-based technique described above, an approach now taken by several popular plugins.

    Pioneered by mbax in his Support multiple Minecraft versions with abstraction/maven tutorial, it involves factoring out the NMS/OBC-dependent code, creating one copy for each version of NMS/OBC, and loading the appropriate version at runtime. Something like this:

    Code:
    // version loading code
            final String packageName = plugin.getServer().getClass().getPackage().getName();
            String cbversion = packageName.substring(packageName.lastIndexOf('.') + 1);
     
            final Class<?> clazz = Class.forName("com.example.plugin.compat." + cbversion + ".NMSHandler");
            NMSManager.provider = (NMSCallProvider) clazz.getConstructor().newInstance();
     
    // interface shared by all implementations, for access by the rest of the plugin
    public interface NMSManager …
     
    // implementation-specific handler for 1.4.7
    package com.example.plugin.compat.v1_4_R1;
    public class NMSHandler implements NMSCallProvider …
     
    // implementation-specific handler for 1.4.6
    package com.example.plugin.compat.v1_4_6;
    public class NMSHandler implements NMSCallProvider …
     
    // etc.
    
    How does this approach stack up?

    • Preserves compile-time type safety
    • Preserves compile-time field/method/class existence checks
    • Avoids unnecessary use of reflection, faster static access (though again, probably negligibly faster)
    • Built-in version detection, relying on the internal obfuscation counter the Bukkit team will increment for you when it changes (v1_4_R1)
    • No unnecessary updating required for old implementation version handlers

    So what plugins are using this or similar techniques?

    VanishNoPacket - The canonical example from mbax, uses NMS handlers exactly as above. Sub-projects in Maven for each NMS handler built against each supported version of CraftBukkit, loaded at runtime. code

    WorldEdit - Very similar, but goes a step further. The "nmsblocks" handlers can be loaded from a designated directory on disk, allowing for updates without updating the rest of the plugin if not needed. Even works without any NMS handler, falling back to a slower but version-independent means of modifying the world. code 2

    Dynmap: Also has refactored version support; in fact, the core codebase is platform-independent (and much of WorldEdit is too). Very good example of abstraction, with separate projects for each platform. code

    QuickShop: Same; checks package version and loads appropriate handler, although they are all in the same file, achieves the same result. code

    .. and probably more, but these are those I'm familiar with


    Appreciate your work on the custom plugin classloader, I learned a lot from it and it was quite instructive, but consider the consequences of package version safeguard stripping – if a plugin written for 1.4.5 references a symbol using the safeguarded packages, for example:

    Code:
    net/minecraft/server/v1_4_5/Packet23VehicleSpawn/h
    
    This is a well-defined identifier uniquely referring to a precise symbol which never changes. It always points to the 1.4.5 implementation's "h" field, which happens to be the vehicle type code.

    However, once package-stripping gets to it, the symbol becomes:

    Code:
    net/minecraft/server/Packet23VehicleSpawn/h
    
    Now the reference is ambiguous. (Class Name, Obfuscated Field Name) is insufficient to reliably identify the symbol, since the obfuscation can change between any version. This is the exact problem the safeguard fixed. The versioned symbol includes three pieces of information, (Class Name, Obfuscated Field Name, Obfuscation Version), disambiguating the obfuscation. Its like the "time" dimension of spacetime.

    Once you strip the version, obfuscated symbols could mean anything. If you also remove the Maven shade version relocation from the server implementation itself, and coincidentally happen to be running the same obfuscation version of the server as the plugin, only then will the symbols match. But what happens when you update?

    • In 1.4.5, "net/minecraft/server/Packet23VehicleSpawn/h" is the vehicle type
    • In 1.4.6, "net/minecraft/server/Packet23VehicleSpawn/h" is the rotation pitch
    • In 1.4.7, "net/minecraft/server/Packet23VehicleSpawn/h" is the rotation pitch

    The code will behave unexpectedly because its referencing the wrong symbol, manipulating the pitch instead of type, that can't be good (and it could be worse with another symbol). Here's the picture with versioned packages:

    • In 1.4.5, "net/minecraft/server/v1_4_5/Packet23VehicleSpawn/h" is the vehicle type
    • In 1.4.6, "net/minecraft/server/v1_4_6/Packet23VehicleSpawn/j" is the vehicle type
    • In 1.4.7, "net/minecraft/server/v1_4_R1/Packet23VehicleSpawn/j" is the vehicle type

    Notice there are no cross-version naming collisions. The plugin cannot accidentally ask for one symbol and get another depending on the version it is executed on. In order to confuse type with pitch, the plugin would have to actively go out of its way to circumvent the safeguard.


    So are there other pieces of information which would together uniquely identify symbols across versions, besides (Class Name, Obfuscated Name, Obfuscation Version)? Not the following:

    • Obfuscated name - of course, changes when Mojang reruns RetroGuard and refactors, cannot be relied upon
    • Declaration index - no, field #7 is vehicle type in 1.4.5 but field #10 in 1.4.6 and 1.4.7; this changes too
    • Data type - both fields are 'int', insufficient disambiguation

    The "obfuscation version" (monotonically increasing with the "game version", but only when the new game version is compiled with new obfuscation: historically, this would exclude: 1.2.4->1.2.5, 1.4.4->1.4.5, and 1.4.6->1.4.7 if the safeguard had been finalized before then) is by definition inexorably to the "obfuscated names", so its only logical to pair them together to create a unique identifier.

    Well, there's at least the starting point for this alternative available on my GitHub. Cross-version remappings for 1.4.7 to 1.4.6, 1.4.5, 1.4.4, 1.4.2, 1.3.2, and 1.3.1 mc-dev, and a working custom classloader (based on bergerkiller's initial work but significantly diverged since then) to remap plugin classes on load. Not naively stripping the version, but mapping only known corresponding symbols. It works and I tested it with a fewer older plugins (but I had to go out of my way to find older versions of the plugins since most have updated, go figure), although the mappings would have to slightly updated for use in vanilla CraftBukkit, since I was mapping to a fully-obfuscated environment for Forge mod compatibility -- in fact, I had to remap all versions of NMS, even the current version, so a custom classloader was a necessity for any non-Bukkit API plugin support at all, and cross-version remapping was a natural extension of this concept.

    However, introducing an ASM-based remapper into the plugin classloader is not without risk, and I'm not sure it would make sense for vanilla CraftBukkit. Here's some of the real problems I've seen:

    • Non-hierarchical codesource URL error from plugins which use
      Code:
      new File(getClass().getProtectionDomain().getCodeSource().getLocation().toURI())
      to get resources (broke Residence)
    • Failure to throw CNFE for non-existent classes (broke CommandHelper)
    • Some classes are too large for ASM, at least with the settings I was using (SQLite overflows the constant pool)

    All these problems can be fixed, I mention them not to get into specifics, but as examples of the kind of added risk you'll see by living with a remapping classloader.

    And even with 100% bug-free static reference remapping, you still have to worry about plugin code like this:

    which dynamically accesses the symbols and classes by variable, so it won't be picked up by org.objectweb.asm.commons.Remapper. Extra code would have to be written to remap reflection. Some simple cases like this:

    Code:
       su.mobIDField = TileEntityMobSpawner.class.getDeclaredField("mobName");
    
    can be statically remapped by pattern matching bytecode:

    Code:
     8: ldc_w         #17                 // class net/minecraft/server/v1_4_R1/TileEntityMobSpawner
      11: ldc           #18                 // String mobName
      13: invokevirtual #19                 // Method java/lang/Class.getDeclaredField:(Ljava/lang/String;)Ljava/lang/reflect/Field;
    
    and replacing the field string constant appropriately (fortunately, plenty of plugins use reflection in this simple manner and can be remapped), but any more sophisticated uses of reflection will require runtime lookups and remaps. Theoretically possible, but non-trivial. Consider for example, java/lang/Class#getMethods, it returns an array of all the methods of a class, so a perfectly comprehensive reflection remapper would have even remap this.. reflection is inherently introspective, not easily remapped.

    And even then assuming perfect reflection remapping is practical, non-obfuscation changes to Minecraft internals would occasionally break implementation-dependent code regardless. So a complete "compatibility layer" would be needed to provide truly safe cross-version plugins. Implementing NMS for 1.4.5, 1.4.6, 1.4.7, etc. on top of the current version of Minecraft, for every release. Someone could do this if there is enough demand, maybe this approach will become more attractive if there are major internal changes, but its a serious effort, I sure am not signing up for :). Taking this thought experiment further, assuming someone does, the NMS-to-NMS layer will likely be buggy (at least at first) because it requires building on volatile obfuscated code rather than a stable API, a shaky foundation if there is one; requiring significant effort to converge to a reasonable standard of quality.

    Long story short, this approach would be a lot of work -- and it would be for supporting something which is unsupported.

    Not the most attractive option imho, especially when there's already a well-supported fully-abstracted (well, to a reasonable extent) layer out there: the Bukkit API. Sure it doesn't provide access to everything, but in most cases the required API bypasses can be quarantined into version-appropriate handlers, as many major plugins have done (WorldEdit, Dynmap, VanishNoPacket, etc.) to provide backwards-compatibility, while retaining the compatibility benefits of using the Bukkit API in other instances.

    But perhaps someone would be interested in developing this cross-version remapper into a separate standalone tool? For admins to run in an attempt (no guarantees) to update their old plugins.

    Concretizing the "non-Bukkit API plugin" (aka "CraftBukkit mod", or my personal favorite, "NMS plugin") concept is an interesting idea. Not sure about the CraftJavaPlugin proposal, but I think having at least some mechanism for server admins to distinguish between pure-API plugins and other plugins would be quite useful.

    I know it has been brought up before, but perhaps making the dichotomy explicit on BukkitDev would be a good first step. This could be mostly automated with PluginAnalyst, a tool to analyze Bukkit plugins for API usage. Then plugins could be categorized as for "Bukkit", or "CraftBukkit"; this option might even already be available on BukkitDev, but it isn't currently enforced afaik.

    However, a further problem is managing the continuum of non-API usage. Some plugins only use a sprinkling of NMS, and can fallback to API or disable optional functionality if the correct NMS is unavailable. Other plugins might require NMS, but only in a minor way. Yet other plugins could extensively depend on NMS, hooking deeply into Minecraft internals, reflecting into private fields, replacing entire classes, etc., basically making their own private version of CraftBukkit. Mapping this continuum onto a field in BukkitDev would require some more thought.


    Use the method signature (name + descriptor) instead of just the name?

    Delegation or inheritance are two possible solutions. CraftBukkit uses both, but mainly delegation. Interestingly, I can only think of one usage of inheritance off hand:

    Code:
    public class WorldServer extends World implements org.bukkit.BlockChangeDelegate {
    
    That's right, CraftBukkit is implementing an API interface in NMS code. BCD is setRawTypeId, setRawTypeIdAndData, setTypeId, setTypeIdAndData, getTypeId, getHeight, and isEmpty. These methods are implemented in net.minecraft.server.World by (semi-)deobfuscated Mojang code:

    Code:
        public boolean setRawTypeIdAndData(int i, int j, int k, int l, int i1) {
    …
                    Chunk chunk = this.getChunkAt(i >> 4, k >> 4);
                    boolean flag = chunk.a(i & 15, j, k & 15, l, i1);
                    this.v(i, j, k);
    …
        }
    
    What if this volatile method changes signature in an update? Would break the interface. And BCD couldn't be changed without breaking Bukkit API backwards-compatibility. Most likely CraftBukkit would have to add a wrapper method to satisfy the interface but still calling into native code. They probably had good reasons for choosing inheritance instead of delegation in this instance, but generally, CraftBukkit uses delegation instead, for good reason.

    If you just want to avoid writing "v1_4_R1" in your code, check out this tutorial I posted a few days ago in Plugin Development - Resources: How to develop plugins using MCP for NMS via ASM remapping. It will allow you to write the source code for your plugins using MCP mappings (nearly complete deobfuscation), and then compile to CB mappings (partial deobfuscation). You'll still need to recompile for each obfuscation version (it is not a safeguard bypass), but at least your NMS source will be less volatile, not to mention more descriptive. Maybe someone could even combine it with the abstraction techniques discussed above to achieve the best of both worlds.

    To give you and bergerkiller a taste, consider this bugfix:
    https://github.com/bergerkiller/BKCommonLib/commit/59cdd355a1b6906316449ccc4b33ba3f1dcd84a1 Fixed minecart saving bug, improved nbt list values setting/adding

    Code:
    // src/main/java/com/bergerkiller/bukkit/common/bases/EntityMinecartBase.java
    import net.minecraft.server.v1_4_R1.EntityMinecart;
    
    public class EntityMinecartBase extends EntityMinecart {
    …
       public void onLoad(CommonTagCompound data) {
    -    super.d((NBTTagCompound) data.getHandle());
    +    super.a((NBTTagCompound) data.getHandle());
       }
    
    with full deobfuscation, the bug is much easier to spot:

    Code:
    import net.minecraft.entity.item.EntityMinecart;
    
    public class EntityMinecartBase extends EntityMinecart {
       public void onLoad(CommonTagCompound data) {
    -    super.writeToNBT((NBTTagCompound) data.getHandle());
    +    super.readEntityFromNBT((NBTTagCompound) data.getHandle());
    }
    
     
    Comphenix, chaseoes, andrewpo and 4 others like this.
Thread Status:
Not open for further replies.

Share This Page