Support merging pre 1.3 Minecraft versions.

[SpaceWalkerRS]

• Added two methods in LoomGradleExtension to query whether the obfuscated jars can be merged (determined by querying the MC version from the project dependency).

• Added clientOfficial and serverOfficial mapping namespaces.

• Added "separate jars" Minecraft provider, intermediary Minecraft provider, and named Minecraft provider.

  • The intermediary and named Minecraft providers merge the jars after remapping them separately.

• Edit: I rended up refactoring most of this. Adding this feature as a new jar configuration lead to some problems, but I was able to merge it into the other jar configurations quite seamlessly. This way if the MERGED configuration is used, Loom will figure out itself whether it should merge and then remap or remap first and then merge.

[SpaceWalkerRS]

I think the mappings merger is broken, but I'm not familiar enough with MIO to fix it.

[thecatcore]

I think the mappings merger is broken, but I'm not familiar enough with MIO to fix it.

This might be because in babric intermediary, if an entry is client or server only, then there is no value for the other side namespace, which might be evaluated as null in MIO if I remember correctly.

[SpaceWalkerRS]

IF I understand it right, MIO will need an update to support the double source namespace, both for reading the intermediary and for reading/writing the merged mapping file.

For classes, you could conceivable hack it together by just putting the second source namespace in the first target namespace slot, but this won't work for fields and methods, since you need both the source name and source descriptor.

[SpaceWalkerRS]

On second thought, wouldn't updating the tiny format like that mean TR needs an update as well as MIO? I'm starting to think it's not worth it trying to merge the mappings into a single file and instead just making some more modifications to Loom to have separate client and server side intermediary and merged (in this case meaning it has both the intermediary and named target namespaces) mapping files.

[Juuxel]

Edit: wrong discussion

[thecatcore]

Hm, I was wondering, how would loader work with this without any changes currently? Is there a way to make it not care about official/obfuscated namespace when in dev environement or is it already the case?

[modmuss50]

Hm, I was wondering, how would loader work with this without any changes currently? Is there a way to make it not care about official/obfuscated namespace when in dev environement or is it already the case?

I believe thats already the case, its worth checking though.

[SpaceWalkerRS]

The two tests for legacy projects failed - I pushed a fix for the no op intermediate mappings provider , hopefully that resolves that.

There are no tests for 'legacy merged' projects yet, though, but I have no clue how to write those tests.

[SpaceWalkerRS]

Most issues are now resolved. The only failing tests are those for <1.3 merged projects, since they require a merged intermediary to work.

[SpaceWalkerRS]

Alright I tried implementing it how you suggested, it wasn't too bad to change it up. I hope I didn't miss any changes that could be reverted but there we are.

I also tried adding some mappings for the legacy project test, but I'm not sure how to go about making the test use those. Any help with that would be appreciated.

this PR now depends on #1070

[modmuss50]

Please see: https://github.com/FabricMC/fabric-loom/pull/1072

This should allow you to adjust the default jar configuration used based on minecraft versions.

[SpaceWalkerRS]

Please see: #1072

This should allow you to adjust the default jar configuration used based on minecraft versions.

That was quick, thanks so much! It should be a small change to this PR, so I'll wait until that is merged in.

[SpaceWalkerRS]

Looking a lot better now, I think the tests still need improving. Do let me know if you want some help with this 👍

I'd appreciate some help, as I'm stuck with both failing tests.

The mapping reader tests fail because of the intermediary source namespace validation. They all fail with 'expected intermediary but found official' which is wrong since all those tests use MC 1.17 which must have official as the source namespace. This most likely means the canMergeJars call fails, which I thought was because the test version meta lacked the releaseTime field. But adding that in did not fix it...

As for the legacy project test, while I did add an intermediary file, I do not know how to go about making the test use it.

[modmuss50]

Looking a lot better now, I think the tests still need improving. Do let me know if you want some help with this 👍

I'd appreciate some help, as I'm stuck with both failing tests.

No worries at all, I can take a look at this, might be a day or two as im going to be a little busy the 1/2 half of this week.

[modmuss50]

Out of intrest what named mappings are you using? Id love to do a full manual end to end test with this, some intermediary names and then names I can make a small mod against.

[modmuss50]

Im happy with this now, would just like to see it working outside of the automated tests. Make sure the game actually runs (both client and server), and that you can apply named mappings.

[SpaceWalkerRS]

Out of intrest what named mappings are you using? Id love to do a full manual end to end test with this, some intermediary names and then names I can make a small mod against.

It's still early days but eventually Ornithe's Feather mappings will be ported to this new merged intermediary, and could be used to test this. At the moment though I do not have any named mappings available for these intermediaries.

[modmuss50]

Pushed some more fixes, no idea how the test was passing on my machine, once I cleared the test caches it started failing. I have a dev env where I have a working server, but the client stalls on startup. I have a feeling this is due to my arm64 mac.

[modmuss50]

Also it doesnt seem to be using a merged jar 🤔

[modmuss50]

Right, sorry for the commit spam. I think I have resolved most of my mess. I cannot test legacy clients on my arm mac (this might be something to solve later), but it seems the merged server isnt working:

java.lang.ClassFormatError: Duplicate interface name "net/minecraft/unmapped/C_2286002" in class file net/minecraft/unmapped/C_4253713
    at java.base/java.lang.ClassLoader.defineClass1(Native Method)
    at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1027)
    at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
    at net.fabricmc.loader.impl.launch.knot.KnotClassLoader.defineClassFwd(KnotClassLoader.java:160)
    at net.fabricmc.loader.impl.launch.knot.KnotClassDelegate.tryLoadClass(KnotClassDelegate.java:355)
    at net.fabricmc.loader.impl.launch.knot.KnotClassDelegate.loadClass(KnotClassDelegate.java:218)
    at net.fabricmc.loader.impl.launch.knot.KnotClassLoader.loadClass(KnotClassLoader.java:119)
    at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:526)
    at net.minecraft.unmapped.C_4507027.m_7189836(C_4507027.java:36)
    at net.minecraft.unmapped.C_7366354.m_7447312(C_7366354.java:53)
    at net.minecraft.unmapped.C_3906126.<init>(C_3906126.java:162)
    at net.minecraft.unmapped.C_7366354.<init>(C_7366354.java:26)
    at net.minecraft.server.MinecraftServer.m_2695328(MinecraftServer.java:253)
    at net.minecraft.server.MinecraftServer.m_0615401(MinecraftServer.java:188)
    at net.minecraft.server.MinecraftServer.run(MinecraftServer.java:335)
    at net.minecraft.unmapped.C_6018901.run(C_6018901.java:492)

This appears it might be an issue with the jar merger?

[SpaceWalkerRS]

Woo, project setup works for me locally as well now! Thanks so much for the help!

it seems the merged server isnt working

I think I know the issue, this'll be due to the changes to the MinecraftJarMerger I made, forgot to do an equals check I suppose.

[SpaceWalkerRS]

alright, it's not very pretty, but this should fix the duplicate interfaces

[SpaceWalkerRS]

I can confirm both the server and client run from the dev environment, at least for the merged jars. Client-only and server-only is broken because the single jar impls no longer use the appropriate source namespace for pre-1.3 versions

[modmuss50]

Client-only and server-only is broken because the single jar impls no longer use the appropriate source namespace for pre-1.3 versions

Ill take a quick look to see if this can be fixed.

[SpaceWalkerRS]

imo the changes made when removing canMoveJars should be reverted - it's not the jar configuration that determines how the mappings are provided, but the MC version

[modmuss50]

imo the changes made when removing canMoveJars should be reverted - it's not the jar configuration that determines how the mappings are provided, but the MC version

My thinking behind this change was to remove the idea of merged jars away from the abstract MinecraftProvider class, you should still be able to SingleJarMinecraftProvider as before (as far as I can tell this is working as it was before). The minecraft version is only used to decide what jar provider to use.

[modmuss50]

Client-only and server-only is broken because the single jar impls no longer use the appropriate source namespace for pre-1.3 versions

This now works as it did before, using named as the only namespace. In an IntermediateMappingsProvider you can query getIsLegacyMerged() to know if the new namespaces should be used or not.

[SpaceWalkerRS]

My thinking behind this change was to remove the idea of merged jars away from the abstract MinecraftProvider class, you should still be able to SingleJarMinecraftProvider as before (as far as I can tell this is working as it was before). The minecraft version is only used to decide what jar provider to use.

Perhaps the method should be renamed then? The idea behind it is that it returns whether the client and server have common obfuscation maps, i.e. whether they are 'merge-able'. If they are not, mappings must be provided with separate client and server namespaces, no matter which jar configuration is used. At the moment it only expects the clientOfficial and serverOfficial namespaces to be used if the legacy merged jar configuration is selected for a pre-1.3 version, while they should also be used for the client-only or server-only jar configuration for pre-1.3 versions.

[SpaceWalkerRS]

In an IntermediateMappingsProvider you can query getIsLegacyMerged() to know if the new namespaces should be used or not.

But isLegacyMerged is only set to true if the jar configuration is the legacy merged one.

[modmuss50]

In an IntermediateMappingsProvider you can query getIsLegacyMerged() to know if the new namespaces should be used or not.

But isLegacyMerged is only set to true if the jar configuration is the legacy merged one.

Yes, its set with: provider.getIsLegacyMerged().set(getProject().provider(() -> getMinecraftProvider() instanceof LegacyMergedMinecraftProvider));

[SpaceWalkerRS]

But isLegacyMerged is only set to true if the jar configuration is the legacy merged one.

Yes, its set with: provider.getIsLegacyMerged().set(getProject().provider(() -> getMinecraftProvider() instanceof LegacyMergedMinecraftProvider));

It should be true for pre-1.3 versions regardless of the jar configuration.

[modmuss50]

It should be true for pre-1.3 versions regardless of the jar configuration.

Im not sure I agree with that, especially if we want to maintain backwards compatibility. You could still use merged mappings with this, by chaning the namespaces in the intermediary provider.

[SpaceWalkerRS]

It should be true for pre-1.3 versions regardless of the jar configuration.

Im not sure I agree with that, especially if we want to maintain backwards compatibility. You could still use merged mappings with this, by chaning the namespaces in the intermediary provider.

Providing mappings differently depending on the jar configuration seems like a bad idea to me. It will lead to the same kind of cache issues we had before Loom 1.3 with different intermediary providers.

[modmuss50]

Providing mappings differently depending on the jar configuration seems like a bad idea to me. It will lead to the same kind of cache issues we had before Loom 1.3 with different intermediary providers.

Thats a good point, ill change it back then 👍

[modmuss50]

Sorry, I didnt realise you had made this change as well, I was working on https://github.com/modmuss50/fabric-loom-1/commit/137d9b5f5a43526cadef6b2fbd788665a029c1a7 I think it makes it a bit clearer, and removes the duplicated logic where it decides on the ns to use. If you feel strongly with what you have im happy to go with it.

[SpaceWalkerRS]

Ah oops I misread your message - I think your changes make a lot of sense, so let's go ahead with those 👍

[modmuss50]

Ah oops I misread your message - I think your changes make a lot of sense, so let's go ahead with those 👍

Thanks, I have pushed those here. I will merge this once the tests pass. Can of course fix/improve things in later PRs 👍

[modmuss50]

Merged, thanks a lot for persisting with this 🎉