Some field references are not remapped when building with Loom 1.5

[SpaceWalkerRS]

I am having great difficulty finding a simple reproduction case for this, so I'll just link the project where I'm experiencing this bug instead. The issue occurs here, a value is assigned to a field from a super class. While references to this super class and methods from it are remapped to intermediary, these two field references are not.

I tried creating a project from the Fabric example mod, but could not reproduce it. But bringing the OSL project back to Loom 1.3 immediately fixed it again (could not use Loom 1.4 'cause in that version client-only and server-only projects were broken).

[gniftygnome]

I have a somewhat similar report for Terraform API's 24w06a alpha release:
TerraformersMC/Terraform#88

IntegerProperty.of(java.lang.String, int, int) definitely still exists but somehow cannot be found in prod ... sometimes. And I also do not know why it is only sometimes.

Additionally, I know arch loom is not supported here, but there are multiple reports of similar issues with arch loom 1.5.

I really wish use of Loom 1.4 was still an available option for the remainder of the 1.20 series...

[modmuss50]

I really need a simple way to reproduce this, and more details. "not remapped" isnt so helpful, when there are about 4 diffrent ways things get remapped.

The crash log in that issue linked from @gniftygnome is in a dev env, what loader version is being used, has the mod been runtime remapped or via loom? It also seems like you cannot reproduce it yourself, so it might just be a broken cache.

[SpaceWalkerRS]

The issue I'm experiencing happens in prod, with the compiled mod jar that is remapped to intemediary by Loom, but it seems (in my case) some field references are not remapped. This means they work fine in dev but will fail in prod.
It will fail in exactly the same way every time I build that project, and I always build with ./gradlew clean build. Going back to Loom 1.3 fixes it, then going to 1.5 and breaks it, again in the same way.
But I don't know what it is that makes it fail, so I have so far been unable to find a simpler case that causes it. I even tried setting up a bare project in the same MC version and just adding those related classes, but without luck.

[gniftygnome]

First of all, yes; I apologize for stating prod. I have multiple open remapping issues with loom 1.5 right now (some of which are in arch loom), and I'm afraid I just got a bit mixed up. The Terraform API version in question was built with loader version 0.15.6, but I am not certain what version was in use for the reported crash. I have asked for details.

Quite possibly the issue I linked in Loom is not related (as you say it is short on details). The arch loom 1.5 ones are more similar (method and variable names not being remapped, only in prod) but I wouldn't report those here; that's for the arch folks to do if they eventually decide it's a fabric loom issue.

[modmuss50]

1.3 fixes it

This suggests that it was a 1.4 change that broke things? Most of the remapping changes I made happened in 1.5, the 1.4 diff to 1.3 is quite small: 1.3...1.4 Can you confirm that 1.4 doesnt work, while 1.3 does?

It is odd that you cannot reproduce it with a small project, I can maybe try to debug it with a large project but its usually a nightmare.

[SpaceWalkerRS]

Unfortunately I am unable to test it in Loom 1.4 because of the bug with client-only and server-only projects.

It is odd that you cannot reproduce it with a small project, I can maybe try to debug it with a large project but its usually a nightmare.

I understand, and I wouldn't ask you to. I will keep trying to find a simpler reproduction case.

[oliviathevampire]

I really need a simple way to reproduce this, and more details. "not remapped" isnt so helpful, when there are about 4 diffrent ways things get remapped.

The crash log in that issue linked from @gniftygnome is in a dev env, what loader version is being used, has the mod been runtime remapped or via loom? It also seems like you cannot reproduce it yourself, so it might just be a broken cache.

I use newest fabric loader, it happens right when I start the game. Afaik it’s not been runtime remapped. Lemme try and do what you suggested

[gniftygnome]

Minimal reproduction case for the issue I'm aware of:
https://github.com/gniftygnome/thing1

This is basically the Fabric Example mod switched to mojmap and modified to load terraform-wood-api-v1 (which uses yarn mappings). In dev, it will crash with the previously reported error. @oliviathevampire pointed out IntegerProperty.of is a mixture of mojmap (IntegerProperty) and yarn (of).

I am not sure whether the issue is in the Terraform API build or the build of this reproduction example.

[modmuss50]

Easy enough to reproduce, many thanks for that. It doesnt take much to go deeper into the rabbit hole.

Enabling mixin debug, its obvious the culpret seems to be: https://github.com/TerraformersMC/Terraform/blob/e876c3efc45db8bc690ddb7cfc66a4ab2c452668/terraform-wood-api-v1/src/main/java/com/terraformersmc/terraform/leaves/mixin/MixinProperties.java#L11

I dont think this mixin is a great design at all (ModifyArg or maybe ModifyConstant? would be better), but thats not what we are talking about.

Spending 10 seconds looking at the orignal terraform-wood-api-v1-10.0.0-alpha.1.jar shows that the method has not been remaped correctly when built:

This is never going to work in prod either. This method should have an intermediary name of: method_11867.

So lets go and try and build Terraform again (I assume the 1.20.5 branch?), it fails to build for me:
Cannot nest jars into none mod jar terraform-api-10.0.0-alpha.2.jar Not sure what im doing wrong

If I just build the single sub project on the output is indeed wrong:

The mappings for this look correct:

I guess I'll try to reproduce this in the example mod next.

[modmuss50]

I believe its caused by fabric.loom.multiProjectOptimisation and only building a single sub project. With MPO you must remap all of the jars at once, especially including the root project. The root project provides all of the classpath for remapping the subprojects.

You have a really werid setup with the buildTerraform tasks, you should just run the build task to build everything. I also dont know why your root project needs to depend and include all of the sub projects I dont see you ever using this?

I have made the following changes, that cleanup your buildscript to make it more idiomatic and faster.

Subject: [PATCH] Fix
---
Index: gradle.properties
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>ISO-8859-1
===================================================================
diff --git a/gradle.properties b/gradle.properties
--- a/gradle.properties	(revision 6099a2a07524f36238382378500cfc655f44d5db)
+++ b/gradle.properties	(date 1708438836561)
@@ -1,4 +1,5 @@
 org.gradle.jvmargs=-Xmx2G
+org.gradle.parallel=true
 fabric.loom.multiProjectOptimisation=true
 
 maven_group=com.terraformersmc.terraform-api
Index: build.gradle
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build.gradle b/build.gradle
--- a/build.gradle	(revision 6099a2a07524f36238382378500cfc655f44d5db)
+++ b/build.gradle	(date 1708439040483)
@@ -6,15 +6,6 @@
 	id 'fabric-loom' version '1.5.+'
 }
 
-dependencies {
-	afterEvaluate {
-		subprojects.each {
-			implementation project(path: "${it.name}", configuration: "namedElements")
-			include project("${it.name}:")
-		}
-	}
-}
-
 allprojects {
 	apply plugin: 'java'
 	apply plugin: 'idea'
@@ -126,24 +117,6 @@
 	}
 }
 
-task buildTerraform {
-	subprojects.each {
-		subprojects.each { dependsOn("${it.name}:build") }
-	}
-}
-
-task publishTerraform {
-	subprojects.each {
-		subprojects.each { dependsOn("${it.name}:publish") }
-	}
-}
-
-task publishTerraformLocally {
-	subprojects.each {
-		subprojects.each { dependsOn("${it.name}:publishToMavenLocal") }
-	}
-}
-
 ext.checkVersion = { archivesBaseName, version ->
 	try {
 		def xml = new URL("https://maven.terraformersmc.com/com/terraformersmc/terraform-api/$archivesBaseName/maven-metadata.xml").text

MPO is quite tempremental, I wouldnt try to get too clever with it.

[gniftygnome]

Thank you for looking at this and for recommending potential improvements!

This is never going to work in prod either. This method should have an intermediary name of: method_11867.

Strangely enough, it does work in prod which is why I could not reproduce. I was testing using 1.20.4 versions of our biome mods and maybe prod was successfully using a mapping from a JiJ'd copy of older Terraform API even though that version was not loaded?

I believe its caused by fabric.loom.multiProjectOptimisation and only building a single sub project.

This behaved itself from Loom 1.1 through 1.4. I think Loom 1.5 introduced some change in behavior that is relevant here, even if ultimately considered WAI.

You have a really werid setup with the buildTerraform tasks

It's all legacy stuff written by other people in the past and I'm no gradle expert so it's tough to make big changes. We presently publish production from a GH workflow with the publishTerraform build target, so I will have to see about changing how it works.

[modmuss50]

This behaved itself from Loom 1.1 through 1.4.

Yeah, im not sure what changed and what is the root cause. I'll leave this issue open to hopefully improve it (by forcing MPO to build all projects)

so I will have to see about changing how it works.

Do let me know on github if you want some help, thankfully you dont have anything too complex there. My patch above should be basically all you need, replacing buildTerraform with build and publishTerraform with publish.

[gniftygnome]

Regarding Ornithe (the project linked by the OP), it also uses MPO and its build.gradle makes ours look simple so this is probably a place for them to look.

[SpaceWalkerRS]

I just got around to testing it, and disabling fabric.loom.multiProjectOptimisation did indeed fix the issue. Thanks so much for the solution!

[TropheusJ]

Also experienced this with Porting Lib, can confirm downgrading back to 1.4 fixed it

[SolidBlock-cn]

Reproduced for me using Loom 1.6. When fabric.loom.multiProjectOptimisation is true, the same happens for sub-projects (not in root projects). When turned to false, fixed.

[modmuss50]

I've been thinking about removing multi project optimisation, as I dont think its required anymore and is very hard to maintain. (Its fundamentally incompatible with upcoming Gradle changes to start) It would be intresting to know if anyone here actually benefits from it? I wasn't aware that so many people were using it, its very much an advanced feature that I didn't expect many projects to need.

It would be a great help if you could test with and without it in Loom 1.6 (anything older and it likely does help), and let me know how you get on.

[gniftygnome]

Terraform API has MPO enabled. We could definitely survive without it though. Here's stats with and without, both cold cache and hot cache. (edit: this is Loom 1.6.11)

With MPO

cold:

54 actionable tasks: 54 executed

real    0m7.266s
user    0m1.029s
sys     0m0.050s

hot:

54 actionable tasks: 54 executed

real    0m5.060s
user    0m1.020s
sys     0m0.045s

Without MPO

cold:

46 actionable tasks: 46 executed

real    0m9.401s
user    0m1.063s
sys     0m0.045s

hot:

46 actionable tasks: 46 executed

real    0m6.730s
user    0m1.041s
sys     0m0.055s

[copyandexecute]

same for me

[modmuss50]

FYI, currently the plan is to remove multiProjectOptimisation in 1.8. I strongly reccomend you remove it from your projects if possible, and report any issues you have without it.

[modmuss50]

MPO has been removed from 1.8.