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

[Question] How to load extra mod with mod #682

Open
SettingDust opened this issue Jun 16, 2022 · 24 comments
Open

[Question] How to load extra mod with mod #682

SettingDust opened this issue Jun 16, 2022 · 24 comments

Comments

@SettingDust
Copy link

SettingDust commented Jun 16, 2022

Here's what I tried:

  1. Running as a language adapter. Implement ModCandidateFinder and inject the mods to FabricLoaderImpl#mods. Working in 1.16, 1.17.
    • Throw ClassNotFoundException with FabricLoader class loader. My classes not in classpath.
    • Throw SecurityException after I add my classes to classpath.
  2. Set system properties for using ArgumentModCandidateFinder. But it's called before language adapter.
@SettingDust SettingDust changed the title [Question] How to add extra mod with mod [Question] How to load extra mod with mod Jun 16, 2022
@warjort
Copy link

warjort commented Jun 16, 2022

Using *Impl classes isn't supported. Unless you are using classes from the api package, your code could break at anytime.

What you are trying to do doesn't seem possible without writing your own subclass of the fabric loader?
The issue is the loading is done in 3 stages:

  • Determining which mods are on the classpath, in the mods folder or in the system property
  • Determining which of those candidate mods actually can or should be loaded, e.g. client only mod - including potentially remapping class names if it is in the wrong format, mojmap vs intermediate or yarn
  • Actually loading/running the mod initialisation code, including any custom language adapters.

So what you describe is in the wrong order. Language adaptors in step 3 can't affect step 1
The initial determination of candidate mods described in step 1 above is hardwired here:

ModDiscoverer discoverer = new ModDiscoverer(versionOverrides, depOverrides);
discoverer.addCandidateFinder(new ClasspathModCandidateFinder());
discoverer.addCandidateFinder(new DirectoryModCandidateFinder(gameDir.resolve("mods"), remapRegularMods));
discoverer.addCandidateFinder(new ArgumentModCandidateFinder(remapRegularMods));

Is there any reason why you can't "jar in jar" your additional mod?

@SettingDust
Copy link
Author

Using *Impl classes isn't supported. Unless you are using classes from the api package, your code could break at anytime.

I can create the impl with the same package name for access package private interface

So what you describe is in the wrong order. Language adaptors in step 3 can't affect step 1 The initial determination of candidate mods described in step 1 above is hardwired here:

The first is use reflection for inject the mods to list.

Is there any reason why you can't "jar in jar" your additional mod?

It's load basing on user config

@warjort
Copy link

warjort commented Jun 17, 2022

It's load basing on user config

Normally the "user config" is install the optional mod.

Your mod code isn't in a "valid state" while the mod loader is determining what mods exist.
So running mod code is unsafe at this point.

The reason is because until it has determined what the mods are, it doesn't know whether the code is in the correct mapping or what mixins should be applied, etc.

In other words, it can't safely load/run mod code until it knows what the mods are so it can read their mod configurations and determine how their classes (and dependent classes) are defined/modified.

@warjort
Copy link

warjort commented Jun 17, 2022

On the package private/reflection.

It doesn't matter how you reference it. The point is referencing implementation details instead of stable apis is brittle.

@SettingDust
Copy link
Author

https://modrinth.com/mod/thatorthis works well in 1.16, 1.17. Need the approach for 1.18.

@warjort
Copy link

warjort commented Jun 17, 2022

https://modrinth.com/mod/thatorthis

That page is littered with the word "hack".
It is not really surprising if your approach is brittle to changes.

Need the approach for 1.18

Your approach should be to propose an api that helps with what you want to do.
And then convince the fabric developers that it is a good idea worth supporting.
Since this would be in the loader it would work for all minecraft versions.

A simple example would be add a system property that defines a callback (a ModCandidateFinder) that can arbitrarily produce mod candidates. Defining what such a callback can and should not do in terms of classloading would be the hard part, including how the class itself gets loaded, see e.g. #680

But such a simple approach maybe too flexible (lacking in error checking and potentially introducing security concerns?).

@SettingDust
Copy link
Author

SettingDust commented Jun 17, 2022

According to #486. The earlier entry point is requested in 2020. And there isn't a implement for that. So, I think I have to do some "hack" for achieving my purpose.

Edit:
I think #680 may useful for reuse of ArgumentModCandidateFinder. I'll try. Thx.

Edit:
https://github.com/FabricMC/fabric-loader/blob/master/src/main/java/net/fabricmc/loader/impl/launch/knot/Knot.java#L130-L149= shows it's earlier than fabric loader

@sfPlayer1
Copy link
Contributor

PR 680 is not meant for this use case, I don't think it'll help you with it. There are other ways like -Dfabric.addMods to go beyond the mods directory.

It is still quite unclear what your application is, the programmatic APIs for working with the mod set and other early loader interactions are WIP. All that's available right now are the system properties and hacky unsupported approaches.

@SettingDust
Copy link
Author

SettingDust commented Jun 17, 2022

It is still quite unclear what your application is

Load mods in sub dir of mods by user config file. Need access the candidate finder.

@sfPlayer1
Copy link
Contributor

The currently officially supported way for this is limited to -Dfabric.addMods=mods/<subdir> (or -Dfabric.addMods=mods to include all subdirs)

@SettingDust
Copy link
Author

The currently officially supported way for this is limited to -Dfabric.addMods=mods/<subdir> (or -Dfabric.addMods=mods to include all subdirs)

Set system properties for using ArgumentModCandidateFinder. But it's called before language adapter.

As the second say, there isn't a way to modify the property before ArgumentModCandidateFinder been constructed.
Can 680 load addonitional lib such as my mod?

@warjort
Copy link

warjort commented Jun 17, 2022

Can 680 load addonitional lib such as my mod?

It is not for mod loading, quite the opposite. See #679 for an explanatio of how the classloading works.
It is intended for "wrapper loaders" that do some work before invoking the fabric loader.

In your case, your wrapper might calculate the fabric.addMods system property before passing control to knot.
But to make this work, you need to configure the launcher to invoke your wrapper instead of fabric directly.

I only mentioned 680 because what is called "loader plugin" in the other discussions would probably need to use this method to load their classes.

@sfPlayer1
Copy link
Contributor

No, 680 is really only useful for code that runs before Fabric Loader, e.g. in-process launchers or wrappers. It doesn't pull in anything extra, but prevents the referenced libraries to be treated as part of the game, leaving them on the system class loader.

You'll have to wait for us to add the appropriate APIs to have a clean solution.

As far as fixing your hacky solution is concerned, running with -Dfabric.debug.logClassLoad should give more insight, you unfortunately didn't provide the full error. Especially if it only breaks in-dev you may be missing a class path group definition (fabric.classPathGroups system property, loom has a dsl for it). If you access a game library such as GSON too early that is now prohibited by by the class loader. Hard to tell more without knowing which class can't be loaded any why.

@SettingDust
Copy link
Author

SettingDust commented Jun 17, 2022

It fine in dev. And below is errors desc.

  1. Load with FabricLoader classpath https://pastes.dev/zavSxFNoce
    Class<?> modInjectorClass = cl.loadClass("io.github.ezforever.thatorthis.internal.ModInjector")
    ClassNotFoundException: io.github.ezforever.thatorthis.internal.ModInjector. My mod classes not in the classpath of fabric loader

  2. Load by FabricLauncherBase.getClass
    Class<?> modInjectorClass = FabricLauncherBase.getClass("io.github.ezforever.thatorthis.internal.ModInjector");
    IllegalAccessError: class net.fabricmc.loader.impl.discovery.ThatOrThisLoadedModCandidateFinder cannot access its superinterface net.fabricmc.loader.impl.discovery.ModCandidateFinder (net.fabricmc.loader.impl.discovery.ThatOrThisLoadedModCandidateFinder is in unnamed module of loader net.fabricmc.loader.impl.launch.knot.KnotClassLoader @482f8f11; net.fabricmc.loader.impl.discovery.ModCandidateFinder is in unnamed module of loader 'app')
    The classpath of Knot not include the ModCandidateFinder.

  3. Load by FabricLoader classpath but inject my classes to classpath. It's complex to reproduce, so the exception will write by my memorize.
    SecurityException says my class has wrong signature can't access the package of ModCandidateFinder

@sfPlayer1
Copy link
Contributor

1 obviously doesn't work because you need to get your classes loaded by knot cl

2 is where you can't use your own ModCandidateFinder because it's package private and faking the package doesn't appear to work across class loaders. You'd have to use one of the existing implementations

3 is probably the same as 2 while also hacking into the jvm

@warjort
Copy link

warjort commented Jun 17, 2022

  1. The error says you still trying to use language adapter which can't ever work?
  2. This says you are trying to load your finder subclass from the mod classloader rather than the loader classloader.
    ThatOrThisLoadedCandidateFinder <- KnotClassLoader
    ModCandidateFinder <- classpath
    You cannot access package private across different classloaders they are not the same package even if they have the same name
    ModCandidateFinder is also not currently supported fabric loader api
  3. Probably similar 2? But you don't explain or show what is happening very well

NOTE: the 680 behaviour to add additional "system" classes is in fabric loader 0.14.8 but you are using 0.14.7

This issue is becoming less of a proper fabric loader issue and more of a "how do hack/structure my code to fool the fabric loader"?

@SettingDust
Copy link
Author

But you don't explain or show what is happening very well

https://stackoverflow.com/questions/2877262/java-securityexception-signer-information-does-not-match

Then, how can I fix the hacky way 🐰

@warjort
Copy link

warjort commented Jun 17, 2022

Ok, this is caused by fabric loader signing the jar

def signingEnabled = ENV.SIGNING_SERVER

That kind of seals the packages.
You can't add new classes into those packages in the same classloader unless you also sign your jar with the same signature.
Of course you can't do that because you don't have access to fabric's private key.

@EZForever
Copy link

Author of ThatOrThis here. I'm 110% against using hacks to inject mods into the loader, but it just can't be done the other way back when the mod is out. Now that Fabric Loader has evolved to support loading mods from additional directories, and class injection just got much harder with that package signature (just why?), I'm willing to do a rewrite. However there are other problems yet to be solved.

You'll have to wait for us to add the appropriate APIs to have a clean solution.

I actually don't think it's the loader's job to provide the APIs. Fabric Loader has one job - loading mods into Minecraft - and it's doing it well enough. All we need is a way to tell the loader which mods to load, and we have fabric.addMods now. Though I'd admit it will be painful to write a wrapper (with Knot, the entrypoint, being an implementation detail) or even half a game launcher to utilize just that. Besides, considering that #81 is still open after 3 years, I guess there's just more important stuffs for the loader devs to do. btw, being harsh really isn't going to help anybody.

@sfPlayer1
Copy link
Contributor

Letting mods control the mod set is quite an important feature, it allows all sorts of updaters, adapters, server-mod-synchronizers and even your application. I expect to land the feature set sooner than later, so I wouldn't recommend sinking much time in a rewrite. Your approach is likely still workable by (ab)using one of our mod candidate finder implementations instead.

Issues like 81 are fairly close to us not wanting the feature by default. The system property is one way to opt into this behavior (-Dfabric.addMods=mods), but the officially recommended approach is a custom game directory per mod set as detailed in https://minecrafthopper.net/help/guides/changing-game-directory/ which also solves any undesirable interactions with configs, saves and server list entries.

@SettingDust
Copy link
Author

@burningtnt
Copy link

burningtnt commented Oct 22, 2023

I faced the same problem while developing Voxel Latest, a mod which modifies Voxel Map 1.17.1 and enables it to run on Minecraft 1.18.1, 1.18.2 and more. However, the license of Voxel Map is ARR. So I have to dynamically download, remap and inject Voxel Map to Fabric Loader instead of using jar-in-jar.

Currently, I inject the jar file with FabricLauncherBase.getLauncher().addToClassPath, inject the mod and its metadata to Fabric Loader with reflection when IMixinConfigPlugin.<clinit> is invoked. Besides, I add mixin config file with Mixins.addConfiguration when IMixinConfigPlugin.getMixins is invoked.
This implementation is hacky and dirty. Besides, it may break with different versions of Fabric Loader. Here is my Injector.

Could fabric provide an API to add mods to Fabric Loaderd dynamically?

@sfPlayer1
Copy link
Contributor

https://github.com/sfPlayer1/fabric-loader/tree/extension

@SettingDust
Copy link
Author

@burningtnt https://github.com/SettingDust/preloading-tricks My own impl for reference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants