Fix Quilt Loader dependency override #55
Conversation
| @@ -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? | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Well, you have to add it conditionally, otherwise doing so loader-side becomes impossible. getCapabilities would be the way to go.
| + // 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); | ||
| + a.because("use quilt loader over fabric loader"); |
There was a problem hiding this comment.
Could calling because without any selections cause any issues?
There was a problem hiding this comment.
From what I read from the API docs, no because there were no selections after all
There was a problem hiding this comment.
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.
| + // 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Tada! No more hacky workarounds for nuking Fabric Loader dependencies!