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

Add possibility to clone KSP installs and create dummy ones #2627

Merged
merged 7 commits into from
Jan 8, 2019

Conversation

DasSkelett
Copy link
Member

@DasSkelett DasSkelett commented Dec 28, 2018

As I am a responsible developer and always test my changes on multiple systems before release...
Because I am currently out of reach of my PC (christmas, family and useless stuff like this), I tried to work on CKAN a bit from a device with no KSP install.
But CKAN is rather bitchy about having a correct KSP install, and doesn't even let you in the GUI (which is the correct behavior in my opinion).

So to solve this issue, I added the ckan ksp fake command to create a dummy KSP directory, containing a GamaData folder, a buildID.txt, a buildID64.txt and a readme.txt.

This should also be useful for quick tests of mod related issues.


Following options are implemented:

  • choose the KSP version
  • get the build number for the version
  • and the path (currently a folder _fakeKSP is created inside the folder from which the command is executed)
  • support DLC with different versions (no validation yet)
  • automatically add the ksp instance to the known instances? Would make sense, I think.
  • add validation
  • more logging
  • add a clone method to clone an existing KSP instance
  • implement the clone method in the cmdline
  • add a buildID64
  • add tests

Side effects:

  • New CopyDirectory() function to properly copy a folder with all subfolders. Implemented as a transaction.
  • New FindKnownVersion() function to find a build number for a ksp version
  • New InBuildMap() bool to check if a version is in the build map.

Right now the command structure is as follows:
ckan ksp fake [commonOptions] name path version dlcVersion

  • where name is the name with which the new instance is added to the known instances
  • and path can be absolute or relative
  • and version is in the form that it can be parsed by KspVersion.Parse() (i.e. e.g. 1.5.1)
  • and dlcVersion is the version for the dlc. Not being evaluated right now. To exclude the DLC, type "none".

And:
ckan ksp clone [commonOptions] instanceNameOrPath newname newpath

  • where instanceNameOrPath is the name of an existing instance (-> see ckan ksp list) or the path to it
  • and name is the name for the new instance
  • and path is the path for the new instance (relative or absolute)
  • This command copies the FULL KSP directory, including all exes, logs, game files and whatsoever.

@HebaruSan
Copy link
Member

@HebaruSan HebaruSan added Enhancement New features or functionality Cmdline Issues affecting the command line Core (ckan.dll) Issues affecting the core part of CKAN In progress We're still working on this Infrastructure Issues affecting everything around CKAN (the GitHub repos, build process, CI, ...) labels Dec 28, 2018
Added option to choose version
Added option to fake DLC too (default = true)
Added option to choose path
Added option to choose name for KSP instance
Add new fake instance to known instances
@DasSkelett
Copy link
Member Author

DasSkelett commented Dec 29, 2018

Added some features, but can't test them because I get the cast error everytime I run ksp fake 1.5.1 testName testPath or similar. Found it!

Cmdline/Action/KSP.cs Outdated Show resolved Hide resolved
Cmdline/Action/KSP.cs Outdated Show resolved Hide resolved
Cmdline/Action/KSP.cs Outdated Show resolved Hide resolved
@HebaruSan
Copy link
Member

HebaruSan commented Dec 29, 2018

General thought here, but would we ever want this capability in GUI or ConsoleUI? If so, it would make sense to migrate the business logic into Core and reduce the CmdLine code to just calling into it. Not sure about the answer to that myself, this might make sense as a text-only utility thing.

EDIT: One argument in favor of that idea is that we could write Tests to make sure the command works correctly.

@DasSkelett
Copy link
Member Author

Thought about this too. Maybe some users want to test mod configurations and dependencies and stuff without manipulating their main KSP directory.
But then, I don't think that's often the case, so you can just quickly type the console command and done.

@politas
Copy link
Member

politas commented Dec 29, 2018

Ooh, don't suppose you could expand it into a more generic "clone install" command with a "--dummy" option to make the new install a blank one?

@DasSkelett
Copy link
Member Author

I can see the reason behind a "clone install", but I think it's a bit counter-intuitive to use a flag behind "clone" to create a new (dummy) instance. In "clone" you want to copy an existing one (which is kinda the definition of the word), in "--dummy"/"fake" you create an entirely new one.
This shouldn't be hidden in a flag, I think having those two as different commands is more intuitive.

@DasSkelett
Copy link
Member Author

DasSkelett commented Dec 30, 2018

Do we have a "mapVersionToBuild" function somewhere?
I thought KspVersion.Parse() does this, but it doesn't. If patch and/or build is undefined, it just stays undefined (-1). So atm, if a user enters ckan ksp fake smth smwh 1.4 none he gets Version 1.4 in the readme.txt and -1 in the buildID.txt.
This leads to AddInstall() to throw an error, because it obviously doesn't know Version 1.5.

So is a function that does this already implemented anywhere, or do I have to write a new one?

@politas
Copy link
Member

politas commented Dec 30, 2018

Ok. I do think there's sense in generalizing the core modules to perform the task, though. They're both about creating a new KSP installation based on a given image. Just that one image is a virtual one.

@HebaruSan
Copy link
Member

I don't believe a function exists to get build IDs. Looks like it wouldn't be too tough using KspBuildMap.KnownVersions though.

If the patch portion is missing, we should probably print an error so the user knows they have to choose between 1.5.0 and 1.5.1.

@DasSkelett
Copy link
Member Author

Thanks. I thought about autocompletion with a 0 if the patch is missing, so "1.5" is parsed to "1.5.0", because 1.5 is a more natural way to say it.
But then, in the netkan files we interpret "1.5" as a range from "1.5.0" to "1.5.XXX", which means all versions starting with "1.5".
But we can't do that in this case, so an error might be the right answer.

@DasSkelett
Copy link
Member Author

@politas So you mean moving the new feature to "core" as @HebaruSan proposed, but with the "clone" function and the "fake" function all in one, and running ckan KSP clone <instanceName> or ckan KSP fake <name> <path> <version> <dlcVersion> both call the new core function.

Did I get this right?

Move most of the feature's code there
Add logging
Create new AddBuildToVersion in Versioning.KspVersion to get the correct build number for a specific version.
@DasSkelett
Copy link
Member Author

So my last commit moved most of the fake-code to KSPManager. Should enable writing tests and implementing it in the GUI or ConsoleUI.

I've decided to split FakeInstance() and CloneInstance in two seperate functions, but CloneInstance() calls FakeInstance() once it parsed the version and stuff.
Hope that's kinda how you imagined it, but I'm open to critic and suggestions.

A new public KspVersion AddBuildToVersion() inside Versioning.KspVersion (better names welcome) tries to find the right build number for the version object it is called on out of the build map using KnownVersions. Then it returns the new "complete" version object.
I decided to throw an error if the patch is not specified, as @HebaruSan proposed.

Also addded some logging.

Core/KSPManager.cs Outdated Show resolved Hide resolved
Core/KSPManager.cs Outdated Show resolved Hide resolved
Added CKAN.Utilities class with CopyDirectory() function
Added Versioning.KspVersion.IsValid() to ensure a specified version is valid.
Added a lot of checks and exceptions and whatsoever.
Still needs unit tests.
@DasSkelett
Copy link
Member Author

DasSkelett commented Dec 31, 2018

Sooooo, I changed quite a bit again.
We have now ckan ksp clone [CommonOptions] <instanceName> <newname> <newpath> to really copy an existing KSP directory, with all folders and files inside.
I created a new Utilities class because I had no idea where else to add a CopyDirectory() function.
Now ckan ksp fake also creates a buildID64.txt, in the case that this will be relevant some day.
I also wrote a public bool IsValid() in Versioning.KspVersion to be able to check if versions are official/real ones.

Edit: I actually found the TestData.CopyDirectory() function right now, it seems a proper one was heavily needed :D.

@DasSkelett DasSkelett changed the title WIP: Add cmdline command to create fake ksp directory WIP: Add possibility to clone KSP installs and create dummy ones Dec 31, 2018
Core/Utilities.cs Outdated Show resolved Hide resolved
@politas
Copy link
Member

politas commented Dec 31, 2018

Will the following command work?
ckan ksp clone <newname> <destdir> --ksp-dir <sourcedir>

@DasSkelett
Copy link
Member Author

DasSkelett commented Dec 31, 2018

No, currently it only accepts instance names known in the registry.
Edit: Is now implemented it as ckan ksp clone <instanceNameOrPath> <newname> <newpath>.

@DasSkelett
Copy link
Member Author

DasSkelett commented Dec 31, 2018

As I'm going through the code, I found that when testing a KSP directory for validity, we only ask if GameData exists (despite the GUI specifically asking for a buildID.txt).
That kinda affects my new clone and fake in the sense that I can't properly test if everything went right.
In my opinion it would make sense to expand the checks for a buildID.txt and a readme.txt.
I'm not sure if they were part of a KSP install since the earliest versions, but I think so.

I overlooked that we do check Version() in Valid()


Edit: In my last commit I added @politas suggestion to be able to clone by path.

Also added the first two tests. The one for CloneInstance_BadInstance_ThrowsNotKSPDirKraken()
somehow doesn't pass, despite the error being thrown. Can someone help?

More edit: I found that the error is thrown at the end of the test, when the KSP instance is disposed.
DisposableKSP.Dispose wants to create a registry instance (and disposes it again 3 lines after?), which is doomed to fail because Instance() wants a CkanDir() and that needs a valid KSP folder.
So, should DisposableKSP work like that / is it only used for good KSP directories?

More more Edit: I now created a regular KSP object out of TestData.bad_ksp_dirs().First(). This should be okay because nothing manipulates the directory in my case.

CKAN.NotKSPDirKraken : Could not find KSP version in buildID.txt or readme.txt
  at CKAN.KSP.CkanDir () [0x00029] in /home/kilian/git-reps/CKAN/Core/KSP.cs:295 
  at CKAN.RegistryManager.Instance (CKAN.KSP ksp) [0x00000] in /home/kilian/git-reps/CKAN/Core/Registry/RegistryManager.cs:250 
  at Tests.Data.DisposableKSP.Dispose () [0x00000] in /home/kilian/git-reps/CKAN/Tests/Data/DisposableKSP.cs:47 
  at Tests.Core.KSPManagerTests.CloneInstance_BadInstance_ThrowsNotKSPDirKraken () [0x00067] in /home/kilian/git-reps/CKAN/Tests/Core/KSPManager.cs:127 
  at (wrapper managed-to-native) System.Reflection.MonoMethod.InternalInvoke(System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x0003b] in <0f8aeac9d63d4b8aa575761bb4e65b79>:0 

Searching build number now reversed in AddBuildToNumber().
Skip buildID.txt and buildID64.txt in FakeInstance() if no build defined.
Restructured exception handling in FakeInstance and CloneInstance -> more test-friendly
Added first two tests.
Cmdline/Action/KSP.cs Outdated Show resolved Hide resolved
Cmdline/Action/KSP.cs Outdated Show resolved Hide resolved
Core/KSPManager.cs Outdated Show resolved Hide resolved
Core/KSPManager.cs Outdated Show resolved Hide resolved
@DasSkelett
Copy link
Member Author

So, if no one finds something to correct, to change or to add anymore, I would say this is ready to merge.

@politas
Copy link
Member

politas commented Jan 3, 2019

The only reason we ask for a BuildID.txt when adding a KSP install is to be able to use the File Selector rather than the directory selector dialogue, because it works way better.

@DasSkelett DasSkelett changed the title WIP: Add possibility to clone KSP installs and create dummy ones Add possibility to clone KSP installs and create dummy ones Jan 5, 2019
@politas politas merged commit 417bad0 into KSP-CKAN:master Jan 8, 2019
@politas
Copy link
Member

politas commented Jan 8, 2019

Cool! Now we just need to add it to the GUI interface.

@HebaruSan
Copy link
Member

@DasSkelett , FYI, the validation scripts are now set up to use this, and working fine!

@DasSkelett DasSkelett deleted the feature/noKSPinstall branch January 18, 2019 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cmdline Issues affecting the command line Core (ckan.dll) Issues affecting the core part of CKAN Enhancement New features or functionality In progress We're still working on this Infrastructure Issues affecting everything around CKAN (the GitHub repos, build process, CI, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants