Support for FG2-era Forge #70

Johni0702 · 2022-01-10T13:38:57Z

Adds support for Forge versions which previously required ForgeGradle 2.
Should be fully functional for building production-ready jars as well as running in a development environment (I only tested IntelliJ but there's no IDE-specific code).

This was developed mostly late November completely independently from #63. Afaict notable differences are:

Significantly smaller diff; as few changes to FG3 code paths as possible (within reason)
No FG code; instead of implementing the old patch algorithm, the old patches are converted to the modern format (which is surprisingly similar) when they are extracted (similarly, old srg files are converted to tsrg when extracted)
Supports the TerminalConsoleAppender (by bumping the log4j version and merging the old "helper" classes, which forge depends on, into the forge jar), this may cause issues if mods depend on other parts of log4j that were changed between 2.0-beta9 and release (seems unlikely), but makes for MUCH better console output
Access transformers are applied directly to the official-mapped jar (remapping the transformer on the fly) instead of converting the whole thing to srg and back
The intermediary mappings are generated by stitch instead of manually traversing all classes
Passes the forge/simple integration test case for 1.8.9 and 1.12.2

I had originally bundled pack200 directly into loom but later switched that out for the external plugin as in #63. I was unable to figure out a nice way to get it into the integration test though (classloader shenanigans where the plugin couldn't see our interface; and just adding it to the test classpath doesn't add it to the nested gradle jvm). The best I could come up with is adding a "runtimeOnly" dependency to the main project but is not ideal if the goal was to not have a dependency on that pack200 implementation (though the java classpath exception applies to it, so this shouldn't be an issue license-wise).

There's two extra commits before the main one, each of which fixes a bug I ran into (see their commit messages for details).

Somewhat complex example project: https://github.com/Sk1erLLC/Patcher/tree/tmp/archloom
(needs archloom published to mavenLocal; features a coremod, tweakers, mixins, 1.8.9 and 1.12.2)

Way easier to read than the previous array-copy implementation and trivial to extend when more file are optional (see next commit).

This jar used to always be included in the cache file list even though it is only used/generated with official mappings. Therefore it would always be missing from the cache when not using official mappings, unnecessarily re-running the patching code.

A fallback SrgProvider is instantiated and initialized here but never added to the provider list, resulting in a NPE right below in `getRawSrgFile`.

shedaniel · 2022-01-10T13:54:15Z

...java/net/fabricmc/loom/configuration/providers/forge/fg2/MinecraftLegacyPatchedProvider.java

+		}
+	}
+
+	private static class FabricSideStripper extends ClassVisitor {


This could be an issue for the Forge 1.13+ setups as well (Noticed it in some classes), I will just keep a comment here so I can go back here in the future

src/main/java/net/fabricmc/loom/configuration/providers/mappings/MappingsProviderImpl.java

src/main/java/net/fabricmc/loom/util/srg/SrgNamedWriter.java

shedaniel · 2022-01-10T14:01:06Z

src/test/resources/projects/forge/simple/build.gradle

@@ -57,6 +57,13 @@ jar {
 	}
 }

+loom {
+	forge {
+		// FIXME should eventually be handled by architectury-pack200 gradle plugin?


Keeping a comment here, yes, this will be handled there once a PR is merged.

We never use the v1 file, so there isn't really any reason to keep it around.

Cause modern versions of Forge do not need them.

src/main/java/net/fabricmc/loom/configuration/providers/forge/ForgeUserdevProvider.java

src/main/java/net/fabricmc/loom/configuration/providers/forge/MinecraftPatchedProvider.java

src/main/java/net/fabricmc/loom/util/srg/SrgNamedWriter.java

Juuxel · 2022-01-10T15:47:16Z

src/test/resources/projects/forge/simple/build.gradle

+loom {
+	forge {
+		// FIXME should eventually be handled by architectury-pack200 gradle plugin?
+		pack200Provider = new dev.architectury.pack200.java.Pack200Adapter()


This stuff should be in its own test since pack200 isn't used in the Forge code for current MC versions

This is only temporary. It'll be moved into the pack200 plugin once this PR is merged (so the pack200 plugin can compile against it).

src/main/java/net/fabricmc/loom/LoomRepositoryPlugin.java

src/main/java/net/fabricmc/loom/LoomGradleExtension.java

src/main/java/net/fabricmc/loom/configuration/providers/forge/ForgeUserdevProvider.java

src/main/java/net/fabricmc/loom/configuration/providers/forge/SrgProvider.java

Juuxel · 2022-01-10T16:03:04Z

build.gradle

@@ -119,13 +119,17 @@ dependencies {
 	implementation ('de.oceanlabs.mcp:mcinjector:3.8.0')
 	implementation ('com.opencsv:opencsv:5.4')

+	// Legacy Forge access transformers
+	implementation ('net.md-5:SpecialSource:1.10.0')


Does the current AT tool not work for old MC versions?

It would require remapping the entire minecraft jar (and presumably also forge? I guess ATs can apply to it as well?) from official to srg and back, only to run the AT tool. Much slower than this implementation which basically just converts the names as it looks them up.

Hmm, I see. You could also remap the AT since we have tools (CadixDev's library) for that.

Usages are more related to the modern Forge than to ModLauncher specifically.

I do not quite remember why I added this in the first place but it seems fine without, so let's not mess with it.

Juuxel · 2022-01-11T17:25:46Z

src/main/java/net/fabricmc/loom/configuration/providers/LaunchProvider.java

 		addDependency(Constants.Dependencies.DEV_LAUNCH_INJECTOR + Constants.Dependencies.Versions.DEV_LAUNCH_INJECTOR, Constants.Configurations.LOOM_DEVELOPMENT_DEPENDENCIES);
 		addDependency(Constants.Dependencies.TERMINAL_CONSOLE_APPENDER + Constants.Dependencies.Versions.TERMINAL_CONSOLE_APPENDER, Constants.Configurations.LOOM_DEVELOPMENT_DEPENDENCIES);
 		addDependency(Constants.Dependencies.JETBRAINS_ANNOTATIONS + Constants.Dependencies.Versions.JETBRAINS_ANNOTATIONS, JavaPlugin.COMPILE_ONLY_CONFIGURATION_NAME);

-		if (getExtension().isForge()) {
+		if (getExtension().isForge() && !getExtension().isLegacyForge()) {


Suggested change

if (getExtension().isForge() && !getExtension().isLegacyForge()) {

if (getExtension().isModernForge()) {

We've assumed that Forge adds its annotation everywhere via its patches but it actually only adds them if a given file needs to have a patch applied for reasons other than just the SideOnly annotation. This would leave some classes/fields/methods without annotation which resulted in an exception when deserializing horses on 1.12.2 (because it references a class which in turn has methods referencing the dedicated server class which is server-only). Instead, we now convert all Fabric annotations (added during merging) to the Forge ones and remove duplicates (where patches also added them).

Juuxel · 2022-05-04T11:05:22Z

This should probably be updated to target dev/0.11.0, as that's where the development is happening, but it might be a bit too much work with the refactors done to both Loom's core/upstream code as well as the Forge-specific parts...

Forge has not published regular source artifacts for old versions, instead the sources zip is bundled within the userdev jar. Co-authored-by: DJtheRedstoner <[email protected]>

wagyourtail · 2022-06-08T14:25:58Z

as well as the Forge-specific parts...

exactly the reason I split fg3+ and fg2 classes in mine, makes it so changes to one don't cascade into the other
(also why am I struggling to get jitpack to build this one... oh. missing yml to bump java?)

Johni0702 added 4 commits January 6, 2022 13:52

Simplify MinecraftPatchedProvider.getGlobalCaches()

ea44973

Way easier to read than the previous array-copy implementation and trivial to extend when more file are optional (see next commit).

Fix missing SrgProvider when generateSrgTiny is false

64a1aba

A fallback SrgProvider is instantiated and initialized here but never added to the provider list, resulting in a NPE right below in `getRawSrgFile`.

Support for FG2-era Forge

f0cafe8

shedaniel reviewed Jan 10, 2022

View reviewed changes

Johni0702 added 2 commits January 10, 2022 15:43

Clean up generated tiny v1 file after converting to v2

0ff81f4

We never use the v1 file, so there isn't really any reason to keep it around.

Generate identity mappings in srg file only for legacy forge

c009507

Cause modern versions of Forge do not need them.

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 Jan 10, 2022

Juuxel requested changes Jan 10, 2022

View reviewed changes

Johni0702 added 7 commits January 11, 2022 16:15

Do not depend on lorenz impl classes to write identity mappings

6a2a900

Use explicit if to separate modern and legacy runs setup

98173e3

Rename isModLauncher -> isModernForge

beb659e

Usages are more related to the modern Forge than to ModLauncher specifically.

Add comments to non-obvious legacy code paths

9eaae61

Flip a few ifs so the modern code path comes before the legacy one

75f0fa8

Only add LegacyMCP repo when using a legacy version

8e30bfd

Do not add mixin to AP configs during IDE sync even for legacy forge

7f33c12

I do not quite remember why I added this in the first place but it seems fine without, so let's not mess with it.

Juuxel self-requested a review January 11, 2022 17:24

Juuxel reviewed Jan 11, 2022

View reviewed changes

Johni0702 added 2 commits January 11, 2022 20:34

Re-use forge accesstransformer tool for legacy forge

4fa31a4

Simplify isForge && !isLegacyForge -> isModernForge

5f33bfd

Johni0702 requested a review from Juuxel January 17, 2022 11:29

Johni0702 added 2 commits February 22, 2022 13:50

Make legacy Forge discover and load coremods from the classpath

f7b4916

Fix missing legacy forge sources

bd1eb92

Forge has not published regular source artifacts for old versions, instead the sources zip is bundled within the userdev jar. Co-authored-by: DJtheRedstoner <[email protected]>

Jab125 mentioned this pull request Jun 12, 2023

Support Legacy Forge versions for loom 1.2 #138

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for FG2-era Forge #70

Support for FG2-era Forge #70

Johni0702 commented Jan 10, 2022

shedaniel Jan 10, 2022

shedaniel Jan 10, 2022

Juuxel Jan 10, 2022

Johni0702 Jan 11, 2022

Juuxel Jan 10, 2022

Johni0702 Jan 11, 2022

Juuxel Jan 11, 2022

Juuxel Jan 11, 2022

Juuxel commented May 4, 2022

wagyourtail commented Jun 8, 2022 •

edited

Loading

	if (getExtension().isForge() && !getExtension().isLegacyForge()) {
	if (getExtension().isModernForge()) {

Support for FG2-era Forge #70

Are you sure you want to change the base?

Support for FG2-era Forge #70

Conversation

Johni0702 commented Jan 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Juuxel commented May 4, 2022

wagyourtail commented Jun 8, 2022 • edited Loading

wagyourtail commented Jun 8, 2022 •

edited

Loading