Multi-instance support #125

AlexanderDzhoganov · 2014-10-21T12:07:46Z

Only works in the GUI for now, doesn't break the command-line client but the new options are not implemented.

Related to #29

pjf · 2014-10-21T12:16:52Z

I don't suppose you can add a commit which fixes that test case, could you?

pjf · 2014-10-21T12:35:03Z

CKAN/CKAN/KSP.cs

@@ -137,20 +185,12 @@ private static string FindGameDir()
                return exe_dir;
            }

-            // Check the registry, maybe it's there.
-            string registry_dir = FindGamedirRegistry();


I might be missing something here, but we really do want to check the registry first. If the user's configured their default KSP install (ckan config gamedir ...), we don't want to go using a different one.

Just tested with the cmdline client. It now always finds my steam install, rather than my testing install which is in the registry. And even if I use ckan config gamedir, it still can't find it.

Oh, looks like I broke the command-line client after all. Let me fix this before merging.

pjf · 2014-10-21T12:45:56Z

Looks like adding a new KSP directory (which has never had a CKAN client see it) causes problems:

http://pastebin.com/cmjDFuXD

pjf · 2014-10-21T12:49:45Z

CKAN/CKAN/ModuleInstaller.cs

@@ -224,9 +218,9 @@ public void InstallList(List<string> modules, RelationshipResolverOptions option
                }
            }

-            if (!notCached.Any())
+            if (!notCached.Any() && !cached.Any())


Wait... If not not cached any, and not cached any... Well, not not cached is cached, and not cached is the opposite of that, and they're anded together, so...

This line really needs a comment. I can't tell what it's supposed to be doing or testing for.

It tests if both notCached and cached are empty

So... if we have no modules cached, and no modules not cached, that means... we're installing no modules and hence we do a roll-back? Under what circumstances would we encounter that? When ModList is empty, which I don't think can happen unless modules is empty...

So wouldn't be be better of testing one of those variables earlier on, and signalling appropriately if they're empty?

It's late and I'm low on sleep, so I may be missing something here, but that's a typical state for developers, so if it's confusing me now it'll almost certainly confuse someone else (possibly future me) later on.

Yep, it'd be better to test at the start of the function.

… AutoStartInstance to registry

pjf · 2014-10-22T06:08:30Z

Let me know if you'd like me to re-review this. I may not get a chance to do so tonight (my apologies), but I hope to do so soon! <3

pjf · 2014-10-22T06:08:48Z

(Today has been a flurry of build crews and plumbers fixing a burst pipe, and has been hectic!)

AlexanderDzhoganov · 2014-10-22T07:03:19Z

I need a few more hours to modify the command line client and we can merge this :)

pjf · 2014-10-23T02:59:45Z

CKAN/CmdLine/Main.cs

+            else
+            {
+                // auto-start instance specified
+                if (KSP.AutoStartInstance != null && KSP.AutoStartInstance != "" && KSP.Instances.ContainsKey(KSP.AutoStartInstance))


Oh, I didn't realise there was an autostart instance. Perfect!

Does this check the current working directory for KSP first? We need that for portable installs.

It uses the old "find default install" method, so it should work.

pjf · 2014-10-23T04:13:03Z

Review finished, going through checklist items now. :)

AlexanderDzhoganov · 2014-10-23T07:31:33Z

I responded to some of your comments :)

For KSP-CKAN#125.

pjf · 2014-10-23T08:02:39Z

Hooray! I super-duper want to get this merged tonight, but I'm occupied right now. Do feel free to make new work on top of this, but if you can make sure it's on a branch that would be grand. :)

You rock, and thanks again! <3

AlexanderDzhoganov · 2014-10-23T16:22:37Z

Ok, here's how I fucked up:

What I thought I was doing was making a branch off my master and then merging it back (into my master). What actually happened was that it merged into KSP-CKAN master o.O . I managed to revert it back but this pull request got closed for some reason.
I hope this is easily fixable but I'm not sure how, I'm sorry, @pjf :)

techman83 · 2014-10-23T23:16:00Z

@AlexanderDzhoganov - Everything is fixable. I've got myself into some weird weird situations with git :)

pjf · 2014-10-24T02:13:59Z

@AlexanderDzhoganov : Aah! The PR closed because github saw its commits merge into KSP-CKAN/master.

I'll go look at how everything is now, but I don't imagine it'll be more than a few minutes work to put back. :)

pjf · 2014-10-24T02:43:03Z

And fixed! It's super easy when you know how! (See #140 on step-by-steps, in case you ever need them!)

- Split management of multiple instances into KSPManager - No need to manually save or load from registry. - Added GetPreferredInstance() as the best way to find the user's preferred instance, if a single preferred instance exists. - Added PortableDir() to install portable installs. - Decoupled transactional file transactions from KSP class. - KSPPathConstants in their own file - KSPPathUtils in its own file. - Some tests, but not enough, darn it! For KSP-CKAN#125

Fixups for PR #125

For KSP-CKAN#125.

- Split management of multiple instances into KSPManager - No need to manually save or load from registry. - Added GetPreferredInstance() as the best way to find the user's preferred instance, if a single preferred instance exists. - Added PortableDir() to install portable installs. - Decoupled transactional file transactions from KSP class. - KSPPathConstants in their own file - KSPPathUtils in its own file. - Some tests, but not enough, darn it! For KSP-CKAN#125

AlexanderDzhoganov added 6 commits October 21, 2014 10:48

Merge remote-tracking branch 'upstream/master'

54a33d1

Merge remote-tracking branch 'upstream/master'

fb45592

Some ModuleInstaller.InstallList changes

f198ca7

First steps towards multi-instance

559ff43

Multi-instance now works in GUI

bf865dc

Fixed command-line client

3d655a7

AlexanderDzhoganov added the ready label Oct 21, 2014

AlexanderDzhoganov added 3 commits October 21, 2014 15:17

Added rename instance window

81f04be

Small fix :)

7549998

Another small fix

969bd8c

pjf reviewed Oct 21, 2014
View reviewed changes

AlexanderDzhoganov added 6 commits October 21, 2014 16:33

GUI improvements

b295947

Renamed depends panel to relationships, major improvements

ad32b29

Added CkanModule to_string and to_file methods, some GUI fixes, added…

711fc12

… AutoStartInstance to registry

added progress reports to FilesystemTransaction, new "launch ksp" button

ca8bc87

Small GUI fixes

4a17942

Downloads are now cancelable

2602a0c

Added multi-instance support to command-line client

01e8a03

AlexanderDzhoganov mentioned this pull request Oct 22, 2014

Support multiple KSP installs. #29

Closed

pjf reviewed Oct 23, 2014
View reviewed changes

pjf added a commit to pjf/CKAN that referenced this pull request Oct 23, 2014

ErrorFormat and TODO when double-uninstalling mods.

4430e00

For KSP-CKAN#125.

pjf added a commit to pjf/CKAN that referenced this pull request Oct 23, 2014

TODO: Less steps in cmdline for new instance setup.

bd23bc5

For KSP-CKAN#125.

AlexanderDzhoganov merged commit fdf649f into KSP-CKAN:master Oct 23, 2014

AlexanderDzhoganov removed the pull request label Oct 23, 2014

pjf self-assigned this Oct 24, 2014

pjf mentioned this pull request Oct 24, 2014

Gitomancy on accidental merge of #125 #140

Closed

pjf added the In progress We're still working on this label Oct 24, 2014

pjf mentioned this pull request Oct 24, 2014

Fixups for PR #125 #146

Merged

AlexanderDzhoganov added a commit that referenced this pull request Oct 24, 2014

Merge pull request #146 from pjf/125_fixups

e62cc9f

Fixups for PR #125

RichardLake pushed a commit to RichardLake/CKAN that referenced this pull request May 30, 2015

ErrorFormat and TODO when double-uninstalling mods.

949b483

For KSP-CKAN#125.

RichardLake pushed a commit to RichardLake/CKAN that referenced this pull request May 30, 2015

TODO: Less steps in cmdline for new instance setup.

b003217

For KSP-CKAN#125.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-instance support #125

Multi-instance support #125

AlexanderDzhoganov commented Oct 21, 2014

pjf commented Oct 21, 2014

pjf Oct 21, 2014

pjf Oct 21, 2014

AlexanderDzhoganov Oct 21, 2014

pjf commented Oct 21, 2014

pjf Oct 21, 2014

AlexanderDzhoganov Oct 21, 2014

pjf Oct 21, 2014

AlexanderDzhoganov Oct 21, 2014

pjf commented Oct 22, 2014

pjf commented Oct 22, 2014

AlexanderDzhoganov commented Oct 22, 2014

pjf Oct 23, 2014

AlexanderDzhoganov Oct 23, 2014

pjf commented Oct 23, 2014

AlexanderDzhoganov commented Oct 23, 2014

pjf commented Oct 23, 2014

AlexanderDzhoganov commented Oct 23, 2014

techman83 commented Oct 23, 2014

pjf commented Oct 24, 2014

pjf commented Oct 24, 2014

Multi-instance support #125

Multi-instance support #125

Conversation

AlexanderDzhoganov commented Oct 21, 2014

pjf commented Oct 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjf commented Oct 21, 2014

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

pjf commented Oct 22, 2014

pjf commented Oct 22, 2014

AlexanderDzhoganov commented Oct 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjf commented Oct 23, 2014

AlexanderDzhoganov commented Oct 23, 2014

pjf commented Oct 23, 2014

AlexanderDzhoganov commented Oct 23, 2014

techman83 commented Oct 23, 2014

pjf commented Oct 24, 2014

pjf commented Oct 24, 2014