Fix GameTest startup crashes from classpath and environment edge cases#664
Fix GameTest startup crashes from classpath and environment edge cases#664IcyNova4 wants to merge 1 commit intoKiltMC:version/1.20.1from
Conversation
- parse built-in mod URLs using URI paths on Windows - guard shared scan data writes in concurrent class scanning - skip environment-mismatched client/server class loading during auto registration - harden remapper classpath library resolution for jar/file providers - add defensive Fabric API redirects for missing jar filesystem providers - ensure Loom runs include jdk.zipfs module
|
I don't know if your code is AI generated or not, but this PR message strongly feels like the message is AI generated. If it is and your code isn't, please avoid that next time. I hate trying to read AI generated PR summaries, it's too "professional" and takes up way too much time for me to sit down and understand. You can leave short messages or just give a quick explanation of what and why, you don't have to AI generate a whole bloody essay. We're all volunteers here developing a Minecraft mod, not a system-critical piece of software. As for the code itself, I don't like it. You're not even supposed to use J25 for developing the mod on this version, you're supposed to be using J17. I hate the environment fixes here, that would "fix" a completely different scenario with gametests that quite frankly does not work with Forge. Forge 1.20.1 doesn't even properly use gametests either, so this part of the code is basically pointless. NeoForge 1.21.1+ does though.
Some of the concerns outlined here are completely valid, but this PR just feels so, so weird in nature. |
|
Apologies if the PR summary was too coherent; I didn't realize literacy was a red flag in this repository. Next time I'll make sure to misspell "functionality" and include more emojis so it feels less "professional" and more like the volunteer effort you're comfortable with. To address the actual code, since we’re apparently ignoring the fixes to nitpick the font choice: "Pointless" GameTest Fixes: Your argument is essentially, "It's already broken on Forge, so stop trying to fix it." That is a fascinating approach to software development. I fixed hard crashes. If you prefer the crashes because they are "authentic" to the Forge experience, feel free to discard the fix. Fabric API / "Weird" Mixins: You call defensive coding "weird." I call it "not letting the server implode because a provider is missing." If the choice is between a "weird" mixin or a crash to desktop, I usually pick the one that lets the user play the game. I fixed Windows path parsing bugs, race conditions in scan data, and runtime crashes. You're worried about the vibe of the PR description. Take the fixes or keep the bugs. It’s all the same to me. |
|
I didn't close this PR immediately because the concerns are valid and that the code didn't feel generated, I mainly took issue with the fact that the way things are being handled are just wrong, and that with the current plague of AI-generated PRs on GitHub repositories, I didn't want to have to deal with the possibility that yet another AI-generated PR came in, especially because of how the message formatting is done it's far too common for AI-generated PRs to follow this formatting. Other people are not nearly as lenient with this, usually it's immediately considered as AI slop and would likely be closed immediately on other mod repositories. Unfortunately due to your account age, that significantly boosted the suspicion, so if I'm wrong then that is on me. My argument with gametests is moreso that even if it is fixed, we can't even really do anything with them. Forge doesn't use them properly in its codebase, we don't have a proper output to compare with to even truly see "Oh hey, this is fixed" or "this is broken" in a lot of places, and because we never encounter this code in production environments, what are we even supposed to do with it? Additionally, your solution here is to read the environment exception and skip loading the class, when Forge doesn't even do that themselves the last time I checked. What needs to be done here is proper environment stripping, not skipping the class outright. The reason why I call the file system code weird is because this PR doesn't solve the core problem with file systems - it works around it. Skipping missing file systems shouldn't be on us to deal with, if an issue occurs then it's either something is wrong in Fabric Loader or there's something wrong with our paths specifically, so the root problem itself needs to be tackled in how the mods themselves are loaded or how they're being introduced into the loader, not by mixing into Fabric API and adding a try-catch to things that are none of our business to deal with. If there's an issue occurring here in Fabric API, it will occur with other mods calling the same method. The root problem itself needs to be fixed. Also, with J17 vs J25, this codebase shouldn't even be loaded with J25 or hell even J21, and the reason why I pointed this out specifically is because of adding modules to the run configs (this is a very popular issue on J25 iirc), which also isn't something we should do either, because then if it works for us but fails in someone else's env, then we're screwed on that end. Once again, this PR has valid concerns to address, and the concept itself is good, but the execution is quite flawed. |
Summary
I tracked down a set of GameTest startup failures and fixed them from highest impact to lowest. The main goal was to stop hard crashes during early startup while keeping behavior unchanged for valid classpath/mod setups.
Errors and fixes (highest impact first)
Provider "jar" not foundcrash during resource/language loadingServerLanguageUtil.getModLanguageFilesandModNioResourcePack.create) callModContainer.getRootPaths(), and some entries were backed by providers that were not available at runtime.getRootPaths()and return an empty list whenProviderNotFoundExceptionis thrown.src/main/java/xyz/bluspring/kilt/mixin/compat/fabric_api/ServerLanguageUtilMixin.javasrc/main/java/xyz/bluspring/kilt/mixin/compat/fabric_api/ModNioResourcePackMixin.javasrc/main/resources/Kilt.mixins.json.Environment mismatch classloading crash (
Cannot load class ... in environment type SERVER/CLIENT)ModLoadingContext.kiltActiveModIdon error paths to avoid stale context.src/main/kotlin/xyz/bluspring/kilt/loader/KiltLoader.ktRemapper classpath robustness issues with non-default file system providers
addLibrarySafe(path)to validate regular archive inputs, catch provider failures, normalizejar:/file:URIs to concrete paths, and then add libraries safely.src/main/kotlin/xyz/bluspring/kilt/loader/remap/KiltRemapper.ktWindows built-in mod path parsing bug
file:/...URLs.url.toURI().toPath()).src/main/kotlin/xyz/bluspring/kilt/loader/KiltLoader.ktConcurrency risk in Forge scan data aggregation
ModFileScanDatawrites occurred in concurrent scan flow.src/main/kotlin/xyz/bluspring/kilt/loader/KiltLoader.ktEnsure zipfs module availability in Loom run configs
--add-modules=jdk.zipfsto Loom run configurations.build.gradle.ktsValidation
runGameTestpasses after these changes (using Java 24 in this environment).IllegalArgumentException: 25.0.1; this is tooling/runtime related and separate from the code fixes in this PR.