-
Notifications
You must be signed in to change notification settings - Fork 150
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
Build against Java 8, Bukkit/Spigot 1.12 (closes #404) #410
Conversation
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.
Bumping dependencies and changing compilation target level are two major changes that address two often very different problems. Bundling two such major changes comes with massive risk (not "danger"), so it is something that should only ever be done out of necessity and not willy-nilly. I'm not saying no thought was put into this, and I'll of course elaborate on my doomsaying 😛
I've addressed the issues of bumping compilation target level in #404. In summary, the bump is based on gut feeling (and because 8 brings lots of quality-of-life changes for us developers), and we have no clue how many server owners such an update will affect. To elaborate: one of the main non-functional aims of the project is to be inclusive and backwards compatible (within reason). Switching compilation target level without any sort of data means potentially stepping on someone (who may not actually be in control of their JRE because they're hosting remotely).
Bumping API versions also carries high risk. Usually, the API is backwards compatible (less so with Spigot than with Bukkit) in the sense that a build compiled against an older version of the API will run on implementations of newer versions of the API. But the API is very rarely forwards compatible (Wolvereness' excellent work with OverMapped in Bukkit is an exception). This means that to be inclusive, it is necessary to "stay old" for as long as possible. There are lots of servers running MobArena on MC versions 1.8 through 1.11 (which is why the mc18 branch exists). Bumping API versions is only ever acceptable in two situations:
- there are breaking changes that need to be addressed for MobArena to run on updated servers, or
- there are highly relevant additions to the API (typically new mobs that require special treatment to be effective/behave properly in MobArena).
Both situations carry heavy risk in that the bump most likely violates the inclusion aim of the project, because certain server versions will be excluded from the new versions of MobArena. The mc18 branch is a nightmare to maintain (especially on Spigot's resources platform since it doesn't support multiple builds very well). But this does give way to the idea of implementing reflection-based support (must have extremely low LOC count for maintainability) for older versions of the API so long as the corresponding MC versions are actively played.
TL;DR: I think the move to Java 8 carries the lowest risk of these two changes, but I'd like some evidence that it is, indeed, low risk before committing to it. An API bump is always high risk, and unless there are breaking changes in 1.12 (I couldn't find any) or highly relevant additions, I see no reason for it. At any rate, they should be handled separately.
@garbagemule It's a fair point. I did some research on this in the Spigot community and got some answers for us to tackle this. bstatsCurrently, there is a platform called bstats which provides the same metrics on Bukkit servers as mcstats did. Roughly ~48,000 servers are measured here. The numbers worth noting:
From this, I believe the Java 8 bump is a safe change. I see the benefits as outweighing the cost. mcstats legacySometimes mcstats will pop back up randomly. From people in the Spigot community, I was told that the Java 8 usage was similar. However, bstats is biased against Minecraft 1.8.8. Allegedly, 34,000 of ~88,000 servers on mcstats use 1.8 while ~11,000 of ~48,000 use it on bstats. |
Perfect! That's exactly the kind of data I was hoping to see! Moving to Java 8 is definitely safe - if any of those 0.7% servers still on Java 7 run MobArena, chances are they're running older versions of it anyway. This is excellent news. As for the vast amount of servers still running older versions of Minecraft, it was expected. When I bumped to 1.10, I had a lot of private messages, github issues, DBO issues, and forum post mentions asking for 1.8 support. Back then, I initially took the stance that due to the breaking changes in the API, there was no feasible way to keep developing for both versions. There still isn't. Maintainability has plummeted after the introduction of the mc18 branch, and every release is tedious and error prone. A couple of different approaches have been suggested, but they are all brittle and suboptimal. The multi-branch approach makes version control tedious and error prone, but at least any given branch (version) is coherent and self-contained. I honestly don't know what to do about it. |
Awesome. 😄 I think this PR is safe to merge then.
Hmm. I propose us focusing on only supporting the latest version of Minecraft and abandoning 1.8 support officially. MobArena is a small project with a limited number of developers and a limited number of time. Based on the data we have now, there are still more servers running 1.12 than 1.8. 1.8 was released on Sept. 2, 2014, making it over three years old as of today. I think people have had long enough to migrate their servers to a newer version. In the Minecraft world, three years is a long time, and I think it's best to treat the 1.8 scenario as the equivalent to Java 7. I see 1.8 as end-of-life and it's not forward-facing either for ourselves or our users to spend a lot of time on it. tl;dr: I don't see it as sustainable to support both 1.8 and <current version> at the same time. We need to pick one and focus on it. Splitting our time between two versions is draining and I fear will lead to burn-out. |
It's safe to update to Java 8, but this PR also bumps the API version, which is not necessary. It does look like 1.13 is going to bring breaking changes, so it may be necessary to bump to 1.13 if MobArena is affected by them. There is no need to bump to 1.12. Now, in terms of abandoning support for older versions, I think there is a very crucial piece to the puzzle that's currently missing in the discussion. People are not sitting on 1.8 and 1.9 because they are lazy or don't know how to update. It is a very deliberate choice, because the game has changed significantly from 1.8 to 1.11. In fact, I don't doubt for a second that people would love to update to the most recent version, if there was a guaranteed way to restore the combat system (and what other "old time functionality" people miss) to that of the 1.8 era. Abandoning 0.7 % of servers due to Java versions is very different from abandoning 20-40 % of servers due to Minecraft versions. It's also worth noting that Mojang actually encourage version jumping via the launcher, while Oracle don't. Maintainability comes down to whether or not the right abstractions can be created. For instance, the idea of "equipping" a player can be easily isolated into a tiny, cohesive module that can differ between branches. Merge conflicts are easily handled by discarding the upstream changes to these modules in the legacy branches, and the rest of the functionality can propagate down without issues. And if the modules are highly cohesive and sufficiently sized, it should be no problem to automate the resolution of these kinds of merge conflicts. An abstraction with reflection for old versions and regular Java for the most recent is another possibility (because the obfuscation in older versions is cemented), but it is much more brittle and a lot less obvious how or why it exists. I know that some of the most active voices in the community are still running older versions. At the very least, I want to explore the possibilities of multi-version support before gutting the idea. It would be a shame to exclude them - especially if the amount of work required to avoid doing so turns out to be a set-and-forget automation deal. |
I'm likely going to take a Java 8 bump and code style sweep in April. |
Forget the code style sweep - since a lot of the ugly spots in the code are due for refactoring or rewriting anyway, there's no point in modernizing them. |
Summary
Problem
Java 7 is EOL and no longer distributed, see #404 for details
Solution
Fixes by building against Java 8, Bukkit/Spigot 1.12
Action
I tested this with
mvn clean package
on my system and it worked. The Travis CI system should also pick this up too.So it should be a simple review + merge, if the change makes sense.