Loader erroneously thinks that a version number string is empty in fabric.mod.json

[Leonerd-04]

Loader version: 0.13.2

This issue happened to me when a mod dependency in my fabric.mod.json had a version number written like so:
"<dependency>": ">= <version>"
with a space between the ">=" and the dependency version number, causing the loader to read the string as empty and crash on running the game. The issue wasn't present without the space. The issue also wasn't present with 0.11.7, which is what I'd updated from when I found this issue.

Here's a full stack trace:

net.fabricmc.loader.impl.FormattedException: net.fabricmc.loader.impl.discovery.ModResolutionException: Mod discovery failed! at net.fabricmc.loader.impl.FabricLoaderImpl.load(FabricLoaderImpl.java:189) at net.fabricmc.loader.impl.launch.knot.Knot.init(Knot.java:142) at net.fabricmc.loader.impl.launch.knot.Knot.launch(Knot.java:71) at net.fabricmc.loader.launch.knot.KnotClient.main(KnotClient.java:28) at net.fabricmc.devlaunchinjector.Main.main(Main.java:86) Caused by: net.fabricmc.loader.impl.discovery.ModResolutionException: Mod discovery failed! at net.fabricmc.loader.impl.discovery.ModDiscoverer.lambda$discoverMods$1(ModDiscoverer.java:122) at net.fabricmc.loader.impl.util.ExceptionUtil.gatherExceptions(ExceptionUtil.java:33) at net.fabricmc.loader.impl.discovery.ModDiscoverer.discoverMods(ModDiscoverer.java:122) at net.fabricmc.loader.impl.FabricLoaderImpl.setup(FabricLoaderImpl.java:204) at net.fabricmc.loader.impl.FabricLoaderImpl.load(FabricLoaderImpl.java:187) ... 4 more Caused by: net.fabricmc.loader.impl.metadata.ParseMetadataException: Error reading fabric.mod.json file for mod at [My mod's directory]\build\resources\main: net.fabricmc.loader.api.VersionParsingException: Version must be a non-empty string! at net.fabricmc.loader.impl.metadata.V1ModMetadataParser.readDependenciesContainer(V1ModMetadataParser.java:471) at net.fabricmc.loader.impl.metadata.V1ModMetadataParser.parse(V1ModMetadataParser.java:151) at net.fabricmc.loader.impl.metadata.ModMetadataParser.readModMetadata(ModMetadataParser.java:160) at net.fabricmc.loader.impl.metadata.ModMetadataParser.readModMetadata(ModMetadataParser.java:123) at net.fabricmc.loader.impl.metadata.ModMetadataParser.parseMetadata(ModMetadataParser.java:52) at net.fabricmc.loader.impl.discovery.ModDiscoverer$ModScanTask.computeDir(ModDiscoverer.java:230) at net.fabricmc.loader.impl.discovery.ModDiscoverer$ModScanTask.compute(ModDiscoverer.java:211) at net.fabricmc.loader.impl.discovery.ModDiscoverer$ModScanTask.compute(ModDiscoverer.java:175) at java.base/java.util.concurrent.RecursiveTask.exec(RecursiveTask.java:100) at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373) at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182) at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655) at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622) at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165) Caused by: net.fabricmc.loader.api.VersionParsingException: Version must be a non-empty string! at net.fabricmc.loader.impl.util.version.VersionParser.parse(VersionParser.java:26) at net.fabricmc.loader.impl.util.version.VersionPredicateParser.parse(VersionPredicateParser.java:58) at net.fabricmc.loader.impl.util.version.VersionPredicateParser.parse(VersionPredicateParser.java:105) at net.fabricmc.loader.api.metadata.version.VersionPredicate.parse(VersionPredicate.java:51) at net.fabricmc.loader.impl.metadata.ModDependencyImpl.<init>(ModDependencyImpl.java:39) at net.fabricmc.loader.impl.metadata.V1ModMetadataParser.readDependenciesContainer(V1ModMetadataParser.java:469) ... 13 more

[Juuxel]

Something that complicates this a bit is that version strings consist of multiple parts separated by spaces (and each part has to match). I'm not sure what the intended behaviour would be with something like >= 2.0.0 - from Loader 0.12's POV, there probably are two parts, >= (the broken part) and 2.0.0 (an exact match). (which is what matches the fabric.mod.json documentation)

[sfPlayer1]

It is not a normal expression, but a list of predicates with prefixes to transform the adjacent version into an interval. Spaces separate individual predicates as per the above comment. Considering the whole file is parsed strictly I'd consider the old behavior an oversight and not ignoring the space the correct behavior. I believe there are only very rare cases of mods having a space there and users can patch the mod file in the worst case, so IMO it is better to keep the new behavior.

The error message is still bad, so please don't close this issue until we've fixed that.

[haykam821]

Keep in mind that semver parses >= 1.0.0 as >=1.0.0, matching Fabric loader's old behavior:

const { validRange } = require("semver");
validRange(">= 1.0.0") // returns '>=1.0.0' rather than null

Even if the new strict behavior is kept for Fabric loader in manifest version 1, I think that semantic version and range parsing should be standardized to match semver's behavior in manifest version 2.

[sfPlayer1]

I don't quite see a reason to copy what a random Javascript library does? To me the extra space is clearly inferior as it would allow something like 1.2.3 < 1.3 which doesn't do what it looks like in a slightly but significantly sneakier way than 1.2.3 <1.3.

[haykam821]

Calling semver a random JavaScript library is disingenuous at best when it has 186 million weekly downloads and documented behavior. Semantic versioning should be checked through these well-defined ranges rather than a confusing implementation that is exclusive to Fabric loader.

The case you mentioned is parsed as 1.2.3 <1.3.0-0 in semver. This can be understood as an intersection of the ranges 1.2.3 and <1.3.0-0, which does not make sense as the entire range can be simplified to just 1.2.3.

[modmuss50]

I have a vuage recollection that that JS Library (the one used by Node/NPM) was used as somewhat of a baseline for the version parsing/comparison. That doesnt mean we should copy it 1-1 however.

I do think loader's existing behaviour is better, but as this was technically a breaking change between 0.11 and 0.12 should the old behaviour be added back, and then removed in a v2 fabric.mod.json format?

[Juuxel]

I have a vuage recollection that that JS Library (the one used by Node/NPM) was used as somewhat of a baseline for the version parsing/comparison.

Yep, the original fabric.mod.json spec (that's still only stored in the wiki as my pr #353 went nowhere) says this:

For semantic versions, the specification follows a rough subset of the NPM semver specification, in particular the following features are supported...