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 the MixinBootstrap init methods public #41

Closed
wants to merge 1 commit into from

Conversation

yuuka-miya
Copy link

This allows external tweakers who wish to control the launch process (like FE, for example) to conduct the Mixin initialization.

Currently, only MixinBootstrap.init() is exposed to external tweakers, and it has been stated that MixinBootstrap.init() will not work in production. Thus, either I live life dangerously by holding my own instance of MixinTweaker, or create a class in org.spongepowered.asm.launch (potentially dangerous too) or I make this PR.

This allows external tweakers who wish to control the launch process (like FE, for example) to conduct the Mixin initialization.
@Mumfrey
Copy link
Member

Mumfrey commented Apr 26, 2015

The methods are package-private for a reason though, there shouldn't be any reason to call those methods directly.

  • In development, call the init method from a coremod. This works because the mixin code is already on the classpath of the AppClassLoader
  • In production when using mixin as a library, call the init method from a coremod or tweaker, this works again because the jar is already on the correct classpath.
  • In production when shading mixin, use the provided tweaker.

If you can explain the use case for not using the mixin tweaker then I'll work out a way around this, but making the methods public without even updating the javadoc is not going to be pulled.

@yuuka-miya
Copy link
Author

This is the use case.

I would use your provided Tweaker, but it does have a few issues:

  1. It does not support FMLCorePluginContainsFMLMod. What this means is if the Mixin tweaker shares a jar with a coremod and a regular mod, only the coremod is loaded. My tweaker forces the FML CoreModManager to reparse the mod container later on in the launch process, when regular @mods are loaded. (issue Force injection of FML coremods/mods does not work on MC 1.7.10 and below #17)

  2. I had some external issues using the Mixin tweaker with my coremod (can't remember exactly, that was in the implementation stage), thus I decided to write my own instead. This also gives me easier access to some launch-time variables I need to load external libraries.

My apologies for not updating the javadocs, as I used the web interface for editing, did not see any references to javadocs, and interpreted this to mean that I would be expected to do the same, seeing as I was re-implementing the Mixin tweaker in this case.

@Mumfrey
Copy link
Member

Mumfrey commented Apr 26, 2015

I see, you're using your own tweaker then. Well to respond to your points

  1. Using the mixin tweaker doesn't preclude you also doing any extra init required in your own tweaker, so you can leave the mixin tweaker to init itself and the coremod and then handle extra tasks in your own tweaker

  2. It would be good to know what those issues were, the entire purpose of using a specific tweaker for the mixin init is that tweakers are only ever loaded once even if specified multiple times, thus the main purpose of the mixin tweaker for production is that if multiple mods use the mixin library then only one tweaker is actually instanced. init will safely short-circuit if called more than once, however register does not because the short-circuting is either handled by init or by the single-instance nature of the tweaker. Making the internal methods public potentially introduces a situation where stuff gets called out of order or in the wrong place.

My point about the javadocs is that I tend to write pretty sparse javadoc on non-public methods, designed to be read in conjunction with the code in order to give it context. When making a method public I would update the javadocs to be more verbose, especially in this instance where the exact nature of calling the methods is pretty delicate and somewhat context-sensitive.

@yuuka-miya
Copy link
Author

Thanks for the clarification. Either way, I could look into writing another PR for MixinTweaker to respect FMLCorePluginContainsFMLMod, then see if the issues I was having resurface under the newer version of Mixin (they happened with the initial implementation, which was quite a while ago). Though if I'm not wrong, it was Mixin instructing FML to load the wrong mod, something which may potentially be fixed by adding FMLCorePluginContainsFMLMod support to MixinTweaker. (done in my own tweaker)

For now, I'll stick to Mixin 0.2 until I have more time to look into things.

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

Successfully merging this pull request may close these issues.

None yet

2 participants