MilkBowl / Vault

Vault of common APIs for Bukkit Plugins
GNU Lesser General Public License v3.0
494 stars 351 forks source link

Major Update and Cleanup #888

Closed Jack1424 closed 2 years ago

Jack1424 commented 2 years ago

Since this plugin hasn't been updated in almost a year, I figured that I would go through it really quick and fix some things. Along with a lot of format and style cleanup, I also changed to Java 18 and Paper, instead of Bukkit. The plugin loads on a Paper 1.19 server and still loads on a 1.8.8 Bukkit server (if you still want to support those), but I'm not sure how to test it. Let me know if you want me to change or undo anything.

I also fixed a few typos in the README and changed all of the links to HTTPS instead of HTTP (they still work).

Jack1424 commented 2 years ago

The build failed... I guess I'll open an issue for that

Geolykt commented 2 years ago

Do note that it appears that your commits are unsigned even though the vigilant mode is active on your account. I'd verify your git settings there

Jack1424 commented 2 years ago

I have to go into my IDE settings and tell it to sign my commits every once in a while. I just forgot to do it this time. Thanks for letting me know, though.

EDIT: I fixed it and my newest commit is verified

Jack1424 commented 2 years ago

There are a lot of plugins that Vault still supports that haven't been updated for almost/over 10 years. It would be great if we could clean those out. I'm not sure what plugins people would still want Vault to support though.

I also found a way to fix the huge mess of Maven warnings that come in about referencing a file in the project directory by using the lib folder as a local repository as seen here, however, it would be annoying to implement it for every single one of the plugins (another reason to get rid of some of them).

I'm aware that the Travis build is failing, but I can build it fine on my PC with Maven. Someone should really fix that.

Geolykt commented 2 years ago

Java 18 has very little adoption and minecraft 1.8 - 1.17 will probably not work with it by default. That is a counterproductive change. Requiring mc 1.19+ has the same energy there.

Furthermore there is little use in using lombok now unless there is a large refractor somewhere hidden (you are changing 10k+ lines of code, this will never get approved either way).

The LATEST release tag is also deprecated in maven (and as far as I know maven will warn you when you attempt to use it) - for good reason.

And well why use paper if there are no paper-exclusive features being used?

Jack1424 commented 2 years ago

The plugin loads without any errors on a new Paper 1.18 server and an old Bukkit 1.8.8 server. If you know of a good way to test these changes (although, they shouldn't break anything), let me know. Vault shouldn't need to still support any versions older than 1.8.9.

I'll remove lombok then. All of these changes are small things that improve readability and run a bit faster. I haven't made any significant changes to any single file.

Maven didn't warn me about using LATEST, but I'll change it to 1.19-R0.1-SNAPSHOT.

Paper is new and extremely active. It works on Bukkit and Spigot and I don't see any reason why not to use it.

Jack1424 commented 2 years ago

I just noticed that there are two PRs open to remove old plugins (#863 and #866). They have been open for a few months with no response from MilkBowl developers. It doesn't look like they are even active on GitHub anymore, which is weird since the plugin is running on over 55,000 servers according to bStats.

Geolykt commented 2 years ago

The plugin loads without any errors on a new Paper 1.18 server

That is impossible - the api-version of 1.19 guarantees a hard-crash on any server implementing any bukkit version in the range of [1.13,1.19)

The plugin loads without any errors on a [...] old Bukkit 1.8.8 server

Let's be real, who has a Bukkit 1.8.8 server running Java 18 of all JDKs? Netty's native transport does not work on J9+, and the fact that native transport can be disabled in not common knowledge. Java 18 is only used across 2.7% of servers according to bstats GLOBALLY, the share will be much much lower for 1.8.8 which is known to be partly still stuck in Java 7 world

Paper is new and extremely active. It works on Bukkit and Spigot and I don't see any reason why not to use it

Paper is a superset of spigot (which in itself is a superset of bukkit). For server devs this is actually neat, but for plugin devs it can be is dangerous. Any atttempt of using a paper-only method will result in a runtime crash that is hard to catch during compile time without specialised infrastructure in place. Furthermore paper provides nothing that vault can use realistically outside of native adventure support (but then - do the few commands that are used need adventure?).

It doesn't look like they are even active on GitHub anymore, which is weird since the plugin is running on over 55,000 servers according to bStats.

It basically rotates around the question of what would Vault need and is it worth it to change. Vault is an API and needs to honour ABI more than any other plugin just because of it's size. See https://xkcd.com/1172/


This PR has the same energy as using delombok on a project which uses lombok and pushing it's results and calling the change "removed massive security vulnerability".

Readability is subjective and the just-in-time-compiler is a thing and is actually incredibly good at it's job.

cerealcable commented 2 years ago

I just noticed that there are two PRs open to remove old plugins (#863 and #866). They have been open for a few months with no response from MilkBowl developers. It doesn't look like they are even active on GitHub anymore, which is weird since the plugin is running on over 55,000 servers according to bStats.

My focus on Vault and MilkBowl has come and gone over the years, but lets be pretty honest here, the stability of Vaults API's has been extremely consistent. Those two PR's won't make a material difference in Vault for anyone other than those looking at the repo, so yeah, sure I could go review those and merge them but I've got priorities in life too beyond Minecraft so it's not the top of the list. Additionally neither of those PR's passed their builds so they can't be merged either.

Let's focus this discussion on things that will make material differences to Vault and not about how often I'm committing or merging PR's on a free open source project. The fact that so many servers still run it is a statement of how little work is needed, it's far from a failed project but more a mature one that isn't needing a significant amount of attention, entirely by design (at least it is now).

Given that this PR is quite large, I would recommend breaking it up into smaller more focused changes instead so it's easier to have these conversations in a more narrow and focused way instead of an all or nothing approach. In my experience, the smaller the changes are the more likely for them to be reviewed, approved and merged.

Jack1424 commented 2 years ago

Thank you for the response. I obviously don't think that you need to focus too much on this project or even actively develop it, but someone needs to come here at least every one or two months to check the PRs and Issues. For example, it appears that all of the PR builds are failing. This PR was originally just a README change, and the build failed, even though I only changed the README. I have opened an issue about this at #889.

I will close this PR and come back with my original small README changes.

rbluer commented 2 years ago

Since this project has not had any development since 2020-07-17, then maybe this project should be considered abandoned. I think it will best serve the whole community to create a new project and move on with developers who are interested in maintaining Vault. Failure to provide an actively maintained plugin will result in fringe plugins surfacing trying to provide similar behaviors.

I'd be happy to help support an active plugin of this nature.

Jack1424 commented 2 years ago

That's what I was thinking. It seems like the developers don't really have enough time to actively maintain or even monitor this repository anymore (which is completely fine and understandable). I think someone should definitely fork it and make a new, maintained plugin. I don't know if I would be the best person to do it, but someone should.

Geolykt commented 2 years ago

I can only talk about the economy side of vault since that is the only one I am familiar with, but in my opinion it is nonsense to fork vault on that side.

Any features that you would implement (e.g. multi-currency support) would need to be optional, otherwise you are going to break dozens - if not hundreds of economy plugins. Furthermore there are a few outdated or not well thought out concepts (such as bank ownership and members) within Vault.

There have already been a few attempts at building up a Vault 2.0 so to speak - but no API is able to move past being in a niche

Jack1424 commented 2 years ago

I'm not saying Vault needs to make any major changes. I'm saying that it needs to be put in the hands of someone who can actively monitor (check this repo and reply to issues and PRs at least once every few months) and maintain it.

I'm not saying that the MilkBowl devs are lazy or anything, they just don't seem to have any time to work on the plugin anymore. To their credit, they have built an amazing plugin that is very low maintenance, but it still needs more maintenance then it's getting right now.

For example, PR builds have been broken for months now. Every PR fails the build, even if someone only changes the README (my PR). This is something that should have been fixed rather quickly.

Geolykt commented 2 years ago

What kind of maintenance would Vault need that isn't an ABI break? PR builds have failed for about a decade now, it is hard to use that as an example of a lack of maintenance.

Jack1424 commented 2 years ago

It is low maintenance, but some good PRs and issues (like #889) should be addressed. However, it doesn't seem like the devs have time anymore to even look at them.

Also, the fact that PR builds, a considerably important feature that shouldn't be extremely difficult to fix, have been broken for a decade now is an example of a lack of maintenance.

rbluer commented 2 years ago

I took over another plugin a few years back which has been my primary focus. So I'm not 100% sure how much time I'd have to dedicate to a new project. I'm willing to contribute, and I have extensive enterprise-level Java development experience, so I can leverage my experience to get up to speed on Vault. I'd really like to see full support for 1.19, which may be mostly testing.

I personally agree that the API is well thought out, especially for the economy interfaces. I too would love to see multiple currency support added, but it should not impact the current APIs. I've had a need to support multiple-currencies for a while and without a common vault API for it, I think it's been an under used feature.

I think what cerealcable had to say was very hopeful and they had a good point too. Is there anything I can help with to try to get some good PR builds? Cerealcable if you're willing to address PRs, then maybe getting clean and smaller PRs would be a manage task on our end of things.

Perhaps the conversation on these topics could be better suited in another thread, or threads? There are a few different topics that can be explored, but at the same time I would not want to exclude current MilkBowl developers from providing their feedback. Perhaps open a new issue or two?

cerealcable commented 2 years ago

@rbluer I get PR and issue emails and I'd expect other maintainers are also getting notified about discussions here. I'm happy to help facilitate PR's and merging, I also agree that smaller PR's usually are much easier to handle for everyone mostly as a means of getting the changes approved but also helpful in keeping changes smaller so we don't release bad quality code.

I've made a PR for fixing the builds, it'll get merged soon enough I imagine.

rbluer commented 2 years ago

That sounds awesome! Thanks for your time and help!