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

FG2 Support #63

Open
wants to merge 29 commits into
base: dev/0.10.0
Choose a base branch
from

Conversation

wagyourtail
Copy link

@wagyourtail wagyourtail commented Dec 10, 2021

add fg2 support to arch-loom, kinda needs #36 but using legacy intermediaries essentially as stubs works so...

the 1.12.2 fg3 versions are still borked, but using 14.23.5.2847 as fg2 works so... good enough for me

Todo

  • - add run configs (they're not in userdev)
  • - make run configs actually work
  • - fix binpatcher
  • - cleanup code

@CLAassistant
Copy link

CLAassistant commented Dec 10, 2021

CLA assistant check
All committers have signed the CLA.

@wagyourtail wagyourtail changed the title Experimental FG2 Support FG2 Support Dec 10, 2021
@wagyourtail wagyourtail marked this pull request as draft December 10, 2021 06:32
@shedaniel
Copy link
Member

Impressive, I will have to look into #36 first, but doesn't legacy forge patches require using pack200?

@wagyourtail
Copy link
Author

wagyourtail commented Dec 10, 2021

idk, it just seemed to work, i thought it needed a --pack200 arg with the binpatcher tho but it didn't throw any errors when I ran in the test mod's build. maybe it didn't actually apply the patch...

@wagyourtail
Copy link
Author

yeah, comparing to a FG env, it appears the patches didn't actually apply, I'll look into it, probably just need to add the pack200 arg

@shedaniel
Copy link
Member

The issue is that pack200 has been removed since J14, and we would need to bundle it somewhere

@wagyourtail
Copy link
Author

isn't arch-loom still built against java 11? really expecting fully modern java on old versions seems secondary

@shedaniel
Copy link
Member

We now fully require J16.

@Juuxel
Copy link
Member

Juuxel commented Dec 10, 2021

Hmm, I'd argue that expecting outdated Java makes no sense.

@wagyourtail
Copy link
Author

yeah, packaging pack200 would be needed then, unless it's provided as a separate dependency somewhere...

@wagyourtail
Copy link
Author

wagyourtail commented Dec 10, 2021

pack200 appears to use native code, so I was thinking find someone's "modern" implementation of pack200 in java and kinda jury rig it onto the old pack200 interface, (or pre-unpack it for binpatcher if we don't need --legacy)

@LlamaLad7
Copy link

ReplayMod's fork of FG works with Java 16 and appears to package pack200. Maybe it would be useful as a reference.

@wagyourtail
Copy link
Author

Yes, we mentioned that on discord i think... they simply re-pack openjdk's pack200 class iirc

@wagyourtail
Copy link
Author

because of module stuff, it looks like I have to fork binpatcher and do it there...

@wagyourtail
Copy link
Author

at this point I know there are wrong checksums, but I don't know why

@wagyourtail
Copy link
Author

wagyourtail commented Dec 11, 2021

ok figured it out. now I just have to figure out how to remap it properly...

@wagyourtail
Copy link
Author

now the final patched mappings are stuck at srg and not remapping to MCP... or even official

@wagyourtail
Copy link
Author

wagyourtail commented Dec 11, 2021

how I actually ended up doing it, I can pull pack200 back out into arch-loom and go back to official binpatcher? should I?

@wagyourtail wagyourtail marked this pull request as ready for review December 12, 2021 12:05
Copy link
Member

@shedaniel shedaniel left a comment

Choose a reason for hiding this comment

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

Please import the checkstyle for the imports (there is an idea plugin for checkstyle), the diff is abnormally big, I can fix the actual checkstyle issues later on, but the imports changes are annoying

Also please keep the indents consistent, Loom uses tabs, not spaces
please also keep unrelated changes to a minimum

I am not too fond of having FG2 code basically copy and pasted here, but I will look into that in the future

import net.fabricmc.loom.configuration.providers.forge.McpConfigProvider.RemapAction;
import net.fabricmc.loom.util.FileSystemUtil;
import net.fabricmc.loom.util.ThreadingUtils;

public class SpecialSourceExecutor {
Copy link
Member

Choose a reason for hiding this comment

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

This file is hard to review, did any functionality here change? Or is it a refactor?

Copy link
Author

Choose a reason for hiding this comment

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

refactor, look between it and the old one and it's pulling the stripping stuff out to another function so I can use it separately

@shedaniel shedaniel added enhancement New feature or request new feature This pull request introduces a (major) new feature priority: low This does not immediately need to be resolved labels Dec 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request new feature This pull request introduces a (major) new feature priority: low This does not immediately need to be resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants