From 66875652f38d01b3ec9ae077385990a360a5e09d Mon Sep 17 00:00:00 2001 From: Paul Fenwick Date: Sun, 26 Oct 2014 20:02:49 +1100 Subject: [PATCH 01/11] FindInstallableFiles throws a BadMetadataKraken instead of returning empty. This closes #93. NB @AlexanderDzhoganov, we have another exception to watch for when installing. --- CKAN/CKAN/ModuleInstaller.cs | 37 ++++++++++++++++++++++++------ CKAN/CKAN/Types/Kraken.cs | 15 ++++++++++++ CKAN/CmdLine/Main.cs | 6 +++++ CKAN/Tests/CKAN/ModuleInstaller.cs | 26 +++++++++++++++++++-- 4 files changed, 75 insertions(+), 9 deletions(-) diff --git a/CKAN/CKAN/ModuleInstaller.cs b/CKAN/CKAN/ModuleInstaller.cs index 354cbf4265..6c14e2a6f0 100644 --- a/CKAN/CKAN/ModuleInstaller.cs +++ b/CKAN/CKAN/ModuleInstaller.cs @@ -541,6 +541,8 @@ private void InstallComponent(InstallableDescriptor stanza, ZipFile zipfile, /// /// Throws a BadInstallLocationKraken if the install stanza targets an /// unknown install location (eg: not GameData, Ships, etc) + /// + /// Throws a BadMetadataKraken if the stanza resulted in no files being returned. /// internal static List FindInstallableFiles(InstallableDescriptor stanza, ZipFile zipfile, KSP ksp) { @@ -621,6 +623,13 @@ internal static List FindInstallableFiles(InstallableDescriptor files.Add(file_info); } + // If we have no files, then something is wrong! (KSP-CKAN/CKAN#93) + if (files.Count == 0) + { + // We have null as the first argument here, because we don't know which module we're installing + throw new BadMetadataKraken(null, String.Format("No files found in {0} to install!", stanza.file)); + } + return files; } @@ -629,23 +638,35 @@ internal static List FindInstallableFiles(InstallableDescriptor /// for this module. /// /// If a KSP instance is provided, it will be used to generate output paths, otherwise these will be null. + /// + /// Throws a BadMetadataKraken if the stanza resulted in no files being returned. /// internal static List FindInstallableFiles(CkanModule module, ZipFile zipfile, KSP ksp) { var files = new List (); - // Use the provided stanzas, or use the default install stanza if they're absent. - if (module.install != null && module.install.Length != 0) + try { - foreach (ModuleInstallDescriptor stanza in module.install) + // Use the provided stanzas, or use the default install stanza if they're absent. + if (module.install != null && module.install.Length != 0) { - files.AddRange(FindInstallableFiles(stanza, zipfile, ksp)); + foreach (ModuleInstallDescriptor stanza in module.install) + { + files.AddRange(FindInstallableFiles(stanza, zipfile, ksp)); + } + } + else + { + ModuleInstallDescriptor default_stanza = GenerateDefaultInstall(module.identifier, zipfile); + files.AddRange(FindInstallableFiles(default_stanza, zipfile, ksp)); } } - else + catch (BadMetadataKraken kraken) { - ModuleInstallDescriptor default_stanza = GenerateDefaultInstall(module.identifier, zipfile); - files.AddRange(FindInstallableFiles(default_stanza, zipfile, ksp)); + // Decorate our kraken with the current module, as the lower-level + // methods won't know it. + kraken.module = module; + throw; } return files; @@ -657,6 +678,8 @@ internal static List FindInstallableFiles(CkanModule module, Zi /// /// This *will* throw an exception if the file does not exist. /// + /// Throws a BadMetadataKraken if the stanza resulted in no files being returned. + /// /// If a KSP instance is provided, it will be used to generate output paths, otherwise these will be null. /// // TODO: Document which exception! diff --git a/CKAN/CKAN/Types/Kraken.cs b/CKAN/CKAN/Types/Kraken.cs index caba06a106..918e78e2f6 100644 --- a/CKAN/CKAN/Types/Kraken.cs +++ b/CKAN/CKAN/Types/Kraken.cs @@ -82,5 +82,20 @@ public TransactionalKraken(string reason = null, Exception inner_exception = nul } } + /// + /// We had bad metadata that resulted in an invalid operation occuring. + /// For example: a file install stanza that produces no files. + /// + public class BadMetadataKraken : Kraken + { + public CkanModule module; + + public BadMetadataKraken(CkanModule module, string reason = null, Exception inner_exception = null) + :base(reason,inner_exception) + { + this.module = module; + } + } + } diff --git a/CKAN/CmdLine/Main.cs b/CKAN/CmdLine/Main.cs index 5176b2fc5b..118dcc3bdb 100644 --- a/CKAN/CmdLine/Main.cs +++ b/CKAN/CmdLine/Main.cs @@ -363,6 +363,12 @@ private static int Install(InstallOptions options) User.WriteLine("If you're lucky, you can do a `ckan update` and try again."); return EXIT_ERROR; } + catch (BadMetadataKraken ex) + { + User.WriteLine("Bad metadata detected for module {0}", ex.module); + User.WriteLine(ex.Message); + return EXIT_ERROR; + } User.WriteLine("\nDone!\n"); diff --git a/CKAN/Tests/CKAN/ModuleInstaller.cs b/CKAN/Tests/CKAN/ModuleInstaller.cs index 40bed89a21..4f1abe2b6c 100644 --- a/CKAN/Tests/CKAN/ModuleInstaller.cs +++ b/CKAN/Tests/CKAN/ModuleInstaller.cs @@ -48,8 +48,6 @@ public void FindInstallableFiles() string dogezip = Tests.TestData.DogeCoinFlagZip(); CkanModule dogemod = Tests.TestData.DogeCoinFlag_101_module(); - Console.WriteLine("{0}", dogezip); - List contents = CKAN.ModuleInstaller.FindInstallableFiles(dogemod, dogezip, null); Assert.IsNotNull(contents); @@ -72,6 +70,30 @@ public void FindInstallableFiles() // TODO: Ensure it's got a file we expect. } + [Test()] + public void No_Installable_Files() + { + // This tests GH #93 + + string dogezip = Tests.TestData.DogeCoinFlagZip(); + CkanModule bugged_mod = Tests.TestData.DogeCoinFlag_101_bugged_module(); + + Assert.Throws(delegate { + CKAN.ModuleInstaller.FindInstallableFiles(bugged_mod, dogezip, null); + }); + + try + { + CKAN.ModuleInstaller.FindInstallableFiles(bugged_mod, dogezip, null); + } + catch (BadMetadataKraken ex) + { + // Make sure our module information is attached. + Assert.IsNotNull(ex.module); + Assert.AreEqual(bugged_mod.identifier, ex.module.identifier); + } + } + private void TestDogeCoinStanza(ModuleInstallDescriptor stanza) { Assert.AreEqual("GameData", stanza.install_to); From 88c7aa9dfb18e7550e7d0046d7dc6b7281d39f80 Mon Sep 17 00:00:00 2001 From: Paul Fenwick Date: Sun, 26 Oct 2014 20:34:28 +1100 Subject: [PATCH 02/11] Refacted ModuleInstaller.Install() to use FindInstallableFiles() Actually, it now uses `InstallModule()`, which does the work. Notable changes in this: - Install() can throw exceptions again. - We move InstallComponent() towards being deprecated - We cry over still needing bundles. - Closes #93 Sample output: ``` About to install... * DogeCoinFlag 1.03 Continue? [Y/N] DogeCoinFlag: Bad metadata detected for module DogeCoinFlag 1.03 No files found in GameData/DogeCoinFlag to install! ``` --- CKAN/CKAN/ModuleInstaller.cs | 104 +++++++++++++++++++---------------- 1 file changed, 56 insertions(+), 48 deletions(-) diff --git a/CKAN/CKAN/ModuleInstaller.cs b/CKAN/CKAN/ModuleInstaller.cs index 6c14e2a6f0..413b589e54 100644 --- a/CKAN/CKAN/ModuleInstaller.cs +++ b/CKAN/CKAN/ModuleInstaller.cs @@ -335,22 +335,23 @@ public List GetModuleContentsList(CkanModule module) /// /// Install our mod from the filename supplied. - /// If no file is supplied, we will fetch() it first. + /// If no file is supplied, we will check the cache or download it. /// Does *not* resolve dependencies; this actually does the heavy listing. /// Use InstallList() for requests from the user. /// - /// XXX: This provides no way to check if the install failed, - /// it *should* throw an exception if it does. /// - private void Install(CkanModule module, string filename = null) + // + // TODO: The name of this and InstallModule() need to be made more distinctive. + + internal void Install(CkanModule module, string filename = null) { User.WriteLine(module.identifier + ":\n"); Version version = registry_manager.registry.InstalledVersion(module.identifier); + // TODO: This really should be handled by higher-up code. if (version != null) { - // TODO: Check if we can upgrade! User.WriteLine(" {0} {1} already installed, skipped", module.identifier, version); return; } @@ -367,44 +368,15 @@ private void Install(CkanModule module, string filename = null) // And a list of files to record them to. var module_files = new Dictionary(); - ZipFile zipfile = null; - - // Open our zip file for processing - try - { - zipfile = new ZipFile(File.OpenRead(filename)); - } - catch (Exception) - { - // TODO: I'm not sure we want to just be returing here - // on error. A failed install is enough of a reason to - // bail out entirely. This should be throwing an exception. - User.Error("Failed to open archive \"{0}\"", filename); - return; - } - - if (module.install == null || module.install.Length == 0) - { - log.DebugFormat("No install stanzas found for {0}, using defaults", module); - - // This throws a FileNotFoundKraken on failure. We intentionally - // don't catch it, because that's an irrecoverable error. - var stanza = GenerateDefaultInstall(module.identifier, zipfile); - InstallComponent(stanza, zipfile, module_files); - } - else - { - // Walk through our install instructions. - foreach (ModuleInstallDescriptor stanza in module.install) - { - InstallComponent(stanza, zipfile, module_files); - } - } + // Install all the things! + InstallModule(module, filename, module_files); // Register our files. registry.RegisterModule(new InstalledModule(module_files, module, DateTime.Now)); // Handle bundled mods, if we have them. + // TODO: Deprecate bundles! + if (module.bundles != null) { foreach (BundledModuleDescriptor stanza in module.bundles) @@ -425,13 +397,16 @@ private void Install(CkanModule module, string filename = null) // Not installed, so let's get about installing it! var installed_files = new Dictionary(); - InstallComponent(stanza, zipfile, installed_files); + InstallComponent(stanza, filename, installed_files); registry.RegisterModule(new InstalledModule(installed_files, bundled, DateTime.Now)); } } // Done! Save our registry changes! + // TODO: From a transaction standpoint, we probably don't want to save the registry, + // and instead want something higher up the call-chain to do that. + registry_manager.Save(); if (onReportModInstalled != null) @@ -513,23 +488,56 @@ internal static ModuleInstallDescriptor GenerateDefaultInstall(string identifier /// /// Install the component described in the stanza. /// Modifies the supplied module_files to contain the files installed. + /// This method should be avoided, as it may be removed in the future. /// - private void InstallComponent(InstallableDescriptor stanza, ZipFile zipfile, + internal void InstallComponent(InstallableDescriptor stanza, string zip_filename, Dictionary module_files) { - List files = FindInstallableFiles(stanza, zipfile, ksp); - foreach (var file in files) { + // TODO: Can we deprecate this code now please? + log.Warn("Soon to be deprecated method InstallComponent called"); - User.WriteLine(" * Copying " + file.source.Name); + using (ZipFile zipfile = new ZipFile(File.OpenRead(zip_filename))) + { + List files = FindInstallableFiles(stanza, zipfile, ksp); + + foreach (var file in files) { + + // TODO: I'd like to replace this with a log.Info + User.WriteLine(" * Copying " + file.source.Name); + + CopyZipEntry(zipfile, file.source, file.destination, file.makedir); + + // TODO: We really should be computing sha1sums again! + module_files.Add(file.destination, new InstalledModuleFile + { + sha1_sum = "" //Sha1Sum (currentTransaction.OpenFile(fullPath).TemporaryPath) + }); + } + } + } - CopyZipEntry(zipfile, file.source, file.destination, file.makedir); + /// + /// Installs the module from the zipfile provided, updating the supplied list of installed files provided. + /// + /// Propagates up a BadMetadataKraken if our install metadata is bad. + /// + internal void InstallModule(CkanModule module, string zip_filename, Dictionary installed_files) + { + using (ZipFile zipfile = new ZipFile(File.OpenRead(zip_filename))) + { + List files = FindInstallableFiles(module, zipfile, ksp); - // TODO: We really should be computing sha1sums again! - module_files.Add(file.destination, new InstalledModuleFile + foreach (var file in files) { - sha1_sum = "" //Sha1Sum (currentTransaction.OpenFile(fullPath).TemporaryPath) - }); + log.InfoFormat("Copying {0}", file.source.Name); + CopyZipEntry(zipfile,file.source,file.destination,file.makedir); + installed_files.Add(file.destination, new InstalledModuleFile + { + // TODO: Re-enable checksums!!! + sha1_sum = "" //Sha1Sum (currentTransaction.OpenFile(fullPath).TemporaryPath) + }); + } } } From 1ae1d899176a6954990a144e4d7275a41f7c9edf Mon Sep 17 00:00:00 2001 From: Paul Fenwick Date: Mon, 27 Oct 2014 13:58:34 +1100 Subject: [PATCH 03/11] Migrated RegistryVersionNotSupported to a Kraken. --- CKAN/CKAN/Registry.cs | 13 +------------ CKAN/CKAN/Types/Kraken.cs | 14 +++++++++++++- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/CKAN/CKAN/Registry.cs b/CKAN/CKAN/Registry.cs index 5fc214bdc4..579c26927c 100644 --- a/CKAN/CKAN/Registry.cs +++ b/CKAN/CKAN/Registry.cs @@ -5,17 +5,6 @@ namespace CKAN { - - internal class RegistryVersionNotSupportedException : Exception - { - public int requested_version; - - public RegistryVersionNotSupportedException(int v) - { - requested_version = v; - } - } - public class Registry { private const int LATEST_REGISTRY_VERSION = 0; @@ -40,7 +29,7 @@ Dictionary available /* TODO: support more than just the latest version */ if (version != LATEST_REGISTRY_VERSION) { - throw new RegistryVersionNotSupportedException(version); + throw new RegistryVersionNotSupportedKraken(version); } installed_modules = mods; diff --git a/CKAN/CKAN/Types/Kraken.cs b/CKAN/CKAN/Types/Kraken.cs index caba06a106..4c73c0c85d 100644 --- a/CKAN/CKAN/Types/Kraken.cs +++ b/CKAN/CKAN/Types/Kraken.cs @@ -82,5 +82,17 @@ public TransactionalKraken(string reason = null, Exception inner_exception = nul } } -} + /// + /// Thrown if we try to load an incompatible CKAN registry. + /// + public class RegistryVersionNotSupportedKraken : Kraken + { + public int requested_version; + public RegistryVersionNotSupportedKraken(int v, string reason = null, Exception inner_exception = null) + :base(reason, inner_exception) + { + requested_version = v; + } + } +} From 5fb917e54192aae94b17d7a002d4c0713edf4791 Mon Sep 17 00:00:00 2001 From: Paul Fenwick Date: Mon, 27 Oct 2014 14:03:55 +1100 Subject: [PATCH 04/11] Registry documentation. --- CKAN/CKAN/Registry.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CKAN/CKAN/Registry.cs b/CKAN/CKAN/Registry.cs index 579c26927c..a1d245fa68 100644 --- a/CKAN/CKAN/Registry.cs +++ b/CKAN/CKAN/Registry.cs @@ -5,6 +5,13 @@ namespace CKAN { + /// + /// This is the CKAN registry. All the modules that we know about or have installed + /// are contained in here. + /// + /// Please try to avoid accessing the attributes directly. Right now they're public + /// so our JSON layer can access them, but in the future they will become private. + /// public class Registry { private const int LATEST_REGISTRY_VERSION = 0; From 0183a2baeb89121a246a4f1dad4b764691f121f1 Mon Sep 17 00:00:00 2001 From: Paul Fenwick Date: Mon, 27 Oct 2014 14:05:05 +1100 Subject: [PATCH 05/11] Moved all registry related files into their own directory. - AvailableModule.cs - InstalledModule.cs - RegistryManager.cs - Registry.cs No changes except for the rename. --- CKAN/CKAN/CKAN.csproj | 8 ++++---- CKAN/CKAN/{ => Registry}/AvailableModule.cs | 0 CKAN/CKAN/{ => Registry}/InstalledModule.cs | 0 CKAN/CKAN/{ => Registry}/Registry.cs | 0 CKAN/CKAN/{ => Registry}/RegistryManager.cs | 0 5 files changed, 4 insertions(+), 4 deletions(-) rename CKAN/CKAN/{ => Registry}/AvailableModule.cs (100%) rename CKAN/CKAN/{ => Registry}/InstalledModule.cs (100%) rename CKAN/CKAN/{ => Registry}/Registry.cs (100%) rename CKAN/CKAN/{ => Registry}/RegistryManager.cs (100%) diff --git a/CKAN/CKAN/CKAN.csproj b/CKAN/CKAN/CKAN.csproj index 78f40fc3e4..db8b4f9c88 100644 --- a/CKAN/CKAN/CKAN.csproj +++ b/CKAN/CKAN/CKAN.csproj @@ -54,12 +54,8 @@ - - - - @@ -75,6 +71,10 @@ + + + + diff --git a/CKAN/CKAN/AvailableModule.cs b/CKAN/CKAN/Registry/AvailableModule.cs similarity index 100% rename from CKAN/CKAN/AvailableModule.cs rename to CKAN/CKAN/Registry/AvailableModule.cs diff --git a/CKAN/CKAN/InstalledModule.cs b/CKAN/CKAN/Registry/InstalledModule.cs similarity index 100% rename from CKAN/CKAN/InstalledModule.cs rename to CKAN/CKAN/Registry/InstalledModule.cs diff --git a/CKAN/CKAN/Registry.cs b/CKAN/CKAN/Registry/Registry.cs similarity index 100% rename from CKAN/CKAN/Registry.cs rename to CKAN/CKAN/Registry/Registry.cs diff --git a/CKAN/CKAN/RegistryManager.cs b/CKAN/CKAN/Registry/RegistryManager.cs similarity index 100% rename from CKAN/CKAN/RegistryManager.cs rename to CKAN/CKAN/Registry/RegistryManager.cs From 2e535c5f596803e6f2a67add1a102c7a8b76b0d5 Mon Sep 17 00:00:00 2001 From: Paul Fenwick Date: Mon, 27 Oct 2014 14:11:57 +1100 Subject: [PATCH 06/11] Registry: Documentation tweak, tweaked one if -> else if. --- CKAN/CKAN/Registry/Registry.cs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/CKAN/CKAN/Registry/Registry.cs b/CKAN/CKAN/Registry/Registry.cs index a1d245fa68..60c14924c1 100644 --- a/CKAN/CKAN/Registry/Registry.cs +++ b/CKAN/CKAN/Registry/Registry.cs @@ -272,6 +272,11 @@ public void DeregisterModule(string module) installed_modules.Remove(module); } + /// + /// Registers the given DLL as having been installed. This provides some support + /// for pre-CKAN modules. + /// + /// Path. public void RegisterDll(string path) { // Oh my, does .NET support extended regexps (like Perl?), we could use them right now. @@ -291,6 +296,9 @@ public void RegisterDll(string path) installed_dlls[modName] = relPath; } + /// + /// Clears knowledge of all DLLs from the registry. + /// public void ClearDlls() { installed_dlls = new Dictionary(); @@ -298,18 +306,16 @@ public void ClearDlls() /// /// Returns the installed version of a given mod. - /// If the mod was autodetected (but present), a "0" is returned. + /// If the mod was autodetected (but present), a version of type `DllVersion` is returned. /// If the mod is not found, a null will be returned. /// - /// The version. - /// Mod name. public Version InstalledVersion(string modName) { if (installed_modules.ContainsKey(modName)) { return installed_modules[modName].source_module.version; } - if (installed_dlls.ContainsKey(modName)) + else if (installed_dlls.ContainsKey(modName)) { return new DllVersion(); } From 927ac3a36d1037dc8dbc3e5eb527d52afec38e99 Mon Sep 17 00:00:00 2001 From: Paul Fenwick Date: Mon, 27 Oct 2014 14:33:16 +1100 Subject: [PATCH 07/11] Added Registry.Installed(), which returns all installed mods. Includes both real mods, and DLLs. Does not yet return virtual packages --- CKAN/CKAN/Registry/InstalledModule.cs | 6 ++++++ CKAN/CKAN/Registry/Registry.cs | 23 +++++++++++++++++++++++ CKAN/CmdLine/Main.cs | 25 +++++++------------------ 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/CKAN/CKAN/Registry/InstalledModule.cs b/CKAN/CKAN/Registry/InstalledModule.cs index 5608ef0749..54590c1ab9 100644 --- a/CKAN/CKAN/Registry/InstalledModule.cs +++ b/CKAN/CKAN/Registry/InstalledModule.cs @@ -8,6 +8,12 @@ public class InstalledModuleFile public string sha1_sum; } + /// + /// A simple clss that represents an installed module. Includes the time of installation, + /// the module itself, and a list of files installed with it. + /// + /// Primarily used by the Registry class. + /// public class InstalledModule { public DateTime install_time; diff --git a/CKAN/CKAN/Registry/Registry.cs b/CKAN/CKAN/Registry/Registry.cs index 60c14924c1..8749f86d62 100644 --- a/CKAN/CKAN/Registry/Registry.cs +++ b/CKAN/CKAN/Registry/Registry.cs @@ -304,6 +304,29 @@ public void ClearDlls() installed_dlls = new Dictionary(); } + /// + /// Returns a dictionary of all modules installed, along with their + /// versions. This includes DLLs, which will have a version type of `DllVersion`. + /// + public Dictionary Installed() + { + var installed = new Dictionary(); + + // Index our DLLs, as much as we dislike them. + foreach (var dllinfo in installed_dlls) + { + installed[dllinfo.Key] = new DllVersion(); + } + + // Index our installed modules (which may overwrite the installed DLLs) + foreach (var modinfo in installed_modules) + { + installed[modinfo.Key] = modinfo.Value.source_module.version; + } + + return installed; + } + /// /// Returns the installed version of a given mod. /// If the mod was autodetected (but present), a version of type `DllVersion` is returned. diff --git a/CKAN/CmdLine/Main.cs b/CKAN/CmdLine/Main.cs index 5176b2fc5b..73244160f2 100644 --- a/CKAN/CmdLine/Main.cs +++ b/CKAN/CmdLine/Main.cs @@ -215,31 +215,20 @@ private static int Scan() private static int List() { - string ksp_path = KSPManager.CurrentInstance.GameDir(); + KSP ksp = KSPManager.CurrentInstance; - User.WriteLine("\nKSP found at {0}\n", ksp_path); - User.WriteLine("KSP Version: {0}\n", KSPManager.CurrentInstance.Version()); + User.WriteLine("\nKSP found at {0}\n", ksp.GameDir()); + User.WriteLine("KSP Version: {0}\n",ksp.Version()); - RegistryManager registry_manager = RegistryManager.Instance(KSPManager.CurrentInstance); - Registry registry = registry_manager.registry; + Registry registry = RegistryManager.Instance(ksp).registry; User.WriteLine("Installed Modules:\n"); - foreach (InstalledModule mod in registry.installed_modules.Values) - { - User.WriteLine("* {0} {1}", mod.source_module.identifier, mod.source_module.version); - } - - User.WriteLine("\nDetected DLLs (`ckan scan` to rebuild):\n"); + var installed = new SortedDictionary(registry.Installed()); - // Walk our dlls, but *don't* show anything we've already displayed as - // a module. - foreach (string dll in registry.installed_dlls.Keys) + foreach (var mod in installed) { - if (! registry.installed_modules.ContainsKey(dll)) - { - User.WriteLine("* {0}", dll); - } + User.WriteLine("* {0} {1}", mod.Key, mod.Value); } // Blank line at the end makes for nicer looking output. From 81ccad8f842f961a7a03174fe19e424f01a5a198 Mon Sep 17 00:00:00 2001 From: Paul Fenwick Date: Mon, 27 Oct 2014 14:54:42 +1100 Subject: [PATCH 08/11] Registry.Installed() now returns provided modules as well. This also adds a new `ProvidesVersion` class to facilitate this. Includes tests for ProvidesVersion. --- CKAN/CKAN/Registry/Registry.cs | 23 +++++++++++++++++++++-- CKAN/CKAN/Types/Version.cs | 33 +++++++++++++++++++++++++++------ CKAN/Tests/CKAN/Version.cs | 14 ++++++++++++++ 3 files changed, 62 insertions(+), 8 deletions(-) diff --git a/CKAN/CKAN/Registry/Registry.cs b/CKAN/CKAN/Registry/Registry.cs index 8749f86d62..42b9b78f08 100644 --- a/CKAN/CKAN/Registry/Registry.cs +++ b/CKAN/CKAN/Registry/Registry.cs @@ -306,7 +306,9 @@ public void ClearDlls() /// /// Returns a dictionary of all modules installed, along with their - /// versions. This includes DLLs, which will have a version type of `DllVersion`. + /// versions. + /// This includes DLLs, which will have a version type of `DllVersion`. + /// This includes Provides, which will have a version of `ProvidesVersion`. /// public Dictionary Installed() { @@ -318,7 +320,24 @@ public Dictionary Installed() installed[dllinfo.Key] = new DllVersion(); } - // Index our installed modules (which may overwrite the installed DLLs) + // Index our provides list, so users can see virtual packages + foreach (var modinfo in installed_modules) + { + Module module = modinfo.Value.source_module; + + // Skip if this module provides nothing. + if (module.provides == null) + { + continue; + } + + foreach (string provided in module.provides) + { + installed[provided] = new ProvidesVersion(module.identifier); + } + } + + // Index our installed modules (which may overwrite the installed DLLs and provides) foreach (var modinfo in installed_modules) { installed[modinfo.Key] = modinfo.Value.source_module.version; diff --git a/CKAN/CKAN/Types/Version.cs b/CKAN/CKAN/Types/Version.cs index 84108c2266..340fb9b0a0 100644 --- a/CKAN/CKAN/Types/Version.cs +++ b/CKAN/CKAN/Types/Version.cs @@ -161,15 +161,36 @@ static Comparison NumComp(string v1, string v2) { } } - // This class represents a DllVersion. They don't have real - // version numbers or anything. + /// + /// This class represents a DllVersion. They don't have real + /// version numbers or anything + /// + public class DllVersion : Version { + public DllVersion() :base("0") + { + } - // We probably need a better way of doing these, but believe - // it or not this is better than what we had before. + override public string ToString() + { + return "autodetected dll"; + } + } - public class DllVersion : Version { - public DllVersion() : base("autodetected_dll") { + /// + /// This class represents a virtual version that was provided by + /// another module. + /// + public class ProvidesVersion : Version { + internal string provided_by; + + public ProvidesVersion(string provided_by) :base("0") + { + this.provided_by = provided_by; + } + override public string ToString() + { + return string.Format("provided by {0}", provided_by); } } } diff --git a/CKAN/Tests/CKAN/Version.cs b/CKAN/Tests/CKAN/Version.cs index 062fe4ca90..3b1893dac5 100644 --- a/CKAN/Tests/CKAN/Version.cs +++ b/CKAN/Tests/CKAN/Version.cs @@ -45,5 +45,19 @@ public void Epoch() Assert.That(v1.IsLessThan(v2)); } + + [Test] + public void DllVersion() + { + var v1 = new CKAN.DllVersion(); + Assert.AreEqual("autodetected dll", v1.ToString()); + } + + [Test] + public void ProvidesVersion() + { + var v1 = new CKAN.ProvidesVersion("SomeModule"); + Assert.AreEqual("provided by SomeModule", v1.ToString()); + } } } \ No newline at end of file From abe67686f0393b978e6a5642bd385e2f037f15e0 Mon Sep 17 00:00:00 2001 From: Paul Fenwick Date: Mon, 27 Oct 2014 15:08:23 +1100 Subject: [PATCH 09/11] Registry.InstalledVersion now reports provided modules. New internal Provides() method helps us find these. --- CKAN/CKAN/Registry/Registry.cs | 39 ++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/CKAN/CKAN/Registry/Registry.cs b/CKAN/CKAN/Registry/Registry.cs index 42b9b78f08..9c7f93f436 100644 --- a/CKAN/CKAN/Registry/Registry.cs +++ b/CKAN/CKAN/Registry/Registry.cs @@ -321,6 +321,31 @@ public Dictionary Installed() } // Index our provides list, so users can see virtual packages + foreach (var provided in Provided()) + { + installed[provided.Key] = provided.Value; + } + + // Index our installed modules (which may overwrite the installed DLLs and provides) + foreach (var modinfo in installed_modules) + { + installed[modinfo.Key] = modinfo.Value.source_module.version; + } + + return installed; + } + + /// + /// Returns a dictionary of provided (virtual) modules, and a + /// ProvidesVersion indicating what provides them. + /// + + // TODO: In the future it would be nice to cache this list, and mark it for rebuild + // if our installed modules change. + internal Dictionary Provided() + { + var installed = new Dictionary(); + foreach (var modinfo in installed_modules) { Module module = modinfo.Value.source_module; @@ -337,18 +362,13 @@ public Dictionary Installed() } } - // Index our installed modules (which may overwrite the installed DLLs and provides) - foreach (var modinfo in installed_modules) - { - installed[modinfo.Key] = modinfo.Value.source_module.version; - } - return installed; } /// /// Returns the installed version of a given mod. /// If the mod was autodetected (but present), a version of type `DllVersion` is returned. + /// If the mod is provided by another mod (ie, virtual) a type of ProvidesVersion is returned. /// If the mod is not found, a null will be returned. /// public Version InstalledVersion(string modName) @@ -362,6 +382,13 @@ public Version InstalledVersion(string modName) return new DllVersion(); } + var provided = Provided(); + + if (provided.ContainsKey(modName)) + { + return provided[modName]; + } + return null; } From 1b10ad7a582111ad551ada4242d816576e864c29 Mon Sep 17 00:00:00 2001 From: Paul Fenwick Date: Mon, 27 Oct 2014 15:22:27 +1100 Subject: [PATCH 10/11] Don't register DLLs which are part of an installed mod. The solution I've got walks through all installed mods every time we find a DLL, making it O(N^2). That would be terrible, but DLL scans are so rare, and the number of mods we have is so small, that I'm considering it perfectly acceptable until someone complains about the speed. Chances are we may completely remove support for pre-CKAN DLLs in the future, anyway. Closes #141 after the user does a `ckan scan`. --- CKAN/CKAN/Registry/Registry.cs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/CKAN/CKAN/Registry/Registry.cs b/CKAN/CKAN/Registry/Registry.cs index 9c7f93f436..8ffaf8cf8c 100644 --- a/CKAN/CKAN/Registry/Registry.cs +++ b/CKAN/CKAN/Registry/Registry.cs @@ -275,10 +275,23 @@ public void DeregisterModule(string module) /// /// Registers the given DLL as having been installed. This provides some support /// for pre-CKAN modules. + /// + /// Does nothing if the DLL is already part of an installed module. /// - /// Path. public void RegisterDll(string path) { + // TODO: This is awful, as it's O(N^2), but it means we never index things which are + // part of another mod. + + foreach (var mod in installed_modules.Values) + { + if (mod.installed_files.ContainsKey(path)) + { + log.DebugFormat("Not registering {0}, it's part of {1}", path, mod.source_module); + return; + } + } + // Oh my, does .NET support extended regexps (like Perl?), we could use them right now. Match match = Regex.Match(path, @".*?(?:^|/)GameData/((?:.*/|)([^.]+).*dll)"); @@ -287,10 +300,11 @@ public void RegisterDll(string path) if (modName.Length == 0 || relPath.Length == 0) { + log.WarnFormat("Attempted to index {0} which is not a DLL", path); return; } - User.WriteLine("Registering {0} -> {1}", modName, relPath); + log.InfoFormat("Registering {0} -> {1}", modName, path); // We're fine if we overwrite an existing key. installed_dlls[modName] = relPath; From 374af3362907d38fabc8121ffd663a1c2fccd0b0 Mon Sep 17 00:00:00 2001 From: Paul Fenwick Date: Mon, 27 Oct 2014 16:45:49 +1100 Subject: [PATCH 11/11] Wrap many disposable resources in `using` blocks. Hopefully this will provide some relief to #179. (I won't say it closes it, but it would be nice if it does!) --- CKAN/CKAN/FilesystemTransaction.cs | 6 ++++ CKAN/CKAN/ModuleInstaller.cs | 32 ++++++++++------- CKAN/CKAN/NetAsyncDownloader.cs | 1 - CKAN/CKAN/Repo.cs | 57 ++++++++++++++++-------------- CKAN/GUI/Configuration.cs | 18 +++++++--- CKAN/KerbalStuff/MainClass.cs | 28 ++++++++------- CKAN/Tests/CKAN/ModuleInstaller.cs | 40 +++++++++++---------- 7 files changed, 107 insertions(+), 75 deletions(-) diff --git a/CKAN/CKAN/FilesystemTransaction.cs b/CKAN/CKAN/FilesystemTransaction.cs index 1665c4062d..6437237940 100644 --- a/CKAN/CKAN/FilesystemTransaction.cs +++ b/CKAN/CKAN/FilesystemTransaction.cs @@ -84,6 +84,12 @@ public void Snapshot(string path) fileManager.Snapshot(path); } + /// + /// Opens a file for writing as part of this transaction. + /// The caller is responsible for closing and disposing of the filehandle as appropraite. + /// + + // TODO: neverOverwrite seems to be unused here, remove it. public FileStream OpenFileWrite(string path, bool neverOverwrite = true) { if (TempFiles.ContainsKey(path)) diff --git a/CKAN/CKAN/ModuleInstaller.cs b/CKAN/CKAN/ModuleInstaller.cs index 413b589e54..9291138bb6 100644 --- a/CKAN/CKAN/ModuleInstaller.cs +++ b/CKAN/CKAN/ModuleInstaller.cs @@ -426,7 +426,12 @@ private string Sha1Sum(string path) try { - return BitConverter.ToString(hasher.ComputeHash(File.OpenRead(path))); + using (var fh = File.OpenRead(path)) + { + string sha1 = BitConverter.ToString(hasher.ComputeHash(fh)); + fh.Close(); + return sha1; + } } catch { @@ -497,7 +502,7 @@ internal void InstallComponent(InstallableDescriptor stanza, string zip_filename // TODO: Can we deprecate this code now please? log.Warn("Soon to be deprecated method InstallComponent called"); - using (ZipFile zipfile = new ZipFile(File.OpenRead(zip_filename))) + using (ZipFile zipfile = new ZipFile(zip_filename)) { List files = FindInstallableFiles(stanza, zipfile, ksp); @@ -524,7 +529,7 @@ internal void InstallComponent(InstallableDescriptor stanza, string zip_filename /// internal void InstallModule(CkanModule module, string zip_filename, Dictionary installed_files) { - using (ZipFile zipfile = new ZipFile(File.OpenRead(zip_filename))) + using (ZipFile zipfile = new ZipFile(zip_filename)) { List files = FindInstallableFiles(module, zipfile, ksp); @@ -694,7 +699,7 @@ internal static List FindInstallableFiles(CkanModule module, Zi internal static List FindInstallableFiles(CkanModule module, string zip_filename, KSP ksp) { // `using` makes sure our zipfile gets closed when we exit this block. - using (ZipFile zipfile = new ZipFile(File.OpenRead(zip_filename))) + using (ZipFile zipfile = new ZipFile(zip_filename)) { log.DebugFormat("Searching {0} using {1} as module", zip_filename, module); return FindInstallableFiles(module, zipfile, ksp); @@ -728,16 +733,17 @@ private void CopyZipEntry(ZipFile zipfile, ZipEntry entry, string fullPath, bool } // It's a file! Prepare the streams - Stream zipStream = zipfile.GetInputStream(entry); - - FileStream output = currentTransaction.OpenFileWrite(fullPath); - - // Copy - zipStream.CopyTo(output); + using (Stream zipStream = zipfile.GetInputStream(entry)) + using (FileStream output = currentTransaction.OpenFileWrite(fullPath)) + { + // Copy + zipStream.CopyTo(output); - // Tidy up. - zipStream.Close(); - output.Close(); + // Tidy up. Possibly not needed inside a `using` block, but almost certainly + // doesn't hurt. + zipStream.Close(); + output.Close(); + } } return; diff --git a/CKAN/CKAN/NetAsyncDownloader.cs b/CKAN/CKAN/NetAsyncDownloader.cs index 83cc5492b0..a3d9a572e9 100644 --- a/CKAN/CKAN/NetAsyncDownloader.cs +++ b/CKAN/CKAN/NetAsyncDownloader.cs @@ -15,7 +15,6 @@ public struct NetAsyncDownloaderDownloadPart public long bytesLeft; public int bytesPerSecond; public Exception error; - public FileStream fileStream; public int lastProgressUpdateSize; public DateTime lastProgressUpdateTime; public string path; diff --git a/CKAN/CKAN/Repo.cs b/CKAN/CKAN/Repo.cs index 218b2708f0..4757ad2c6b 100644 --- a/CKAN/CKAN/Repo.cs +++ b/CKAN/CKAN/Repo.cs @@ -62,45 +62,50 @@ internal static void UpdateRegistry(Uri repo, Registry registry) string repo_file = Net.Download(repo); - // Open our zip file for processing - var zipfile = new ZipFile(File.OpenRead(repo_file)); + using (var zipfile = new ZipFile(repo_file)) + { + // Clear our list of known modules. + registry.ClearAvailable(); - // Clear our list of known modules. - registry.ClearAvailable(); + // Walk the archive, looking for .ckan files. + string filter = @"\.ckan$"; - // Walk the archive, looking for .ckan files. - string filter = @"\.ckan$"; + foreach (ZipEntry entry in zipfile) + { + string filename = entry.Name; - foreach (ZipEntry entry in zipfile) - { - string filename = entry.Name; + // Skip things we don't want. + if (! Regex.IsMatch(filename, filter)) + { + log.DebugFormat("Skipping archive entry {0}", filename); + continue; + } - // Skip things we don't want. - if (! Regex.IsMatch(filename, filter)) - { - log.DebugFormat("Skipping archive entry {0}", filename); - continue; - } + log.DebugFormat("Reading CKAN data from {0}", filename); - log.DebugFormat("Reading CKAN data from {0}", filename); + // Read each file into a string. + string metadata_json; + using (var stream = new StreamReader(zipfile.GetInputStream(entry))) + { + metadata_json = stream.ReadToEnd(); + stream.Close(); + } - // Read each file into a string. - string metadata_json = new StreamReader(zipfile.GetInputStream(entry)).ReadToEnd(); + log.Debug("Converting from JSON..."); - log.Debug("Converting from JSON..."); + CkanModule module = CkanModule.FromJson(metadata_json); - CkanModule module = CkanModule.FromJson(metadata_json); + log.InfoFormat("Found {0} version {1}", module.identifier, module.version); - log.InfoFormat("Found {0} version {1}", module.identifier, module.version); + // Hooray! Now save it in our registry. + registry.AddAvailable(module); + } - // Hooray! Now save it in our registry. - registry.AddAvailable(module); + zipfile.Close(); } - // Clean up! - zipfile.Close(); + // Remove our downloaded meta-data now we've processed it. File.Delete(repo_file); - } } } \ No newline at end of file diff --git a/CKAN/GUI/Configuration.cs b/CKAN/GUI/Configuration.cs index 4fe28b3829..b4fdc53066 100644 --- a/CKAN/GUI/Configuration.cs +++ b/CKAN/GUI/Configuration.cs @@ -33,7 +33,14 @@ public static Configuration LoadOrCreateConfiguration(string path, string defaul public static Configuration LoadConfiguration(string path) { var serializer = new XmlSerializer(typeof (Configuration)); - var configuration = (Configuration) serializer.Deserialize(new StreamReader(path)); + + Configuration configuration; + using (var stream = new StreamReader(path)) + { + configuration = (Configuration) serializer.Deserialize(stream); + stream.Close(); + } + configuration.m_Path = path; return configuration; } @@ -41,9 +48,12 @@ public static Configuration LoadConfiguration(string path) public static void SaveConfiguration(Configuration configuration, string path) { var serializer = new XmlSerializer(typeof (Configuration)); - var writer = new StreamWriter(path); - serializer.Serialize(writer, configuration); - writer.Close(); + + using (var writer = new StreamWriter(path)) + { + serializer.Serialize(writer, configuration); + writer.Close(); + } } } } \ No newline at end of file diff --git a/CKAN/KerbalStuff/MainClass.cs b/CKAN/KerbalStuff/MainClass.cs index 99d2d1ca0f..8303bdd52d 100644 --- a/CKAN/KerbalStuff/MainClass.cs +++ b/CKAN/KerbalStuff/MainClass.cs @@ -180,25 +180,29 @@ private static JObject DeserializeFromStream(Stream stream) /// private static JObject ExtractCkanInfo(string filename) { - var zipfile = new ZipFile(File.OpenRead(filename)); - - foreach (ZipEntry entry in zipfile) + using (var zipfile = new ZipFile(filename)) { - // Skip everything but embedded .ckan files. - if (! Regex.IsMatch(entry.Name, ".CKAN$", RegexOptions.IgnoreCase)) + foreach (ZipEntry entry in zipfile) { - continue; - } + // Skip everything but embedded .ckan files. + if (! Regex.IsMatch(entry.Name, ".CKAN$", RegexOptions.IgnoreCase)) + { + continue; + } - log.DebugFormat("Reading {0}", entry.Name); + log.DebugFormat("Reading {0}", entry.Name); - Stream zipStream = zipfile.GetInputStream(entry); + using (Stream zipStream = zipfile.GetInputStream(entry)) + { + JObject meta_ckan = DeserializeFromStream(zipStream); + zipStream.Close(); - JObject meta_ckan = DeserializeFromStream(zipStream); - - return meta_ckan; + return meta_ckan; + } + } } + // No metadata found? Uh oh! throw new MetadataNotFoundKraken(filename); } diff --git a/CKAN/Tests/CKAN/ModuleInstaller.cs b/CKAN/Tests/CKAN/ModuleInstaller.cs index 4f1abe2b6c..e32f912d08 100644 --- a/CKAN/Tests/CKAN/ModuleInstaller.cs +++ b/CKAN/Tests/CKAN/ModuleInstaller.cs @@ -14,31 +14,33 @@ public class ModuleInstaller public void GenerateDefaultInstall() { string filename = Tests.TestData.DogeCoinFlagZip(); - var zipfile = new ZipFile(File.OpenRead(filename)); - - ModuleInstallDescriptor stanza = CKAN.ModuleInstaller.GenerateDefaultInstall("DogeCoinFlag", zipfile); + using (var zipfile = new ZipFile(filename)) + { + ModuleInstallDescriptor stanza = CKAN.ModuleInstaller.GenerateDefaultInstall("DogeCoinFlag", zipfile); - TestDogeCoinStanza(stanza); + TestDogeCoinStanza(stanza); - // Same again, but screwing up the case (we see this *all the time*) - ModuleInstallDescriptor stanza2 = CKAN.ModuleInstaller.GenerateDefaultInstall("DogecoinFlag", zipfile); + // Same again, but screwing up the case (we see this *all the time*) + ModuleInstallDescriptor stanza2 = CKAN.ModuleInstaller.GenerateDefaultInstall("DogecoinFlag", zipfile); - TestDogeCoinStanza(stanza2); + TestDogeCoinStanza(stanza2); - // Now what happens if we can't find what to install? + // Now what happens if we can't find what to install? - Assert.Throws(delegate { - CKAN.ModuleInstaller.GenerateDefaultInstall("Xyzzy", zipfile); - }); + Assert.Throws(delegate + { + CKAN.ModuleInstaller.GenerateDefaultInstall("Xyzzy", zipfile); + }); - // Make sure the FNFKraken looks like what we expect. - try - { - CKAN.ModuleInstaller.GenerateDefaultInstall("Xyzzy",zipfile); - } - catch (FileNotFoundKraken kraken) - { - Assert.AreEqual("Xyzzy", kraken.file); + // Make sure the FNFKraken looks like what we expect. + try + { + CKAN.ModuleInstaller.GenerateDefaultInstall("Xyzzy", zipfile); + } + catch (FileNotFoundKraken kraken) + { + Assert.AreEqual("Xyzzy", kraken.file); + } } }