-
-
Notifications
You must be signed in to change notification settings - Fork 165
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
adde Folia support #3617
adde Folia support #3617
Conversation
This is an interesting surprise, thanks! Looks like there are ~100-200 bukkit based servers running Java version <17 which this PR drops support for. If there is some way to retain support for them I'd like to consider it - one option could be placing the folia code in new gradle module and using reflection to create the FoliaPlatformLayer instance in the bukkit module - I'm not 100% sure if gradle allows that sort of thing I'll review these two PRs when I have the time, thanks! |
That's a good idea. I will try this method later! |
After consulting the relevant information and making some attempts... Yes, maybe it could be compatible both 11 and 17 with different libs, but it's too complicateed to done. In contrast, the effort made is much higher than the harvest, and the cost performance is too low. I choose to give up on this attemption, it's more than I can handle. :( If Plan insist on looking back to under java17 servers, mybe this merge should be postponed, until the number of servers using older versions of Java is negligible. |
Thanks for trying it out! Could you tell me what you attempted so that I can avoid repeating those attempts? :) |
Sure, I'll get back to you when I'm tidying it up。 |
Howerver, here is the problem I encountered......
:( |
Alright, I think trying this option in the folia module might do something, not sure https://docs.gradle.org/current/kotlin-dsl/gradle/org.gradle.api.plugins/-java-plugin-convention/disable-auto-target-jvm.html It might also be possible to try include the folia module in the plugin module instead of bukkit module (This whole comment assumes a new folia module is added to Plan project) |
wow, thanks for this information. it works now. I tested it on paper 1.16.5(java11) and folia 1.20.4(java21), both of them can loaded Plan proply. |
now this can be compiled by CI-CD(workflow)
I did some test about CI-CD, it failed on 'aggregateJavadocs' step.
|
Javadoc building fails, folia needs it's own gradle module, with pal-folia as dependency, and then that module would be shadowed in :plugin module, so that :bukkit module doesn't have Java 17 dependencies The folia module doesn't need any Java files inside it as far as I'm aware This should work at runtime since plugin module outputs the jar and bukkit module can still access the class via reflection at that point |
Do we want to move Folia's PAL into this repository as was done with Fabric, or keep it as is? |
It's fine to keep as is, just the dependency needs to be moved :) I think fabric was more involved since it included all sorts of other things on top of PAL that would make it seem disingenuous if PAL contained fabric, while it seems to work fine for Folia |
so javadoc gen may success
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.
Some little things, also have to make sure all scheduled stuff works correctly but code-wise lgtm
|
||
dependencies { | ||
shadow "net.playeranalytics:platform-abstraction-layer-folia:$palVersion" | ||
} |
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.
} | |
} | |
There still have javadoc problem, folia module can't pass 'aggregateJavadocs' step. |
This comment was marked as resolved.
This comment was marked as resolved.
Alright, Looks like the folia module is redundant (and can be removed). All the funky dependency business can be done in the plugin module dependencies {
...
shadow "net.playeranalytics:platform-abstraction-layer-folia:$palVersion"
}
java {
// This is required to prevent the shadow plugin from
// adding the folia platform abstraction layer to the runtime classpath
disableAutoTargetJvm()
} I didn't have Folia installed so I couldn't test if that starts up, but at least Plan starts with Java 11 with this setup, and aggregateJavadocs works too. |
1This will also work. But I think disabling jvm inspections directly in the plugin is too aggressive and potentially dangerous. With specificed jvm version, now we don't need to disable jvm check in folia module too. 2To pass javadocs aggregate, I changed shadow to runtimeOnly:
⬇️
So this pal won't be involved in to aggregateJavadocs progress. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
I think that's the real problem I encountered. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
You can merge master to this branch and that should sort out the issue |
…into folia-support
Great, it works now. |
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.
Great work! :) I'll merge this after testing that it works one more time.
Building folia itself seems to be an hour job so feel free to test it as well 😅
I'd be interested in error free behavior of:
- Plugin enable on Folia
- Plugin reload /plan reload
- Plugin disable
- Player join + Player leave adding a Session to the website
- Player join + Gamemode change + Nether portal + Player leave adding a Session with overworld time data in Survival and Creative, and nether data in creative
- Player join + Server shutdown + Server start adding a Session
- Server running for few minutes adding Player online count, TPS and other performance data to Performance tab
Here is the folia I built my self, if you want you can use this for test. Three of begining I have tested:
rest of those you can check on my own server. I am using it for several days. Eveything looks great fine, |
Folia is well available from official sources through PaperMC's downloads API, see for example https://api.papermc.io/v2/projects/folia/versions/1.20.4/builds/31/downloads/folia-1.20.4-31.jar Per PaperMC, the downloads are intentionally API-only since the platform is still very experimental. |
Tested:
|
Yes, this event is disabled on folia currently. |
Alright then everything seems to be working as intended :) |
Thank you for all the correspondence and quick changes, it has made it easy to get this PR to the finish line & Thanks to @AnttiMK for PR review :) |
Hopefully they will eventually sort that out, until then I'll just put a disclaimer to the change log and wiki for Folia about this data being wrong |
Your checklist for this pull request
🚨 Please review the guidelines for contributing to this repository.
/Plan/common/src/main/java/com/djrapitops/plan/delivery/rendering/html/Contributors.java
/Plan/common/src/main/java/com/djrapitops/plan/settings/locale/LangCode.java
Description
Add support for folia!
Noted: It needs to merge mu update for Platform-abstraction-layer it add an new abstract layer for folia.