Fix NeoForge data source cache keys#119
Open
mezz wants to merge 4 commits into
Open
Conversation
## Problem The cached binary-patch path (`no-recompile` path) could reuse patched Minecraft artifacts built from an older NeoForge userdev jar. `binaryPatch` keyed the clean Minecraft jar, binpatcher tool, and textual args, but not the userdev-embedded `patches.lzma` data that binpatcher applies to the clean jar. The same issue existed for access transformers: `transformSources` and `applyDevTransforms` recorded data source IDs, but not the userdev-backed data contents. Changing the NeoForge userdev jar must invalidate these cache keys even when Minecraft/NeoForm inputs and tool args are unchanged. ## Observed Failure NeoForge PR #3288 was released in NeoForge `26.2.0.10-beta`. It changed the `RenderArmEvent` proxy parameters and updated `ClientHooks.renderSpecificFirstPersonArm` to the new eight-argument signature: neoforged/NeoForge#3288 (comment) JEI then updated from NeoForge `26.2.0.6-beta`, which still used the old five-argument signature, to `26.2.0.11-beta`. CI reused cached `binaryPatch` and `applyDevTransforms` outputs while producing `26.2.0.11-beta` artifacts. The client recipe sync test loaded `minecraft-patched-26.2.0.11-beta.jar` with `neoforge-26.2.0.11-beta-universal.jar`, then crashed because the patched Minecraft jar still invoked the old `ClientHooks.renderSpecificFirstPersonArm` method. This first surfaced in JEI 26.2 build 63: https://ci.blamejared.com/job/mezz/job/jei/job/26.2/63/consoleText ## Fix Add userdev-backed data source hashes to the NeoForge-specific cached binary-patch actions. `binaryPatch` now keys on the binary patch data source, and `transformSources`/`applyDevTransforms` key on the access-transformer data sources, matching the generic NeoForm data-placeholder behavior. ## Tests The regression tests model pairs of NeoForge userdev jars with the same NeoForm config, binpatcher command, sources jar, and universal jar while changing either embedded binary patch data or access-transformer data. They assert that `binaryPatch`, `transformSources`, and `applyDevTransforms` cache keys change for their corresponding userdev data changes, and stay stable when the userdev contents are identical.
|
Member
|
I'm surprised that this was not done before. I thought the data source system should already have handled this. 🤔 |
Author
|
trying to do some git archareology to see some background on where the current state came from:
|
Author
|
So for example, the addition here: ck.addStrings("access transformers data ids", accessTransformersData);
ck.addPaths("additional access transformers", additionalAccessTransformers);aa4f177#diff-862bc71488e496a3e6ffd97c9dd6909d7b96628deba9deea2ca8d83212c58a69R74-R75 these look like they'd do the same thing but only the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The cached binary-patch path (
no-recompilepath) could reuse patched Minecraft artifacts built from an older NeoForge userdev jar.binaryPatchkeyed the clean Minecraft jar, binpatcher tool, and textual args, but not the userdev-embeddedpatches.lzmadata that binpatcher applies to the clean jar.The same issue existed for access transformers:
transformSourcesandapplyDevTransformsrecorded data source IDs, but not the userdev-backed data contents. Changing the NeoForge userdev jar must invalidate these cache keys even when Minecraft/NeoForm inputs and tool args are unchanged.Observed Failure
NeoForge PR #3288 was released in NeoForge
26.2.0.10-beta. It changed theRenderArmEventproxy parameters and updatedClientHooks.renderSpecificFirstPersonArmto the new eight-argument signature: neoforged/NeoForge#3288 (comment)JEI then updated from NeoForge
26.2.0.6-beta, which still used the old five-argument signature, to26.2.0.11-beta. CI reused cachedbinaryPatchandapplyDevTransformsoutputs while producing26.2.0.11-betaartifacts. The client recipe sync test loadedminecraft-patched-26.2.0.11-beta.jarwithneoforge-26.2.0.11-beta-universal.jar, then crashed because the patched Minecraft jar still invoked the oldClientHooks.renderSpecificFirstPersonArmmethod.This first surfaced in JEI 26.2 build 63:
https://ci.blamejared.com/job/mezz/job/jei/job/26.2/63/consoleText
Fix
Add userdev-backed data source hashes to the NeoForge-specific cached binary-patch actions.
binaryPatchnow keys on the binary patch data source, andtransformSources/applyDevTransformskey on the access-transformer data sources, matching the generic NeoForm data-placeholder behavior.Tests
The regression tests model pairs of NeoForge userdev jars with the same NeoForm config, binpatcher command, sources jar, and universal jar while changing either embedded binary patch data or access-transformer data.
They assert that
binaryPatch,transformSources, andapplyDevTransformscache keys change for their corresponding userdev data changes, and stay stable when the userdev contents are identical.