Rework mod discovery, resolution and loading #461

sfPlayer1 · 2021-06-29T04:12:24Z

changes

the input mod candidates are now allowed to have duplicates, Loader will select the most recent compatible version among the mods directory contents, any nested mods and other sources
nested mods won't be loaded unless the encompassing mod is also loaded
nested mods no longer get added before they are determined to be active
mod resolution errors are somewhat improved, "empty clause" and similar should be gone
mod discovery handles nested mods more efficiently
metadata parsing rewinds the gson reader instead of re-reading the raw file
mod resolution supports load conditions now
mod resolution biases towards including recommended mods
mod resolution determines the immediate failure cause, the minimal set of broken dependencies overall and the best solution to fix the problem (which mods to add/remove/replace)
mod selection priorities take the nesting level into account if the versions are identical
nested mods use regular files in .fabric/processedMods instead of jimfs
mod resolution is now compatible with vanilla sat4j
the mod dependency api carries the dependency kind instead of 5 kind specific getters
removed practically unused and subpar n.f.loader.api.VersionPredicate
added new predicate api, including parsing, evaluation and inspection
migration from URL to Path for mod origins
moved metadata checking to metadata parsing
mod load order changed due to implementation changes, but this is undefined territory anyway
the mod order will be randomized in-dev
new system property -Dfabric.debug.loadLate=modA,modB,.. to move mods to the end of the load order for debugging/workaround purposes
new system property -Dfabric.debug.throwDirectly to disable exception gathering while invoking entry points etc and throw the originally encountered exception directly (helps with misleading console output while debugging)

associated future changes

make use of the extra resolution failure data
better mod resolution errors and display in general
expose mod load conditions (api+json)
negative bias from conflicts declarations
sat4j shading

src/main/java/net/fabricmc/loader/impl/discovery/Explanation.java

haykam821 · 2021-06-29T04:32:37Z

src/main/java/net/fabricmc/loader/impl/util/version/VersionBoundsImpl.java

+	@Override
+	public String toString() {
+		if (min == null) {
+			if (max == null) {
+				return "(-∞,∞)";
+			} else {
+				return String.format("(-∞,%s%c", max, maxInclusive ? ']' : ')');
+			}
+		} else if (max == null) {
+			return String.format("%c%s,∞)", minInclusive ? '[' : '(', min);
+		} else {
+			return String.format("%c%s,%s%c", minInclusive ? '[' : '(', min, max, maxInclusive ? ']' : ')');
+		}
+	}


I don't think it's a good idea to use mathematical notation for a toString implementation. I'd avoid using toString for user-readable information, and even then, this notation in general is not user-friendly.

This isn't supposed to be interpreted by the user either, IMO it does a fair job for its main applications (devs working on version analysis similar to my installable mod inference code).

I would say +inf is better than +∞, just in case some people's computer is not UTF-8 encoding.

Java should convert it properly, I'd rather have a more compact format for this niche thing than not.

liach · 2021-06-29T06:45:58Z

src/main/resources/fabric-installer.json

-      {
-        "name": "com.google.jimfs:jimfs:1.2-fabric",
-        "url": "https://maven.fabricmc.net/"
-      },


Does the new ow2 sat4j need to be shipped though?

The plan is to shade it, so not relevant for the installer.

liach · 2021-06-29T06:46:51Z

These changes are too big. I think the better way to review is to just look at the source code without looking at the diff.

sfPlayer1 · 2021-06-29T08:34:58Z

These changes are too big. I think the better way to review is to just look at the source code without looking at the diff.

Most of the n.f.loader.discovery classes are practically rewritten, checking the diff for them is indeed rather pointless

modmuss50

Looks good, too much to do an indpeth review but looks a lot more approachable overall.

src/main/java/net/fabricmc/loader/impl/FabricLoaderImpl.java

src/main/java/net/fabricmc/loader/impl/discovery/ModCandidate.java

Chocohead · 2021-06-30T01:22:23Z

src/main/java/net/fabricmc/loader/api/Version.java


 /**
 * Represents a version of a mod.
 *
 * @see ModMetadata#getVersion()
 */
-public interface Version {
+public interface Version extends Comparable<Version> {


Is there any concern here from potential existing implementations which wouldn't implement Comparable? I can't think of any but it is technically a backward compatibility problem

Hmm, are users even supposed to implement this interface?

They shouldn't need to, but that hasn't stopped people doing odd things with internals in the past

This isn't really supported and somewhat pointless (can't put that Version anywhere in loader). The JVM doesn't verify whether a class implements everything, so would only throw AbstractMethodError on access.

src/main/java/net/fabricmc/loader/impl/discovery/ModResolver.java

Chocohead · 2021-06-30T01:52:11Z

src/main/java/net/fabricmc/loader/impl/discovery/ModSolver.java

+		solverPrepTime = System.nanoTime();
+
+		IPBSolver solver = SolverFactory.newDefaultOptimizer();
+		solver.setTimeout(60); // 60s


Should it ever be taking close to a minute to resolve mod dependencies?

Should be <1s even for a large modpack, but doesn't hurt to leave some room. I went for 60s because it's the most I'd want to wait. It actually usually recovers itself when the limit is hit, just proceeding with a potentially non-optimal result.

Chocohead · 2021-06-30T01:55:49Z

src/main/java/net/fabricmc/loader/impl/discovery/ModSolver.java

+				int[] comps = ((SemanticVersionImpl) v).getVersionComponents();
+
+				if (pr == null) { // no prerelease, use large pr segment
+					pr = "zzzzzzzz";


Would this be fine if someone used a prerelease of zzzzzzzzz (ie with one more z)?

If said mod is present, the condition a line above fails and selects another code path, otherwise the least mod version suggested to be installed will be slightly off.

src/main/java/net/fabricmc/loader/impl/discovery/ResultAnalyzer.java

Chocohead · 2021-06-30T02:04:07Z

src/main/java/net/fabricmc/loader/impl/discovery/ResultAnalyzer.java

+	}
+
+	private static String getCandidateFriendlyVersion(ModCandidate candidate) {
+		return candidate.getVersion().getFriendlyString();


Will (the use of) this result in errors like The developer(s) of x_mod have found that version * of y_mod critically conflicts with their mod.? If so, where the version is wildcarded could it be made clearer along the lines of The developer(s) of x_mod have found that y_mod critically conflicts with their mod., so that it is more apparent there is no version which could ever work?

I don't think so, ModCandidate is always a concrete mod, not a dependency reference.

src/main/java/net/fabricmc/loader/impl/discovery/ResultAnalyzer.java

src/main/java/net/fabricmc/loader/api/metadata/ModMetadata.java

haykam821 · 2021-06-30T05:40:03Z

src/main/java/net/fabricmc/loader/api/metadata/ModMetadata.java

-	Collection<ModDependency> getDepends();
+	@Deprecated
+	default Collection<ModDependency> getDepends() {
+		return getDependencies().stream().filter(d -> d.getKind() == ModDependency.Kind.DEPENDS).collect(Collectors.toList());


There are five methods with the same implementation, except for their kind filter. I would create an abstract method that uses a parameter to dictate the kind filter instead.

Java 8 doesn't allow private interface methods, so can't factor it out without accessing the implementation or adding even more methods. These API methods are practically unused, so no need to add convenience helpers..

haykam821 · 2021-06-30T05:40:34Z

src/main/java/net/fabricmc/loader/api/metadata/ModDependency.java

-	Set<VersionPredicate> getVersionRequirements();
+	Collection<VersionPredicate> getVersionRequirements();
+
+	enum Kind {


I'm not a find of 'kind'. Would 'type', 'relation', or 'relationship' be better here?

"type" is too overloaded with other meanings in this concept, relation* is wrong since ModDependency itself is the relation. Not sure what's wrong with calling depends, recommends , ... kinds of dependencies.

haykam821 · 2021-06-30T05:42:35Z

src/main/java/net/fabricmc/loader/api/metadata/ModDependency.java

+		Kind(String key, boolean positive, boolean soft) {
+			this.key = key;
+			this.positive = positive;
+			this.soft = soft;


I'd document this better and give it a more meaningful name to clarify that dependency kinds that are soft only produce warnings rather than failing mod resolution.

it's to indicate it being a soft dependency (opposite of a hard dependency), whether it produces a warning is unspecified (suggests doesn't, recommends/conflicts do)

What is a hard dependency?

hard = must be met, soft = should be met

haykam821 · 2021-06-30T05:45:02Z

src/main/java/net/fabricmc/loader/api/metadata/version/VersionComparisonOperator.java

+import net.fabricmc.loader.api.Version;
+import net.fabricmc.loader.impl.util.version.SemanticVersionImpl;
+
+public enum VersionComparisonOperator {


The last two names clash with the others. I'd change the first four names so that they describe the functionality rather than the serialized form.

This is actually surprisingly hard, particularly because ~ and ^ have a lower bound that isn't properly covered by the current name. Suggestions?

I changed the description for ~ and ^. I tried adjusting the other names, but it causes more confusion than sticking with the operators. The class itself represents the operators in the first place and has nothing to do with the user friendly translation that happens in ReasonAnalyzer.

src/main/java/net/fabricmc/loader/impl/discovery/ModLoadCondition.java

haykam821 · 2021-06-30T05:56:30Z

src/main/java/net/fabricmc/loader/impl/discovery/ResultAnalyzer.java

+import net.fabricmc.loader.api.metadata.version.VersionPredicate;
+import net.fabricmc.loader.api.metadata.version.VersionPredicate.PredicateTerm;
+
+final class ResultAnalyzer {


It would be nice to have tests for this.

True, but not high priority. The class itself will likely see substantial changes, it is just a quick and dirty adaption of the old error generation logic.

src/main/java/net/fabricmc/loader/impl/discovery/ResultAnalyzer.java

haykam821 · 2021-06-30T05:58:58Z

src/main/java/net/fabricmc/loader/impl/game/minecraft/launchwrapper/FabricTweaker.java

@@ -167,8 +168,12 @@ public void injectIntoClassLoader(LaunchClassLoader launchClassLoader) {
 	}

 	@Override
-	public void propose(URL url) {
-		launchClassLoader.addURL(url);
+	public void addToClassPath(Path path) {


ClassPath should be Classpath to be consistent with the typical capitalization both within Fabric loader and in general.

Not sure about that actually, java started to separate it lately (--class-path instead of --classpath)..

src/main/java/net/fabricmc/loader/impl/metadata/DependencyOverrides.java

Technici4n · 2021-06-30T09:50:30Z

Looks generally good, however my loader knowledge is close to zero. I assume if there is an issue it will be easy to address.

Is some testing planned in case that happens?

sfPlayer1 · 2021-06-30T09:53:06Z

We want to do some manual testing with the Mixin changes before actually releasing 0.12, other than that there are only plans/ideas for making Loader more testable. I already did some local testing ofc.

Technici4n · 2021-06-30T09:53:48Z

Yes ofc, I was referring to automated or at least semi-automated testing.

liach · 2021-06-30T12:16:58Z

src/main/java/net/fabricmc/loader/impl/discovery/ModDiscoverer.java

-				ModCandidate candidate = new ModCandidate(i, normalizedUrl, depth, requiresRemap);
-				boolean added;
+			if (!metadata.loadsInEnvironment(envType)) {
+				return null;


Looking at #375, should we add a warning here? Though if a mod is not loaded, it won't appear in loader's mod list, and should be quite clear in that case.

The mod explicitly opted out of being loaded, doesn't look warning worthy to me?

Hmm, now I am thinking about using the 5-tier dependency system to handle the environment thing: a mod can declare "breaks" to strictly crash when loading on one side and "conflicts" for less-strict warning.

In fact, I think we can add an extra minecraft client builtin mod; the whole dedicated server is shipped within the client, so it will just count as part of minecraft builtin mod. If a mod opts-in to be client-only, it can declare a hard dependency on client mod; otherwise, it can declare a hard breaks on client (though I see no point for a mod to do so)

Hmm, @Chocohead what's the main problems with this?

Now, after some thinking, I feel such a split by using jij for client-specific code is actually a good approach, somewhat more feasible than adding @Environment annotations and having less compile-time guarantees

One would just declare the environment in the mod json instead of messing with dependencies

I don't think we should be encouraging/offering people to crash if their mod is on the wrong side. Right now we avoid loading it which is a much more user friendly approach to the problem. Forge in comparison expects mods to handle it themselves (unlike in the past with the old clientOnlyMod) but also doesn't offer a way to crash on the wrong side.

src/main/java/net/fabricmc/loader/api/metadata/version/VersionComparisonOperator.java

liach

Looks quite good in general. Hope you can commit the change that actually ships sat4j wholesale so people can build and test it more easily

haykam821 reviewed Jun 29, 2021

View reviewed changes

liach reviewed Jun 29, 2021

View reviewed changes

liach added the enhancement label Jun 29, 2021

liach requested review from modmuss50 and liach June 29, 2021 07:55

modmuss50 approved these changes Jun 29, 2021

View reviewed changes

src/main/java/net/fabricmc/loader/impl/FabricLoaderImpl.java Outdated Show resolved Hide resolved

src/main/java/net/fabricmc/loader/impl/discovery/ModCandidate.java Outdated Show resolved Hide resolved

Chocohead reviewed Jun 30, 2021

View reviewed changes

haykam821 reviewed Jun 30, 2021

View reviewed changes

sfPlayer1 added 3 commits June 30, 2021 11:41

Rework mod discovery, resolution and loading

8aefe3b

Implement PR feedback

436c4fc

More PR feedback

44cced7

sfPlayer1 force-pushed the master branch from ca3ab97 to 44cced7 Compare June 30, 2021 09:46

liach removed their request for review June 30, 2021 10:46

This was referenced Jun 30, 2021

Change mod resolving to use sat4j's partial boolean functionality rather than minisat. #279

Closed

Update JIMFS to 1.2 #396

Closed

Make small changes mandatory for mod dependency overrides. #363

Closed

liach reviewed Jun 30, 2021

View reviewed changes

liach mentioned this pull request Jun 30, 2021

Conditional dependencies #464

Open

Rename VersionBounds to VersionInterval

5552ca4

Chocohead reviewed Jun 30, 2021

View reviewed changes

src/main/java/net/fabricmc/loader/api/metadata/version/VersionComparisonOperator.java Outdated Show resolved Hide resolved

src/main/java/net/fabricmc/loader/api/metadata/version/VersionComparisonOperator.java Outdated Show resolved Hide resolved

Fix typo

f4cf8f0

liach approved these changes Jul 2, 2021

View reviewed changes

sfPlayer1 merged commit 8879128 into FabricMC:master Jul 3, 2021

EZForever mentioned this pull request Jul 24, 2021

Stuffs to do EZForever/ThatOrThis#6

Open

19 tasks

Rework mod discovery, resolution and loading #461

Rework mod discovery, resolution and loading #461

Conversation

sfPlayer1 commented Jun 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liach commented Jun 29, 2021

sfPlayer1 commented Jun 29, 2021

modmuss50 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Technici4n commented Jun 30, 2021

sfPlayer1 commented Jun 30, 2021

Technici4n commented Jun 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liach left a comment

Choose a reason for hiding this comment

sfPlayer1 commented Jun 29, 2021 •

edited