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

[Feature]: Upgrade to latest compatible version when a version conflict on absolute latest is encountered #3849

Closed
lamont-granquist opened this issue Jun 10, 2023 · 4 comments · Fixed by #4023
Assignees
Labels
Core (ckan.dll) Issues affecting the core part of CKAN Enhancement New features or functionality Relationships Issues affecting depends, recommends, etc.

Comments

@lamont-granquist
Copy link

Problem

So for example I have an RP-1 v1.x install which I'd like to upgrade but keep on v1.x while installing the available compatible upgradable mods.

So for example I have:

% ckan list | egrep RealismOverhaul
^ RealismOverhaul v14.23.0.0

When I try to upgrade that I get:

% ckan upgrade RealismOverhaul
The following inconsistencies were found:
* RP-0 v1.13.2.2 missing dependency RealismOverhaul v15.99.0.0 or earlier

This is correct if I wanted to upgrade to RealismOverhaul v16.x, however CKAN-meta indicates that RealismOverhaul-v15.0.1.0.ckan is available to be installed. Really I'm trying to upgrade to that v15.x version which is compatible.

I can get that to work manually by becoming my own depsolver with:

% ckan upgrade RealismOverhaul=v15.0.1.0
About to upgrade:

 * Upgrade: Realism Overhaul v14.23.0.0 to v15.0.1.0 (github.com, 18.1 MiB)

Continue? [Y/n]

Suggestion

Probably create some flag which would allow doing more complicated depsolving to upgrade only to the latest compatible versions with all over currently installed mods.

Alternatives

No response

Additional context

I'm not sure how to do this with --all and what the correct workflow should be.

It may be possible that old versions of RP-1 1.x didn't have the constraint on <= v15.99 -- I'm not sure if all the RP-1 metadata got retconned or not to have that constraint added.

But certainly the situation could happen where CKAN decides to pick a lower version of RP-1 without the constraint and it installs RealismOverhaul v16.x, instead of the other way around and picking the latest RP-1 and an earlier version of RealismOverhaul.

I don't know exactly what to suggest about the ambiguity in that case, but I'd mostly be happy if I could upgrade RP-1 in isolation and thereby disallow downgrades of that mod, then it would make it easier on the depsolver to find the latest non-conflicting version of RealismOverhaul.

I know that this is how bundler/rubygems works (also that it is somewhat difficult to implement and this is probably getting close to where depsolvers become NP-complete)

@lamont-granquist
Copy link
Author

lamont-granquist commented Jun 10, 2023

Sort of to be clear, the idea is to make this work as a way to upgrade an RP-1 v1.x install and keep it an RP-1 v1.x install:

ckan upgrade RP-0
ckan upgrade --all

That fails now with:

% ckan upgrade --all
The following inconsistencies were found:
* ROEngines v2.1.0.0 conflicts with RP-0 v1.13.2.2

ROEngines has a similar problem with RealismOverhaul where there's a newer version than what I have installed but the latest version isn't compatible. And pretty much for every other RO-related mod. The manual way is a bit painful.

Suspect this will come up more as more people want to upgrade their legacy RP-1 installs but stay on legacy RP-1.

Clearly I'm also a CLI user (and Mac user) and I haven't tried to see what happens in the GUI and couldn't see anything in the help (this might be a doc bug for all I know).

@HebaruSan HebaruSan added Enhancement New features or functionality Core (ckan.dll) Issues affecting the core part of CKAN Relationships Issues affecting depends, recommends, etc. labels Jun 10, 2023
@lamont-granquist
Copy link
Author

Forgot my CKAN version:

You are using CKAN version v1.32.0

@HebaruSan
Copy link
Member

Thanks for submitting this, @lamont-granquist!

Findings while investigating this:

  • AvailableModule.DependsAndConflictsOK already checks conflicts, reverse conflicts and forward dependencies, but not reverse dependencies:
    private static bool DependsAndConflictsOK(CkanModule module, IEnumerable<CkanModule> others)
    {
    others = others.Memoize();
    if (module.depends != null)
    {
    foreach (RelationshipDescriptor rel in module.depends)
    {
    // If 'others' matches an identifier, it must also match the versions, else fail
    if (rel.ContainsAny(others.Select(m => m.identifier)) && !rel.MatchesAny(others, null, null))
    {
    log.DebugFormat("Unsatisfied dependency {0}, rejecting", rel);
    return false;
    }
    }
    }
    var othersMinusSelf = others.Where(m => m.identifier != module.identifier).Memoize();
    if (module.conflicts != null)
    {
    // Skip self-conflicts (but catch other modules providing self)
    foreach (RelationshipDescriptor rel in module.conflicts)
    {
    // If any of the conflicts are present, fail
    if (rel.MatchesAny(othersMinusSelf, null, null, out CkanModule matched))
    {
    log.DebugFormat("Found conflict with {0}, rejecting", matched);
    return false;
    }
    }
    }
    // Check reverse conflicts so user isn't prompted to choose modules that will error out immediately
    var selfArray = new CkanModule[] { module };
    foreach (CkanModule other in othersMinusSelf)
    {
    if (other.conflicts != null)
    {
    foreach (RelationshipDescriptor rel in other.conflicts)
    {
    if (rel.MatchesAny(selfArray, null, null))
    {
    log.DebugFormat("Found reverse conflict with {0}, rejecting", other);
    return false;
    }
    }
    }
    }
    return true;
    }

    This would prevent RP-0's version specific dependency on RealismOverhaul from limiting the latter mod's upgradeable versions. I don't think there was a motivation for leaving it like this other than an abundance of caution when changing the resolver, so it should be possible to change as long as we leave room for further testing and fixing.
  • CmdLine.Install and CmdLine.Upgrade rely on RelationshipResolver (after Parallelize for performance, relationship resolver improvements #3917 it will be their own translation code) to get CkanModules from identifiers. This code simply uses Registry.LatestAvailable without passing the installed parameter to allow checking of relationships:
    return CkanModule.FromIDandVersion(registry, name, GameVersion);

    module = registry.LatestAvailable(mod, ksp_version)
    ?? registry.InstalledModule(mod)?.Module;

    So that would have to be updated as well.
  • Plugging those two gaps gets us part of the way there. Passing only the installed modules could cause mods to get "stuck" if they have mutual version-specific dependencies, because it might be that neither one can be upgraded without breaking the other one, even if later versions of both mods would be OK together. We need some kind of more complex logic to get around this; so far I have been experimenting with first getting the absolute latest versions of the requested mods (all if --all), then passing those mods to the resolving logic to enforce their version limits (which in the case of RP-0 would dial back RealismOverhaul to an earlier allowed version). So far this gives the desired behavior in all the use cases that I have checked (and I've written some tests for it as well just to be sure).
  • It gets really complicated when we get to the GUI. Since we have an Upgrade column that appears as a checkbox if an installed mod is upgradeable and a - if it's not, we need to be able to answer Yes or No to the question of whether each mod is upgradeable, when we first load the list of mods. But it's actually a more complicated question, because whether a mod is upgradeable can depend on what else is being upgraded.
    I think the most common use case is the user clicking the "upgrade all" button, so that should always work smoothly, and we can do that with logic similar to what we'll have to do for --all: get the very latest versions of all installed mods, then enforce their version limitations on each other. If that logic shows a mod as upgradeable, then we show an Upgrade checkbox for it, because there is some possible changeset capable of upgrading it.
    What then of the case where the user tries to upgrade only one mod in GUI, which should not be upgradeable without upgrading other mods? I think this would be rare enough that some awkwardness ought to be permissible; the checkbox could show up, the user could check it, but we could display an error if they try to proceed. We could keep installed mods with a "held" label (see Allow label to pin installed mod version #3220) out of the initial changeset, which would allow us to hide the checkbox when we know a version limit is very unlikely to disappear.
    That's pretty do-able, but it does create a lot of corner cases to cover. For example, if I add or remove a "held" label, I'd need to re-check the state of all the Update checkboxes. That's probably computationally trivial but might be tricky to find a good spot in the code to make it happen. The worst complications are on the Versions tab, where clicking Upgrade moves the checkbox to the version to be upgraded, which might be changing constantly as you choose more mods to upgrade. That's the point I've gotten to so far, and hopefully I'll have a breakthrough in how to make that work soon.

@NathanKell
Copy link
Contributor

Thanks for poking at this! 👍
FWIW we don't anticipate breaking compat again for a few months, but I would expect to do so again early next year, so it'd be awesome if this worked by then. Obviously survivable if it's not however! (And, uh, our timelines are at best guesstimates :D )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core (ckan.dll) Issues affecting the core part of CKAN Enhancement New features or functionality Relationships Issues affecting depends, recommends, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants