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

Make where-CKAN-would-install logic consistent #3931

Merged
merged 1 commit into from
Nov 24, 2023

Conversation

HebaruSan
Copy link
Member

Problem

In an extremely obscure and unlikely case, you could get an error similar to this while installing a mod with CKAN if you had previously installed another mod manually:

screenshot

(This exact error has a different cause, but it uses the same error message template.)

Specifically, it could happen if:

  • You're installing a module containing a file with a name of the form identifier-something-else.dll, where "identifier" is the mod's identifier and "something-else" is a string indicating that it's not the real DLL
  • You have manually installed a file in game data with a name of the form identifier.dll

Cause

Where the code to detect manually installed mods checks that the filename starts with identifier. with a dot, the loop over the installing files checked whether the file started with the identifier, but it didn't check what the next character was. In effect very slightly different logic was being used for the files on disk and the files being installed. This is very unlikely to cause a real problem because the identifier still has to match the file on disk, but it's still better to keep it tidy when something like this is spotted.

Changes

  • Now we use the exact same logic for installing files as for already installed unmanaged files
  • Now three new test cases exercise this logic, one of which fails under the old logic and passes under the new. To make this work:
    • The DogeCoinPlugin ZIP file is updated to contain GameData/DogeCoinPlugin/Plugins/DogeCoinPlugin.dll instead of GameData/DogeCoinFlag/plugin/dogecoin.dll
    • A new DogeCoinPluginAddonFerram ZIP file is created containing GameData/DogeCoinPlugin/Plugins/DogeCoinPlugin-Addon-Ferram.dll so we can test the case that is fixed by the change in Core

Adding multiple reviewers based on familiarity with the issue. Since we only need one approval, don't feel obligated to look at this if you're disinclined.

(Related to KSP-CKAN/NetKAN#9856 and giuliodondi/kOS-Ferram#3 but not a fix for either one.)

@HebaruSan HebaruSan added Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN Tests Issues affecting the internal tests labels Nov 22, 2023
@HebaruSan HebaruSan force-pushed the fix/unmanaged-kos-addon-error branch 2 times, most recently from 97e2665 to bc888fb Compare November 22, 2023 23:19
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.

Awesome! Concise, clear, and super sensible to use the same implementation, with tests! Love your work @HebaruSan

@HebaruSan HebaruSan merged commit 0a97717 into KSP-CKAN:master Nov 24, 2023
10 checks passed
@HebaruSan HebaruSan deleted the fix/unmanaged-kos-addon-error branch November 24, 2023 12:41
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 Tests Issues affecting the internal tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants