QuiltMC / rfcs

Repository for requests for comments for proposing changes to the Quilt Project.
Other
61 stars 33 forks source link

RFC 0056: Permit multiple version constraints in a single version specifier. #56

Closed AlexIIL closed 1 year ago

AlexIIL commented 2 years ago

The quilt.mod.json specification currently only permits a single version constraint pattern per version specifier, which means you can only use a single version comparison per dependency object. While a dependency object's versions field permits an array instead of a single string, it is compared using "ANY" logic - so you could depend on (for example) exactly minecraft 1.18.1, or exactly 1.18.2 ("versions": [ "=1.18.1", "=1.18.2" ]).

Where this stops being useful is when you want a larger version range, for example between minecraft 1.14 and 1.17 - a naive way of writing this would be "versions": [ ">=1.14", "<1.17" ]. Unfortunately this doesn't work, since that actually depends on any version of minecraft. The current way around this is to use two nearly identical dependency objects - for example:

"depends": [
  {
    "id": "minecraft",
    "versions": ">=1.14"
  },
  {
    "id": "minecraft",
    "versions": "<1.17"
  }
]

However the above format is quite long, and would be even worse if additional fields are used in the dependency object, since they would have to be added to both.

This RFC proposes a more clear syntax for "ANY" vs "ALL" behavior for version specifiers:

"depends": [
  {
    "id": "minecraft",
    "versions": { "all": [">=1.14", "<1.17"] }
  }
]

An additional change is that quilt-loader will validate version ranges to ensure they make sense - so accidentally depending on "any version" without using *, or accidentally disallowing every version will be caught early. (Since we have the breaks object to handle this).

Risks

Failing to load quilt mods with invalid version specifiers may break existing quilt mods that have invalid or redundant specifiers, which is unfortunate. However I'd rather we broke them now (and informed them that their version specifiers don't work like how they expect) than accidentally allow invalid/redundant versions now and in the future.

A previous version of this PR included logic to break mods which used the old format. Since mods have been around for a long time by now, and I don't want to force everyone to use dependency overrides just to make their mods work, this PR no longer breaks mods which might not be specified correctly.

AlexIIL commented 2 years ago

Sidenote 1: this will require us to (at least partially) break quilt-loader's API to implement. However I think it's a useful change, and few mods probably use quilt's dependency apis yet (if any), so it shouldn't hurt too much.

Sidenote 2: I looked through the relevant RFCs before and didn't see anything about not wanting this functionality. If I missed a discussion though please send it to me.

AlexIIL commented 2 years ago

After a brief discussion on discord I'm going to re-do this using more explicit language, rather than having arrays have a different meaning to separated strings.

lukebemish commented 1 year ago

Any updates on what this RFC needs or where it could be taken up? A way to specify version constraint intersections is currently a bit of a limitation in the quilt.mod.json format; is there a discussion that was had about where this should go or a PR that supersedes this?

AlexIIL commented 1 year ago

I've finally updated this to use any and all directly, rather than arrays vs spaces in separators. As such this is ready for review again!

gdude2002 commented 1 year ago

Before I review this - how does this PR tie in to #64? It seems you make that change in this PR as well, so it doesn't make sense to have a second PR with the same change.

AlexIIL commented 1 year ago

I consider this "good enough" to be moved to Final Comment Period, which will end in 4 days on June 14th

This obsoletes #64.