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

Migrate to official mappings #2311

Merged

Conversation

embeddedt
Copy link
Contributor

Goal

This PR migrates the Sodium codebase to use official mappings. The remapping was largely automated using Mercury & MercuryMixin, with some manual fixups afterwards to fix compile errors. As such, imports may have been shuffled around compared to trunk.

This work resolves #2277.

Renaming Sodium classes to use Mojmap-inspired names is not planned for this PR, as it's a separate job from compiling against Mojmap. Such renames would greatly complicate verifying correctness of the remapping (see below).

Testing

I have decompiled a built JAR from this commit and 0.5.8. All mod classes (that is, any non-@Mixin class) produce identical source files except NativeImageHelper (which is only different due to an accessor method name changing). Thus, extra manual verification should not be required for any of these.

Unfortunately, it appears that unlike Architectury's toolchain, Fabric Loom emits mapped names into mixin annotations. This means that all of the mixin classes have different bytecode, and my decompilation trick does not work to verify them. What I can say is that the mod appears completely functional in dev, so I am doubtful that there are any incorrectly remapped mixins.

@embeddedt
Copy link
Contributor Author

embeddedt commented Feb 3, 2024

Mixins have been validated using the following procedure:

  • Run the game with -Dmixin.debug.export=true and copy the contents of .minecraft/.mixin.out/class to separate directories for both 0.5.8 & this PR.
    You will also need to cause Mixin's auditing to run on startup and load all mixins. One option is to insert MixinEnvironment.getCurrentEnvironment().audit(); somewhere in the mod initialization process.
  • Decompile the classfiles by running Vineflower on each of the directories.
  • Use the following command to strip sessionId = from the decompiled mixin annotations to avoid false diffs: find sodium_upstream_decomp sodium_embedded_decomp -name \*.java | xargs sed -i '/sessionId = "/d' (replace directory names as needed)
  • Compare the two directories in your favorite diff tool (e.g. Meld), and validate that the final classes are identical aside from some accessor renames (which should be harmless).

Credit to @IThundxr for suggesting this approach.

@jellysquid3 jellysquid3 self-requested a review February 3, 2024 04:42
@jellysquid3 jellysquid3 added the A-refactor Area: Refactoring label Feb 3, 2024
Copy link
Member

@jellysquid3 jellysquid3 left a comment

Choose a reason for hiding this comment

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

Verified that everything is remapped correctly, including mixins.

@jellysquid3
Copy link
Member

We need to figure out how to migrate the branches for #2016 and #1851 to the official mappings before we can merge this.

@embeddedt
Copy link
Contributor Author

#2016 has been migrated, and #1851 is trivial enough to either merge the commit into or simply redo on top of the Mojmap tree.

@jellysquid3 jellysquid3 changed the base branch from dev to 1.20.4/0.5-multiloader February 8, 2024 06:11
@CaffeineMC CaffeineMC locked as resolved and limited conversation to collaborators Feb 8, 2024
@jellysquid3 jellysquid3 merged commit f6c0eea into CaffeineMC:1.20.4/0.5-multiloader Feb 8, 2024
1 check passed
@embeddedt embeddedt deleted the fabric/mojmap branch February 18, 2024 00:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-refactor Area: Refactoring
Development

Successfully merging this pull request may close these issues.

Migrate to Mojang ("official") mappings
2 participants