Skip to content
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

Jump to bottom

Fix Quilt Loader dependency override #55

Merged
merged 3 commits into from
Oct 22, 2024
Merged

Fix Quilt Loader dependency override #55

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a250ee
Fix Quilt Loader dependency override
EnnuiL Oct 21, 2024
cd21b03
Readd 0.0.0+ to the version
EnnuiL Oct 21, 2024
8cda979
Replace "all" with "configureEach"
EnnuiL Oct 21, 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
51 changes: 15 additions & 36 deletions patches/0005-Prefer-Quilt-over-Fabric-Loader.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,66 +5,46 @@ Subject: [PATCH] Prefer Quilt over Fabric Loader


diff --git a/src/main/java/net/fabricmc/loom/LoomGradlePlugin.java b/src/main/java/net/fabricmc/loom/LoomGradlePlugin.java
index d19cd534d53871aa1f2618cf493b66a3676a6f4c..e5a694936cfc55e5b1f6b263441f5203a7355572 100644
index d19cd534d53871aa1f2618cf493b66a3676a6f4c..3d6cb4517a825338995efe1aef5be2feb2314f01 100644
--- a/src/main/java/net/fabricmc/loom/LoomGradlePlugin.java
+++ b/src/main/java/net/fabricmc/loom/LoomGradlePlugin.java
@@ -30,6 +30,9 @@ import java.util.Objects;
@@ -30,7 +30,11 @@ import java.util.Objects;
import com.google.common.collect.ImmutableMap;
import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
+
+import net.fabricmc.loom.util.FabricCapabilities;
+
import org.gradle.api.Project;
+import org.gradle.api.artifacts.component.ModuleComponentIdentifier;
import org.gradle.api.plugins.PluginAware;

@@ -83,6 +86,9 @@ public class LoomGradlePlugin implements BootstrappedPlugin {
import net.fabricmc.loom.api.LoomGradleExtensionAPI;
@@ -83,6 +87,18 @@ public class LoomGradlePlugin implements BootstrappedPlugin {
project.apply(ImmutableMap.of("plugin", "java-library"));
project.apply(ImmutableMap.of("plugin", "eclipse"));

+ // Tell Gradle that Quilt Loader/Fabric Loader are the same thing
+ project.getDependencies().getComponents().all(FabricCapabilities.class);
+
// Setup extensions
project.getExtensions().create(LoomGradleExtensionAPI.class, "loom", LoomGradleExtensionImpl.class, project, LoomFiles.create(project));
project.getExtensions().create("fabricApi", FabricApiExtension.class);
diff --git a/src/main/java/net/fabricmc/loom/configuration/mods/ModConfigurationRemapper.java b/src/main/java/net/fabricmc/loom/configuration/mods/ModConfigurationRemapper.java
index 009d91ecafc42640135d9ebf5c1eb06be323e115..ac9c0ac012c82a50cb95eb4a9ac8a77613ae255f 100644
--- a/src/main/java/net/fabricmc/loom/configuration/mods/ModConfigurationRemapper.java
+++ b/src/main/java/net/fabricmc/loom/configuration/mods/ModConfigurationRemapper.java
@@ -43,6 +43,7 @@ import org.gradle.api.artifacts.Configuration;
import org.gradle.api.artifacts.FileCollectionDependency;
import org.gradle.api.artifacts.MutableVersionConstraint;
import org.gradle.api.artifacts.ResolvedArtifact;
+import org.gradle.api.artifacts.component.ModuleComponentIdentifier;
import org.gradle.api.artifacts.dsl.DependencyHandler;
import org.gradle.api.artifacts.query.ArtifactResolutionQuery;
import org.gradle.api.artifacts.result.ArtifactResult;
@@ -131,6 +132,18 @@ public class ModConfigurationRemapper {
configsToRemap.put(entry.getSourceConfiguration().get(), remappedConfig);
}
}
+ // Round 0: Constraints
+ // TODO: this is probably the wrong place for this
+ // Explain to Gradle how to handle conflicts
+ for (Configuration c : configsToRemap.keySet()) {
+ project.getConfigurations().configureEach(c -> {
+ // this mirrors how it's done at https://docs.gradle.org/current/userguide/dependency_capability_conflict.html#sub:capabilities for gradle 8.10.2
+ var rs = c.getResolutionStrategy().getCapabilitiesResolution();
+ rs.withCapability("net.fabricmc:fabric-loader", a -> {
+ a.getCandidates().stream().filter(id -> id instanceof ModuleComponentIdentifier && ((ModuleComponentIdentifier) id).getModule().equals("quilt-loader"))
+ a.getCandidates().stream().filter(id -> id.getId() instanceof ModuleComponentIdentifier moduleId && moduleId.getModule().equals("quilt-loader"))
+ .findFirst().ifPresent(a::select);
Copy link

Choose a reason for hiding this comment

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

Couldn't this cause an older version of qloader to be selected?
But either way, this should handle the case when the version of the transitive floader dependency is newer than the floader version the newest qloader version in the list provides, failing the build in that case. If the user wants to ignore the transitive dependency's requests, that should be done explicitly using an exclude, not implicitly.
I think once the TODO mentioned in the other review is fixed, just using selectHighestVersion should do the trick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but like we have talked, you want the user-chosen Quilt Loader to supercede all transitives Quilt Loaders, although this is on the implicit exclusion zone; Maybe your approach would be technically better (and you have presented decent solutions like the GradleX plugin), but I don't see reasons why we should be more hostile to the user in the process when other toolchains have just completely hidden the problem on the first place;

While I'd maybe be fine in an alternate universe where Quilt was more dominant (mainly because mods would be more willing to fix their deps)? We don't live there; right now, we are burdened by an ecosystem that does not want to adapt towards us; so yeah.. explicit excluding (and encouragement to not have transient deps) won't happen as far as I know

Copy link

@kb-1000 kb-1000 Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We didn't talk about that part specifically, no. Without any customization, the standard Gradle mechanisms do exactly that: use the latest version of the candidates that fulfills all requirements. It doesn't care about which of those the user specifies. (well, perhaps that should also be a failure scenario... buuut yeah)
Maven prefers what the user specifies... by preferring whatever is closest to the root of resolution, IIRC, completely disregarding everything else like a dependency deeper down wanting Foo 9.6 instead of Foo 1.2 (it should be obvious by now that Maven is not a good source of ideas)
So imagine this: I have a Fabric project depending on Loader 0.6 or something, and add a dependency on one depending on 0.15 and actually using newly added APIs. Gradle will silently resolve 0.15, and it'll work (although perhaps not as intended).
I now have a Quilt project depending on Quilt Loader 0.16 (which is ofc older) and add a dependency on that project. As implemented, it'll silently ignore the Fabric Loader dependency, and silently use Quilt Loader 0.16 - which will break.
Ignoring dependencies like that should always be conscious and explicit decisions by the user.
Oh and, the order of elements in that list is undefined.

+ a.because("use quilt loader over fabric loader");
Copy link

Choose a reason for hiding this comment

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

Could calling because without any selections cause any issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I read from the API docs, no because there were no selections after all

Copy link

Choose a reason for hiding this comment

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

The javadoc isn't clear about this, so it's safe to say it's undefined what happens. It's an easy fix, either way, just turn the method reference into a lambda and move the because call inside.

+ });
+ }

// Round 1: Discovery
// Go through all the configs to find artifacts to remap and
+ });
+
// Setup extensions
project.getExtensions().create(LoomGradleExtensionAPI.class, "loom", LoomGradleExtensionImpl.class, project, LoomFiles.create(project));
project.getExtensions().create("fabricApi", FabricApiExtension.class);
diff --git a/src/main/java/net/fabricmc/loom/util/FabricCapabilities.java b/src/main/java/net/fabricmc/loom/util/FabricCapabilities.java
new file mode 100644
index 0000000000000000000000000000000000000000..a0b77bedca33b58cc7112002dae4527cc5aa61c1
index 0000000000000000000000000000000000000000..f4dc3b7a9de753ac12b38aa4903fe342ecc71370
--- /dev/null
+++ b/src/main/java/net/fabricmc/loom/util/FabricCapabilities.java
@@ -0,0 +1,43 @@
@@ -0,0 +1,42 @@
+/*
+ * This file is part of fabric-loom, licensed under the MIT License (MIT).
+ *
Expand Down Expand Up @@ -102,8 +82,7 @@ index 0000000000000000000000000000000000000000..a0b77bedca33b58cc7112002dae4527c
+ var id = componentMetadataContext.getDetails().getId();
+ if (id.getGroup().equals("org.quiltmc") && id.getName().equals("quilt-loader")) {
+ componentMetadataContext.getDetails().allVariants(
+ // TODO: actually provide the version here?
Copy link

Choose a reason for hiding this comment

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

This TODO probably shouldn't have been removed, since this isn't fixed. Once loader adds a capability to its component metadata, a possible approach might be a hardcoded version range list in loom or somewhere on the meta or maven server for versions older than that.
This would currently fail when loader adds the capability, too, see https://docs.gradle.org/8.10/javadoc/org/gradle/api/capabilities/MutableCapabilitiesMetadata.html#addCapability(java.lang.String,java.lang.String,java.lang.String)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly? I do wonder if you can add the capability conditionally; but yeah, don't worry about it; we do have plans to tackle it Loader-side; I do worry about the possibility of hardcoding such a version range list, but oh well

Copy link

Choose a reason for hiding this comment

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

Well, you have to add it conditionally, otherwise doing so loader-side becomes impossible. getCapabilities would be the way to go.

+ v -> v.withCapabilities(c -> c.addCapability("org.fabricmc", "fabric.loader", "0.0.0+" + id.getVersion()))
+ v -> v.withCapabilities(c -> c.addCapability("net.fabricmc", "fabric-loader", "0.0.0+" + id.getVersion()))
+ );
+ }
+ }
Expand Down
Loading