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

Don't throw exceptions for dependency conflicts #2766

Merged
merged 2 commits into from
Jun 1, 2019

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented May 24, 2019

Problem

If you try to select both RealismOverhaul and KerbalJointReinforcementNext to be installed, this exception is thrown:

image

CKAN.DependencyNotSatisfiedKraken: Exception of type 'CKAN.DependencyNotSatisfiedKraken' was thrown.
   at CKAN.RelationshipResolver.ResolveStanza(IEnumerable`1 stanza, SelectionReason reason, RelationshipResolverOptions options, Boolean soft_resolve, IEnumerable`1 old_stanza)
   at CKAN.RelationshipResolver.Resolve(CkanModule module, RelationshipResolverOptions options, IEnumerable`1 old_stanza)
   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.MainModList.ComputeConflictsFromModList(IRegistryQuerier registry, IEnumerable`1 change_set, KspVersionCriteria ksp_version)
   at CKAN.Main.<UpdateChangeSetAndConflicts>d__63.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
   at CKAN.Main.<ModList_CellValueChanged>d__281.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()

Found while looking into KSP-CKAN/NetKAN#7203, though this may or may not be related to the issues described there.

Cause

RO depends on KJRContinued, which conflicts with KJRNext.

Prior to #2660, a conflict between a module to be installed and a dependency would be caught here:

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}");
}

After #2660, we check conflicts in LatestAvailableWithProvides, so the conflict with the dependency causes this block to find zero candidates:

List<CkanModule> candidates = descriptor
.LatestAvailableWithProvides(registry, kspversion, modlist.Values)
.Where(mod => descriptor1.WithinBounds(mod) && MightBeInstallable(mod))
.ToList();

So this throws:

if (candidates.Count == 0)
{
if (!soft_resolve)
{
log.InfoFormat("Dependency on {0} found but it is not listed in the index, or not available for your version of KSP.", descriptor.ToString());
throw new DependencyNotSatisfiedKraken(reason.Parent, descriptor.ToString());
}

The calling code isn't expecting exceptions because it sets its options to suppress them:

CKAN/GUI/MainModList.cs

Lines 1026 to 1032 in 19f7302

var options = new RelationshipResolverOptions
{
without_toomanyprovides_kraken = true,
proceed_with_inconsistencies = true,
without_enforce_consistency = true,
with_recommends = false
};

Changes

Now if the initial search for candidates comes up empty, we try again without passing the mod list (which is how it worked prior to #2660, the mod list was ignored until later). This will still allow us to find non-conflicting older versions of a module when necessary (in the first pass), while allowing dependency conflicts to be handled without throwing exceptions (in the second pass).

image

Highlighting hidden conflicts

As a side fix, if you filter the list and then select a mod to install that has a conflict with a hidden mod (the RealismOverhaul / KJRNext combo above is easy for this), only the conflicting rows that were currently visible would be highlighted with a red background. The invisible ones would remain white, including after the filter was changed to make them visible again.

Now all conflicts are highlighted red regardless of whether they're visible. As a side effect of this, the conflict marking code is slightly more efficient, because it no longer needs to loop over every visible row.

@HebaruSan HebaruSan added Bug Core (ckan.dll) Issues affecting the core part of CKAN Pull request Relationships Issues affecting depends, recommends, etc. labels May 24, 2019
@DasSkelett
Copy link
Member

DasSkelett commented May 24, 2019

On Linux with Mono there's no error thrown 🤔
Neither are tho columns painted red.

Only the message in the info bar is displayed:
grafik

Edit: If I select KJR Continued manually too, then I see some redness. but only for both KJRs, not RO.
But still no error thrown.

Edit Edit: To clarify: My comment refers to the master branch.

@DasSkelett
Copy link
Member

On your branch, if I select RO and KJR Continued, KJR Continued and KJR Next are highlighted red, but not Realism Overhaul, as in your screenshot.

@HebaruSan
Copy link
Member Author

Hmm, I think the last time we saw Windows display that window for an exception and Mono not, the exception was still being thrown in both and just not displayed in Mono. I think that would explain the lack of row highlights on master—the code that sets up the conflict to mark the rows doesn't run because the exception is thrown first.

For the second thing, can you set that up and then click the "X" to quit and then screenshot the "are you sure" popup? That gives a few clues as to what the found conflicts are. Otherwise I'll take a look in Linux in a bit.

@DasSkelett
Copy link
Member

Screenshot of the conflicts-on-quit message:
Screenshot

@HebaruSan
Copy link
Member Author

Thanks! So it is including RO in the list of conflicts, it's just not coloring the row. I'll see if I can figure that out...

@HebaruSan
Copy link
Member Author

Huh, that's weird.

image

I guess what exact steps are you taking?

@HebaruSan
Copy link
Member Author

I think this may be related to searching/filters. If I just find the rows in the big list, I get the highlights as above, but if I filter for "realism", click RO, then change the filter to "kjr" and click KJRNext, then I see what you see:

image

@DasSkelett
Copy link
Member

I think this may be related to searching/filters. If I just find the rows in the big list, I get the highlights as above, but if I filter for "realism", click RO, then change the filter to "kjr" and click KJRNext, then I see what you see:

[image]

Yep! That's it!

@HebaruSan
Copy link
Member Author

Yeah, the code in Main.ConflictsUpdated only loops over visible rows, so if there are conflicts on rows that are currently excluded from the filter, they'll keep their plain white backgrounds.

CKAN/GUI/Main.cs

Lines 101 to 118 in b996c59

foreach (DataGridViewRow row in ModList.Rows)
{
GUIMod module = (GUIMod)row.Tag;
string value;
if (Conflicts != null && Conflicts.TryGetValue(module, out value))
{
string conflict_text = value;
foreach (DataGridViewCell cell in row.Cells)
{
cell.ToolTipText = conflict_text;
}
if (row.DefaultCellStyle.BackColor != Color.LightCoral)
{
row.DefaultCellStyle.BackColor = Color.LightCoral;
ModList.InvalidateRow(row.Index);
}

Luckily we already have MainModList.full_list_of_mod_rows, a Dictionary mapping from identifiers to rows, which as you might suspect from its name isn't affected by filters. So we should be able to not only fix that bug, but also make ConflictsUpdated faster.

@HebaruSan
Copy link
Member Author

OK, the latest commit should fix that issue.

@HebaruSan HebaruSan requested a review from DasSkelett June 1, 2019 02:26
Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

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

Works! No more errors thrown, a little bit of red paint here and there, even after changing filters, perfect!

@HebaruSan HebaruSan merged commit 01f46c4 into KSP-CKAN:master Jun 1, 2019
@HebaruSan HebaruSan deleted the fix/dep-conf branch June 25, 2019 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Core (ckan.dll) Issues affecting the core part of CKAN Pull request Relationships Issues affecting depends, recommends, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants