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

Fix Field Migration #168

Merged
merged 2 commits into from
Nov 13, 2023
Merged

Fix Field Migration #168

merged 2 commits into from
Nov 13, 2023

Conversation

shedaniel
Copy link
Member

#162 is caused by SrgMerger dropping the migrated fields. The easiest and most reliable way to fix this is to apply the migration to both base and srg tiny, instead of only applying to the base and let SrgMerger merge the mappings.

The fix seems pretty reliable.

@shedaniel shedaniel added bug Something isn't working priority: medium This issue or PR should be worked on soon-ish Forge This issue is specific to the Forge platform. labels Nov 12, 2023
@shedaniel shedaniel changed the title Fix #162 Fix Field Migration Nov 12, 2023
Copy link
Member

@Juuxel Juuxel left a comment

Choose a reason for hiding this comment

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

I trust you that it works, I don't understand the field migrator to begin with :ioa:

Table<String, String, String> fieldDescriptorMap = HashBasedTable.create();

for (Map.Entry<FieldMember, String> entry : migratedFields) {
fieldDescriptorMap.put(entry.getKey().owner, entry.getKey().field, entry.getValue());
}

MemoryMappingTree mappings = new MemoryMappingTree();
injectMigration(project, fieldDescriptorMap, rawTinyMappings, tinyMappings);
injectMigration(project, fieldDescriptorMap, rawTinyMappingsWithSrg, tinyMappingsWithSrg);
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this, it makes applying the same thing to the Mojang-merged mappings much easier too

Copy link

github-actions bot commented Nov 12, 2023

Test Results

  95 files  ±  0    95 suites  ±0   3h 48m 8s ⏱️ + 7m 57s
595 tests ±  0  583 ✔️ +  1  1 💤 ±0  11  - 1 
629 runs   - 13  584 ✔️  - 12  1 💤 ±0  44  - 1 

For more details on these failures, see this check.

Results for commit 3448898. ± Comparison against base commit 683a866.

This pull request removes 24 and adds 13 tests. Note that renamed tests count towards both.

 '
Fabric-Loom-Remap: broken
Fabric-Loom-Remap: false
Fabric-Loom-Remap: true
Something: Hello
], #1]
], #2]
], #3]
], #4]
…
net.fabricmc.loom.test.integration.UnpickTest ‑ unpick build [version: 8.3, #1]
net.fabricmc.loom.test.integration.UnpickTest ‑ unpick build [version: 8.5-20230908221250+0000, #0]
net.fabricmc.loom.test.integration.UnpickTest ‑ unpick decompile [version: 8.3, #1]
net.fabricmc.loom.test.integration.UnpickTest ‑ unpick decompile [version: 8.5-20230908221250+0000, #0]
net.fabricmc.loom.test.unit.ArtifactMetadataTest ‑ Should Remap [shouldRemap: false, entries: [fabric.mod.json:{}, META-INF/MANIFEST.MF:Manifest-Version: 1.0
Fabric-Loom-Remap: false

], #2]
net.fabricmc.loom.test.unit.ArtifactMetadataTest ‑ Should Remap [shouldRemap: false, entries: [hello.json:{}, META-INF/MANIFEST.MF:Manifest-Version: 1.0
Fabric-Loom-Remap: broken

], #6]
net.fabricmc.loom.test.unit.ArtifactMetadataTest ‑ Should Remap [shouldRemap: false, entries: [hello.json:{}, META-INF/MANIFEST.MF:Manifest-Version: 1.0
Fabric-Loom-Remap: false

], #4]
net.fabricmc.loom.test.unit.ArtifactMetadataTest ‑ Should Remap [shouldRemap: false, entries: [hello.json:{}, META-INF/MANIFEST.MF:Manifest-Version: 1.0
Something: Hello

], #7]
net.fabricmc.loom.test.unit.ArtifactMetadataTest ‑ Should Remap [shouldRemap: true, entries: [fabric.mod.json:{}, META-INF/MANIFEST.MF:Manifest-Version: 1.0
Fabric-Loom-Remap: true

], #3]
net.fabricmc.loom.test.unit.ArtifactMetadataTest ‑ Should Remap [shouldRemap: true, entries: [hello.json:{}, META-INF/MANIFEST.MF:Manifest-Version: 1.0
Fabric-Loom-Remap: true

], #5]
…

♻️ This comment has been updated with latest results.

@shedaniel shedaniel merged commit e3b51e9 into dev/1.4 Nov 13, 2023
179 of 195 checks passed
@shedaniel shedaniel deleted the fix/issue_162 branch November 13, 2023 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Forge This issue is specific to the Forge platform. priority: medium This issue or PR should be worked on soon-ish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants