Make fabric.mod.json path configurable at configuration stage.
#1364
Conversation
| @Nullable | ||
| public static FabricModJson createFromSourceSetsNullable(Project project, SourceSet... sourceSets) throws IOException { | ||
| final File file = SourceSetHelper.findFirstFileInResource(FABRIC_MOD_JSON, project, sourceSets); | ||
| final RegularFileProperty fmjPath = project.getExtensions().getByType(LoomGradleExtensionAPI.class).getFabricModJsonPath(); |
There was a problem hiding this comment.
I dont think this right place to do this, FabricModJsonHelpers.getModsInProject is the place for it.
There was a problem hiding this comment.
What exactly? My implementation was based on the fact that the createFromSourceSetsNullable method is called from the Loom and Fabric API extensions (LoomGradleExtensionApiImpl.getModVersion and FabricApiAbstractSourceSet.configureSourceSet), so using the property here would provide the desired file to those too.
|
A thing to consider in a next commit is the way to deal with fmj used in the datagen source set. Currently it still uses the old method, but it would also be helpful to be able to specify its path. |
src/main/java/net/fabricmc/loom/configuration/fabricapi/FabricApiAbstractSourceSet.java
Outdated
Show resolved
Hide resolved
| * Returns the list of mods provided by either {@code overrideFile} property | ||
| * or {@code fabric.mod.json} in the {@code sourceSets} array. | ||
| */ | ||
| public static List<FabricModJson> getModsInProject(Project project, Provider<File> overrideFile, SourceSet... sourceSets) { |
There was a problem hiding this comment.
Why is this needed? Can you just not always get it from the extension? I dont get why this is needed in the fabricapi extension?
There was a problem hiding this comment.
Im happy with the API design and rough implementation. However it really needs some proper thought about what is a fatal error and what isnt. The null handling is a bit of a mess here, its not clear what can be null and what isnt.
I think what we really want is that if a user specifies a custom FMJ path, it should be a non null file that exists, and is a valid FMJ. Aything other than that should cause a hard crash.
Also please make sure to run checkstyle / spotless (there is an intelij plugin that makes it easy to see what is wrong). If you run gradlew build -x test locally you can verify that it will build on CI. (Dont try to run all the tests locally otherwise you will be waiting all day)
src/main/java/net/fabricmc/loom/util/fmj/FabricModJsonFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/net/fabricmc/loom/configuration/fabricapi/FabricApiAbstractSourceSet.java
Show resolved
Hide resolved
src/main/java/net/fabricmc/loom/util/fmj/FabricModJsonFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/net/fabricmc/loom/util/fmj/FabricModJsonFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/net/fabricmc/loom/api/fabricapi/DataGenerationSettings.java
Outdated
Show resolved
Hide resolved
src/main/java/net/fabricmc/loom/api/fabricapi/DataGenerationSettings.java
Outdated
Show resolved
Hide resolved
src/main/java/net/fabricmc/loom/util/fmj/FabricModJsonHelpers.java
Outdated
Show resolved
Hide resolved
src/main/java/net/fabricmc/loom/util/fmj/FabricModJsonHelpers.java
Outdated
Show resolved
Hide resolved
Not my issue 🤷
|
It was, I fixed it by running |
| */ | ||
| public static List<FabricModJson> getModsInProject(Project project) { | ||
| final LoomGradleExtension extension = LoomGradleExtension.get(project); | ||
| var overrideFile = extension.getFabricModJsonPath().getAsFile(); |
There was a problem hiding this comment.
FYI: I see you like to use var wherever possible, this isnt something we tend to do. In other fabric projects we disallow var other than for new instance creataion. e.g var a = new Object() is fine as you can clearly see the type. Loom doesnt enforce this as there are some places where gradle forces you to use an obtuse type, but for simple types such a File in this case I wouldnt choose to use var as it makes the code harder to read.
Dont worry about it for this PR its just a minor nit pick.
There was a problem hiding this comment.
Thanks for pointing out. I used var for consistency with the other cases I've seen in the area, but I can replace it if needed.
There was a problem hiding this comment.
Ive done it :) I also moved the tests to one of the existing files, this is becuase each test file gets a new github actions job, there was nothing wrong with having its own file but it will be a tiny bit quicker. Im happy with this PR now :) Thanks.
| import net.fabricmc.loom.task.GenerateSourcesTask; | ||
| import net.fabricmc.loom.util.DeprecationHelper; | ||
| import net.fabricmc.loom.util.MirrorUtil; | ||
| import net.fabricmc.loom.util.fmj.FabricModJson; |
|
You two are the best <3 |
3 participants
Motivation
Multi-version mods often have a shared source directory, which is then processed to generated sources for each subproject. Currently Loom expects
fabric.mod.jsonto be insrc/main/resources(or client)` of the project's folder.In multi-version projects, this file is located in the shared source, and due to the hardcoded path, Loom doesn't recognize it, leading to issues with using injected interfaces.
Implementation
This PR adds
fabricModJsonPathproperty to the Loom extension. If specified that file is used, otherwise falling back to the existing behaviour.