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

Add findDuplicateMappingFiles task #617

Merged
merged 7 commits into from
Jul 31, 2024

Conversation

supersaiyansubtlety
Copy link
Contributor

@supersaiyansubtlety supersaiyansubtlety commented Jul 23, 2024

Adds findDuplicateMappingFiles gradle task that checks for mapping files that map the same class.

Useful when merging/rebasing.
It just matches the first line of each file against "^CLASS net/minecraft/unmapped/C_\\w+" and if it finds any files with the same one it throws an exception listing them.

edit: it matches against this now:

"^CLASS (?:net/minecraft|com/mojang/blaze3d)/(?:\\w+/)*\\w+(?= )"

Also makes mappingLint depend on findDuplicateMappingFiles.

Edit: also removes current duplicate mapping files so the build run can complete.

@supersaiyansubtlety supersaiyansubtlety added enhancement new feature or request s: small PRs with less than 200 lines labels Jul 23, 2024
@supersaiyansubtlety supersaiyansubtlety self-assigned this Jul 23, 2024
@supersaiyansubtlety
Copy link
Contributor Author

supersaiyansubtlety commented Jul 23, 2024

Output looks like this (these are actual duplicates currently on the 1.21 branch):
edit: see new logging below

> Found classes mapped by multiple files!
      CLASS net/minecraft/unmapped/C_zjsuejaf is mapped by:
          C:\Users\super\Projects\Minecraft\Modding\Quilt\quilt-mappings\mappings\net\minecraft\datafixer\fix\AttributeIdFix.mapping
          C:\Users\super\Projects\Minecraft\Modding\Quilt\quilt-mappings\mappings\net\minecraft\datafixer\fix\AttributeModifierIdFix.mapping
      CLASS net/minecraft/unmapped/C_dxtlxcwy is mapped by:
          C:\Users\super\Projects\Minecraft\Modding\Quilt\quilt-mappings\mappings\net\minecraft\datafixer\fix\RemoveEmptyItemInSuspiciousBlockFix.mapping
          C:\Users\super\Projects\Minecraft\Modding\Quilt\quilt-mappings\mappings\net\minecraft\datafixer\fix\RemoveEmptyItemInBrushableBlockFix.mapping

@supersaiyansubtlety
Copy link
Contributor Author

It would also be trivial to add checking for empty files, files that don't match the regex, and files that don't have the mapping extension.
Do we care about those?

@supersaiyansubtlety
Copy link
Contributor Author

supersaiyansubtlety commented Jul 23, 2024

Went ahead and added those checks.
It also checks non-obfuscated classes now and the logging is more similar to MappingLintTask.
The new logging:

Found 1 class mapped by multiple files!
	CLASS net/minecraft/unmapped/C_xyxorzsk is mapped by:
		C:\Users\super\Projects\Minecraft\Modding\Quilt\quilt-mappings\mappings\net\minecraft\WorldVersion.mapping
		C:\Users\super\Projects\Minecraft\Modding\Quilt\quilt-mappings\mappings\net\minecraft\WorldVersion2.mapping2
Found 1 empty file!
	C:\Users\super\Projects\Minecraft\Modding\Quilt\quilt-mappings\mappings\net\minecraft\WorldVersionEMPTY.mapping3
Found 1 file not mapping a class!
	C:\Users\super\Projects\Minecraft\Modding\Quilt\quilt-mappings\mappings\net\minecraft\WorldVersionNO_CLASS.mapping
Found 2 files without the mapping extension!
	C:\Users\super\Projects\Minecraft\Modding\Quilt\quilt-mappings\mappings\net\minecraft\WorldVersion2.mapping2
	C:\Users\super\Projects\Minecraft\Modding\Quilt\quilt-mappings\mappings\net\minecraft\WorldVersionEMPTY.mapping3

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':findDuplicateMappingFiles'.
> Found 1 class mapped by multiple files, 1 empty file, 1 file not mapping a class, and 2 files without the mapping extension! See the log for details.

@supersaiyansubtlety supersaiyansubtlety added the reviews needed please review this PR label Jul 23, 2024
@supersaiyansubtlety
Copy link
Contributor Author

private static final Pattern MINECRAFT_CLASS = Pattern.compile("^CLASS net/minecraft/(?:\\w+/)*\\w+(?= )");
private static final Pattern BLAZE_CLASS = Pattern.compile("^CLASS com/mojang/blaze3d/(?:\\w+/)*\\w+(?= )");

these could be generalized to

private static final Pattern ANY_CLASS = Pattern.compile("^CLASS (?:\\w+/)+\\w+(?= )");

idk if we want to allow classes outside of net.minecraft and com.mojang.blaze3d though

@OroArmor
Copy link
Member

Until a good reason comes, I think disallowing anything outside of those two root packages is a good idea.

Copy link
Member

@ix0rai ix0rai left a comment

Choose a reason for hiding this comment

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

fantastic work!

@ix0rai ix0rai added the t: toolchain changes to the quilt mappings toolchain label Jul 24, 2024
Copy link
Member

@OroArmor OroArmor left a comment

Choose a reason for hiding this comment

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

can we have the check task depend on this like the MappingsLint task?

@supersaiyansubtlety
Copy link
Contributor Author

can we have the check task depend on this like the MappingsLint task?

Sure; instead of MappingsLint's dependency on findDuplicateMappingFiles, or in addition to it?

@OroArmor
Copy link
Member

Oh! I missed that! Looks good!

@OroArmor OroArmor added the final-comment-period is approved and will soon be merged if no issues are raised label Jul 24, 2024
@OroArmor
Copy link
Member

@ix0rai with this, do we want to enforce that all PRs are up to date before merging (like QSL)? it'll make merging multiple PRs in a row annoying, but will make sure that this is caught in a PR and not once 3 branches are merged at once

@ix0rai
Copy link
Member

ix0rai commented Jul 25, 2024

@ix0rai with this, do we want to enforce that all PRs are up to date before merging (like QSL)? it'll make merging multiple PRs in a row annoying, but will make sure that this is caught in a PR and not once 3 branches are merged at once

I think we should, it's probably possible to automate checking them using actions and merge queue

@OroArmor OroArmor merged commit 71d7aed into QuiltMC:1.21 Jul 31, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new feature or request final-comment-period is approved and will soon be merged if no issues are raised reviews needed please review this PR s: small PRs with less than 200 lines t: toolchain changes to the quilt mappings toolchain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants