-
Notifications
You must be signed in to change notification settings - Fork 201
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
Some field references are not remapped when building with Loom 1.5 #1044
Comments
I have a somewhat similar report for Terraform API's 24w06a alpha release:
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... |
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. |
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. |
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. |
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. |
Unfortunately I am unable to test it in Loom 1.4 because of the bug with client-only and server-only projects.
I understand, and I wouldn't ask you to. I will keep trying to find a simpler reproduction case. |
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 |
Minimal reproduction case for the issue I'm aware of: 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 I am not sure whether the issue is in the Terraform API build or the build of this reproduction example. |
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 This is never going to work in prod either. This method should have an intermediary name of: So lets go and try and build Terraform again (I assume the 1.20.5 branch?), it fails to build for me: 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. |
I believe its caused by You have a really werid setup with the 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. |
Thank you for looking at this and for recommending potential improvements!
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?
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.
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 |
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)
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 |
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. |
I just got around to testing it, and disabling |
Also experienced this with Porting Lib, can confirm downgrading back to 1.4 fixed it |
- Related to FabricMC/fabric-loom#1044 Signed-off-by: Hendrix-Shen <[email protected]>
see FabricMC/fabric-loom#1044 Some fields have not been remapped correctly after building with Loom 1.5+. Disabling the multi project optimisation fixes the issue for now.
Reproduced for me using Loom 1.6. When |
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. |
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)
|
same for me |
FYI, currently the plan is to remove |
Signed-off-by: 秋雨落 <[email protected]>
Signed-off-by: 秋雨落 <[email protected]>
MPO has been removed from 1.8. |
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).
The text was updated successfully, but these errors were encountered: