Fix comparison of 1.0.0 to 1.0

[HebaruSan]

Problem

The bot says Protractor doesn't match any game versions:

Cause

This module has an interesting .version file:

  "KSP_VERSION": {
    "MAJOR": 1,
    "MINOR": 0,
    "PATCH": 2
  },
  "KSP_VERSION_MIN": {
    "MAJOR": 1,
    "MINOR": 0,
    "PATCH": 0
  },
  "KSP_VERSION_MAX": {
    "MAJOR": 1,
    "MINOR": 0,
    "PATCH": -1
  }

That -1 is supposed to mean unbounded, so all 1.0 versions are included.

Netkan translates this into:

    "ksp_version_min": "1.0.0",
    "ksp_version_max": "1.0",

Internally, CKAN models an undefined value as -1, and then blindly compares it to defined values here:

CKAN/Core/Versioning/KspVersion.cs

Lines 736 to 756 in cfa2893

	var majorCompare = _major.CompareTo(other._major);
	if (majorCompare == 0)
	{
	var minorCompare = _minor.CompareTo(other._minor);
	if (minorCompare == 0)
	{
	var patchCompare = _patch.CompareTo(other._patch);
	return patchCompare == 0 ? _build.CompareTo(other._build) : patchCompare;
	}
	else
	{
	return minorCompare;
	}
	}
	else
	{
	return majorCompare;
	}

Since -1 is less than 0, the range between 1.0.0 and 1.0 is incorrectly determined to be empty here:

CKAN/Core/Types/GameComparator/StrictGameComparator.cs

Lines 26 to 36 in cfa2893

	if (module.ksp_version_min <= module.ksp_version_max)
	{
	var minRange = module.ksp_version_min.ToVersionRange();
	var maxRange = module.ksp_version_max.ToVersionRange();
	moduleRange = new KspVersionRange(minRange.Lower, maxRange.Upper);
	}
	else
	{
	return false;
	}

And no game versions are permitted to match, even though some of them are greater or equal to 1.0.0 and less than or equal to 1.0.

Changes

Now the min <= max check uses KspVersionRange to determine whether it's looking at a non-empty interval rather than simply comparing the KspVersion objects, since the latter operation cannot return meaningful, unambiguous values for less-or-equal. This will allow such modules to match their game versions properly.

Considered and not done

We could have changed how version comparisons work when one has an undefined patch, but this would break many many tests, so I looked for another way.

[DasSkelett]

Uhm, now netkan translates it to:

    "ksp_version": "1.0.0",

[HebaruSan]

Yikes, that's not good...

EDIT: That happens here:

CKAN/Netkan/Transformers/AvcTransformer.cs

Lines 161 to 166 in cfa2893

	// If we have both a minimum and maximum KSP version...
	if (kspMin.CompareTo(kspMax) == 0)
	{
	// ...and they are equal, then just set ksp_version
	json["ksp_version"] = kspMin.ToString();
	}

This says, if the min is (numerically/logically) equal to the max, then put the min in ksp_version, otherwise set the _min and _max properties independently. The implication is that if CompareTo doesn't return 0, it must be returning -1 to signal that the min is less than the max, but in this case, it returns 1 because the min is greater than the max.

It's very tricky to try to represent both points and ranges with the same type.

[HebaruSan]

Luckily KspVersion also defines an Equals, which is more appropriate for that check. I get the original metadata generated for Protractor now.

[HebaruSan]

I need to do some software archaeology to figure out whether it's wise to update the failing tests...

EDIT: Rather than changing the core of the game version comparison logic, now we'll try a more modest change using KspVersionRange to improve that <= check. I still get Protractor to inflate without errors, but now hopefully those old tests will pass as well.

[DasSkelett]

That's much easier to review, thanks :)
Works and looks good!