-
-
Notifications
You must be signed in to change notification settings - Fork 644
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
let extensions specify geyser api version instead of base api version #3880
base: master
Are you sure you want to change the base?
Conversation
core/src/main/java/org/geysermc/geyser/extension/GeyserExtensionLoader.java
Outdated
Show resolved
Hide resolved
Bumping the base api version seems to have fixed building. |
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.
Project lead needs to take a look at this
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.
Besides these comments I'd remove the major,minor,patch fields of GeyserExtensionDescription and use ApiVersion there as well
if (description.majorApiVersion() != 1) { | ||
GeyserImpl.getInstance().getLogger().error(GeyserLocale.getLocaleStringLog("geyser.extensions.load.failed_api_version", name, description.apiVersion())); | ||
return; | ||
} else { |
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.
Else not needed here because of the return
GeyserImpl.getInstance().getLogger().error(GeyserLocale.getLocaleStringLog("geyser.extensions.load.failed_api_version", name, description.apiVersion())); | ||
return; | ||
// Completely different API version - or if the extension requires new API features, being backwards compatible | ||
if (!GeyserApi.api().geyserApiVersion().isCompatible(description.majorApiVersion(), description.minorApiVersion(), description.patchApiVersion())) { |
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.
I think we should have more methods in ApiVersion.
We want to deny extensions that don't have the same major version.
I don't even think we need to log a warning for plugins using an api version that is still compatible (same major version).
But if we do, I would want to run an description.version().isOlderThan(geyserApiVersion())
or geyserApiVersion().isNewerThan(description.version())
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.
We would not issue a warning though - isCompatible returns only false when the requested api version is newer than the given (outdated Geyser, or some dev extension), or when the major version is completely different.
The warning only exists since we would be switching from specifying base api version -> geyser api version; as to not break backwards compatibility. see here
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.
Right, forgot what the switch meant for already existing extensions their yml file.
But still, as long as the major version is the same there can't be situations where the minor and patch versions matter.
So we only have to do:
if (GeyserApi.api().geyserApiVersion().major() != description.major()) {
// workaround for the switch to Geyser API versioning
if (GeyserApi.api().baseApiVersion().major() != 1) {
// incompatible extension version
}
}
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.
This would unfortunately allow extensions that rely on newer api compared to the present api to be loaded; which would crash Geyser :/
It in theory should not happen if Geyser is up to date.. but we cant guarantee that.
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.
Major version etc. used to match the base api, that was 1.0.0.
1(.0.0) != 2(.1.1), but 1 == 1, so it's compatible.
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.
I understand what you're trying to say now.
The api version in the extension should be the minimum supported version, so we should indeed check if the api version is newer or equal to the minimum supported api version of the extension. However we only have to check the major and minor version if we follow semantic versioning.
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.
However we only have to check the major and minor version if we follow semantic versioning
part of Redned's proposal on Discord was that api additions bump the patch version, which this PR was following.
It would be nice to follow semantic versioning though. In that case, api additions would bump minor. Every major minecraft update (1.21, 1.22) would bump the major version, allowing for deprecations to be removed (in spirit with red's proposal).
That would cause extensions to break on every minecraft update though. I vaguely remember a conversation about wanting to avoid that. I guess the api major could be bumped for removals, but not necessarily for every large minecraft update.
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.
wouldnt that also require Geyser to be bumped on each major version, since api is supposed to follow geyser versioning?
I personally would stick to what is proposed tbh; but that's just my two cents
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.
I guess the api major could be bumped for removals, but not necessarily for every large minecraft update.
Yes, there is no need to do a major version bump for Minecraft updates because there have been no changes to the api. It does need to be bumped when we remove deprecated stuff though.
Needs rebase or merge |
Relies on GeyserMC/api#2
Changes: