Select compatible KSP versions #1957

grzegrzk · 2016-12-12T21:06:05Z

Hi,

I have modified CKAN so user can select which versions of KSP are compatible which his installation (for example user can select that all mods compatible with 1.2.* should be available for him/her).

In my opinion it is especially important now when Squad is releasing frequent small updates that do not break compatibility of majority of mods.

From the user perspective it looks like this: http://imgur.com/vqXRVoI

Changes in code:

I have introduced KspVersionCriteria that is used in many cases where KspVersion was used. Usually logic is not changed, because eventually KspVersion was just passed down to the GameComparator without touching.
KspVersionCriteria stores all game versions that should be considered compatible.
I have updated GameComparator implementations to use KspVersionCriteria. Also, now user can specify game version like "1.2" (e.g omitting patch/build version) and it will be interpreted as "install all mods compatible with 1.2.*"
New menu item is introduced: Compatible KSP Versions. When it is selected new dialog box is opened.
Compatible game versions selected by user are stored in CKAN folder as .json file.
When KSP is updated user is forced to review stored compatible versions of game.

I hope it will be useful for more people than just me :).

…jor KSP versions

… no longer displayed there

…during startup if KSP has bean updated in the meantime

…code

grzegrzk · 2016-12-12T21:18:35Z

I can see that continuous integration is failing but I am not sure what can be the cause. It says:

System.IO.DirectoryNotFoundException: Could not find a part of the path "/home/travis/build/KSP-CKAN/CKAN/build/Tests/bin/Debug/CKAN.Tests.dll".

I can successfully build CKAN tests project. Are there any more details available of the possible cause?

politas · 2016-12-13T10:56:16Z

Note: checking out 'grzegrzk/master'.
...
HEAD is now at 7d31e12... Corrected StrictGameComparator, corrected tests, removed unnecessary code

$ make test
bin/build
XBuild Engine Version 14.0
Mono, Version 4.6.2.0
Copyright (C) 2005-2013 Various Mono authors
Configuration: Debug Platform: AnyCPU
Configuration: Debug Platform: AnyCPU
Configuration: Debug Platform: AnyCPU
/usr/lib/mono/xbuild/14.0/bin/Microsoft.Common.targets: warning : Reference 'Autofac, Version=3.5.0.0, Culture=neutral, PublicKeyToken=17863af14b0044da' not resolved
CompatibleKspVersionsDialog.cs(4,7): error CS0246: The type or namespace name Autofac' could not be found. Are you missing an assembly reference? Main.Designer.cs(1541,44): warning CS0108: CKAN.Main.Update' hides inherited member `System.Windows.Forms.Control.Update()'. Use the new keyword if hiding was intended
/usr/lib/mono/4.5-api/System.Windows.Forms.dll (Location of the symbol related to previous warning)
Configuration: Debug Platform: AnyCPU
"xbuild" unexpectedly returned exit value 1 at (eval 8) line 12.
at bin/build line 2177
Makefile:8: recipe for target 'build' failed
make: *** [build] Error 1

Doesn't like Autofac - looks like we haven't used it in the CKAN-GUI space previously, so you'll need to add the reference.

I like the sound of this update. Something that lets users break their version compatibility without destroying the main system is a great idea! What does it currently do if the (actual) version of KSP is upgraded between CKAN loads?

techman83 · 2016-12-13T12:34:58Z

I like the sound of this update. Something that lets users break their version compatibility without destroying the main system is a great idea! What does it currently do if the (actual) version of KSP is upgraded between CKAN loads?

Don't we already have that functionality? yo-yo mode etc, just no GUI for it.

grzegrzk · 2016-12-13T15:33:53Z

Doesn't like Autofac - looks like we haven't used it in the CKAN-GUI space previously, so you'll need to add the reference.

Thanks for the hint, I have corrected it.

I like the sound of this update. Something that lets users break their version compatibility without destroying the main system is a great idea!

Good to hear - I was missing this feature for some time and I hoped it is in-line with general CKAN philosophy

What does it currently do if the (actual) version of KSP is upgraded between CKAN loads?

It displays a warning during CKAN startup or during KSP installation folder switching and forces the user to review selected compatible versions.

Don't we already have that functionality? yo-yo mode etc, just no GUI for it.

As far as I could see YoyoGameComparator returns true regardless of KSP version so it is very unsafe - as an user you have all or nothing. Also, as you mentioned there is no GUI for it.

politas · 2016-12-13T19:24:42Z

Just a side note, @grzegrzk - if you plan to contribute further to the CKAN, you'll find it much easier to do development of changes in a branch, rather than your master. I should be able to review this in detail tomorrow. @ayan4m1, if you have a chance to look it over, I'd appreciate that.

@Techman:Don't we already have that functionality? yo-yo mode etc, just no GUI for it.

The trouble I have with the previous attempts to get this is that it relied on us making judgement calls about which KSP versions were "close enough", and I have yet to find any two versions of KSP (apart from rushed out patches that don't really count) that didn't break something.

dbent

Changes look sensible to me for the most part. Additional comments:

I know the codebase is very inconsistent, but new code should follow the appropriate style (which happens to be the general style for all C# code). Basically PascalCase all the things except for private fields which should be _underscoreCamelCase and local variables which should be camelCase.

There should be a mechanism by which the compatibility list can be manipulated on the command line. Something like:

 > ckan ksp compat list
 Version     Actual
 ----------  ------
 1.2.2.1622  Yes
 > ckan ksp compat add 1.2.1
 > ckan ksp compat list
 Version     Actual
 ----------  ------
 1.2.2.1622  Yes
 1.2.1       No
 > ckan ksp compat forget 1.2.1
 > ckan ksp compat list
 Version     Actual
 ----------  ------
 1.2.2.1622  Yes
 > ckan ksp compat add foobar
 ERROR: 'foobar' is not a valid KSP version.
 > ckan ksp compat forget foobar
 ERROR: 'foobar' is not a valid KSP version.
 > ckan ksp compat forget 1.2.1
 > ckan ksp compat forget 1.2.2.1622
 ERROR: Cannot forget the actual KSP version.

I could probably do that since you've handled the actual hard part of dealing with the GUI (oh how I loathe the GUI). Of course all of these commands would support the --ksp and --kspdir flags to select a specific installation.

Can the user mark their installation is not compatible with the actual version of their installation? (They probably shouldn't be able to and I made that an error in my CLI example above).

dbent · 2016-12-13T18:55:06Z

Core/Types/GameComparator/StrictGameComparator.cs


-            return moduleRange.IsSupersetOf(gameVersionRange);


Why is this change necessary? IsSupersetOf should do exactly what is intended and I believe handles an edge case this code does not.

Now I assume that user can specify that his/her KSP is compatible with "1.0", which is interpreted as "1.0.*" - that is, all mods that are compatible with 1.0.x.y (x and y can be any value) are compatible with this KSP. The old implementation fails in this case.

I have added some test cases to GameComparator. This case is covered by TestStrictGameComparator, test case:
new object[] { "1.0.4.1234", "1.0.4", true },
where "1.0.4.1234" is mod compatibility and "1.0.4" is specified KSP version.

If you have some corner case that should be taken into consideration please let me know what is it and why - I will try to cover it.

I see, so what we really want to test is it the ranges intersect each other. I've added such a method in #1958. It returns a new KspVersionRange which is equal to the intersection or null if there is no intersection. Feel free to cherry-pick or merge in.

I also added equivalent tests.

I see, so what we really want to test is it the ranges intersect each other. I've added such a method in #1958. It returns a new KspVersionRange which is equal to the intersection or null if there is no intersection. Feel free to cherry-pick or merge in.

I merged it and it fails in some corner cases. Example: mod specifies min/max supported KSP version, min=1.0.4, max=null, game version=1.0.4. It should return "true" but returns "false" (no intersection).

I tried to thoroughly test the code that I used here because it is tricky :). I can move it to some another place and name the method accordingly, but...I do not know how to name it ("There are only two hard things in Computer Science: cache invalidation and naming things." :) )

Should be fixed in c36ace0, the problem was unbounded bounds (i.e. KspVersionBound.Unbounded) needed to be treated as highest and lowest possible values in KspVersionBound.Highest() and KspVersionBound.Lowest(). Test cases have been added for those situations. Thanks.

EDIT: Fix commit

Also, the saying is:

There are only two hard things in Computer Science: cache invalidation, naming things, and off by one bugs.

😉

Also, the saying is:

There are only two hard things in Computer Science: cache invalidation, naming things, and off by one bugs.

Ha ha, your are right :)

I have pushed the code that uses IntersectWith - all tests are green now :)

dbent · 2016-12-13T18:57:08Z

Core/Versioning/KspVersionCriteria.cs

+            this.versions.AddRange(compatibleVersions);
+        }
+
+        public List<KspVersion> Versions {


Please keep record-like classes immutable. In this case Versions should only be exposed as an IEnumerable<KspVersion> so that client code can't willy-nilly modify the collection (without doing something potentially unsafe like explicitly casting it back to a List<>).

Actually, I'd rather have this class implement IEnumerable<KspVersion> and get rid of the Versions property. It'd simplify a lot of code using this.

Please keep record-like classes immutable.

It now returns _versions.AsReadOnly(), is it OK?

Actually, I'd rather have this class implement IEnumerable

If the name is changed to KspVersionSet as you suggested then it would be OK, but I am not sure about naming change (please see my comment above).

It now returns _versions.AsReadOnly(), is it OK?

Good enough for me.

dbent · 2016-12-13T19:03:40Z

Core/Versioning/KspVersionCriteria.cs

+
+namespace CKAN.Versioning
+{
+    public class KspVersionCriteria


I think KspVersionSet is more descriptive of what this class actually does at the moment. If at some point in the future we actually need additional properties on it that make it "criteria" we can subclass it and add those properties there.

The class should also behave like a set and only keep a single copy of duplicate KspVersion objects. This should be easy as KspVersion implements IEquatable<KspVersion>.

I think KspVersionSet is more descriptive

I had a feeling that KspVersionCriteria is more general and can be adjusted in the future to behave differently if needed. I assume that you will take care of this code in the future more than me so you have the last word, but in my opinion "Criteria" is OK. Let me know if I should change it.

The class should also behave like a set and only keep a single copy of duplicate KspVersion

Done

Fair enough.

dbent · 2016-12-13T19:06:11Z

Core/Versioning/KspVersionCriteria.cs

+            this.versions.Add (v);
+        }
+
+        public KspVersionCriteria(KspVersion v, List<KspVersion> compatibleVersions)


Rather than having two constructors how about just a single one with a variadic parameter:

public KspVersionSet(params KspVersion[] versions) { }

The one that is now is more convenient to use from CKAN.KSP.VersionCriteria()

dbent · 2016-12-13T19:33:24Z

Core/KSP.cs

+
+        private string CompatibleKspVersionsFile()
+        {
+            return Path.Combine(CkanDir(), "compatible_ksp_versions.json");


As a sidenote we should really come up with a centralized mechanism to store both global and local settings data that is not the registry. But that's a story for another day.

grzegrzk · 2016-12-13T20:37:44Z

if you plan to contribute further to the CKAN, you'll find it much easier to do development of changes in a branch, rather than your master.

From my perspective it does not matter - I think of my clone as separate feature branch. But if it is more convenience for you that I create new branch I will do it in the future - no problem, just let me know what you prefer :).

new code should follow the appropriate style (which happens to be the general style for all C# code)

I tried to spot all cases where I introduced something against code style and I have corrected it. If you spot something more please let me know - I will correct it as well. In my daily work I am not from C# world but from Java world, so such style nuances are hard to spot for me :).

I could probably do that since you've handled the actual hard part of dealing with the GUI (oh how I loathe the GUI).

I would really appreciate if you could do it - I have the opposite feelings that GUI is much more comfortable to deal with for me - both as an user and as a programmer.

Can the user mark their installation is not compatible with the actual version of their installation? (They probably shouldn't be able to and I made that an error in my CLI example above).

There is no possibility for user to mark anything as "not compatible" - the user can only add compatibility with over versions.

dbent · 2016-12-13T23:16:36Z

I tried to spot all cases where I introduced something against code style and I have corrected it. If you spot something more please let me know - I will correct it as well. In my daily work I am not from C# world but from Java world, so such style nuances are hard to spot for me :).

No problem, that's what I figured. The codebase is a mishmash of styles from all sorts of languages.

I would really appreciate if you could do it - I have the opposite feelings that GUI is much more comfortable to deal with for me - both as an user and as a programmer.

We're going to have to clone you. Most programmers I know hate GUI programming. :)

There is no possibility for user to mark anything as "not compatible" - the user can only add compatibility with over versions.

Cool.

dbent · 2016-12-13T23:35:53Z

Replace compatibility check with IntersectWith() and this looks fine to me.

But as a general approach I'm wondering if just having users specify a min and max (so basically a KspVersionRange) is a better experience? Is there a use case for someone to specify compatibility with 1.2.0 and 1.2.2 but not 1.2.1?

politas · 2016-12-14T02:15:35Z

@dbent: Of course all of these commands would support the --ksp and --kspdir flags to select a specific installation.

I could not figure out that CLI option! I knew there had to be one. @dbent , any chance you could write up a comprehensive list of CLI options that I can make into a CKAN-CLI how-to?

…ect'

…ed more unit tests to GameComparator

ghost · 2016-12-14T23:37:12Z

I sadly do not have advice, but I say, thank you for this.

politas · 2016-12-14T23:43:16Z

Main.Designer.cs(1541,44): warning CS0108: `CKAN.Main.Update' hides inherited member `System.Windows.Forms.Control.Update()'. Use the new keyword if hiding was intended
/usr/lib/mono/4.5-api/System.Windows.Forms.dll (Location of the symbol related to previous warning)
CompatibleKspVersionsDialog.cs(84,35): warning CS0168: The variable `ex' is declared but never used
CompatibleKspVersionsDialog.Designer.cs(205,18): warning CS1717: Assignment made to same variable; did you mean to assign something else?
Main.Designer.cs(1526,44): warning CS0169: The private field `CKAN.Main.UpdateCol' is never used

A reversion bug, there. We try to avoid hiding inherited members of system libraries.

I think this is down to us monodevelop users changing the designer.cs directly, and VisualStudio overwritiing the changes. We really want that column to be called "UpdateCol", not "Update". If you can make that change in VisualStudio, I'd be grateful.

Oh, there's that 'ex' variable not being used, too.

politas · 2016-12-14T23:51:17Z

CompatibleKspVersionsDialog.Designer.cs(249,45): warning CS0108: `CKAN.CompatibleKspVersionsDialog.CancelButton' hides inherited member `System.Windows.Forms.Form.CancelButton'. Use the new keyword if hiding was intended
/usr/lib/mono/4.5-api/System.Windows.Forms.dll (Location of the symbol related to previous warning)

Can you rename that button to get rid of this build warning, too?

politas · 2016-12-14T23:54:13Z

Core/Types/GameComparator.cs(84,42): warning CS0414: The private field `Tests.Core.Types.GameComparator.TestStrictGameComparatorCases' is assigned but its value is never used
Core/Types/GameComparator.cs(132,42): warning CS0414: The private field `Tests.Core.Types.GameComparator.TestStrictGameComparatorMinMaxCases' is assigned but its value is never used

And a couple of warnings in the Tests module as well.

politas · 2016-12-15T03:19:24Z

I also note that (at least on Linux) the Apply and Cancel buttons aren't showing up on the Changeset tab after clicking "Apply Changes".

ghost · 2016-12-15T11:15:53Z

@politas Same on Windows

grzegrzk · 2016-12-15T16:28:58Z

I also note that (at least on Linux) the Apply and Cancel buttons aren't showing up on the Changeset tab after clicking "Apply Changes".

@politas Same on Windows

I do not know what is going on but when I change Main.cs using designer in Visual Studio it completely messes up a lot of controls and design. It caused the buttons to disappear. I reverted the file to the state before my changes and added menu item by hand. Now it works without problem.

I also got rid of compilation warnings.

dbent · 2016-12-15T21:11:20Z

I could not figure out that CLI option! I knew there had to be one. @dbent , any chance you could write up a comprehensive list of CLI options that I can make into a CKAN-CLI how-to?

@politas Probably, but they're pretty much all in here. At some point I want to do a complete overhaul of how the CLI works.

politas · 2016-12-15T21:31:08Z

GUI/Main.Designer.cs

+            this.compatibleKSPVersionsToolStripMenuItem.Name = "compatibleKSPVersionsToolStripMenuItem";
+            this.compatibleKSPVersionsToolStripMenuItem.Size = new System.Drawing.Size(233, 24);
+            this.compatibleKSPVersionsToolStripMenuItem.Text = "Compatible KSP versions";
+            //


this.CompatibleKspVersionsToolStripMenuItem.Click += new System.EventHandler(this.CompatibleKspVersionsToolStripMenuItem_Click);

I think you missed this line

Definitely, corrected

politas · 2016-12-15T22:47:34Z

The trouble is that the .Designer.cs files have been changed by hand several times, but that doesn't update the .resx file, which I believe is where Visual Studio keeps its knowledge of the layout in a binary format.

ghost · 2016-12-16T00:39:42Z

finally with this, i can join the 100+ mod master race with the latest ksp

ayan4m1 · 2016-12-16T16:05:15Z

@politas the resx only stores things like icons, images, and localized string tables. The Designer.cs is the entirety of the "form layout/position/style" information.

@grzegrzk

Merge #1957 Select compatible KSP versions by @grzegrzk Merge #1958 Add IntersectWith method to KspVersionRange by @dbent

politas · 2016-12-16T22:41:00Z

Wonderful stuff!! @dbent, are you likely to be able to add CLI commands for this functionality in the next couple of days? If not, then I'll push out a new release without waiting for it, then we can do a point release to catch up the CLI.

Dazpoet · 2016-12-16T22:46:34Z

All hail @grzegrzk ! Implementer of the most requested feature of all time 👍

grzegrzk · 2016-12-17T09:31:51Z

Thx guys, I am glad that you like it :)

politas · 2016-12-17T21:26:24Z

@grzegrzk, do you have a KSP forum user account I can reference when crediting your submission? You definitely deserve much kudos from the CKAN user community for this.

grzegrzk · 2016-12-17T21:27:48Z

@politas Yes, I am Ker_nale. Thx for mentioning me :).

drewcassidy · 2018-05-03T03:47:34Z

Is there any way to access this without the GUI?

HebaruSan · 2018-05-03T04:00:19Z

Yes, try ckan compat.

grzegrzk added 6 commits December 11, 2016 17:02

Introduced KspVersionCriteria, started to use it across the application

295ac5a

User can select compatible KSP versions - list is filtered & updated

0fa45cb

Redesigned dialog to choose compatible versions + it also displays ma…

cac3002

…jor KSP versions

Reverted changes made in ChooseKspInstance - the "settings" button is…

a35bcd1

… no longer displayed there

Compatible versions are stored on disk + warning dialog is displayed …

e184753

…during startup if KSP has bean updated in the meantime

Corrected StrictGameComparator, corrected tests, removed unnecessary …

7d31e12

…code

pjf added the Pull request label Dec 12, 2016

grzegrzk added 2 commits December 13, 2016 16:07

Corrected reference to autofac + removed unnecessary code

bb45796

Correction for tests

858a02c

Argh! Removed dependency added by accident when playing with nuget

4ea4e8a

dbent requested changes Dec 13, 2016

View reviewed changes

dbent reviewed Dec 13, 2016

View reviewed changes

grzegrzk added 2 commits December 13, 2016 20:57

Naming changes to adhere to C# rules

d1a58c6

KSP class now accepts only distinct compatible versions

03176b4

Add IntersectWith method to KspVersionRange

0763a89

dbent mentioned this pull request Dec 13, 2016

Add IntersectWith method to KspVersionRange #1958

Merged

dbent approved these changes Dec 13, 2016

View reviewed changes

grzegrzk and others added 4 commits December 14, 2016 20:12

Merge remote-tracking branch 'upstream/feature/kspversionrange_inters…

4b13979

…ect'

Fix Highest/Lowest comparisons with unbounded bounds

c36ace0

Merge remote-tracking branch 'upstream/feature/kspversionrange_inters…

c52d721

…ect'

StrictGameComparator is using now KspVersionRange.IntersectWith + add…

c40376d

…ed more unit tests to GameComparator

grzegrzk added 3 commits December 15, 2016 16:59

Reverted Main.Designer.cs to last valid state

a28e013

Added by hand menu item to Main.Designer.cs

17926c5

Got rid of compilation warnings

3aba61f

Got rid of compilation warnings in tests

b8ac281

politas reviewed Dec 15, 2016

View reviewed changes

Added missing event handler to "Compatible KSP versions" menu item

bb29084

politas merged commit bb29084 into KSP-CKAN:master Dec 16, 2016

politas added a commit that referenced this pull request Dec 16, 2016

Merge #1957, #1958 Select Compatible KSP versions

4c1f0cc

Merge #1957 Select compatible KSP versions by @grzegrzk Merge #1958 Add IntersectWith method to KspVersionRange by @dbent

politas removed the Pull request label Dec 16, 2016

politas mentioned this pull request Dec 24, 2016

Introducing IGameComparatorExemptions in order to waive checking version compatibility for select modules. (Take 2) #1965

Closed

This was referenced Nov 5, 2017

Expert Installer Mode - Bypass Version Restrictions #1563

Closed

How do we sensibly handle "forced" installs? #1173

Closed

Select compatible KSP versions #1957

Select compatible KSP versions #1957

Conversation

grzegrzk commented Dec 12, 2016 • edited

grzegrzk commented Dec 12, 2016

politas commented Dec 13, 2016

techman83 commented Dec 13, 2016

grzegrzk commented Dec 13, 2016 • edited

politas commented Dec 13, 2016

dbent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbent Dec 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzegrzk Dec 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzegrzk commented Dec 13, 2016

dbent commented Dec 13, 2016

dbent commented Dec 13, 2016

politas commented Dec 14, 2016

ghost commented Dec 14, 2016

politas commented Dec 14, 2016 • edited

politas commented Dec 14, 2016

politas commented Dec 14, 2016

politas commented Dec 15, 2016

ghost commented Dec 15, 2016

grzegrzk commented Dec 15, 2016

dbent commented Dec 15, 2016

politas Dec 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

politas commented Dec 15, 2016

ghost commented Dec 16, 2016

ayan4m1 commented Dec 16, 2016

politas commented Dec 16, 2016

Dazpoet commented Dec 16, 2016

grzegrzk commented Dec 17, 2016

politas commented Dec 17, 2016

grzegrzk commented Dec 17, 2016 • edited

drewcassidy commented May 3, 2018

HebaruSan commented May 3, 2018

grzegrzk commented Dec 12, 2016 •

edited

grzegrzk commented Dec 13, 2016 •

edited

dbent Dec 14, 2016 •

edited

grzegrzk Dec 13, 2016 •

edited

politas commented Dec 14, 2016 •

edited

politas Dec 15, 2016 •

edited

grzegrzk commented Dec 17, 2016 •

edited