Select jar configuration based on which sides are present #1182
Conversation
| // if no configuration is selected by the user, attempt to select one | ||
| // based on the mc version and which sides are present for it | ||
| if (!metadataProvider.getVersionMeta().downloads().containsKey("server")) { | ||
| jarConfiguration = MinecraftJarConfiguration.CLIENT_ONLY; |
There was a problem hiding this comment.
No, this is wrong. It should set the value in the extension, otherwise all the other places like use this configuration will not be in agreement.
There was a problem hiding this comment.
It would likely be best to move all of this logic into the convension as a Provider. I do worry that the metadata might not have been populated if someone requests the value too soon though.
There was a problem hiding this comment.
currently the MetadataProvider is only available through the MinecraftProvider, it'd require adding that to the extension to use it in the configuration provider
There was a problem hiding this comment.
It already is part of the internal extenstion, you have the project, so you can use LoomGradleExtension.get to get the internal one.
There was a problem hiding this comment.
Alright I moved it into the convention. It throws an error if the metadata provider is accessed to soon as well.
| if (jarConfiguration == MinecraftJarConfiguration.MERGED && !metadataProvider.getVersionMeta().isVersionOrNewer(Constants.RELEASE_TIME_1_3)) { | ||
| jarConfiguration = MinecraftJarConfiguration.LEGACY_MERGED; | ||
| if (jarConfiguration == MinecraftJarConfiguration.MERGED) { | ||
| // if no configuration is selected by the user, attempt to select one |
There was a problem hiding this comment.
This isnt always stricly true, as a user could have requested a merged jar. I know its unlikely and changing this would be a much bigger change. Overall im not sure how much benefit this provides.
There was a problem hiding this comment.
How would the user select a merged jar? It's the default and there are API methods only for changing it to client-only or server-only. Regardless, if one side does not exist then the build will fail on the merger configuration anyway.
There was a problem hiding this comment.
The user could set the value of LoomGradleExtensionAPI.getMinecraftJarConfiguration(), I think its unlikely so I wouldnt worry too much about it.
if one side does not exist then the build will fail on the merger configuration anyway.
Yes, thats expected. I dont think its a lot to ask for someone to set this.
There was a problem hiding this comment.
I see, that's a fair point. It would be possible to leave the value empty instead of setting the Convention to merged, that way you'd know if the user selected a specific configuration or not.
I agree it's not a big deal to set this. It's a minor inconvenience but this is also a minor change to alleviate that so I thought it was fair.
src/main/java/net/fabricmc/loom/configuration/CompileConfiguration.java
Outdated
Show resolved
Hide resolved
|
Overall im not too keen on this, its impossible to know for sure what the user wants, and its not a lot to ask them to set it manually. If the error messages arent good enough it might be best to just improve thoes. |
2 participants
If the user does not specifically set the client-only or server-only configuration, then automatically configure this for versions where only one side exists. This does not happen at all for new versions anymore, but is common for Minecraft versions prior to Release 1.3, and the norm for versions prior to Beta 1.0.