Skip to content

Select jar configuration based on which sides are present #1182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 30, 2024
Merged

Select jar configuration based on which sides are present #1182

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6d3d54
auto select jar configuration based on which sides are present
SpaceWalkerRS Sep 30, 2024
8ea0a10
move logic into jar configuration property convention
SpaceWalkerRS Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/main/java/net/fabricmc/loom/configuration/CompileConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,16 @@ private synchronized void setupMinecraft(ConfigContext configContext) throws Exc

var jarConfiguration = extension.getMinecraftJarConfiguration().get();

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// based on the mc version and which sides are present for it
if (!metadataProvider.getVersionMeta().downloads().containsKey("server")) {
jarConfiguration = MinecraftJarConfiguration.CLIENT_ONLY;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently the MetadataProvider is only available through the MinecraftProvider, it'd require adding that to the extension to use it in the configuration provider

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already is part of the internal extenstion, you have the project, so you can use LoomGradleExtension.get to get the internal one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright I moved it into the convention. It throws an error if the metadata provider is accessed to soon as well.

} else if (!metadataProvider.getVersionMeta().downloads().containsKey("client")) {
jarConfiguration = MinecraftJarConfiguration.SERVER_ONLY;
} else if (!metadataProvider.getVersionMeta().isVersionOrNewer(Constants.RELEASE_TIME_1_3)) {
jarConfiguration = MinecraftJarConfiguration.LEGACY_MERGED;
}
}

// Provide the vanilla mc jars
Expand Down