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

Fix issues when provides is removed #2740

Merged

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Apr 22, 2019

Problem

If a module provides another module and then later we remove that property, installing a mod that depends on the provided mod can prompt you to make the same decision multiple times and raise exceptions like this:

Unhandled Exception: System.ArgumentException: Already contains module:TextureReplacerReplaced
   at CKAN.RelationshipResolver.Add(CkanModule module, SelectionReason reason)
   at CKAN.RelationshipResolver.AddModulesToInstall(IEnumerable`1 modules)
   at CKAN.RelationshipResolver..ctor(IEnumerable`1 modulesToInstall, IEnumerable`1 modulesToRemove, RelationshipResolverOptions options, IRegistryQuerier registry, KspVersionCriteria kspversion)
   at CKAN.RelationshipResolver..ctor(IEnumerable`1 modulesToInstall, IEnumerable`1 modulesToRemove, RelationshipResolverOptions options, IRegistryQuerier registry, KspVersionCriteria kspversion)
   at CKAN.ModuleInstaller.InstallList(List`1 modules, RelationshipResolverOptions options, IDownloader downloader)
   at CKAN.CmdLine.Install.RunCommand(KSP ksp, Object raw_options)
   at CKAN.CmdLine.Install.RunCommand(KSP ksp, Object raw_options)
   at CKAN.CmdLine.Install.RunCommand(KSP ksp, Object raw_options)
   at CKAN.CmdLine.Install.RunCommand(KSP ksp, Object raw_options)
   at CKAN.CmdLine.Install.RunCommand(KSP ksp, Object raw_options)
   at CKAN.CmdLine.Install.RunCommand(KSP ksp, Object raw_options)
   at CKAN.CmdLine.MainClass.RunSimpleAction(Options cmdline, CommonOptions options, String[] args, IUser user, KSPManager manager)
   at CKAN.CmdLine.MainClass.Execute(KSPManager manager, CommonOptions opts, String[] args)
   at CKAN.CmdLine.MainClass.Main(String[] args)

Cause

See KSP-CKAN/NetKAN#7149 for full details of the situation that brought this to our attention.

After the user chooses a CkanModule, CmdLine discards the full CkanModule in favor of just the identifier here:

// Add the module to the list.
options.modules.Add(ex.modules[result].identifier);

When we next try to resolve that install command, it uses the latest version of the module rather than the one the user selected.

GUI is slightly more complicated. When you check the box for a module that depends on a virtual module, the prompt to choose a module appears immediately, and the checkbox of the user's choice is checked in the mod list.

CKAN/GUI/MainDialogs.cs

Lines 86 to 87 in 7b9da8c

if (module != null)
MarkModForInstall(module.identifier);

This approach can't work in the general case, because there is no checkbox for installing an older version of a module (related to why I haven't taken a stab at #2715). Instead, this can only ever install the latest version, even if it's not what the relationship resolver gave us to select.

(ConsoleUI already handles this mostly correctly, except that the user is allowed to upgrade into an inconsistent state, which I'm not attempting to address here.)

Changes

Now CmdLine tracks its list of mods to install as a list of CkanModules, and if the user needs to choose a providing mod, the exact CkanModule they chose is added to that list and we try again.

Now GUI won't prompt you to choose providing mods until you apply the changes, and it will install the version of the modules that the relationship resolver gives it.

Fixes #2738.

Two tests are deleted:

  • ComputeChangeSetFromModList_WithConflictingMods_ThrowsInconsistentKraken wasn't working as intended, see MainModList test is not working correctly #2741. It was trying to test that conflicts between installed and installing mods are caught, which is not broken in these changes:
    image
  • TooManyProvidesCallsHandlers was trying to confirm that the provides-resolution prompt would appear when you click a mod with a virtual dependency, but that's just not how it works anymore. Now we handle them at install time.

Fixes #2741.

@HebaruSan HebaruSan added Bug GUI Issues affecting the interactive GUI Cmdline Issues affecting the command line Pull request labels Apr 22, 2019
@Olympic1 Olympic1 removed the Bug label Apr 22, 2019
@HebaruSan HebaruSan added Bug In progress We're still working on this labels Apr 22, 2019
@HebaruSan HebaruSan added In progress We're still working on this and removed In progress We're still working on this labels Apr 22, 2019
@HebaruSan
Copy link
Member Author

HebaruSan commented Apr 23, 2019

Let's do a 1.26.2 release without this. It's kind of a big change and it would be nice to have a bug-fix release with no new bugs in it.

@politas
Copy link
Member

politas commented May 1, 2019

In accordance with new release process, it'll be 1.26.2 that we release, making 1.26.1 the dev release that allows people to use the built-in upgrader. Sorry I've been a bit absent - messing about with switching to a different Distro - I'm giving Manjaro a try.

@HebaruSan HebaruSan removed the In progress We're still working on this label May 7, 2019
@HebaruSan HebaruSan force-pushed the fix/install-with-inconsistent-provides branch from c8852b6 to 18cf66e Compare May 8, 2019 19:40
@DasSkelett
Copy link
Member

DasSkelett commented May 28, 2019

Just tested if this would fix KSP-CKAN/NetKAN#7210 (comment),
but it crashes:

342 [1] INFO CKAN.CmdLine.CommonOptions (null) - Verbose logging enabled
3034 [1] INFO CKAN.RegistryManager (null) - Loaded CKAN registry at /home/user/git-reps/Fake-1.7.0/CKAN/registry.json
3392 [1] INFO CKAN.RegistryManager (null) - Saving CKAN registry at /home/user/git-reps/Fake-1.7.0/CKAN/registry.json
3977 [1] INFO CKAN.CmdLine.Install (null) - Installing from local CKAN file "/media/user/HDD-1TB/git-reps/CKAN/_build/InfernalRoboticsNext-v3.0.0.ckan"
3984 [1] INFO CKAN.CmdLine.MainClass (null) - CKAN exiting.

Unhandled Exception:
CKAN.ModuleNotFoundKraken: Module not found: InfernalRoboticsNext=v3.0.0 
  at CKAN.Registry.LatestAvailable (System.String module, CKAN.Versioning.KspVersionCriteria ksp_version, CKAN.RelationshipDescriptor relationship_descriptor) [0x00034] in <447d8868b5384b5c91c32e107f6dca01>:0 
  at CKAN.CmdLine.Install+<>c__DisplayClass7_0.<RunCommand>b__0 (System.String id) [0x00012] in <447d8868b5384b5c91c32e107f6dca01>:0 
  at System.Linq.Enumerable+SelectListIterator`2[TSource,TResult].ToList () [0x0002a] in <35ad2ebb203f4577b22a9d30eca3ec1f>:0 
  at System.Linq.Enumerable.ToList[TSource] (System.Collections.Generic.IEnumerable`1[T] source) [0x0001f] in <35ad2ebb203f4577b22a9d30eca3ec1f>:0 
  at CKAN.CmdLine.Install.RunCommand (CKAN.KSP ksp, System.Object raw_options) [0x0029a] in <447d8868b5384b5c91c32e107f6dca01>:0 
  at CKAN.CmdLine.MainClass.RunSimpleAction (CKAN.CmdLine.Options cmdline, CKAN.CmdLine.CommonOptions options, System.String[] args, CKAN.IUser user, CKAN.KSPManager manager) [0x00391] in <447d8868b5384b5c91c32e107f6dca01>:0 
  at CKAN.CmdLine.MainClass.Execute (CKAN.KSPManager manager, CKAN.CmdLine.CommonOptions opts, System.String[] args) [0x00281] in <447d8868b5384b5c91c32e107f6dca01>:0 
  at CKAN.CmdLine.MainClass.Main (System.String[] args) [0x000a1] in <447d8868b5384b5c91c32e107f6dca01>:0 
[ERROR] FATAL UNHANDLED EXCEPTION: CKAN.ModuleNotFoundKraken: Module not found: InfernalRoboticsNext=v3.0.0 
  at CKAN.Registry.LatestAvailable (System.String module, CKAN.Versioning.KspVersionCriteria ksp_version, CKAN.RelationshipDescriptor relationship_descriptor) [0x00034] in <447d8868b5384b5c91c32e107f6dca01>:0 
  at CKAN.CmdLine.Install+<>c__DisplayClass7_0.<RunCommand>b__0 (System.String id) [0x00012] in <447d8868b5384b5c91c32e107f6dca01>:0 
  at System.Linq.Enumerable+SelectListIterator`2[TSource,TResult].ToList () [0x0002a] in <35ad2ebb203f4577b22a9d30eca3ec1f>:0 
  at System.Linq.Enumerable.ToList[TSource] (System.Collections.Generic.IEnumerable`1[T] source) [0x0001f] in <35ad2ebb203f4577b22a9d30eca3ec1f>:0 
  at CKAN.CmdLine.Install.RunCommand (CKAN.KSP ksp, System.Object raw_options) [0x0029a] in <447d8868b5384b5c91c32e107f6dca01>:0 
  at CKAN.CmdLine.MainClass.RunSimpleAction (CKAN.CmdLine.Options cmdline, CKAN.CmdLine.CommonOptions options, System.String[] args, CKAN.IUser user, CKAN.KSPManager manager) [0x00391] in <447d8868b5384b5c91c32e107f6dca01>:0 
  at CKAN.CmdLine.MainClass.Execute (CKAN.KSPManager manager, CKAN.CmdLine.CommonOptions opts, System.String[] args) [0x00281] in <447d8868b5384b5c91c32e107f6dca01>:0 
  at CKAN.CmdLine.MainClass.Main (System.String[] args) [0x000a1] in <447d8868b5384b5c91c32e107f6dca01>:0 

"Normal" output would be:

347 [1] INFO CKAN.CmdLine.CommonOptions (null) - Verbose logging enabled
2989 [1] INFO CKAN.RegistryManager (null) - Loaded CKAN registry at /home/user/git-reps/Fake-1.7.0/CKAN/registry.json
3352 [1] INFO CKAN.RegistryManager (null) - Saving CKAN registry at /home/user/git-reps/Fake-1.7.0/CKAN/registry.json
3894 [1] INFO CKAN.CmdLine.Install (null) - Installing from local CKAN file "/media/user/HDD-1TB/git-reps/CKAN/_build/InfernalRoboticsNext-v3.0.0.ckan"
3916 [1] INFO CKAN.RelationshipResolver (null) - KerbalJointReinforcementNext v4.0.10 would cause conflicts, excluding it from consideration
3921 [1] INFO CKAN.RelationshipResolver (null) - KerbalJointReinforcementNext v4.0.10 would cause conflicts, excluding it from consideration
About to install...
* Infernal Robotics - Next v3.0.0 (cached)
* TweakScale - Rescale Everything! v2.4.2.0 (cached)
* Module Manager 4.0.2 (cached)
Continue? [Y/n] n
Installation canceled at user request.
254218 [1] INFO CKAN.CmdLine.MainClass (null) - CKAN exiting.

GUI doesn't crash, behaves like master.

@Olympic1
Copy link
Member

@DasSkelett are you still getting that error?

@DasSkelett
Copy link
Member

Well, this specific case is solved, and I can't recreate the crash with custom relationships for now.
Seems to be a weird edge case with provides/conflicts.

@Olympic1 Olympic1 merged commit 18cf66e into KSP-CKAN:master Jul 6, 2019
Olympic1 added a commit that referenced this pull request Jul 6, 2019
@HebaruSan HebaruSan deleted the fix/install-with-inconsistent-provides branch July 6, 2019 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Cmdline Issues affecting the command line GUI Issues affecting the interactive GUI Pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MainModList test is not working correctly Modding Bug that repeats "TextureReplacerReplaced"
4 participants