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.

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

Better version overrides in Netkan #3265

Merged
merged 1 commit into from
Jan 14, 2021

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Jan 13, 2021

Problems

See KSP-CKAN/NetKAN#8291; if mod authors only track compatibility on SpaceDock (i.e. don't use a KSP-AVC version file), then the handy prompt to update compatibility when a new version is released just ends up changing the single compatible game version, so that for example compatibility with 1.11 is gained while compatibility with 1.10 is lost, when they may be expecting the total compatibility with prior versions to be remembered over the long term. All of @KSPSnark's mods have been subject to this forever, see KSP-CKAN/NetKAN#6629 and KSP-CKAN/NetKAN#6639 for the last time we tried wrestling with it.

The most natural thing is to add this:

    "x_netkan_override": [ {
        "version": "1.10",
        "override": {
            "ksp_version_min": "1.8"
        }
    } ],

But that currently errors out:

2194 [1] FATAL CKAN.NetKAN.Program (null) - ksp_version mixed with ksp_version_(min|max)

Cause

SpaceDock's compatibility goes into ksp_version and nowhere else. The override would need to delete that property, because it's not valid to mix ksp_version_min with ksp_version, but then the useful info it contained would be lost, and we would be back to 100% manual maintenance for compatibility of such mods rather than pulling author-generated data from SpaceDock.

Background

Our AVC processing code has some pretty good logic for combining and overriding version ranges (which is part of why version files are so useful). Essentially it calculates an overall minimum and overall maximum from two input ranges, and then figures out the right metadata to represent the resulting range. This logic has usefulness for more than just AVC handling.

Changes

  • Now AvcTransformer.ApplyVersions is moved to ModuleService (except for the handling of x_netkan_trust_version_file which only makes sense for AVC), and calling code is updated to use the new location
  • Now VersionedOverrideTransformer uses special logic to handle overrides of the compatibility properties; instead of just setting them blindly (and often generating invalid metadata), it passes them to ModuleService.ApplyVersions

After these changes, the above override works as intended:

    "ksp_version_min": "1.8",
    "ksp_version_max": "1.11.0",

This will allow us to fix the compatibility of @KSPSnark's mods with just some slight additional manual maintenance, while still taking advantage of the author-generated data from SpaceDock.

This is the first part of fixing KSP-CKAN/NetKAN#8291 (the next part will be updating the netkans for the affected mods).

@HebaruSan HebaruSan added Enhancement New features or functionality Pull request Netkan Issues affecting the netkan data labels Jan 13, 2021
@HebaruSan HebaruSan requested a review from DasSkelett January 13, 2021 22:37
@HebaruSan

This comment has been minimized.

@HebaruSan

This comment has been minimized.

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

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

Reasonable improvement, gives us more power over compatibility data while still making use of the author-provided data. Thanks!

@HebaruSan HebaruSan merged commit 94fad32 into KSP-CKAN:master Jan 14, 2021
@HebaruSan
Copy link
Member Author

#3263 looks to have worked properly.

@HebaruSan HebaruSan deleted the fix/netkan-versioning branch January 14, 2021 19:54
@HebaruSan
Copy link
Member Author

HebaruSan commented Jan 14, 2021

HotSpot broke because it has many overrides that match and assumes they will overwrite each other, not expand compatibility as with AVC (and this PR):

Will probably send a PR for that metanetkan to make the overrides only match their individual ranges.

PlaneMode, too:

Luckily there probably aren't many users still on KSP 1.2.2 anymore to be affected by this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New features or functionality Netkan Issues affecting the netkan data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants