From fafdb09cdd2ba24c4192bd4423a6107eefef9fa3 Mon Sep 17 00:00:00 2001 From: DasSkelett Date: Thu, 13 Feb 2020 01:23:14 +0100 Subject: [PATCH 1/3] Respect installing modules during dependency resolution Currently when uninstalling dependencies of a mod and installing a new mod that provides these dependencies, CKAN will remove the dependent mod, even though its dependencies will be satisfied after the process completed. Example: Scatterer depends on Scatterer-config and Scatterer-sunflare. Spectra provides both of these modules. When you have Scatterer installed with its default config + sunflare and mark Spectra for installation, CKAN correctly reports conflicts. Then you de-select the dafault config + sunflare so they get uninstalled. Even though Spectra provides these modules too (which triggered the TooManyModsProvide conflict), CKAN (both GUI and ConsoleUI) wants to remove Scatterer. This is due to the mods that are about to be installed not being respected in `FindReverseDependencies()` > `FindUnsatisfiedDepends()` > `MatchesAny()`. This commit adds an optional argument to `FindUnsatisfiedDependencies()`, `IEnumerable modulesToInstall`, which will be added to the list of "currently installed" mods, so `FindUnsatisfiedDepends()` considers them and their provided modules. --- ConsoleUI/DependencyScreen.cs | 2 +- ConsoleUI/InstallScreen.cs | 4 +-- Core/ModuleInstaller.cs | 16 ++++------ Core/Registry/IRegistryQuerier.cs | 4 ++- Core/Registry/Registry.cs | 37 ++++++++++++++-------- Core/Relationships/RelationshipResolver.cs | 10 +++--- Core/Relationships/SanityChecker.cs | 1 - GUI/Main/MainInstall.cs | 2 +- GUI/Main/MainModList.cs | 10 +++--- Tests/Core/ModuleInstaller.cs | 6 ++-- 10 files changed, 50 insertions(+), 42 deletions(-) diff --git a/ConsoleUI/DependencyScreen.cs b/ConsoleUI/DependencyScreen.cs index f2f2ffb22c..ae67c2139b 100644 --- a/ConsoleUI/DependencyScreen.cs +++ b/ConsoleUI/DependencyScreen.cs @@ -7,7 +7,7 @@ namespace CKAN.ConsoleUI { /// - /// Screen for letting the user choose additional pacakges to install + /// Screen for letting the user choose additional packages to install /// based on others they've selected /// public class DependencyScreen : ConsoleScreen { diff --git a/ConsoleUI/InstallScreen.cs b/ConsoleUI/InstallScreen.cs index 20c7539360..8aa6aa8e7f 100644 --- a/ConsoleUI/InstallScreen.cs +++ b/ConsoleUI/InstallScreen.cs @@ -54,14 +54,14 @@ public override void Run(Action process = null) } // FUTURE: BackgroundWorker - + HashSet possibleConfigOnlyDirs = null; RegistryManager regMgr = RegistryManager.Instance(manager.CurrentInstance); ModuleInstaller inst = ModuleInstaller.GetInstance(manager.CurrentInstance, manager.Cache, this); inst.onReportModInstalled = OnModInstalled; if (plan.Remove.Count > 0) { - inst.UninstallList(plan.Remove, ref possibleConfigOnlyDirs, regMgr); + inst.UninstallList(plan.Remove, ref possibleConfigOnlyDirs, regMgr, true, plan.Install); plan.Remove.Clear(); } NetAsyncModulesDownloader dl = new NetAsyncModulesDownloader(this, manager.Cache); diff --git a/Core/ModuleInstaller.cs b/Core/ModuleInstaller.cs index 80e55420bf..75a06bc3fd 100644 --- a/Core/ModuleInstaller.cs +++ b/Core/ModuleInstaller.cs @@ -727,7 +727,10 @@ internal static void CopyZipEntry(ZipFile zipfile, ZipEntry entry, string fullPa /// This *DOES* save the registry. /// Preferred over Uninstall. /// - public void UninstallList(IEnumerable mods, ref HashSet possibleConfigOnlyDirs, RegistryManager registry_manager, bool ConfirmPrompt = true, IEnumerable installing = null) + public void UninstallList( + IEnumerable mods, ref HashSet possibleConfigOnlyDirs, + RegistryManager registry_manager, bool ConfirmPrompt = true, IEnumerable installing = null + ) { mods = mods.Memoize(); // Pre-check, have they even asked for things which are installed? @@ -740,8 +743,9 @@ public void UninstallList(IEnumerable mods, ref HashSet possible // Find all the things which need uninstalling. IEnumerable revdep = mods .Union(registry_manager.registry.FindReverseDependencies( - mods.Except(installing ?? new string[] {}))) - .Memoize(); + mods.Except(installing?.Select(m => m.identifier) ?? new string[] {}), + installing + )).Memoize(); IEnumerable goners = revdep.Union( registry_manager.registry.FindRemovableAutoInstalled( registry_manager.registry.InstalledModules.Where(im => !revdep.Contains(im.identifier))) @@ -796,12 +800,6 @@ public void UninstallList(IEnumerable mods, ref HashSet possible User.RaiseMessage("Done!\r\n"); } - public void UninstallList(string mod, ref HashSet possibleConfigOnlyDirs, RegistryManager registry_manager) - { - var list = new List { mod }; - UninstallList(list, ref possibleConfigOnlyDirs, registry_manager); - } - /// /// Uninstall the module provided. For internal use only. /// Use UninstallList for user queries, it also does dependency handling. diff --git a/Core/Registry/IRegistryQuerier.cs b/Core/Registry/IRegistryQuerier.cs index 27f1f7c640..9b9acd4da1 100644 --- a/Core/Registry/IRegistryQuerier.cs +++ b/Core/Registry/IRegistryQuerier.cs @@ -74,7 +74,9 @@ List LatestAvailableWithProvides( /// /// Finds and returns all modules that could not exist without the listed modules installed, including themselves. /// - IEnumerable FindReverseDependencies(IEnumerable modules); + IEnumerable FindReverseDependencies( + IEnumerable modulesToRemove, IEnumerable modulesToInstall = null + ); /// /// Find auto-installed modules that have no depending modules diff --git a/Core/Registry/Registry.cs b/Core/Registry/Registry.cs index 02c89a2cbc..bb814c5b2a 100644 --- a/Core/Registry/Registry.cs +++ b/Core/Registry/Registry.cs @@ -1028,44 +1028,44 @@ public List GetSanityErrors() /// Acts recursively and lazily. /// internal static IEnumerable FindReverseDependencies( - IEnumerable modules_to_remove, - IEnumerable orig_installed, + IEnumerable modulesToRemove, + IEnumerable origInstalled, IEnumerable dlls, IDictionary dlc ) { - modules_to_remove = modules_to_remove.Memoize(); - orig_installed = orig_installed.Memoize(); + modulesToRemove = modulesToRemove.Memoize(); + origInstalled = origInstalled.Memoize(); var dllSet = dlls.ToHashSet(); // The empty list has no reverse dependencies // (Don't remove broken modules if we're only installing) - if (modules_to_remove.Any()) + if (modulesToRemove.Any()) { // All modules in the input are included in the output - foreach (string starter in modules_to_remove) + foreach (string starter in modulesToRemove) { yield return starter; } while (true) { // Make our hypothetical install, and remove the listed modules from it. - HashSet hypothetical = new HashSet(orig_installed); // Clone because we alter hypothetical. - hypothetical.RemoveWhere(mod => modules_to_remove.Contains(mod.identifier)); + HashSet hypothetical = new HashSet(origInstalled); // Clone because we alter hypothetical. + hypothetical.RemoveWhere(mod => modulesToRemove.Contains(mod.identifier)); - log.DebugFormat("Started with {0}, removing {1}, and keeping {2}; our dlls are {3}", string.Join(", ", orig_installed), string.Join(", ", modules_to_remove), string.Join(", ", hypothetical), string.Join(", ", dllSet)); + log.DebugFormat("Started with {0}, removing {1}, and keeping {2}; our dlls are {3}", string.Join(", ", origInstalled), string.Join(", ", modulesToRemove), string.Join(", ", hypothetical), string.Join(", ", dllSet)); // Find what would break with this configuration. var broken = SanityChecker.FindUnsatisfiedDepends(hypothetical, dllSet, dlc) .Select(x => x.Key.identifier).ToHashSet(); // Lazily return each newly found rev dep - foreach (string newFound in broken.Except(modules_to_remove)) + foreach (string newFound in broken.Except(modulesToRemove)) { yield return newFound; } // If nothing else would break, it's just the list of modules we're removing. - HashSet to_remove = new HashSet(modules_to_remove); + HashSet to_remove = new HashSet(modulesToRemove); if (to_remove.IsSupersetOf(broken)) { @@ -1075,7 +1075,7 @@ IDictionary dlc // Otherwise, remove our broken modules as well, and recurse. broken.UnionWith(to_remove); - modules_to_remove = broken; + modulesToRemove = broken; } } } @@ -1083,10 +1083,19 @@ IDictionary dlc /// /// Return modules which are dependent on the modules passed in or modules in the return list /// - public IEnumerable FindReverseDependencies(IEnumerable modules_to_remove) + public IEnumerable FindReverseDependencies( + IEnumerable modulesToRemove, + IEnumerable modulesToInstall = null + ) { var installed = new HashSet(installed_modules.Values.Select(x => x.Module)); - return FindReverseDependencies(modules_to_remove, installed, new HashSet(installed_dlls.Keys), _installedDlcModules); + if (modulesToInstall != null) + { + // Pretend the mods we are going to install are already installed, so that dependencies that will be + // satisfied by a mod that is going to be installed count as satisfied. + installed = installed.Concat(modulesToInstall).ToHashSet(); + } + return FindReverseDependencies(modulesToRemove, installed, new HashSet(installed_dlls.Keys), _installedDlcModules); } /// diff --git a/Core/Relationships/RelationshipResolver.cs b/Core/Relationships/RelationshipResolver.cs index 6c9a2b048d..cbf88963b0 100644 --- a/Core/Relationships/RelationshipResolver.cs +++ b/Core/Relationships/RelationshipResolver.cs @@ -311,7 +311,7 @@ private void Resolve(CkanModule module, RelationshipResolverOptions options, IEn // is true. var sub_options = (RelationshipResolverOptions) options.Clone(); sub_options.with_suggests = false; - + old_stanza = old_stanza?.Memoize(); log.DebugFormat("Resolving dependencies for {0}", module.identifier); @@ -364,8 +364,7 @@ private void ResolveStanza(IEnumerable stanza, Selection else if (descriptor.ContainsAny(modlist.Keys)) { CkanModule module = modlist.Values - .Where(m => descriptor.ContainsAny(new string[] { m.identifier })) - .FirstOrDefault(); + .FirstOrDefault(m => descriptor.ContainsAny(new string[] { m.identifier })); if (options.proceed_with_inconsistencies) { conflicts.Add(new KeyValuePair(module, reason.Parent)); @@ -392,8 +391,7 @@ private void ResolveStanza(IEnumerable stanza, Selection { CkanModule module = registry.InstalledModules .Select(im => im.Module) - .Where(m => descriptor.ContainsAny(new string[] { m.identifier })) - .FirstOrDefault(); + .FirstOrDefault(m => descriptor.ContainsAny(new string[] { m.identifier })); if (options.proceed_with_inconsistencies) { conflicts.Add(new KeyValuePair(module, reason.Parent)); @@ -688,7 +686,7 @@ public override string Reason get { return " Requested by user.\r\n"; } } } - + public class NoLongerUsed: SelectionReason { public override string Reason diff --git a/Core/Relationships/SanityChecker.cs b/Core/Relationships/SanityChecker.cs index 0c3b929ff2..fdee41a29d 100644 --- a/Core/Relationships/SanityChecker.cs +++ b/Core/Relationships/SanityChecker.cs @@ -1,4 +1,3 @@ -using System; using System.Collections.Generic; using System.Linq; using CKAN.Extensions; diff --git a/GUI/Main/MainInstall.cs b/GUI/Main/MainInstall.cs index e26e4b6534..9cf202ae04 100644 --- a/GUI/Main/MainInstall.cs +++ b/GUI/Main/MainInstall.cs @@ -177,7 +177,7 @@ out Dictionary> supporters processSuccessful = false; if (!installCanceled) { - installer.UninstallList(toUninstall, ref possibleConfigOnlyDirs, registry_manager, false, toInstall.Select(m => m.identifier)); + installer.UninstallList(toUninstall, ref possibleConfigOnlyDirs, registry_manager, false, toInstall); processSuccessful = true; } } diff --git a/GUI/Main/MainModList.cs b/GUI/Main/MainModList.cs index ab61ed5e4a..b221f7bb97 100644 --- a/GUI/Main/MainModList.cs +++ b/GUI/Main/MainModList.cs @@ -848,19 +848,21 @@ public IEnumerable ComputeChangeSetFromModList( var installed_modules = registry.InstalledModules.Select(imod => imod.Module).ToDictionary(mod => mod.identifier, mod => mod); - foreach (var dependency in registry.FindReverseDependencies( + + foreach (var dependent in registry.FindReverseDependencies( modules_to_remove .Select(mod => mod.identifier) - .Except(modules_to_install.Select(m => m.identifier)) + .Except(modules_to_install.Select(m => m.identifier)), + modules_to_install )) { //TODO This would be a good place to have an event that alters the row's graphics to show it will be removed CkanModule depMod; - if (installed_modules.TryGetValue(dependency, out depMod)) + if (installed_modules.TryGetValue(dependent, out depMod)) { CkanModule module_by_version = registry.GetModuleByVersion(depMod.identifier, depMod.version) - ?? registry.InstalledModule(dependency).Module; + ?? registry.InstalledModule(dependent).Module; changeSet.Add(new ModChange(module_by_version, GUIModChangeType.Remove, null)); modules_to_remove.Add(module_by_version); } diff --git a/Tests/Core/ModuleInstaller.cs b/Tests/Core/ModuleInstaller.cs index 3592b57f9a..547fc423bd 100644 --- a/Tests/Core/ModuleInstaller.cs +++ b/Tests/Core/ModuleInstaller.cs @@ -440,7 +440,7 @@ public void UninstallModNotFound() { HashSet possibleConfigOnlyDirs = null; // This should throw, as our tidy KSP has no mods installed. - CKAN.ModuleInstaller.GetInstance(manager.CurrentInstance, manager.Cache, nullUser).UninstallList("Foo", ref possibleConfigOnlyDirs, CKAN.RegistryManager.Instance(manager.CurrentInstance)); + CKAN.ModuleInstaller.GetInstance(manager.CurrentInstance, manager.Cache, nullUser).UninstallList(new List {"Foo"}, ref possibleConfigOnlyDirs, CKAN.RegistryManager.Instance(manager.CurrentInstance)); }); manager.CurrentInstance = null; // I weep even more. @@ -520,10 +520,10 @@ public void InstallList_IdentifierEqualsVersionSyntax_InstallsModule() { $"{mod.identifier}={mod.version}" }; - + // Act inst.InstallList(modules, new RelationshipResolverOptions(), CKAN.RegistryManager.Instance(manager.CurrentInstance)); - + // Assert Assert.IsTrue(File.Exists(mod_file_path)); From 25261900617053587bc22bf8a7cf932bb903858f Mon Sep 17 00:00:00 2001 From: DasSkelett Date: Thu, 13 Feb 2020 23:17:38 +0100 Subject: [PATCH 2/3] Don't report to-be-installed modules as unsatisfied dependents --- Core/Registry/Registry.cs | 30 +++++++++++++++++------ Tests/Core/Relationships/SanityChecker.cs | 2 +- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/Core/Registry/Registry.cs b/Core/Registry/Registry.cs index bb814c5b2a..779bcf0934 100644 --- a/Core/Registry/Registry.cs +++ b/Core/Registry/Registry.cs @@ -1027,8 +1027,15 @@ public List GetSanityErrors() /// Finds and returns all modules that could not exist without the listed modules installed, including themselves. /// Acts recursively and lazily. /// + /// Modules that are about to be removed. + /// Optional list of modules that are about to be installed. + /// Modules that are already installed + /// Installed DLLs + /// Installed DLCs + /// List of modules whose dependencies are about to be or already removed. internal static IEnumerable FindReverseDependencies( IEnumerable modulesToRemove, + IEnumerable modulesToInstall, IEnumerable origInstalled, IEnumerable dlls, IDictionary dlc @@ -1050,6 +1057,12 @@ IDictionary dlc { // Make our hypothetical install, and remove the listed modules from it. HashSet hypothetical = new HashSet(origInstalled); // Clone because we alter hypothetical. + if (modulesToInstall != null) + { + // Pretend the mods we are going to install are already installed, so that dependencies that will be + // satisfied by a mod that is going to be installed count as satisfied. + hypothetical = hypothetical.Concat(modulesToInstall).ToHashSet(); + } hypothetical.RemoveWhere(mod => modulesToRemove.Contains(mod.identifier)); log.DebugFormat("Started with {0}, removing {1}, and keeping {2}; our dlls are {3}", string.Join(", ", origInstalled), string.Join(", ", modulesToRemove), string.Join(", ", hypothetical), string.Join(", ", dllSet)); @@ -1058,6 +1071,13 @@ IDictionary dlc var broken = SanityChecker.FindUnsatisfiedDepends(hypothetical, dllSet, dlc) .Select(x => x.Key.identifier).ToHashSet(); + if (modulesToInstall != null) + { + // We added modules to the hypothetical list which are not yet installed, + // but not their dependencies (we don't know them yet). + // Thus the SanityChecker wants to remove them again -> don't let him. + broken = broken.Except(modulesToInstall.Select(m => m.identifier)).ToHashSet(); + } // Lazily return each newly found rev dep foreach (string newFound in broken.Except(modulesToRemove)) { @@ -1089,13 +1109,7 @@ public IEnumerable FindReverseDependencies( ) { var installed = new HashSet(installed_modules.Values.Select(x => x.Module)); - if (modulesToInstall != null) - { - // Pretend the mods we are going to install are already installed, so that dependencies that will be - // satisfied by a mod that is going to be installed count as satisfied. - installed = installed.Concat(modulesToInstall).ToHashSet(); - } - return FindReverseDependencies(modulesToRemove, installed, new HashSet(installed_dlls.Keys), _installedDlcModules); + return FindReverseDependencies(modulesToRemove, modulesToInstall, installed, new HashSet(installed_dlls.Keys), _installedDlcModules); } /// @@ -1121,7 +1135,7 @@ IDictionary dlc var instCkanMods = installedModules.Select(im => im.Module); return autoInstMods.Where( im => autoInstIds.IsSupersetOf(FindReverseDependencies( - new List { im.identifier }, instCkanMods, dlls, dlc))); + new List { im.identifier }, null, instCkanMods, dlls, dlc))); } /// diff --git a/Tests/Core/Relationships/SanityChecker.cs b/Tests/Core/Relationships/SanityChecker.cs index f5863c7da7..c74293fbb5 100644 --- a/Tests/Core/Relationships/SanityChecker.cs +++ b/Tests/Core/Relationships/SanityChecker.cs @@ -340,7 +340,7 @@ private static void TestDepends( var dll_count = dlls.Count; var mods_count = mods.Count; - var results = CKAN.Registry.FindReverseDependencies(to_remove, mods, dlls, dlc); + var results = CKAN.Registry.FindReverseDependencies(to_remove, null, mods, dlls, dlc); // Make sure nothing changed. Assert.AreEqual(remove_count, to_remove.Count, message + " remove count"); From 7ae5ba606ef3dea03c05baf78e4e5b52ad972100 Mon Sep 17 00:00:00 2001 From: DasSkelett Date: Sat, 15 Feb 2020 20:01:14 +0100 Subject: [PATCH 3/3] Interset broken with 'origInstalled' instead of removing 'modulesToInstall' --- Core/Registry/Registry.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Core/Registry/Registry.cs b/Core/Registry/Registry.cs index 779bcf0934..fcc8438408 100644 --- a/Core/Registry/Registry.cs +++ b/Core/Registry/Registry.cs @@ -1073,10 +1073,10 @@ IDictionary dlc if (modulesToInstall != null) { - // We added modules to the hypothetical list which are not yet installed, - // but not their dependencies (we don't know them yet). - // Thus the SanityChecker wants to remove them again -> don't let him. - broken = broken.Except(modulesToInstall.Select(m => m.identifier)).ToHashSet(); + // Make sure to only report modules as broken if they are actually currently installed. + // This is mainly to remove the modulesToInstall again which we added + // earlier to the hypothetical list. + broken.IntersectWith(origInstalled.Select(m => m.identifier)); } // Lazily return each newly found rev dep foreach (string newFound in broken.Except(modulesToRemove))