Skip to content
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

Use LIBRARY instead of GAMELIBRARY as default JarJar mod file type #9939

Open
wants to merge 1 commit into
base: 1.20.x
Choose a base branch
from

Conversation

thedarkcolour
Copy link
Contributor

Fixes #8878

Currently, when a library is included in a mod via JarJar, its FMLModType is assumed to be GAMELIBRARY if none is specified in the library's manifest file. This can be problematic for Lanuage Providers, like Kotlin for Forge. Its code is written in Kotlin, which makes calls to certain classes in the Kotlin standard library, which KFF must include because it provides the Kotlin language. It currently shades the Kotlin libraries, which causes incompatibilities with any other mod that tries to shade or JarJar Kotlin. Not good.

Since Forge made the jump to JPMS back in Minecraft 1.17, including the Kotlin libraries with JarJar was impossible. This is because JarJar loads the Kotlin libraries as GAMELIBRARY files, which are loaded in the GAME module layer, while LANGPROVIDER and LIBRARY files are loaded in the PLUGIN layer, which is a parent of the GAME layer. Due to the way JPMS works, classes in the GAME layer can access classes from the PLUGIN layer, but not vice versa. Since Kotlin's language provider component is in the PLUGIN layer, it throws an IllegalAccessError whenever it tries to reference the Kotlin libs from JarJar.

Solving this issue is pretty straightforward; change the default FMLModType of a JarJar file from GAMELIBRARY to LIBRARY. There is no reason to use GAMELIBRARY by default when LIBRARY is still accessible from the GAME layer, just like GAMELIBRARY files are. In nearly every case of a library not specifying its FMLModType, it is because that library is a non-Minecraft library that does not need access to Minecraft classes.

It was brought up in the Discord that this change is bad because "it doesn[']t support game libraries." If you would like to include a GAMELIBRARY, then that file should specify its FMLModType explicitly in its manifest. It is unreasonable to expect Jetbrains to specify FMLModType for the Kotlin libraries; it is, however, reasonable to expect a mod author to specify FMLModType.

Another suggestion that was mentioned in the Discord was to "allow the metadata to specify what layer the library needs to go into." This was my first idea for solving the problem, too. However, this solution would cause conflicts when two mods JarJar the same library but specify that it should go into a different plugin layer. This could potentially be resolved by choosing the earliest ancestor layer, for example, in the case of two mods specifying a file goes into GAME and PLUGIN layers, then the PLUGIN layer would be chosen. If I misinterpreted this comment and the suggestion was to allow libraries to define which layer they're loaded into, then that is functionally equivalent to them specifying FMLModType.

I think this is the simplest and best solution that doesn't require much maintenance effort. The only breaking change would be for mods that actually use GAMELIBRARIES (which I have never seen deliberately used), in which case those mods should update their manifests.

@CLAassistant
Copy link

CLAassistant commented Apr 28, 2024

CLA assistant check
All committers have signed the CLA.

@autoforge autoforge bot added Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. 1.20 labels Apr 28, 2024
@autoforge autoforge bot requested a review from a team April 28, 2024 22:44
@autoforge autoforge bot added the Bug This request reports or fixes a new or existing bug. label Apr 28, 2024
@LexManos
Copy link
Member

Yes, if two mods JiJ a lib and specify a different layer, the 'earliest' would win. And we'd have to deal with the class not found crashes if one of them says the wrong thing just as if this was changed to the default.

My intention from the very beginning before JiJ was even a thing, was for Mod jars to define metadata for the jars they contain, if they do not define it themselves. It was as simple as meta = subJar.hasModMetadata() ? subJar.modMetdata() : mainJar.getFile(subJar.path + ".manifest") but it got complicated when others got involved.

Anyways, this is a hack fix, that can cause a lot of problems for any libraries that do not specify they need to be on the GAME layer when they should. Which happens because modders love messing things up, especially in this system which nobody truly understands.

To mitigate that, is there any possible way to, instead of having a hardcoded default, make the default propagate from the containing jar file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20 Bug This request reports or fixes a new or existing bug. Triage This request requires the active attention of the Triage Team. Requires labelling or reviews.
Projects
None yet
3 participants