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

A provided mod can't satisfy a suggestion #1234

Closed
Postremus opened this issue Jul 3, 2015 · 9 comments
Closed

A provided mod can't satisfy a suggestion #1234

Postremus opened this issue Jul 3, 2015 · 9 comments
Assignees
Labels
Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN ★★☆

Comments

@Postremus
Copy link
Contributor

Steps to reproduce:

  1. Install ProceduralFairing. It suggest PFTextures.
  2. "ProceduralFairings-ForEverything" should be suggested, but it isn't. It provides PFTextures.

Tested on ckan 1.10.0, ksp 1.0.4. Only tested the gui so far.

Reported by Felger on IRC

@Postremus Postremus added Bug Something is not working as intended GUI Issues affecting the interactive GUI ★★☆ labels Jul 3, 2015
@mheguy
Copy link
Contributor

mheguy commented Jul 3, 2015

Same behaviour in Win7-64bit command-line.

@Postremus Postremus added Core (ckan.dll) Issues affecting the core part of CKAN and removed GUI Issues affecting the interactive GUI labels Jul 4, 2015
@Postremus
Copy link
Contributor Author

Further investigation showed, that the gui calls registry.LatestAvailable(..) instead of registry.LatestAvailableWithProvides(..)

HebaruSan added a commit to HebaruSan/CKAN that referenced this issue Dec 2, 2017
@HebaruSan
Copy link
Member

This is included in the text UI in #2177. Just need to catch ModuleNotFoundKraken here and call LatestAvailableWithProvides:

CKAN/GUI/MainInstall.cs

Lines 183 to 199 in 6aac488

// if the mod is available for the current KSP version _and_
// the mod is not installed _and_
// the mod is not already in the install list
if (registry.LatestAvailable(mod.name, CurrentInstance.VersionCriteria()) != null
&& !registry.IsInstalled(mod.name)
&& !toInstall.Any(m => m.identifier == mod.name))
{
// add it to the list of chooseAble mods we display to the user
if (!chooseAble.ContainsKey(mod.name))
{
chooseAble.Add(mod.name, new List<string>());
}
chooseAble[mod.name].Add(identifier);
}
}
// XXX - Don't ignore all krakens! Those things are important!
catch (Kraken)

@politas
Copy link
Member

politas commented Jan 2, 2018

I don't think that commit made it into #2177

@HebaruSan
Copy link
Member

Sorry, what I meant was that ConsoleUI allows "provides" mods to satisfy suggestions. #2177 indeed did not modify GUI.

@politas
Copy link
Member

politas commented Jan 3, 2018

I can't find that commit "Satisfy suggestions w/ provides as per #1234" in the master tree, and the code as it is doesn't include that change. I think you maybe added it to your branch after it was merged, or didn't push the commit through to GitHub in time?

@HebaruSan
Copy link
Member

HebaruSan commented Jan 3, 2018

This is the code that does it. If LatestAvailable doesn't find anything, ConsoleUI falls back to LatestAvailableWithProvides.

} catch (ModuleNotFoundKraken) {
// LatestAvailable throws if you recommend a "provides" name,
// so ask the registry again for provides-based choices
List<CkanModule> opts = registry.LatestAvailableWithProvides(
dependency.name,
manager.CurrentInstance.VersionCriteria(),
dependency
);
foreach (CkanModule provider in opts) {
if (!registry.IsInstalled(provider.identifier)
&& !alreadyInstalling.Contains(provider.identifier)) {
// Default to not installing because these probably conflict with each other
AddDep(provider.identifier, false, identifier);
}
}

I think the commit you're referencing was an error and had to be removed; if I recall correctly, it treated the mod being installed as the ID to look up for provides, whereas we actually want to look at its suggests/recommends.

@politas
Copy link
Member

politas commented Jan 3, 2018

Ah, now I understand! Thanks for persisting in clearing up my confusion.

@HebaruSan
Copy link
Member

Heh, thanks for persisting in asking; I was just as confused until I remembered the story with that commit.

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 ★★☆
Projects
None yet
Development

No branches or pull requests

4 participants