From 8d737766024936e820f92a4bb873742133346360 Mon Sep 17 00:00:00 2001 From: Paul Hebble Date: Sun, 31 Dec 2017 07:04:07 +0000 Subject: [PATCH] Don't clear available modules till after the new list is ready --- Cmdline/Action/Update.cs | 2 +- Core/Net/Repo.cs | 340 +++++++++++--------- Core/Registry/Registry.cs | 13 +- Tests/Core/Net/NetAsyncModulesDownloader.cs | 9 +- Tests/Core/Net/Repo.cs | 19 +- Tests/Core/Relationships/SanityChecker.cs | 9 +- 6 files changed, 218 insertions(+), 174 deletions(-) diff --git a/Cmdline/Action/Update.cs b/Cmdline/Action/Update.cs index 022cfb6fc8..573d11c89e 100644 --- a/Cmdline/Action/Update.cs +++ b/Cmdline/Action/Update.cs @@ -136,7 +136,7 @@ private void UpdateRepository(CKAN.KSP ksp, string repository = null) var updated = repository == null ? CKAN.Repo.UpdateAllRepositories(registry_manager, ksp, user) - : CKAN.Repo.Update(registry_manager, ksp, user, true, repository); + : CKAN.Repo.Update(registry_manager, ksp, user, repository); user.RaiseMessage("Updated information on {0} available modules", updated); } diff --git a/Core/Net/Repo.cs b/Core/Net/Repo.cs index c9f5f4c34f..4e34745ef2 100644 --- a/Core/Net/Repo.cs +++ b/Core/Net/Repo.cs @@ -25,106 +25,47 @@ public static class Repo // Forward to keep existing code compiling, will be removed soon. public static readonly Uri default_ckan_repo = CKAN.Repository.default_ckan_repo_uri; - internal static void ProcessRegistryMetadataFromJSON(string metadata, Registry registry, string filename) - { - log.DebugFormat("Converting metadata from JSON."); - - try - { - CkanModule module = CkanModule.FromJson(metadata); - log.DebugFormat("Found {0} version {1}", module.identifier, module.version); - registry.AddAvailable(module); - } - catch (Exception exception) - { - // Alas, we can get exceptions which *wrap* our exceptions, - // because json.net seems to enjoy wrapping rather than propagating. - // See KSP-CKAN/CKAN-meta#182 as to why we need to walk the whole - // exception stack. - - bool handled = false; - - while (exception != null) - { - if (exception is UnsupportedKraken || exception is BadMetadataKraken) - { - // Either of these can be caused by data meant for future - // clients, so they're not really warnings, they're just - // informational. - - log.InfoFormat("Skipping {0} : {1}", filename, exception.Message); - - // I'd *love a way to "return" from the catch block. - handled = true; - break; - } - - // Look further down the stack. - exception = exception.InnerException; - } - - // If we haven't handled our exception, then it really was exceptional. - if (handled == false) - { - if (exception == null) - { - // Had exception, walked exception tree, reached leaf, got stuck. - log.ErrorFormat("Error processing {0} (exception tree leaf)", filename); - } - else - { - // In case whatever's calling us is lazy in error reporting, we'll - // report that we've got an issue here. - log.ErrorFormat("Error processing {0} : {1}", filename, exception.Message); - } - - throw; - } - } - } - /// - /// Download and update the local CKAN meta-info. - /// Optionally takes a URL to the zipfile repo to download. - /// Returns the number of unique modules updated. + /// Download and update the local CKAN meta-info. + /// Optionally takes a URL to the zipfile repo to download. + /// Returns the number of unique modules updated. /// public static int UpdateAllRepositories(RegistryManager registry_manager, KSP ksp, IUser user) { - // If we handle multiple repositories, we will call ClearRegistry() ourselves... - registry_manager.registry.ClearAvailable(); - // TODO this should already give us a pre-sorted list SortedDictionary sortedRepositories = registry_manager.registry.Repositories; + List allAvail = new List(); foreach (KeyValuePair repository in sortedRepositories) { log.InfoFormat("About to update {0}", repository.Value.name); - UpdateRegistry(repository.Value.uri, registry_manager.registry, ksp, user, false); + List avail = UpdateRegistry(repository.Value.uri, ksp, user); log.InfoFormat("Updated {0}", repository.Value.name); + // Merge all the lists + allAvail.AddRange(avail); + } + // Save allAvail to the registry if we found anything + if (allAvail.Count > 0) + { + registry_manager.registry.SetAllAvailable(allAvail); + // Save our changes. + registry_manager.Save(enforce_consistency: false); } - - // Save our changes. - registry_manager.Save(enforce_consistency: false); ShowUserInconsistencies(registry_manager.registry, user); - // Return how many we got! - return registry_manager.registry.Available(ksp.VersionCriteria()).Count; - } - - public static int Update(RegistryManager registry_manager, KSP ksp, IUser user, Boolean clear = true, string repo = null) - { - if (repo == null) + List metadataChanges = GetChangedInstalledModules(registry_manager.registry); + if (metadataChanges.Count > 0) { - return Update(registry_manager, ksp, user, clear, (Uri)null); + HandleModuleChanges(metadataChanges, user, ksp); } - return Update(registry_manager, ksp, user, clear, new Uri(repo)); + // Return how many we got! + return registry_manager.registry.Available(ksp.VersionCriteria()).Count; } /// - /// Updates the supplied registry from the URL given. - /// This does not *save* the registry. For that, you probably want Repo.Update + /// Retrieve available modules from the URL given. /// - internal static void UpdateRegistry(Uri repo, Registry registry, KSP ksp, IUser user, Boolean clear = true) + private static List UpdateRegistry(Uri repo, KSP ksp, IUser user) { // Use this opportunity to also update the build mappings... kind of hacky ServiceLocator.Container.Resolve().Refresh(); @@ -139,30 +80,40 @@ internal static void UpdateRegistry(Uri repo, Registry registry, KSP ksp, IUser catch (System.Net.WebException) { user.RaiseMessage("Connection to {0} could not be established.", repo); - return; - } - - // Clear our list of known modules. - if (clear) - { - registry.ClearAvailable(); + return null; } // Check the filetype. FileType type = FileIdentifier.IdentifyFile(repo_file); + List newAvailable = null; switch (type) { case FileType.TarGz: - UpdateRegistryFromTarGz(repo_file, registry); + newAvailable = UpdateRegistryFromTarGz(repo_file); break; case FileType.Zip: - UpdateRegistryFromZip(repo_file, registry); + newAvailable = UpdateRegistryFromZip(repo_file); break; } - List metadataChanges = new List(); + // Remove our downloaded meta-data now we've processed it. + // Seems weird to do this as part of a transaction, but Net.Download uses them, so let's be consistent. + file_transaction.Delete(repo_file); + return newAvailable; + } + + /// + /// Find installed modules that have different metadata in their equivalent available module + /// + /// Registry to scan + /// + /// List of CkanModules that are available and have changed metadata + /// + private static List GetChangedInstalledModules(Registry registry) + { + List metadataChanges = new List(); foreach (InstalledModule installedModule in registry.InstalledModules) { string identifier = installedModule.identifier; @@ -170,7 +121,7 @@ internal static void UpdateRegistry(Uri repo, Registry registry, KSP ksp, IUser Version installedVersion = registry.InstalledVersion(identifier); if (!(registry.available_modules.ContainsKey(identifier))) { - log.InfoFormat("UpdateRegistry, module {0}, version {1} not in repository ({2})", identifier, installedVersion, repo); + log.InfoFormat("UpdateRegistry, module {0}, version {1} not in registry", identifier, installedVersion); continue; } @@ -189,62 +140,63 @@ internal static void UpdateRegistry(Uri repo, Registry registry, KSP ksp, IUser metadataChanges.Add(registry.available_modules[identifier].module_version[installedVersion]); } } + return metadataChanges; + } - if (metadataChanges.Any()) + /// + /// Resolve differences between installed and available metadata for given ModuleInstaller + /// + /// List of modules that changed + /// Object for user interaction callbacks + /// Game instance + private static void HandleModuleChanges(List metadataChanges, IUser user, KSP ksp) + { + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < metadataChanges.Count; i++) { - StringBuilder sb = new StringBuilder(); - - for (int i = 0; i < metadataChanges.Count; i++) - { - CkanModule module = metadataChanges[i]; - - sb.AppendLine(string.Format("- {0} {1}", module.identifier, module.version)); - } + CkanModule module = metadataChanges[i]; + sb.AppendLine(string.Format("- {0} {1}", module.identifier, module.version)); + } - if (user.RaiseYesNoDialog(string.Format(@"The following mods have had their metadata changed since last update: + if (user.RaiseYesNoDialog(string.Format(@"The following mods have had their metadata changed since last update: {0} You should reinstall them in order to preserve consistency with the repository. Do you wish to reinstall now?", sb))) + { + ModuleInstaller installer = ModuleInstaller.GetInstance(ksp, new NullUser()); + // New upstream metadata may break the consistency of already installed modules + // e.g. if user installs modules A and B and then later up A is made to conflict with B + // This is perfectly normal and shouldn't produce an error, therefore we skip enforcing + // consistency. However, we will show the user any inconsistencies later on. + + // Use the identifiers so we use the overload that actually resolves relationships + // Do each changed module one at a time so a failure of one doesn't cause all the others to fail + foreach (string changedIdentifier in metadataChanges.Select(i => i.identifier)) { - ModuleInstaller installer = ModuleInstaller.GetInstance(ksp, new NullUser()); - // New upstream metadata may break the consistency of already installed modules - // e.g. if user installs modules A and B and then later up A is made to conflict with B - // This is perfectly normal and shouldn't produce an error, therefore we skip enforcing - // consistency. However, we will show the user any inconsistencies later on. - - // Use the identifiers so we use the overload that actually resolves relationships - // Do each changed module one at a time so a failure of one doesn't cause all the others to fail - foreach (string changedIdentifier in metadataChanges.Select(i => i.identifier)) + try { - try - { - installer.Upgrade( - new[] { changedIdentifier }, - new NetAsyncModulesDownloader(new NullUser()), - enforceConsistency: false - ); - } - // Thrown when a dependency couldn't be satisfied - catch(ModuleNotFoundKraken) - { - log.WarnFormat("Skipping installation of {0} due to relationship error.", changedIdentifier); - user.RaiseMessage("Skipping installation of {0} due to relationship error.", changedIdentifier); - } - // Thrown when a conflicts relationship is violated - catch (InconsistentKraken) - { - log.WarnFormat("Skipping installation of {0} due to relationship error.", changedIdentifier); - user.RaiseMessage("Skipping installation of {0} due to relationship error.", changedIdentifier); - } + installer.Upgrade( + new[] { changedIdentifier }, + new NetAsyncModulesDownloader(new NullUser()), + enforceConsistency: false + ); + } + // Thrown when a dependency couldn't be satisfied + catch (ModuleNotFoundKraken) + { + log.WarnFormat("Skipping installation of {0} due to relationship error.", changedIdentifier); + user.RaiseMessage("Skipping installation of {0} due to relationship error.", changedIdentifier); + } + // Thrown when a conflicts relationship is violated + catch (InconsistentKraken) + { + log.WarnFormat("Skipping installation of {0} due to relationship error.", changedIdentifier); + user.RaiseMessage("Skipping installation of {0} due to relationship error.", changedIdentifier); } } } - - // Remove our downloaded meta-data now we've processed it. - // Seems weird to do this as part of a transaction, but Net.Download uses them, so let's be consistent. - file_transaction.Delete(repo_file); } private static bool MetadataEquals(CkanModule metadata, CkanModule oldMetadata) @@ -393,7 +345,28 @@ private static bool RelationshipsAreEquivalent(List a, L return true; } - private static int Update(RegistryManager registry_manager, KSP ksp, IUser user, Boolean clear = true, Uri repo = null) + /// + /// Set a registry's available modules to the list from just one repo + /// + /// Manager of the regisry of interest + /// Game instance + /// Object for user interaction callbacks + /// Repository to check + /// + /// Number of modules found in repo + /// + public static int Update(RegistryManager registry_manager, KSP ksp, IUser user, string repo = null) + { + if (repo == null) + { + return Update(registry_manager, ksp, user, (Uri)null); + } + + return Update(registry_manager, ksp, user, new Uri(repo)); + } + + // Same as above, just with a Uri instead of string for the repo + public static int Update(RegistryManager registry_manager, KSP ksp, IUser user, Uri repo = null) { // Use our default repo, unless we've been told otherwise. if (repo == null) @@ -401,10 +374,13 @@ private static int Update(RegistryManager registry_manager, KSP ksp, IUser user, repo = default_ckan_repo; } - UpdateRegistry(repo, registry_manager.registry, ksp, user, clear); - - // Save our changes! - registry_manager.Save(enforce_consistency: false); + List newAvail = UpdateRegistry(repo, ksp, user); + if (newAvail != null && newAvail.Count > 0) + { + registry_manager.registry.SetAllAvailable(newAvail); + // Save our changes! + registry_manager.Save(enforce_consistency: false); + } ShowUserInconsistencies(registry_manager.registry, user); @@ -413,14 +389,13 @@ private static int Update(RegistryManager registry_manager, KSP ksp, IUser user, } /// - /// Updates the supplied registry from the supplied zip file. - /// This will *clear* the registry of available modules first. - /// This does not *save* the registry. For that, you probably want Repo.Update + /// Returns available modules from the supplied tar.gz file. /// - internal static void UpdateRegistryFromTarGz(string path, Registry registry) + private static List UpdateRegistryFromTarGz(string path) { log.DebugFormat("Starting registry update from tar.gz file: \"{0}\".", path); + List modules = new List(); // Open the gzip'ed file. using (Stream inputStream = File.OpenRead(path)) { @@ -474,22 +449,26 @@ internal static void UpdateRegistryFromTarGz(string path, Registry registry) // Convert the buffer data to a string. string metadata_json = Encoding.ASCII.GetString(buffer); - ProcessRegistryMetadataFromJSON(metadata_json, registry, filename); + CkanModule module = ProcessRegistryMetadataFromJSON(metadata_json, filename); + if (module != null) + { + modules.Add(module); + } } } } } + return modules; } /// - /// Updates the supplied registry from the supplied zip file. - /// This will *clear* the registry of available modules first. - /// This does not *save* the registry. For that, you probably want Repo.Update + /// Returns available modules from the supplied zip file. /// - internal static void UpdateRegistryFromZip(string path, Registry registry) + private static List UpdateRegistryFromZip(string path) { log.DebugFormat("Starting registry update from zip file: \"{0}\".", path); + List modules = new List(); using (var zipfile = new ZipFile(path)) { // Walk the archive, looking for .ckan files. @@ -516,13 +495,76 @@ internal static void UpdateRegistryFromZip(string path, Registry registry) stream.Close(); } - ProcessRegistryMetadataFromJSON(metadata_json, registry, filename); + CkanModule module = ProcessRegistryMetadataFromJSON(metadata_json, filename); + if (module != null) + { + modules.Add(module); + } } zipfile.Close(); } + return modules; } + private static CkanModule ProcessRegistryMetadataFromJSON(string metadata, string filename) + { + log.DebugFormat("Converting metadata from JSON."); + + try + { + CkanModule module = CkanModule.FromJson(metadata); + log.DebugFormat("Found {0} version {1}", module.identifier, module.version); + return module; + } + catch (Exception exception) + { + // Alas, we can get exceptions which *wrap* our exceptions, + // because json.net seems to enjoy wrapping rather than propagating. + // See KSP-CKAN/CKAN-meta#182 as to why we need to walk the whole + // exception stack. + + bool handled = false; + + while (exception != null) + { + if (exception is UnsupportedKraken || exception is BadMetadataKraken) + { + // Either of these can be caused by data meant for future + // clients, so they're not really warnings, they're just + // informational. + + log.InfoFormat("Skipping {0} : {1}", filename, exception.Message); + + // I'd *love a way to "return" from the catch block. + handled = true; + break; + } + + // Look further down the stack. + exception = exception.InnerException; + } + + // If we haven't handled our exception, then it really was exceptional. + if (handled == false) + { + if (exception == null) + { + // Had exception, walked exception tree, reached leaf, got stuck. + log.ErrorFormat("Error processing {0} (exception tree leaf)", filename); + } + else + { + // In case whatever's calling us is lazy in error reporting, we'll + // report that we've got an issue here. + log.ErrorFormat("Error processing {0} : {1}", filename, exception.Message); + } + + throw; + } + return null; + } + } private static void ShowUserInconsistencies(Registry registry, IUser user) { diff --git a/Core/Registry/Registry.cs b/Core/Registry/Registry.cs index fa623abd7d..a5da74740e 100644 --- a/Core/Registry/Registry.cs +++ b/Core/Registry/Registry.cs @@ -347,13 +347,16 @@ private void SealionTransaction() #endregion - /// - /// Clears all available modules from the registry. - /// - public void ClearAvailable() + public void SetAllAvailable(IEnumerable newAvail) { SealionTransaction(); + // Clear current modules available_modules = new Dictionary(); + // Add the new modules + foreach (CkanModule module in newAvail) + { + AddAvailable(module); + } } /// @@ -365,7 +368,7 @@ public void AddAvailable(CkanModule module) var identifier = module.identifier; // If we've never seen this module before, create an entry for it. - if (! available_modules.ContainsKey(identifier)) + if (!available_modules.ContainsKey(identifier)) { log.DebugFormat("Adding new available module {0}", identifier); available_modules[identifier] = new AvailableModule(identifier); diff --git a/Tests/Core/Net/NetAsyncModulesDownloader.cs b/Tests/Core/Net/NetAsyncModulesDownloader.cs index be9f327c08..876ac9580d 100644 --- a/Tests/Core/Net/NetAsyncModulesDownloader.cs +++ b/Tests/Core/Net/NetAsyncModulesDownloader.cs @@ -14,6 +14,7 @@ namespace Tests.Core.Net public class NetAsyncModulesDownloader { + private CKAN.RegistryManager manager; private CKAN.Registry registry; private DisposableKSP ksp; private CKAN.IDownloader async; @@ -29,13 +30,12 @@ public void Setup() // Give us a registry to play with. ksp = new DisposableKSP(); - registry = CKAN.RegistryManager.Instance(ksp.KSP).registry; - registry.ClearAvailable(); + manager = CKAN.RegistryManager.Instance(ksp.KSP); + registry = manager.registry; registry.ClearDlls(); registry.Installed().Clear(); - // Make sure we have a registry we can use. - CKAN.Repo.UpdateRegistry(TestData.TestKANZip(), registry, ksp.KSP, new NullUser()); + CKAN.Repo.Update(manager, ksp.KSP, new NullUser(), TestData.TestKANZip()); // Ready our downloader. async = new CKAN.NetAsyncModulesDownloader(new NullUser()); @@ -128,4 +128,3 @@ public void RandSdownload() } } - diff --git a/Tests/Core/Net/Repo.cs b/Tests/Core/Net/Repo.cs index bd583edcc2..e4f0b3be84 100644 --- a/Tests/Core/Net/Repo.cs +++ b/Tests/Core/Net/Repo.cs @@ -8,6 +8,7 @@ namespace Tests.Core.Net [TestFixture] public class Repo { + private CKAN.RegistryManager manager; private CKAN.Registry registry; private DisposableKSP ksp; @@ -15,8 +16,8 @@ public class Repo public void Setup() { ksp = new DisposableKSP(); - registry = CKAN.RegistryManager.Instance(ksp.KSP).registry; - registry.ClearAvailable(); + manager = CKAN.RegistryManager.Instance(ksp.KSP); + registry = manager.registry; registry.ClearDlls(); registry.Installed().Clear(); } @@ -30,7 +31,7 @@ public void TearDown() [Test] public void UpdateRegistryTarGz() { - CKAN.Repo.UpdateRegistry(TestData.TestKANTarGz(), registry, ksp.KSP, new NullUser()); + CKAN.Repo.Update(manager, ksp.KSP, new NullUser(), TestData.TestKANTarGz()); // Test we've got an expected module. CkanModule far = registry.LatestAvailable("FerramAerospaceResearch", new KspVersionCriteria(KspVersion.Parse("0.25.0"))); @@ -41,10 +42,10 @@ public void UpdateRegistryTarGz() [Test] public void UpdateRegistryZip() { - CKAN.Repo.UpdateRegistry(TestData.TestKANZip(), registry, ksp.KSP, new NullUser()); + CKAN.Repo.Update(manager, ksp.KSP, new NullUser(), TestData.TestKANZip()); // Test we've got an expected module. - CkanModule far = registry.LatestAvailable("FerramAerospaceResearch", new KspVersionCriteria (KspVersion.Parse("0.25.0"))); + CkanModule far = registry.LatestAvailable("FerramAerospaceResearch", new KspVersionCriteria(KspVersion.Parse("0.25.0"))); Assert.AreEqual("v0.14.3.2", far.version.ToString()); } @@ -54,7 +55,7 @@ public void BadKanTarGz() { Assert.DoesNotThrow(delegate { - CKAN.Repo.UpdateRegistry(TestData.BadKANTarGz(), registry, ksp.KSP, new NullUser()); + CKAN.Repo.Update(manager, ksp.KSP, new NullUser(), TestData.BadKANTarGz()); }); } @@ -62,9 +63,9 @@ public void BadKanTarGz() public void BadKanZip() { Assert.DoesNotThrow(delegate - { - CKAN.Repo.UpdateRegistry(TestData.BadKANZip(), registry, ksp.KSP, new NullUser()); - }); + { + CKAN.Repo.Update(manager, ksp.KSP, new NullUser(), TestData.BadKANZip()); + }); } } } diff --git a/Tests/Core/Relationships/SanityChecker.cs b/Tests/Core/Relationships/SanityChecker.cs index f005096a20..3ba1366dee 100644 --- a/Tests/Core/Relationships/SanityChecker.cs +++ b/Tests/Core/Relationships/SanityChecker.cs @@ -13,6 +13,7 @@ namespace Tests.Core.Relationships [TestFixture] public class SanityChecker { + private CKAN.RegistryManager manager; private CKAN.Registry registry; private DisposableKSP ksp; @@ -21,12 +22,11 @@ public void Setup() { ksp = new DisposableKSP(); - registry = CKAN.RegistryManager.Instance(ksp.KSP).registry; - registry.ClearAvailable(); + manager = CKAN.RegistryManager.Instance(ksp.KSP); + registry = manager.registry; registry.ClearDlls(); registry.Installed().Clear(); - - Repo.UpdateRegistry(TestData.TestKANZip(), registry, ksp.KSP, new NullUser()); + CKAN.Repo.Update(manager, ksp.KSP, new NullUser(), TestData.TestKANZip()); } [Test] @@ -191,4 +191,3 @@ private static void TestDepends(List to_remove, List mods, L } } } -