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

Make jarjar use LIBRARY not GAMELIBRARY by default #96

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

lukebemish
Copy link
Contributor

@lukebemish lukebemish commented Mar 7, 2024

Changes the default FMLModType of jarJar-ed deps to be LIBRARY instead of GAMELIBRARY.

Currently, if a language provider wishes to jarJar a non-Minecraft-related library, and use it within the language provider definition itself, the fact that the defaul FMLModType, if the jarJar-ed jar doesn't specify anything, is GAMELIBRARY means that the langauge provider cannot use this. Language providers or other jars loading at that same level - such as GML, a groovy language provider whose implementation is written in groovy - can solve this by JiJing a version of the library with an FMLModType of LIBRARY added to the manifest. This is fine, until a mod JiJs that library or one of its dependencies, of a newer version, and doesn't change the manifest, in which case that version will be loaded and the language provider will fail to find the library. See GroovyMC/GroovyModLoader#23.

A few alternative potential approaches have been considerred to solve this problem:

  • Consider this to be an unresolvable conflict. This is a non-solution, as this particular conflict is resolvable - the mod which JiJs the requested library "normally", without a change to the MANIFEST, doesn't actually need it on that layer, it's just there by default
  • Allow mods to specify that they want a library to be loaded as a given type. In this scenario, a jarJar-ed jar with no explicit type declared would be loaded on GAMELIBRARY, unless some jar JiJing that module specified a different mod type. This runs into issues because other mods might inherently expect several JiJed libraries to be loaded on the same layer, without realizing - for instance, language provider A might JiJ library B, which consumes service X, and request that B load as a LIBRARY. Mod C JiJs library B as well as library D, which provides service X, and doesn't care where they load. Library B would get loaded as a LIBRARY but library D would be loaded as a GAMELIBRARY, so the service would not be discovered, even though with just mod C present it would be discovered just fine, leading to a rather unpleasant bug.
  • The solution proposed here - make libraries load as LIBRARY if they do not specify an FMLModType

This solution is comparatively safer than the other solutions, as it only restricts the classpath that JiJed libraries might load on. Furthermore, libraries that are aware of Minecraft or mod code can be expected to declare an FMLModType, while libraries which have nothing to do with modding or MC, and might be needed by language providers or the like, cannot be expected to do so, so it lines up with expectations in that regard. The only cases where this might be expected to cause issues are when a mod is ATing or mixin-ing a dependency that it JiJs, in which case they would be forced into the same situation which language providers are in now. I expect such cases to be comparatively rare, or at any rate substantially rarer than language providers or other things loading at that level wanting to use a library they bundle in their implementation. Furthermore, FML does not allow transformation of the non-mod libraries which Minecraft itself uses - such as brigadier or DFU - in this way anyways, so moving the default FMLModType to match that makes this more consistent.

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@Technici4n
Copy link
Member

As this is a breaking change we probably want to wait for 1.20.5.

Copy link
Member

@Technici4n Technici4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the PR. If the library does not use Minecraft classes, it makes sense to always load it in LIBRARY.

@Technici4n Technici4n merged commit ed93dac into neoforged:main Mar 25, 2024
3 checks passed
@lukebemish lukebemish deleted the jarjar-default-library branch March 25, 2024 19:37
rfresh2 added a commit to rfresh2/XaeroPlus that referenced this pull request Apr 25, 2024
this change: neoforged/FancyModLoader#96
causes LambdaEvents to be loaded as a LIBRARY instead of GAMELIBRARY

for some reason this prevents it from being able to load XP classes

The only way to get it to load as a GAMELIBRARY is to modify the LambdaEvents jar manifest which is not easily doable here. So instead i just shadowing it into the XP package path to work around this
@thedarkcolour
Copy link
Contributor

I just saw this. Thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants