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 auto-remove during upgrade #3913

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Sep 19, 2023

Problem

In KSP-RO/RP-1@b9f0855, RP-1's dependency on MagiCore was removed from its metanetkan, and this change first reached users with the indexing of RP-1 v2.12 in KSP-CKAN/CKAN-meta@98a3461. When they upgraded, RP-1-ExpressInstall and all of its auto-installed dependencies were uninstalled (which should not have happened!). The dependency was subsequently restored pending a CKAN fix.

How to reproduce for testing

  1. Use a fresh game instance of KSP1 1.12.5
  2. In CKAN, go to the settings and replace the default repo with this URL to simulate using CKAN before the problematic upgrade (name doesn't matter):
    https://github.com/KSP-CKAN/CKAN-meta/archive/0451525c2c9cefd16914032a2ca38d69c9e3f21c.tar.gz
  3. Click Refresh to update metadata (will happen automatically if using a current dev build)
  4. Install RP-1-ExpressInstall
  5. When prompted to choose a version of RP-1, choose the 2.x series version
    (the other choices you make don't matter, issue will happen regardless)
  6. Note that with the current dev build, there will be an extremely long delay while it computes the recommendations on the extremely long RP-1 dependency list because Show recommendations of full changeset with opt-out #3892 was merged, Recommendation suppression property for netkans KSP-RO/RP-1#2233 was reverted, and Recommendation suppression property for netkans KSP-RO/RP-1-ExpressInstall-Graphics#4 and Recommendation suppression property for netkans KSP-RO/RealismOverhaul#2913 have not yet been merged. I have sent a reminder about this.
    Those pull requests have now been merged, however I forgot that that will not help us here as we are intentionally reaching back into the archive to use old versions of the metadata for testing. But at least now it will not be as slow after the next client release.
  7. Now go back to the CKAN settings and replace your repo with this URL to simulate using CKAN right after the release of RP-1 v2.12:
    https://github.com/KSP-CKAN/CKAN-meta/archive/98a346180f649c277e026a18cd69d0c3b5055ac8.tar.gz
  8. Click Refresh to update metadata (will happen automatically if using a current dev build).
    You should see four mods ready to upgrade.
  9. Choose to upgrade those mods and click Apply
    image
  10. This is where RP-1-ExpressInstall (and many other mods) would have been removed previously. Instead you should see this with this PR's changes:
    image
  11. Finish upgrading and confirm that MagiCore is gone and RP-1-ExpressInstall is still there

Cause

  • Upgrading RP-1 triggers auto-removal of MagiCore, as it should
  • GUI always calls ModuleInstaller.UninstallList before ModuleInstaller.InstallList before ModuleInstaller.Upgrade:
    if (!canceled && toUninstall.Count > 0)
    {
    installer.UninstallList(toUninstall.Select(m => m.identifier),
    ref possibleConfigOnlyDirs, registry_manager, false, toInstall);
    toUninstall.Clear();
    }
    if (!canceled && toInstall.Count > 0)
    {
    installer.InstallList(toInstall, opts.Value, registry_manager, ref possibleConfigOnlyDirs, downloader, false);
    toInstall.Clear();
    }
    if (!canceled && toUpgrade.Count > 0)
    {
    installer.Upgrade(toUpgrade, downloader, ref possibleConfigOnlyDirs, registry_manager, true, true, false);
    toUpgrade.Clear();
    }

    ... so the first change that happens is removal of MagiCore
  • The CKAN core code performing that removal doesn't know anything about the pending upgrade, it just knows it's been asked to remove MagiCore and that the currently installed version of RP-1 depends on MagiCore
  • When that code removes MagiCore, it also removes all mods that depend on it, including the currently installed (old) version of RP-1. This is mildly surprising and not ideal, but not necessarily a serious problem yet.
  • However, that dependency search is transitive, so everything depending on RP-1 is also removed, including RP-1 Express! This is when things definitely are going wrong.
  • The removal of RP-1 Express also triggers the automatic removal of any mods marked as auto-installed that only RP-1 Express depended on. This is desirable in a narrow sense, but in the context of removing a mod that we don't really want to remove, it just makes a bigger mess.
  • When the Remove step finishes, essentially everything is uninstalled
  • Since we're not installing anything, the InstallList step does nothing
  • Finally the Upgrade step happens, which causes all the "Update" mods in the changeset to be reinstalled:
    image
    ... so RP-1, RealismOverhaul, RealFuels, and KSPCommunityFixes.
  • But RP-1 Express was never in the changeset, so it doesn't get reinstalled!

Changes

  • Now ModuleInstaller.Upgrade handles auto-removal internally
    (ModuleInstaller.UninstallList already handled auto-removal internally)
    • Tests are added for auto-removal
  • Now GUI doesn't pass auto-removals to ModuleInstaller.UninstallList unless the changeset consists only of installs and auto-removals, because we want them to be handled by Upgrade and/or UninstallList whenever possible
  • While working on this, I discovered that the refactor of the DLL scanning code from Refactor repository and available module handling #3904 throws if you have multiple DLLs with the same filename (which the KSP-RO team is currently doing unintentionally by shipping System.dll, System.Core.dll, and System.Xml.dll in several of their ZIPs). Now this is fixed courtesy of GroupBy.
  • While working on this, I discovered that RP-1-ExpressInstall has enough tags to wrap the display in GUI, but the panel containing them doesn't resize to fit. Now it does.
  • While working on this, I noticed that if you select a very large changeset, it takes a long time to calculate the recommendations, during which CKAN shows the changeset tab and is responsive but doesn't look busy. Now the hourglass cursor appears during this to show that it is doing something.
  • The right-click reinstall option now uses Upgrade instead of cobbling together its own custom changeset of uninstalls and installs, to take advantage of this smarter handling of auto-removal for that use case (fixes the part of [Bug]: CKAN 1.33 producing *numerous* problems on Kubuntu 20.04 #3912 where it's unnecessarily trying to uninstall Parallax-StockTextures). When this option is used, the changeset will say "Re-install (user requested)" in the English locale.

Fixes #3911.

@HebaruSan HebaruSan added Bug Something is not working as intended Enhancement New features or functionality GUI Issues affecting the interactive GUI Core (ckan.dll) Issues affecting the core part of CKAN Relationships Issues affecting depends, recommends, etc. labels Sep 19, 2023
@NathanKell
Copy link
Contributor

Oh man we provided a lot of problems, didn't we :D

@HebaruSan
Copy link
Member Author

@NathanKell, the KSP-RO project can be relied upon to probe formerly rare corner cases and sometimes turn them into core requirements overnight. 😜

Copy link
Member

@techman83 techman83 left a comment

Choose a reason for hiding this comment

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

That looks like a sensible set of operations and an improvement of the logic. I shall leave the testing of the changes to @NathanKell / others.

I'd prefer the extra changes live their own commits, so context isn't lost if we have to look back through the history of why something was changed. But I'll leave it with your best judgement @HebaruSan

Great outcome!

@HebaruSan HebaruSan merged commit 52c8a9a into KSP-CKAN:master Sep 20, 2023
10 checks passed
@HebaruSan HebaruSan deleted the fix/upgrade-auto-removal branch September 20, 2023 03:44
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 Enhancement New features or functionality GUI Issues affecting the interactive GUI Relationships Issues affecting depends, recommends, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-removal of MagiCore at upgrade of RP-1 caused RP-1 Express to be removed
3 participants