Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relocated entirety of net.kyori packages #717

Closed
wants to merge 2 commits into from
Closed

Conversation

OakLoaf
Copy link

@OakLoaf OakLoaf commented Apr 22, 2024

Currently only a few of the net.kyori packages are relocated and it can/is causing incompatibilities with other plugins making use of other versions of adventure/MiniMessage.

Resolves #712

@Xemorr
Copy link

Xemorr commented Apr 26, 2024

This issue is very important, causing incompatibilities for me

@AbhigyaKrishna
Copy link
Collaborator

We can't really relocate everything from net.kyoti as it will cause incompatibility with dependent plugins.

@Xemorr
Copy link

Xemorr commented May 1, 2024

We can't really relocate everything from net.kyoti as it will cause incompatibility with dependent plugins.

They shouldn't be relying on PacketEvents' dependency on Adventure, currently any server that includes PacketEvents, or has a plugin that shades PacketEvents, can cause conflicts with other plugins.

@OakLoaf
Copy link
Author

OakLoaf commented May 1, 2024

We can't really relocate everything from net.kyoti as it will cause incompatibility with dependent plugins.

Adventure has a pretty good explanation in their FAQ as to why developers should relocate it - the same goes for other dependencies

@AbhigyaKrishna
Copy link
Collaborator

Relocating adventure-api will cause all the imports from net.kyori package to be unusable as the qualifier/package of the library will change. We are providing adventure as a api usage. After relocation of adventure, the said functions and classes will have a different qualifier and will no longer be compatible when used.

@OakLoaf
Copy link
Author

OakLoaf commented May 2, 2024

Relocating adventure-api will cause all the imports from net.kyori package to be unusable as the qualifier/package of the library will change. We are providing adventure as a api usage. After relocation of adventure, the said functions and classes will have a different qualifier and will no longer be compatible when used.

I'm not fully understanding what you're saying, relocating a package does not require you to change the imports within PacketEvents, it would require people using PacketEvents to update their imports but that's not really a good reason to leave it causing incompatibilities with other plugins.

Did you read the link I sent by the way?

@Chevels
Copy link

Chevels commented May 17, 2024

Indeed, as long as this issue is not fixed we simply cannot connect to the server if we install PacketEvents... :/

@Xemorr
Copy link

Xemorr commented May 17, 2024

🐐🐐🐐🐐

@Xemorr
Copy link

Xemorr commented May 17, 2024

Once this is merged, reminder to edit this wiki page:
https://github.com/retrooper/packetevents/wiki/Shading-PacketEvents

and remove the code telling the user to relocate net.kyori themselves, as this PR will do it for the user.

@AbhigyaKrishna
Copy link
Collaborator

Relocating adventure is not as simple as it seems. You can try building packetevents with the relocation and running on your server.

@Xemorr
Copy link

Xemorr commented May 17, 2024

Relocating adventure is not as simple as it seems. You can try building packetevents with the relocation and running on your server.

In the case, where the plugin using packetevents shades packetevents, then it is being relocated to the EXACT same location as is recommended in the wiki page. This means it is not a breaking change for any plugin that shades it. I would bet on this being the most common use case of packetevents in public plugins as shading is more convenient for the end user.

The case where it could break is in the non-shaded scenario, if they use the adventure api that is shaded within PacketEvents, but there are not going to be many plugins that do this for a start, and those that do just need to be recompiled.

I would be surprised if there are that many plugin developers that aren't shading, that are even aware that PacketEvents comes with its own implementation of Adventure. So ultimately, yes it is a "breaking change" by the strict semantic versioning definition, and yes, it will require a bump to a 3.X.X, but plugin developers are able to just increment the major version number in their build script and continue on as normal.

It is best practice to relocate libraries used by a project when shading, as stated here in adventure and here in PacketEvents.

@AbhigyaKrishna
Copy link
Collaborator

So, here is the full scenario if you didn't get what the issue here is due to relocation.

In the current scenario we have some of the classes of adventure relocated according to our needs. First of relocation does mean it a better concept or a better approach to something. In your issue #781, you have a incompatibility error with the breaking changes which adventure applied in their library. You are using adventure version 4.14.0 while we are on 4.16.0. I already made contribution for supporting older adventure version but there may be some loopholes which got left out and you guys are facing.

Adventure on its own doesn't have any backward compatibility, in it and it doesn't need to. They would obvious have to choose a better approach over their previous if any. And one more thing to add is, paper/spigot having previous versions of adventure api included in it. So we are stuck here having to support older versions of everything on our own.

Ever wondered?

  • Why doesn't any of the java library project doesn't relocate their external dependencies?
  • Why didn't paper (library project over bukkit/craftbukkit/nms) didn't relocate adventure api? I mean according pr issue, they should have.

Relocation is a task for mitigating conflicting dependency version which an application uses, that can be present in the jvm. It is not a convention to which should be followed in every project specially in libraries.

PacketEvents is not an application, its a library.

For library projects, if we relocate our external dependencies, then any dependent project have to use our relocation in their codebase for the particular external library. This will be a compatibility issue for them as they wouldn't be able to use objects of our relocation in any other library with the same external dependency. For a library project, we can relocate "non-widely used" external libraries as they won't cause any major issue in most cases. But for libraries as big as adventure it is not possible to relocate it.

Lets say we relocated adventure in packetevents, we now have net.kyori at io.github.retrooper.packetevents.kyori according to this pr. Now we are working on a paper plugin project,

package me.abhigya.plugin;

import com.github.retrooper.packetevents.PacketEvents;
import com.github.retrooper.packetevents.event.*;
import com.github.retrooper.packetevents.event.simple.*;
import com.github.retrooper.packetevents.protocol.packettype.PacketType;
import com.github.retrooper.packetevents.util.TimeStampMode;
import com.github.retrooper.packetevents.wrapper.play.server.WrapperPlayServerChatMessage;
import io.github.retrooper.packetevents.factory.spigot.SpigotPacketEventsBuilder;
import org.bukkit.entity.Player;
import org.bukkit.plugin.java.JavaPlugin;

public class MyAwsomePlugin extends JavaPlugin {
    @Override
    public void onLoad() {
        PacketEvents.setAPI(SpigotPacketEventsBuilder.build(this));
        PacketEvents.getAPI().load();
    }

    @Override
    public void onEnable() {
        PacketEvents.getAPI().getSettings().debug(false).bStats(true).checkForUpdates(true).timeStampMode(TimeStampMode.MILLIS).reEncodeByDefault(true);
        PacketEvents.getAPI().init();

        SimplePacketListenerAbstract listener = new SimplePacketListenerAbstract(PacketListenerPriority.HIGH) {
            @Override
            public void onPacketPlaySend(PacketPlaySendEvent event) {
                if (event.getPacketType() == PacketType.Play.Server.CHAT_MESSAGE) {
                    WrapperPlayServerChatMessage packet = new WrapperPlayServerChatMessage(event);
                    ((Player) event.getPlayer()).sendMessage(packet.getMessage().getChatContent());
                }
            }
        };
        PacketEvents.getAPI().getEventManager().registerListener(listener);
    }

    @Override
    public void onDisable() {
        PacketEvents.getAPI().terminate();
    }
}

Now we have a issue here, we can't just pass packet.getMessage().getChatContent() to Player#sendMessage. We know they are of adventure's Component type. But now they are incompatible classes as packet.getMessage().getChatContent() returns an object with qualifier io.github.retrooper.packetevents.kyori.adventure.api.Component but paper requires an object of net.kyori.adventure.api.Component. They are now incompatible to be morphed into one another.

This is one of the simplest incompatibility issue for relocation. There are more such reasons to not relocate dependencies in an api project. Same reasons apply to bukkit, spigot and paper not relocating gson, adventure and other dependencies in their project.

If you still didn't get why relocation couldn't be done, you can message me on discord at abhigyakrishna.

@Chevels
Copy link

Chevels commented May 18, 2024

That's all well and good, but in this case what should the poor server administrators do who, like me, find themselves having to install PacketEvents because a plugin that we bought requires it to work, and no one can connect to server anymore??
In my case, I've been running my server for 13 years, I'm a passionate and good-willed girl, but I've never heard of this adventure library shit, and I have absolutely no idea how to change version. Where is this library? It comes from paper, a plugin, Mojang... from what??
Because the developer of the plugin (AntiCheatAddition) kindly responded to me off the mark, and made me understand that he pays no attention to this error; and the guys from PacketEvents, you tell us that you can't do anything... What do we do then?

All I know as a user is that if I install PacketEvents on my server, no one can connect. So, what do we do ? Are we doomed to not being able to use plugins that depend on PackentEvents?

@AbhigyaKrishna
Copy link
Collaborator

AbhigyaKrishna commented May 18, 2024

All I know as a user is that if I install PacketEvents on my server, no one can connect. So, what do we do ? Are we doomed to not being able to use plugins that depend on PackentEvents?

Best case scenario, I could try to have a patch going for this issue but I am not sure we could fix this on our side. You have to ask the plugin developer to either shade and relocate packetevents and adventure or update your server to use latest version of adventure.

In my case, I've been running my server for 13 years, I'm a passionate and good-willed girl, but I've never heard of this adventure library shit, and I have absolutely no idea how to change version. Where is this library? It comes from paper, a plugin, Mojang... from what??

If you are running your server for 13 years, you should atleast have some knowledge of JVM and libraries and how plugins interact with each other. It's not as simple as dropping all plugins in a plugins folder expecting everything to work out of the box. Everything have their compatibility needs and issues.

@Chevels
Copy link

Chevels commented May 18, 2024

All I know as a user is that if I install PacketEvents on my server, no one can connect. So, what do we do ? Are we doomed to not being able to use plugins that depend on PackentEvents?

Best case scenario, I could try to have a patch going for this issue but I am not sure we could fix this on our side. You have to ask the plugin developer to either shade and relocate packetevents and adventure or update your server to use latest version of adventure.

In my case, I've been running my server for 13 years, I'm a passionate and good-willed girl, but I've never heard of this adventure library shit, and I have absolutely no idea how to change version. Where is this library? It comes from paper, a plugin, Mojang... from what??

If you are running your server for 13 years, you should atleast have some knowledge of JVM and libraries and how plugins interact with each other. It's not as simple as dropping all plugins in a plugins folder expecting everything to work out of the box. Everything have their compatibility needs and issues.

Oh don't worry, I think I have a good basis for managing a server and its plugins, without any pretension however, but there are sometimes new problems and in this case I learn. This is the case here, however that does not make me a developer because with family, children, work, the house to run, my server and my community to manage... I unfortunately do not have the time to learn to code. Apart from assembly language 68000 (haha that goes back, only old hands should understand what I'm talking about^^), I know nothing about Java development, apart from a few notions.
For example, you tell me that I need to update my server to change the version of my Adventure API. Great, but what should I update? Paper? something else ? If it's Paper, we can't change version yet because 1.20.5 breaks too many plugins (33 out of 90) which are not yet up to date. Not to mention the thousands of command_blocks I have to reprogram for our RP.

This API is certainly familiar to budding developers, but for the end user, there is nothing. I even looked to see if there was a .jar version to add it to the server as a plugin (after all, you never know...), but after 2 hours of searching, the only .jar I found doesn't load because it's not a plugin, as I expected. So what to do to update it on the user side? (server admin side)

As for the developer of the plugin in question, I will therefore turn to him now to talk to him about this, but he himself does not seem to realize what you have just told me, unless he turns a deaf ear so as not to having to get my hands dirty ^^ I'll see.

Thank you for your response and have a good weekend!

@AbhigyaKrishna
Copy link
Collaborator

Worked on a new way of patching adventure library on #814. Relocation was never needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependencies aren't relocated
5 participants