-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add Options support to JarInJar locator #10683
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
base: 1.21.x
Are you sure you want to change the base?
Conversation
We now use a custom FileSystem that is designed to be fast and safe to use with ZipFileSystem's caching system. Added system to have Forge ship options for JarJar files. These files will only be selected when modders request them
...der/src/main/java/net/minecraftforge/fml/loading/moddiscovery/JarInJarDependencyLocator.java
Show resolved
Hide resolved
...der/src/main/java/net/minecraftforge/fml/loading/moddiscovery/JarInJarDependencyLocator.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/groovy/net/minecraftforge/forge/tasks/JarJarMetadataOptions.java
Show resolved
Hide resolved
...der/src/main/java/net/minecraftforge/fml/loading/moddiscovery/JarInJarDependencyLocator.java
Show resolved
Hide resolved
...der/src/main/java/net/minecraftforge/fml/loading/moddiscovery/JarInJarDependencyLocator.java
Outdated
Show resolved
Hide resolved
...der/src/main/java/net/minecraftforge/fml/loading/moddiscovery/JarInJarDependencyLocator.java
Outdated
Show resolved
Hide resolved
...der/src/main/java/net/minecraftforge/fml/loading/moddiscovery/JarInJarDependencyLocator.java
Show resolved
Hide resolved
...der/src/main/java/net/minecraftforge/fml/loading/moddiscovery/JarInJarDependencyLocator.java
Outdated
Show resolved
Hide resolved
...der/src/main/java/net/minecraftforge/fml/loading/moddiscovery/JarInJarDependencyLocator.java
Outdated
Show resolved
Hide resolved
...der/src/main/java/net/minecraftforge/fml/loading/moddiscovery/JarInJarDependencyLocator.java
Show resolved
Hide resolved
...der/src/main/java/net/minecraftforge/fml/loading/moddiscovery/JarInJarDependencyLocator.java
Outdated
Show resolved
Hide resolved
...der/src/main/java/net/minecraftforge/fml/loading/moddiscovery/JarInJarDependencyLocator.java
Outdated
Show resolved
Hide resolved
|
Alright, just to summarize some discussions from discord. This will be done to isolate features and make potential backporting easier. Speaking of backporting. There is no immediate plans to pull this into 1.20.1, However if Llama wanted to attempt it and submit a Pr then he is more then welcome. If not it'd be on the list of things to back port after the new toolchain is done. Along with things like executable jars, and performance fixes. So to be clear, it IS an eventual goal to backport a lot of things to older version when possible. As for the gradle side. It will always be an opt-in system for enabling Mixin or MixinExtras. This is a hard requirement that I am applying. We are trying to get away from the 'fg is magic' because then the side effects can get out of control fairly easily. However it should be as simple as a one line option in your gradle. The specific is to be determined but something like It has also been discussed to 'auto-enable' mixinextras if we detect that a mod has mixins at all. I'll leave the specifics of that magic to Paint. This PR is specifically designed to allow for Forge to package options. Not specifically any option. Hence the split I said in the first paragraph. |
|
The opt-in hard requirement should at least be some sort of declaration of your Mixins config JSON. ForgeGradle Magic can take care of additional behavior from there. This magic can be refined in future updates, but for now I think just checking for that will suffice. |
That might actually be an option. Tho it would require reading the config as well as the manifest. Again I'll leave this up to Paint for the runtime magic. Either way it'll be simple and we'll have examples. |
|
Please prefix the PR with [1.21.10]. Because I don't have push access, I cannot modify PR details. We can discuss GitHub permissions at a later date. I've already alluded to the GitHub permissions write-up I've been working on. It's not top priority, but know that I'm aware of it and am working on it. |
|
From @PaintNinja:
https://discord.com/channels/1129059589325852724/1129095235889270844/1431671142053183660 |
This is determined by the `MixinConfigs` manifest attribute and the `-mixin.config` CLI arg (the latter for mod dev env).
This is a re-write of the JarInJar locator to both be more efficient by leveraging roimfs.
As well as adding the ability for us to ship 'options' for JarInJar dependencies.
Yall know the history of this, MixinExtras and people being to vain/lazy to JiJ it themselves.
See my big wall of a comment in JarInJarDependencyLocator for some explinations of how it works and why I decided to do things the way I did.
Requires roimfs, which i'll publish after adding benchmarks.
Doesn't strictly need MinecraftForge/SecureModules@c5ead60 but that fixes some issues/makes it not wrap in UnionFileSytem.
Requires MinecraftForge/JarJar#11 which will be pulled once this is all finalized.
Compatibility wise there shouldn't be any concerns. As all code changes are private inside the locator, and it supports the old JIJ metadata file format.
There are behavior changes tho, namely the use of roimfs instead of JarIJar's FileSystem.
But the FileSystem is still shipped, so people can still use it. And Mod were designed to be on any FileSystem. So it shouldn't be an issue. Tho in a future breaking window, I suggest we stop shipping JIJFS and let that project die.
Thanks @Jonathing for the gradle work to build the json file.