-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add server version util class #10253
Conversation
I believe the concept here is great, I am currently unsure if the impl and api methods are the best tho. A) int varargs seems weird. This would lock the API to only support major versions, which I am unsure is something we want to commit to. |
Wouldn't it make more sense to transfer this Check Logic into the PaperLib and rather return Try Version String and Protocol Version into the bootstrap context ? So this is just a suggestion |
I think this stuff fits best into the server, because only the server can reliably tell what it implements. The checking for classes (what PaperLib does) might not properly reflect everything depending on what is checked |
Well, if PaperLib can test on the basis of given things. Bootstrap context or other things. You don't have to do anything with Reflections if the correct values are given. I see such a version check more as an ultitiy than that it should be part of a server software. But that's more my perspective. |
A) I will think how I could change that B) Sadly, "implements" is a reserved word by java, that's why it's "implement". And what are the guarantees that that other fork won't use the same namespace? If someone named their fork exactly the same as other fork in my opinion it's their fight, I made this method mostly for popular forks, like folia, since if a plugin will make a special support for a fork, it's really not likely it will make it for a small unknown fork. |
kek, yea you are right. flame my tired brain. |
You can look at the implementation, it's based on 3 information from the nms
Also this pr doesn't modify any classes, just adds some new, so it's reaaaalllyyy cheap to maintain.
Why should I use paper lib if I only support paper, in a paper plugin, just to check the version..... |
Maybe then |
What about |
I think The "api" in the method name makes it a bit too verbose |
But I also think that some sort of name spacing wouldn't be bad. It looks weird to provide a String imo. |
I'm pleased to see the discussion taking place here, let me respond to respond points first:
But isn't Paper as much Spigot (the server, not the plugin), as it is CraftBukkit?
The point here is feature-based interaction and enablement. I strongly believe this should be called .supports() or similar. Supports Folia? Yes. Suports X-Y patch from 1 year ago? No. Supports a sub-feature of a patch from 9 months ago? Yes.
If the API operates on strings, idk how do you want to enforce it? Change the API to explicitly require a parent namespace key + child key? This is little different from a convention calling features like "paper.versioningapi". This then will be similar to how browsers implement experimental/proprietary CSS features by adding a -webkit or -moz prefix. Versioning of the Features? If this idea evolves to a feature-support API, at some point it'll be useful and necessary to retrieve a feature's individual version number. The game/server revision could be used for it, but it's more work and less reliable than knowing a certain patches' exact version. "Just add a more specific key to a patch" is not the complete answer, because there can be a patch gap between a behavioral change/feature's implementation and the understanding that it warrants a new, more specific feature key. Although I'm surprised by vararg version ints too, my question is different. How to work with and represent weekly snapshots? And the absolute edge case, special editions like April 1st (ok this is over the top). For "isVersion" I would add CAPS docs to instead use greater/lower than for versioning (especially if this PR moves toward capability-based API queries). Because isVersion should be understood as hardcoding an exact version in the majority of cases. I find this is a better (and more gentle) solution for a seamless multi-version and multi-vendor support than reflection/exceptions. It'd be better placed in upstream (Spigot). |
I think this will bring too much trouble to maintain, all changes to features are made to have backwards compatibility, so you may just need to check for the version + fork api presence to see if the new feature capability is here
As I see it, very soon the hard fork will happen, then it will be very hard to say that the server is implementing the spigot/bukkit api, adding something with the scope of removing it very soon is not a bright idea in my opinion
The point is the implemented fork api, as novice developers often may forget to store the value if the server is folia, meaning they will have often Class#forName calls, which are expensive, this new method should be safe to recommend in docs and other places for new developers. Also this is a way easier method than to find the class and copy it for class check.
I thought about changing PaperServerInfo#is to a string which just checks for equals with current version and PaperServerInfo#isAtLeast will just mean if the current version appeared after the specified release version or if it's the current release version
Personally, I don't want to care about the spigot api, if people want to still support spigot, then they may continue to just deal with the missing api like this, I didn't support spigot in my plugins for a long time and recommend others to do the same (even though I understand that private plugins are a lot easier to make without spigot support than public/paid plugins) |
Made a few changes which we discussed here, like using a key to check for implemented api and support for non-release versions. The support for snapshots/pre-releases could be made better but I don't think that it's an environment for which we may want to spend too much effort to give the best support possible. But at least this pr already defines a possible future behaviour that developers may expect, and which can be further discussed here. The usage and the output were updated to reflect the changes. |
I really like this concept but I am unsure if the class should be named |
Idk, PaperServerInfo sounds better for me, but if someone else agrees then i can change that to ServerInfo |
Well, I think for Paper it's fine either way but I was much more thinking of forks for example. Sure, they can patch that name again but naming it ServerInfo is much more generic than PaperServerInfo. |
I agree with DerEchtePilz on this one. |
"VersionInfo" or "Version" already does not go well with isImlementing, also limits future possibilities to add utility methods Who knows what other information about the server people may want to get in the bootstrap, naming it "ServerInfo" will give green light to a lot of read only operations that may be invoked before CraftServer exists, I just added mostly only version methods because that's the most used thing right know but which is inaccessible in bootstrap, and don't think other methods may fit well in this pr
|
Rebased on 1.20.5 |
Wouldn't this be a great addition to the removal of relocation to provide a straightforward way of getting the version? I mean it would only work from 1.20.(5/6)+ but I guess it's good to include both in the same version so there is a nice and modern alternative. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So after a bit of discussion on this, I think we'd be (for now) best of to only
pull changes regarding full release versions.
Snapshot handling is complex, specifically with isAtLeast.
The commits in this PR will keep the logic around, so when we do get to releasing paper for snapshots they can serve as starting point.
Beyond that, see below for changes for request.
Again, thank you for the pr 🥳
Applied all requested changes |
Fixed the javadoc which failed the workflow |
Applied all requested changes |
Hey @masmc05! Thanks for your work on this pull request! ❤️ We've merged (most) of your work into #10729 where we've done some other changes related to versioning, and we'll be continuing this pull request over there. Feel free to comment over there. Note: there are some minor differences, and there are one or two methods missing in #10729 currently. |
Yeah, I noticed, I'm glad that my work was able to help creating an even bigger addition to paper. |
Currently, it's impossible to get the information about the server from bootstrap, as
Bukkit.getServer()
is null.The goal of this pr is to allow an easy access to information about the server, even from bootstrap.
Here's an example usage:
which results in those outputs (Bukkit version remained the same since it's unknown how it will be in reality for a snapshot etc, just changed the version.json file to ones provided in other versions)
For 1.20.6: