Fix installation of AVP while removing EVE default configs

[DasSkelett]

Or: Fix resolution of circular dependencies when conflicts are involved.
Or: Another open heart surgery of the relationship resolver.

Problem

Have EnvironmentalVisualEnhancements with its default configs EnvironmentalVisualEnhancements-HR installed.
Now deselect EnvironmentalVisualEnhancements-HR and select AVP.

See this double error message:

AVP-8kTextures v1.12 depends on AstronomersVisualPack, which is not compatible with the currently installed game version

The issue goes down to MightBeInstallable() returning false for any AstronomersVisualPack,
because LatestAvailableWithProvides() can't find any provider for AVP-Textures,
because they all depend on AstronomersVisualPack again,
which LatestAvailableWithProvides() / AvailableModule.Latest() sees as incompatible
due to the conflict with EnvironmentalVisualEnhancements-HR,
which AvailableModule.Latest() doesn't know we're uninstalling.

In short: We pass the list of actually installed modules to AvailableModule.Latest() instead of the theoretical one of the RelationshipResolver.

Another bug: our invocations of FindRemovableAutoInstalled() also try to remove mods that will be re-required by new mods we're about to install, so those mods get scheduled once for removal and once for reinstallation. Not a big problem, but preferably it shouldn't remove them in the first hand.

And: If the install step in MainInstall.InstallMods() throws a TooManyModsProvide kraken and it gets resolved by the user, it tries to start again from the beginning, re-trying to remove mods that have already been removed in the first round.

Changes

The whole chain of LatestAvailableWithProvides() methods gets a new optional installed parameter, next to toInstall. If given, it passes that one along to AvailableModule.Latest() instead of Registry.InstalledModules.
Additionally, MightBeInstallable() passes the current source/parent of the stanza we're investigating as toInstall LatestAvailableWithProvides(), since it's not yet in the list of installed modules. Basically we assume that the stanza source is compatible when checking its dependencies. I don't think this was needed to fix the above bug, but it should help when force-installing outdated mods.

I've written a unit test that models this relationship constellation. I'm actually quite proud of this one, it works really well. You can cherry-pick / checkout that commit on itself to see the test fail.

To fix FindRemovableAutoInstalled() staging removals of dependencies we need for new modules again, we concatenate the list of installed modules with the list of modules we're about to install, wrapped in pseudo-InstalledModules.

And to fix the installation failure when TooManyModsProvide is thrown after the removal step, we clear the change lists after finishing them.

[DasSkelett]

I'm still seeing a conflict message in the status bar even after I click to uninstall EVE-HR. Should it disappear?

I see, in other cases it does disappear (tested with RealismOverhaul and KerbalJointReinforcementNext). Ugh, no idea where that goes wrong.

1. Install EVE and EVE-HR

2. Check the box for AVP. The apply button remains disabled because of the conflict.

3. Uncheck the box for EVE-HR. The apply button lights up because the conflict is gone.

4. Recheck the box for EVE-HR to go back to what we had in step 2. The conflict should be back, but the apply button remains enabled. Something's not going back to its previous state somehow.

Interesting, maybe connected to the above problem? EVE-HR doesn't get added to the changeset, though. So the relationship resolver is doing something right, but somewhere along the chain the error seems to be lost.

[DasSkelett]

Okay, turns out that by sending the more accurate list of installed mods the resolver is now "too good", which causes it to not find any candidates at all, which in turn causes a DependencyNotSatisfiedKraken kraken to be thrown:

CKAN/Core/Relationships/RelationshipResolver.cs

Line 456 in f053ee4

throw new DependencyNotSatisfiedKraken(reason.Parent, descriptor.ToString());

... instead of going on with the resolution and adding the mod to a list of conflicts if proceed_with_inconsistencies is set:

CKAN/Core/Relationships/RelationshipResolver.cs

Lines 500 to 524 in f053ee4

	CkanModule conflicting_mod = fixed_mods.FirstOrDefault(mod => mod.ConflictsWith(candidate));
	if (conflicting_mod == null)
	{
	// Okay, looks like we want this one. Adding.
	Add(candidate, reason);
	Resolve(candidate, options, stanza);
	}
	else if (soft_resolve)
	{
	log.InfoFormat("{0} would cause conflicts, excluding it from consideration", candidate);
	}
	else
	{
	if (options.proceed_with_inconsistencies)
	{
	Add(candidate, reason);
	conflicts.Add(new KeyValuePair<CkanModule, CkanModule>(conflicting_mod, candidate));
	conflicts.Add(new KeyValuePair<CkanModule, CkanModule>(candidate, conflicting_mod));
	}
	else
	{
	throw new InconsistentKraken(
	$"{conflicting_mod} conflicts with {candidate}");
	}
	}

And in UpdateChangeSetAndConflicts() when calling ComputeConflictsFromModList() we don't want an exception to be thrown, but want a list of conflicting mods:

CKAN/GUI/Model/ModList.cs

Lines 453 to 461 in f053ee4

	var resolver = new RelationshipResolver(
	modules_to_install.Except(modules_to_remove),
	modules_to_remove,
	options, registry, ksp_version
	);
	return resolver.ConflictList.ToDictionary(
	item => new GUIMod(item.Key, registry, ksp_version),
	item => item.Value
	);

CKAN/GUI/Controls/ManageMods.cs

Line 1602 in f053ee4

new_conflicts = ModList.ComputeConflictsFromModList(registry, user_change_set, inst.VersionCriteria());

So I think for the second attempt to get candidates in ResolveStanza():

CKAN/Core/Relationships/RelationshipResolver.cs

Lines 442 to 448 in f053ee4

	candidates = descriptor
	.LatestAvailableWithProvides(registry, GameVersion)
	.Where(mod => !modlist.ContainsKey(mod.identifier)
	&& descriptor1.WithinBounds(mod)
	// Start with the source of the stanza being considered compatible
	&& MightBeInstallable(mod, null, stanzaSource))
	.ToList();

we should pass and empty installed_modules to depend.LatestAvailableWithProvides() within MightBeInstallable(), and to LatestAvailableWithProvides().
Which seems to work, I hope this doesn't introduce a bunch of new bugs...

[DasSkelett]

Changes of the last commit:
Remove the stanzaSource parameter of ResolveStanza() again, use reason.Parent instead.

MightBeInstallable() now takes a list of installed modules as argument as well, instead of reading it from this.installed_modules.

In the first search for candidates in ResolveStanza(), pass installed_modules as well, missed that before. Also pass installed_modules to MightBeInstallable().

In the second search for candidates (if the previous didn't find any), pass an empty list for installed to LatestAvailableWithProvides() and MightBeInstallable(). Also send a null stanzaSource. This ensures that we find any potential satisfiers without considering conflicts to already installed mods, in case we want to proceed_with_inconsistencies or soft_resolve, and add it to the list of conflicts instead of throwing an exception (which some callers expect).

This also solved the conflict message not disappearing after deselecting the conflicting mod(s).

And AnyOfRelationshipDescriptor.LatestAvailableWithProvides() also passes along the installed argument now.

[HebaruSan]

This search is really handy for testing btw:

[HebaruSan]

The stuff that I was testing last time now works seemingly flawlessly, thanks for patching it up. 👍

[HebaruSan]

LGTM. Can we add a test or two so we have better guard rails the next time we want to change the resolver?

[DasSkelett]

I can try to expand UninstallingConflictingModule_InstallingRecursiveDependencies_ResolvesSuccessfully to also cover the toggle-on toggle-off unexpected exception issue.

[DasSkelett]

Okay, squashed the fixup commit, and expanded the new unit test to also cover what happened behind the scenes when doing the toggle switcharoo. It's basically scenario 2, which shouldn't have thrown any exceptions because we had proceed_with_inconsistencies = true, but it did. The test makes sure there is no exception thrown, and that the conflict list contains the two mods.

[HebaruSan]

Really nice, will be great to have this. 🐏

Is it time to start preparing to release v1.30.2?