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

Fix dependency resolution in mod upgrades #3200

Merged

Conversation

DasSkelett
Copy link
Member

@DasSkelett DasSkelett commented Nov 16, 2020

Problem

Upgrading mods that previously provided a module and depend on it in the next version currently fails:

About to upgrade...

Downloading "https://github.com/R-T-B/Kopernicus/releases/download/UBE-release-46/KopernicusBE_1101_Release46.zip"
Module "Kopernicus Bleeding Edge Beta - DEV RELEASE" successfully installed
The following inconsistencies were found:
* Kopernicus-BE UBEE_1101_46 missing dependency ModularFlightIntegrator 1.2.7.0 or later
Error during installation!
If the above message indicates a download error, please try again. Otherwise, please open an issue for us to investigate.
If you suspect a metadata problem: https://github.com/KSP-CKAN/NetKAN/issues/new/choose
If you suspect a bug in the client: https://github.com/KSP-CKAN/CKAN/issues/new/choose
ModularFlightIntegrator 1.2.7.0 conflicts with Kopernicus-BE UBEE_1101_45

In this case, Kopernicus-BE v1.10.1-45 provided ModularFlightIntegrator, but Kopernicus-BE v1.10.1-46 no longer does so and has it as dependency instead (see #3193). CKAN should automatically pull in MFI to satisfy the new dependency.

Causes

  1. ModuleInstaller.Upgrade(IEnumerable<string> identifiers, ...) doesn't tell the newly created RelationshipResolver that it also removes the old modules before adding the new ones. So the dependency is wrongly satisfied by the old module.
  2. RelationshipResolver(IEnumerable<string> modulesToInstall, IEnumerable<string> modulesToRemove, ...) tries to find matching CkanModules for all the identifiers in modulesToInstall and modulesToRemove. It does so by calling TranslateModule() -> FromIDandVersion(), that ultimately returns the latest compatible module.
    This is fine for modulesToInstall, however for modulesToRemove we only want search through the currently installed modules (we can only remove modules that are actually installed after all).
  3. RelationshipResolver.ResolveStanza() checks whether dependencies of the new modules are already satisfied by the installed modules, however it does so by querying the registry, which might have outdated information. Thus it incorrectly assumes the dependencies are satisfied (by the old mod versions). Only the resolvers own installed_modules knows about the modules that are going to be removed.

Changes

  1. ModuleInstaller.Upgrade() now tells the relationship resolver which modules it's about remove by passing the same identifier list for modulesToRemove as it passes for modulesToInstall.
  2. RelationshipResolver() now uses registry.InstalledModule() to convert identifiers to CkanModules for modulesToRemove. We can use the registry here since we're just in the process of creating the resolver and installed_modules is only declared and populated a few steps later.
  3. RelationshipResolver.ResolveStanza() now looks into the resolvers own installed_modules instead of querying the potentially outdated registry. installed_modules is already reduced by the modules that are about to be removed, thus the resolver has to find other mods that can satisfy the .dependencies

@DasSkelett DasSkelett added Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN Relationships Issues affecting depends, recommends, etc. labels Nov 16, 2020
@DasSkelett DasSkelett requested a review from HebaruSan November 16, 2020 21:43
Core/ModuleInstaller.cs Outdated Show resolved Hide resolved
@DasSkelett DasSkelett force-pushed the fix/upgrade-relationship-resolution branch from a67bd66 to f6c6810 Compare November 16, 2020 23:34
1) `ModuleInstaller.Upgrade()` now tells the relationship resolver which modules it's about remove.
2) `RelationshipResolver()` now uses `registry.InstalledModule()` to convert identifiers to `CkanModule`s for `modulesToRemove`.
3) `RelationshipResolver.ResolveStanza()` now looks into the resolvers own `installed_modules` instead of querying the potentially outdated registry.
4) Improve the code documentation in these areas.
@DasSkelett DasSkelett force-pushed the fix/upgrade-relationship-resolution branch from f6c6810 to 7872227 Compare November 17, 2020 02:47
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.

🎸

@HebaruSan HebaruSan merged commit 5c64c4c into KSP-CKAN:master Nov 19, 2020
@DasSkelett DasSkelett deleted the fix/upgrade-relationship-resolution branch November 21, 2020 21:49
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.

2 participants