Closed netkan-bot closed 9 years ago
netkan-bot
commented
9 years ago 
in a terminal try ckan install InfernalRobotics=0.21.2 that'll solve it temporarily but yes the problem lies with the initial v which we can't do anything about except ask the mod author to keep a consisten versioning schema.
Maybe @erendrake can be of some assistance in this?
netkan-bot
commented
9 years ago 
C# has the System.Text.RegularExpressions namespace for such things ( https://msdn.microsoft.com/en-us/library/system.text.regularexpressions.regex(v=vs.110).aspx )
Just have it ignore all leading characters that aren't numbers, and break apart the string at each period. Then it's a matter of converting the tokens to numbers, and comparing each set, and stopping once you've determined one version number to be greater than the other.
It's something that CKAN should just make a call on as far as version processing, and write it into the spec, as having users run a command line command when they are usign the GUI isn't going to work, because they GUI is just going to update it back to v0.21 , which is a bad user experience.
Edit: I had a look into the code, and the right fix for this is to change the dictionary<string, mod> that stores all of the available mods in CKAN-CORE to a dictionary<version, mod> where version implements IComparable, does the comparison smartly, has a constructor that takes a string as an argument, and serializes to a normalized string form (which shears the preceding v that's causing so many issues).
Alternatively, some can go in by hand and modify the version numbers for the problematic mods, which will probably take less work in the short term, but be a continual problem in the long term.
netkan-bot
commented
9 years ago 
It's something that CKAN should just make a call on as far as version processing, and write it into the spec,
It has been in the spec since September 2014. By the spec "v0" is sorted before "0". It comes down to "v" vs "" via lexicographic order which has empty strings coming earlier.
The "correct" way to solve this via the spec is a epoch number which is used when the version numbering changes or a mistake has been made.
netkan-bot
commented
9 years ago 
@Dazpoet we were talking about this on the team today because this is a bummer. if we remove the versions from kerbal stuff will they get removed from CKAN?
netkan-bot
commented
9 years ago
@RichardLake That's not what I'm seeing in CKAN-Core when running CKAN-GUI in a VS 2013 debugging session.
So we're on the same page, I setup the project by forking both CKAN-GUI and CKAN-CORE, opening CKAN-GUI's SLN, removing the test project, and the DLL reference to CKAN-CORE, then adding CKAN-CORE's project to CKAN-GUI's, so I have full source access to everything within CKAN-GUI (and related projects), without having to stare at .NET disassembly - which is completely useless for what we're trying to do.
The core problem I'm seeing is not with the Epoch being converted correctly (which it actually is in the case of Infernal Robotics), but it's parsing the rest of the string, which should be rewritten to take advantage of the .NET String.Split() functionality to break the version string into sections by the "." char.
The problem lies with how you do the version comparison, which is in Version.cs => Version.CompareTo(), as it keeps treating things like "v0.21" to be greater than other versions like "0.21.1" and "0.21.2" because of how you're doing the string compare, and V will always be greater than a number because of how string sorting works.
In any case, here's how you fix the Infernal Robotics version comparison problem, by rewriting this method in Version.cs in CKAN-CORE.
public int CompareTo(Version that)
{
if (that.epoch == epoch && that.version == version)
{
return 0;
}
// Compare epochs first.
if (epoch < that.epoch)
{
return -1;
}
else if (epoch > that.epoch)
{
return 1;
}
// Epochs are the same. Do the dance described in
// https://github.com/KSP-CKAN/CKAN/blob/master/Spec.md#version-ordering
// new stuff from here down
string[] thisVersion =
version.Contains(".")
? version.Split('.')
: new string[] { "" }
;
string[] thatVersion =
that.version.Contains(".")
? that.version.Split('.')
: new string[] { "" }
;
// catches the 1.2.3 and 1.2 comparison differences
int maxSize =
thisVersion.Length < thatVersion.Length
? thisVersion.Length
: thatVersion.Length
;
int retVal;
int thisFragInt, thatFragInt;
// ingore first since that's the epoch
for (int x = 1; x < maxSize; x++)
{
// attempt int conversion
if (int.TryParse(thisVersion[x], out thisFragInt) && int.TryParse(thatVersion[x], out thatFragInt))
{
// check if equal, if not return
if (thisFragInt > thatFragInt) { return 1; }
else if (thisFragInt < thatFragInt) { return -1; }
}
else
{
// not ints, toss exception and default to string compare
retVal = String.Compare(thisVersion[x], thatVersion[x]);
if (retVal != 0) { return retVal; }
}
}
// whover's got the most segments wins
if (thisVersion.Length < thatVersion.Length) { return -1; }
else if (thisVersion.Length > thatVersion.Length) { return 1; }
else { return 0; }
}
@Almagnus1 My apologies. In the second sentence of my last comment I swapped the strings. Corrected it should be
By the spec "0" is sorted before "v0". It comes down to "" vs "v" via lexicographic order which has empty strings coming earlier.
Hence the code having "v0.21" > "0.21.2" is correct behaviour. The comment about epoch numbers was to suggest a way to fix the metadata, not a suggestion that they are responsible for this issue.
netkan-bot
commented
9 years ago
@RichardLake Then the spec doesn't make sense, and should be amended to follow what a human would naturally see as the newer version.
I'm gonna put the above into a pull request for the core, because it does resolve the Infernal Robotics issue, and CKAN-GUI also loads a bit faster from what I can tell - which is because the current method has several method calls in it, which is slowing down how fast CKAN's doing the compares, as it's hitting that method several hundred, if not a thousand, times before it loads the GUI.
netkan-bot
commented
9 years ago
The pull request has been completed, which is https://github.com/KSP-CKAN/CKAN-core/pull/109
The code that I wrote is short circuiting, so as soon as it can be determined that one version string is greater than another, it will stop comparing.
As CKAN-CORE currently does, it first compares the epochs, and if the epochs are equal, then it will look at the rest of the version string, starting by splitting the strings into an array of strings using '.' as the character to split the version string. If there is no '.' in the string, it will assign "" to the string array as the only entry. It also retains the original check to see if both strings are equal before attempting to process the version string.
It then steps through each segment of the version number, until it has stepped through the second to whichever string array has the fewest entries. It will attempt to convert the numbers to integers and do an int comparison on them (so 12 will always be greater than 2), and if one of the numbers cannot be converted to an integer, it will do a string compare on that section of the version number.
If both sections are common (like what we're seeing with the Infernal Robotics version strings), then whoever has the most segments (most strings in the string array from the split) will be determined to be the "newer" version, so a 0.21.1 will always be considered greater than a 0.21.
If after the above the two version strings have still not been determined to be different, it will declare them to be equal. This way a 0.021.1 will match a 0.21.1.
The end result is that this approach solves the issue Infernal Robotics is having, and basically avoids the issue if another modder does the same thing in the future.
netkan-bot
commented
9 years ago 
I'm going to be that terrible person who says we're not going to change the spec because a mod has changed its versioning at some point. I am happy with any or all of the following:
v to the version number. This would prevent almost every error we've seen to date caused by a change in versioning format¹.ckan.exe upgrade InfernalRobotics=0.21.2 command-line syntax after grabbing the new metadata, or they can uninstall and then re-install IR, but all future users and updates should work fine³.Of these, the first two are great for long-term error prevention, and the third gets things working right now. So I suggest we do all of them.
They're also all much faster than changing the spec. If any of the 600+ existing modules fail to compare correctly under the proposed changes, then the only clean way of updating is for the code is to check the spec_version in the metadata and pick either the historical or new version comparison routine based upon that. I guarantee that will introduce more issues than it solves.
¹ It wouldn't have caught the time when RealChutes turned everyone's parachutes into bacon. But let's face it, that was the most awesome error report I've ever received. ² It's also because our spec is a subset of the Debian spec, and they've had a long time dealing with hard problems so we don't have to. ³ Apparently the GUI can struggle if a mod is installed which isn't in the upstream metadata. This is a bug and needs fixing if it still exists. The core and cmdline client handle the situation fine.
netkan-bot
commented
9 years ago
This is a situation where it should make sense to a human, so the following two version numbers should be treated as identical:
v0.21 0.21
And in situations where us humans choose a different version number than CKAN, like:
v0.21 0.21.2
The one the humans chose (which would be 0.21.2) should be the one that CKAN also chooses.
Just because it's based off of the Debian spec, doesn't mean the CKAN spec must maintain 100% compliance to the debian spec.
And in all cases, CKAN should be doing what the humans would do, not following some arcane spec because it's what this other open source project did. That's basically saying "we should follow corporate policy because it's corporate policy and must be followed".
To get back to changes to the code pull request, all that needs to happen is tweaking the epoch processing (which is unchanged from what CKAN currently uses), and then deciding if a different overload of the String.Split() method is needed, should there be need to use more delimiters than just '.'
netkan-bot
commented
9 years ago
Edit: Off topic. Post is at https://gist.github.com/RichardLake/d70fb80bf21b069626bf
netkan-bot
commented
9 years ago
I would contend that beside the first, fifth, and sixth version numbers, everything should have been caught long before it was indexed.
For those, 0.1beta and 1:0 for the fifth and sixth because the epoch is 1. Everything else gets string compared because it's breaking spec.
Speaking of which, let's get back to the core issue here:
CKAN has been allowing version numbers that do not match your own spec to be indexed, and that has created this entire mess. Infernal Robotics isn't the first one to break spec, it just happens to be the most high profile.
Citing your own spec:
The mod_version may contain only alphanumerics and the characters . + (full stop, plus) and should start with a digit.
Alphanumerics, going by the Debian definition, are A-Za-z0-9 only.
What's the point of having a spec if you are allowing mods to be indexed that aren't actually following the spec?
netkan-bot
commented
9 years ago
You are correct. I have been getting off topic.
The version data is coming form the KerbalStuff integration - I'm not sure if this can currently be fixed via the ".netkan". Would manually adding a ".ckan" to the meta-data repo cause issues with the NetKan bot?
The mod_version may contain only alphanumerics and the characters . + (full stop, plus) and should start with a digit.
This is a difference in interpretation. I take the "should" to mean that a version still starting without a digit is still to spec. As such "v0.21.2" is a valid mod version string.
netkan-bot
commented
9 years ago
IMO, the preceding 'v' should be something that's just accepted as something that's just there, and pulled from the string if it's detected when determining version or epoch.
Epoch detection can also be written a bit smarter, as it really should be using a string.split instead of a regex, then checking string split size to see if it's exactly 2.
Maybe it's a good idea to add functionality to Version so that it knows if it's spec compliant or not (ignoring the leading 'v', that is)
Then have a rule in processing where compliant always wins, so it gives CKAN something to point at when things aren't updating.
I also want to take a profiler to CKAN GUI, as I'm suspecting there's probably a fair amount of performance that can be improved upon in CKAN-CORE, which will generally lead to a better user experience for everyone.
Edit: Touching on the should in the spec, version should be adjusted to always start with so a valid version would look like: [v?][epoch?][version]
netkan-bot
commented
9 years ago 
This same issue has struck RasterPropMonitor and RasterPropMonitor-Core.
netkan-bot
commented
9 years ago
@erendrake removing older versions from KS will not remove them from CKAN, the latter must be done manually .
Just removing v from older files might affect people using those versions? If someone could do a manual test that'd be awesome!
I'll try and read up on this problem tomorrow and see if I can find a solution.
netkan-bot
commented
9 years ago
Maybe it's time to start enforcing the spec, starting with visual flagging of the version numbers if they are compliant or not, like turning the version number red in the CKAN client if it's non compliant without actually changing the logic. This way you can give the mod authors a major KSP release or two in place before implementing any change to how version numbers are being processed (with the exception of the leading 'v' which started this issue).
@pjf : I've taken a look through most of the version numbers on CKAN, and about 60% (or better) of the versions numbers visible in CKAN are matching the regex of:
^[v]?[\d]+([.]?[\da-zA-Z]+)*$
While I may have missed this, I'm not seeing anything using an epoch.
As the current spec isn't being enforced currently, maybe it's time to consider using the de facto standard of what most people are doing with version numbers and just run with that.
I did do a pull request last night, that focuses on validating that the version number actually meets spec, and then parsing the version as described above (this time catch all of it). It was killed when I had realized that the forks had diverged, so I haven't had time to make the changes purely additive, rather than having changes that would replace code that's already there - which is something I like to avoid doing before hitting the release code pruning where commended out code gets removed.
While it does implement the spec changes I have mentioned previously, it looks to be a bit faster (no submethod calls is the most likely reason), which has given me about a second or so of faster load time on an SSD on my i7.
Once I get home after work (so I have more time to look at it than just lunch), I'll be able to bring it current with the changes that have occurred within Version.cs since the last pull request.
netkan-bot
commented
9 years ago
Ok, new pull request that uses defines to toggle between old and new comparison logic, which is https://github.com/KSP-CKAN/CKAN-core/pull/121
It also adds in a way to validate the version upon checking it, although it's currently hardwired to this regex:
^[v]?([\d][:])?[\d]+(.?[\da-zA-Z]+)*$
That's easily adjustable, as it's one of the fields near the top of the file.
That can be adjusted to whatever validation regex you want to use, as the current regex you are using does not check the version if it's valid, which is why we end up with things in CKAN that don't meet spec.
The new version processing is a solution that (once again) solves all of the version problems that has been brought up by the community, optimizes processing of the version numbers based off of what the majority of version numbers in CKAN looks like, and does it in less lines of code (once the older processing stuff is removed from the file, that is), and does it faster than the current processing.
netkan-bot
commented
9 years ago
It's worth noting that the new netkan pre-processor now supports the x_netkan_force_v field, which if set to true will add a v to the start of any version it indexes if it's not there already. This not only fixes things for the vast majority of cases where upstream sources have seen variability of versioning numbers. It doesn't require any new client releases, spec changes, and has no impact on mods we index where the feature is turned off. I'll be adding a similar field for prepending epochs, as this covers the situation where version numbers go backwards or sideways in all other circumstances (we have a boatload of these around April Fool's).
netkan-bot
commented
9 years ago
If that's the case, then it basically fixes the issue we've seen with Infernal Robotics (and the other mods), so then the discussion is largely academic at this point, rather than needing to get something fixed right now.
One thing I want to touch on is the difference of the regex patterns that both comparison methods are using, which are:
Live: ^(?:(?< epoch >[0-9]+):)?(?< version >.*)$
My last pull request: ^[v]?([\d][:])?[\d]+(.?[\da-zA-Z]+)*$
On closer inspection, that epoch segment should be "([\d]+[:])?" Hooray for programming after work late at night >.> Moving on....
The actual thing I want to point out is the version validation, or lack thereof in the currently used version regex, which is one of the reasons why there's such compliance issues. The version should know if it's valid or not.
That's one of the reasons why there's non-compliant version strings in CKAN.
This leads into the next question: What should the validation regex pattern be?
Personally, I'd rather go with what I'm using above (with the above tweak >.> ), as it acknowledges what most of the 300 someodd mods on CKAN are currently doing for version, while also dealing with that infernal leading v.
If I need to go rewrite what I've have submitted so it can match a tighter validation regex, I'll go do that, as I'd rather get this right and move on to another issue.
netkan-bot
commented
9 years ago
One more thought:
Should epochs that aren't present be set to -1? This always ensures that a version with an actual epoch of 0 will be considered to be greater than a version that doesn't have an epoc.
Also: Should recognizing '+' not be part of the spec? I can alter behavior so that '+' will be treated as a delimiter when strings are split, assuming there aren't multiple '.' or '+' clumped together in the version string.
pjf
commented
9 years ago I apologize that I've been quiet in the discussion here. While there's been extensive discussion, the fact is that changing how we compare versions comes with great risk (see #963), both to breaking comparisons between extant releases (of which we index thousands). Directives to our indexing bots can take care of the common cases, and I suggest we continue along this vein as the lower risk alternative should future problems arise.
Having said that, to answer specific questions raised in the thread:
Should epochs that aren't present be set to -1?
Nope, because when people bump the epoch, they always start by bumping it to 1.
The actual thing I want to point out is the version validation, or lack thereof in the currently used version regex, which is one of the reasons why there's such compliance issues. The version should know if it's valid or not.
This bothers me too. We're encumbered by the fact that mod authors will write versions in all sorts of crazy ways. Obviously we have been accommodating them, and declaring a whole bunch of extant releases as invalid wouldn't be good for our users, or our mod authors.
We probably should update the spec to be a little more lenient on what can be in a version string to reflect reality. I think it would make sense to have the netkan indexer and related systems issue warnings if a newly indexed version compares as being older than an already existing version of the same mod. I've opened #969 to track this.
@Almagnus1, thank you for all the effort you've put into this, and I apologise that we're not merging your version changes. We'd still very much like you to continue to contribute to the CKAN as you feel best able.
~ Paul
My KSP-AVC told me that a new version of Infernal Robotics is available. I went to check Ckan, but there was no version available. Normally i check for the CKAN-Meta commits to see if something interesting has been updated. So i did this time.
I saw that Infernal Robotics 21.2 has been commited. Why am I not getting this update in my client?
I have downloaded the master.zip and saw that the .ckan files are in there (maybe something with cache?). I also made a copy of my repo list, available mods and installed mods. See this link for the files.
Same goes for RasterPropMonitor 19.2.2.
Edit: I see that some of the .ckan files contain a v. InfernalRobotics-v0.21.ckan vs InfernalRobotics-0.21.2.ckan
Edit 2 Installing the .ckan file directly doesn't seem to help either. It will still download the previous version, even tough the .ckan file specifies the correct download link.