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

Respect installing modules during dependency resolution #3002

Merged
merged 3 commits into from
Feb 15, 2020

Conversation

DasSkelett
Copy link
Member

@DasSkelett DasSkelett commented Feb 13, 2020

Problem

Found out thanks to this comment on the Spectra forum thread.

Currently when uninstalling dependencies of a mod and installing a new mod that also provides these dependencies, CKAN will remove the dependent mod, even though its dependencies will be satisfied after the process completed, thanks to the new mod.

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 default 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.

Cause

This is due to the mods that are about to be installed not being respected in FindReverseDependencies() > FindUnsatisfiedDepends() > MatchesAny().
Only the currently installed mods are taken into account for the dependency resolution.

Changes

This commit adds an optional argument to the public FindReverseDependencies(), IEnumerable<CkanModule> modulesToInstall.
This list is passed to the internal FindReverseDependencies() (which also takes a new argument for this.
In each "find broken depends" pass, this list is added to the list of already installed mods, so FindUnsatisfiedDepends() considers them and their provided modules.

Mods that are about to be installed with dependencies will be likely reported as broken by FindUnsatisfiedDepends(), because their dependencies aren't yet included in modsToInstall (this happens later in the changeset calculation/installation process).
So to make sure we only return broken modules that are already installed, we intersect broken with origInstalled.

UninstallList now takes a IEnumerable<CkanModule> installing instead of IEnumerable<string> installing which it passes on to FindReverseDependencies().
ConsoleUI/InstallScreen.Run() and GUI/Main/MainInstall.InstallMods() are changed to pass the list of mods that are about to be installed to UninstallList().
GUI/MainModList.ComputeChangeSetFromModList() also passes modules_to_install.

I've removed the UninstallList() override which takes a single string instead of a list of mods to uninstall, because it was only used in one single test.
This test now creates a new List<string> on the fly.

Closes #1909

@DasSkelett DasSkelett added Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN labels Feb 13, 2020
@DasSkelett DasSkelett requested a review from HebaruSan February 13, 2020 16:29
@HebaruSan

This comment has been minimized.

@DasSkelett DasSkelett added the Relationships Issues affecting depends, recommends, etc. label Feb 13, 2020
@HebaruSan

This comment has been minimized.

@HebaruSan

This comment has been minimized.

@DasSkelett
Copy link
Member Author

DasSkelett commented Feb 13, 2020

I installed Scatterer w/ defaults, then Spectra. The changelog still says it's going to uninstall Scatterer:

image

Is that right?

It doesn't happen if you've ModuleManager already installed, interestingly (and that was always the case during my testing...)
I'll try to find out what's going on there.

@HebaruSan
Copy link
Member

Oops, this was also the install where I was partway through the "kind":"dlc" project. That might be contributing...

@HebaruSan

This comment has been minimized.

@HebaruSan

This comment has been minimized.

@HebaruSan
Copy link
Member

Yeah, I think that's the problem, FindReverseDependencies is going to tell us that we need to uninstall things that aren't even installed yet. Needs to be smarter around that.

@DasSkelett
Copy link
Member Author

DasSkelett commented Feb 13, 2020

I moved the lines where the new modules are added to the installed modules inside the internal FindReverseDependencies(), and subtract future modules again before returning them.

Fixed

This should be rather safe to do, because we don't rely on FindReverseDependencies() to find out which additional mods have to be installed to satisfy all dependencies, that's the job of RealtionshipResolver later on in the process.

I didn't encounter any freezes / deadlocks with this now (just Spectra taking a long time to install due to its size).

@HebaruSan
Copy link
Member

Nice, it worked perfectly that time! I can go back and forth between Spectra and the default configs at will. This is some great progress!

I'm seeing this in the log though:

2547507 [1] ERROR CKAN.RelationshipResolver (null) - Assertion failed: Adding Scatterer-config twice in relationship resolution

I'll take a closer look at the code sometime in the next day or so.

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<CkanModule> modulesToInstall`, which will be added to the list of "currently installed" mods,
so `FindUnsatisfiedDepends()` considers them and their provided modules.
Copy link
Member

@HebaruSan HebaruSan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, the subtleties of this are more clear to me now: the installing list is added temporarily and removed in each iteration just so it can satisfy other modules' dependencies. Any installing module with dependencies will probably itself be counted as "broken" since dependencies aren't added to the list, but that's fine since we remove them anyway (and re-add them in the next pass). I initially thought of this as having more resolution logic in and around it, but it's really only doing one simple thing.

@HebaruSan HebaruSan merged commit 7ae5ba6 into KSP-CKAN:master Feb 15, 2020
@DasSkelett DasSkelett deleted the fix/respect-provides branch February 15, 2020 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN Relationships Issues affecting depends, recommends, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replacing dependencies
2 participants