Be more lenient with version checks when updating the minor version of the game#594
Merged
DerEchtePilz merged 10 commits intodev/devfrom Aug 14, 2024
Merged
Be more lenient with version checks when updating the minor version of the game#594DerEchtePilz merged 10 commits intodev/devfrom
DerEchtePilz merged 10 commits intodev/devfrom
Conversation
That includes an update from, for example, 1.21 -> 1.21.1, but not, for example, 1.21.1 -> 1.22 Also changes build order to first package actual artifacts (shade, plugin) and then runs tests
Timongcraft
reviewed
Aug 12, 2024
Timongcraft
reviewed
Aug 12, 2024
...bukkit/commandapi-bukkit-vh/src/main/java/dev/jorel/commandapi/CommandAPIVersionHandler.java
Outdated
Show resolved
Hide resolved
willkroboth
requested changes
Aug 12, 2024
...bukkit/commandapi-bukkit-vh/src/main/java/dev/jorel/commandapi/CommandAPIVersionHandler.java
Outdated
Show resolved
Hide resolved
commandapi-core/src/main/java/dev/jorel/commandapi/InternalConfig.java
Outdated
Show resolved
Hide resolved
commandapi-core/src/main/java/dev/jorel/commandapi/CommandAPIConfig.java
Outdated
Show resolved
Hide resolved
...forms/commandapi-bukkit/commandapi-bukkit-plugin-mojang-mapped/src/main/resources/config.yml
Outdated
Show resolved
Hide resolved
commandapi-platforms/commandapi-bukkit/commandapi-bukkit-plugin/src/main/resources/config.yml
Outdated
Show resolved
Hide resolved
...bukkit/commandapi-bukkit-vh/src/main/java/dev/jorel/commandapi/CommandAPIVersionHandler.java
Show resolved
Hide resolved
...bukkit/commandapi-bukkit-vh/src/main/java/dev/jorel/commandapi/CommandAPIVersionHandler.java
Outdated
Show resolved
Hide resolved
...bukkit/commandapi-bukkit-vh/src/main/java/dev/jorel/commandapi/CommandAPIVersionHandler.java
Outdated
Show resolved
Hide resolved
willkroboth
requested changes
Aug 13, 2024
commandapi-core/src/main/java/dev/jorel/commandapi/CommandAPI.java
Outdated
Show resolved
Hide resolved
commandapi-core/src/main/java/dev/jorel/commandapi/CommandAPIConfig.java
Outdated
Show resolved
Hide resolved
Co-authored-by: willkroboth <[email protected]>
DerEchtePilz
force-pushed
the
dev/minor-version-leniency
branch
from
bba5e08 to
f063532
Compare
DerEchtePilz
force-pushed
the
dev/minor-version-leniency
branch
from
f063532 to
e0e73c5
Compare
willkroboth
reviewed
Aug 14, 2024
...forms/commandapi-bukkit/commandapi-bukkit-plugin-mojang-mapped/src/main/resources/config.yml
Outdated
Show resolved
Hide resolved
...bukkit/commandapi-bukkit-vh/src/main/java/dev/jorel/commandapi/CommandAPIVersionHandler.java
Show resolved
Hide resolved
...bukkit/commandapi-bukkit-vh/src/main/java/dev/jorel/commandapi/CommandAPIVersionHandler.java
Outdated
Show resolved
Hide resolved
...bukkit/commandapi-bukkit-vh/src/main/java/dev/jorel/commandapi/CommandAPIVersionHandler.java
Outdated
Show resolved
Hide resolved
...bukkit/commandapi-bukkit-vh/src/main/java/dev/jorel/commandapi/CommandAPIVersionHandler.java
Outdated
Show resolved
Hide resolved
willkroboth
requested changes
Aug 14, 2024
...ommandapi-bukkit-plugin-mojang-mapped/src/main/java/dev/jorel/commandapi/CommandAPIMain.java
Outdated
Show resolved
Hide resolved
...ommandapi-bukkit-test-tests/src/test/java/dev/jorel/commandapi/CommandAPIVersionHandler.java
Outdated
Show resolved
Hide resolved
Co-authored-by: willkroboth <[email protected]>
willkroboth
approved these changes
Aug 14, 2024
willkroboth
added a commit
that referenced
this pull request
Sep 1, 2024
This was a similar issue to what was brought up here: #594 (comment). It seems that having `NMS_1_21_R1::new` as a method reference still loads the `NMS_1_21_R1` class enough that Java gets mad. On 1.19, trying to load the CommandAPI gives `java.lang.VerifyError: Bad type on operand stack - Type 'net/minecraft/commands/CommandBuildContext' (current frame, stack[1]) is not assignable to 'net/minecraft/core/HolderLookup$a'` with the stacktrace going through that line. Moving all references to the `NMS_1_21_R1` class into the if statements allowed loading on 1.19 as expected. That does mean the slight inconvenience of having to specify and update the latest NMS object in two place.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, when updating to any new Minecraft version that is not currently supported, we throw an
UnsupportedVersionException.This is not that great since even minor updates require an update to not throw that exception.
This pull request enables, when setting the config option, an update to minor versions of the game:
UnsupportedVersionExceptionsince presumably we still want to double check compatibility for major versions.So why the new config option when we have the option of just loading the latest NMS version?
Well, the latest NMS version completely breaks support for older versions, this only does something when a server version is used that the main part of
CommandAPIVersionHandler#getPlatform()does not handle currently.Further considerations:
Currently, this does not include a new config option for the plugin version but I think this is something that could and should be done before merging.