Fix support for Forge 50+ #227
Conversation
src/main/java/net/fabricmc/loom/configuration/providers/mappings/MappingConfiguration.java
Outdated
Show resolved
Hide resolved
| @@ -50,7 +50,7 @@ public abstract class MixinExtensionApiImpl implements MixinExtensionAPI { | |||
| public MixinExtensionApiImpl(Project project) { | |||
| this.project = Objects.requireNonNull(project); | |||
| this.useMixinAp = project.getObjects().property(Boolean.class) | |||
| .convention(project.provider(() -> !LoomGradleExtension.get(project).isNeoForge())); | |||
| .convention(project.provider(() -> !LoomGradleExtension.get(project).isNeoForge() && (!LoomGradleExtension.get(project).isForge() || !LoomGradleExtension.get(project).getForgeProvider().usesMojangAtRuntime()))); | |||
There was a problem hiding this comment.
Does the PR fix the crash bypassed by 8c2b72a?
There was a problem hiding this comment.
I never experienced any crash?
To get a crash here you would need to ref the property in your own buildscripts early, so imo just don't do it?
There was a problem hiding this comment.
I never experienced any crash? To get a crash here you would need to ref the property in your own buildscripts early, so imo just don't do it?
I think this crash was Architectury Plugin's fault, if so, the Arch Plugin fix should fix that problem if that was the cause.
There was a problem hiding this comment.
So the crash in that commit is caused by architectury plugin, this should just require a change in arch plugin (which is in a separate branch there)
I think that is a better solution than making this false for forge.
Why didn't this break unit tests?
src/main/java/net/fabricmc/loom/configuration/providers/forge/ForgeRunsProvider.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Could you add a known forge mod to the test to see if it gets remapped correctly?
There was a problem hiding this comment.
I can add it, but the tests won't be able to pick it up if it doesn't get remapped properly anyways, so I am not sure if it makes sense. I usually just have an external project.
Alternative to #225.