Output to hashed mappings #14
Conversation
Includes merging intermediary and hashed into one main mappings tree so that dependencies can be remapped when required.
|
|
||
| void setIntermediaryMinecraftProvider(IntermediaryMinecraftProvider<?> intermediaryMinecraftProvider); | ||
|
|
||
| + void setHashedMinecraftProvider(HashedMinecraftProvider<?> hashedinecraftProvider); |
There was a problem hiding this comment.
There's a typo of Minecraft in the parameter name
| + /** | ||
| + * Hashed mappings are a hashed version of the raw official mappings. | ||
| + * | ||
| + * @see <a href="https://github.com/QuiltMC/mappings-hasher/">github.com/QuiltMC/mappings-hasher/</a> |
There was a problem hiding this comment.
| + * @see <a href="https://github.com/QuiltMC/mappings-hasher/">github.com/QuiltMC/mappings-hasher/</a> | |
| + * @see <a href="https://github.com/QuiltMC/rfcs/blob/master/specification/0041-hashed-mappings-specification.md</a> |
| + private TinyRemapper createTinyRemapper(MappingsNamespace intermediateMappings) { | ||
| try { | ||
| - TinyRemapper tinyRemapper = TinyRemapperHelper.getTinyRemapper(project, "intermediary", "named"); | ||
| + TinyRemapper tinyRemapper = TinyRemapperHelper.getTinyRemapper(project, "hashed", "named"); |
There was a problem hiding this comment.
Shouldn't the namespace depend on which intermediateMappings you provided in the argument?
There was a problem hiding this comment.
Oh, yes, I might have left that in from debugging
| return false; | ||
| } | ||
|
|
||
| - if (!MappingsNamespace.INTERMEDIARY.toString().equals(mappings.getSrcNamespace())) { |
There was a problem hiding this comment.
Doesn't this break depending on mods with transitive AWs in Intermediary?
There was a problem hiding this comment.
The mapping tree passed is just the normal loom mapping provider but with the source namespace switched to hashed, it's not the namespace of the transitive AW.
There was a problem hiding this comment.
Wait you might be right actually, I'll have to do a little more digging.
| - for (Path minecraftJar : extension.getMinecraftJars(MappingsNamespace.INTERMEDIARY)) { | ||
| - remapper.readClassPathAsync(minecraftJar); |
There was a problem hiding this comment.
It looks like you never fill the intermediary remapper's classpath?
There was a problem hiding this comment.
Hmm, there was a reason I did this but I don't remember it, you're right it would probably make more sense to populate the remapper's classpath.
| + // Hashed is always the base layer | ||
| + builtLayers.add(new HashedMappingsSpec()); |
There was a problem hiding this comment.
This is okay as long as we're able to layer Intermediary on top of Hashed, for e.g. Quilted Fabric API
There was a problem hiding this comment.
Why would intermediary get layered on top of hashed? All the dependencies get remapped to named for dev and hashed in loader and intermediary is merged into the mapping tree regardless
| +import net.fabricmc.loom.util.SidedClassVisitor; | ||
| +import net.fabricmc.tinyremapper.TinyRemapper; | ||
| + | ||
| +public abstract sealed class HashedMinecraftProvider<M extends MinecraftProvider> extends AbstractMappedMinecraftProvider<M> permits HashedMinecraftProvider.MergedImpl, HashedMinecraftProvider.SingleJarImpl, HashedMinecraftProvider.SplitImpl { |
There was a problem hiding this comment.
I think in this case what you should do instead is modify the Intermediary minecraft provider to be extendable by hashed, and then only override the methods you actually have to change, since i believe most of this class is duplicated?
There was a problem hiding this comment.
That would be smarter yes lol, I'll do that now
There was a problem hiding this comment.
I honestly don't know if this is worth all the changes it would take, it would be hard to make this work with the mess of generics that is MinecraftJarConfiguration.
| + | ||
| + if (mappings.equals("net.fabricmc:intermediary")) return MappingsNamespace.INTERMEDIARY; | ||
| + | ||
| + return MappingsNamespace.HASHED; |
There was a problem hiding this comment.
This logic is unsound; it will assume any "intermediate_mappings" value besides fabric intermediary is hashed, but with loader plugins we can't promise that's the case
There was a problem hiding this comment.
That's true, I did this so it would fall back to hashed. I can instead have it explicitly check for hashed or an empty value (implicitly hashed) and have it throw an exception if the value's unknown if that's preferred.
|
This is what I get for messing with branch names lol. |
This pr adds support for hashed mappings within loom. Mods will now, by default, output with hashed mappings instead of intermediary. Additionally, all dependencies will be able to be remapped from either hashed or intermediary to named when in a dev environment.
This probably shouldn't be merged until loader has hashed support. Also, QuiltMC/sponge-mixin-compile-extensions#1 has to be merged and published on the maven for mixins to get remapped correctly.