search

KSP-CKAN / CKAN

The Comprehensive Kerbal Archive Network
https://forum.kerbalspaceprogram.com/index.php?/topic/197082-*
Other
1.96k stars 349 forks source link

Manually-installed ModName.Unity.dll detected as ModName's DLL #3506

Closed ihsoft closed 2 years ago

ihsoft commented 2 years ago

Background

Have you made any manual changes to your GameData folder (i.e., not via CKAN)?

Yes. I've created a folder KIS2 with some DLLs in it.

These files are not anyhow related to KIS that is supposed to be installed by CKAN.

Problem

CKAN refuses to install KIS v1.28 because it believes this mod has already been installed into a different folder, which is not true.

Steps to reproduce

I'm not sure if the issue can be reproduced by simply creating the files mentioned above. However, the error text implies that the checking is wrong. It complains about KIS.Unity.dll which is not anyhow similar to KIS.dll. And I can confirm that the assembly names inside these DLLs are completely different.

CKAN error code (if applicable):

About to install:

 * Kerbal Inventory System 1.28 (spacedock.info, 24.3 MiB)
Downloading "https://spacedock.info/mod/1909/Kerbal%20Inventory%20System%20(KIS)/download/1.28"
DLL for module KIS found at GameData/KIS2/Plugins/KIS.Unity.dll, but it's not where CKAN would install it. Aborting to prevent multiple copies of the same mod being installed. To install this module, uninstall it manually and try again.
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
HebaruSan commented 2 years ago

Yeah, that's how the DLL detection works. Don't try to confuse it by using similar names, I guess. KIS2.Unity.dll should be fine (the .Unity part is interpreted as a versioning string because ModuleManager's DLLs are named in that style).

Here's the regex in question:

https://github.com/KSP-CKAN/CKAN/blob/068a8dd70b66499841c4f2b32facecf7464046fc/Core/Registry/Registry.cs#L838-L846

The .Unity part is matched by the .* on the last line.

ihsoft commented 2 years ago

Hmm. It's a kind of loose assumption that the main module cannot have dots in the name. It's good when the name is under control of the modder, but it may not be the case always (e.g. if it's an external dependency). And it can break someone's other mod installation via CKAN. I was just lucky that this case raised on my machine, but it could easily be missed.

If the version part of the module name needs to be omitted in the check, then it makes sense to explicitly check for the version piece. The following pattern should cover 99% of the cases: (?<modname>.+?)[-]?[0-9.]*\.dll. Btw, it also fixes handling of "GameData/Foo/Foo-1.2.dll" case (from the code comments). The existing pattern captures "Foo-1" instead of "Foo".

HebaruSan commented 2 years ago

Trying to impose format limitations on mod version strings has not worked very well in the past. E.g., it's fairly common to have versions like v1.0 or 1.2a, which that regex would not handle.

Does renaming the DLL to KIS2.Unity.dll solve the problem you were having?

ihsoft commented 2 years ago

Trying to impose format limitations on mod version strings has not worked very well in the past

But you are already imposing the format limitation (and yes, it never works well)! The question is how far you wish to go make it less error prone. In the current approach a good standing mod installation can be broken by another mod, which by itself is good standing too. It was a pure luck that I broke my own mod, but it could easily be some other's mod. And I won't be even noticing it.

Does renaming the DLL to KIS2.Unity.dll solve the problem you were having?

Since it's owned by me, it's not be a big deal to change the name. But no, it doesn't solve the problem. Next time some one else may dare to use 'KIS." prefix in his mod, and it will break all users who are using "KIS".

If scanning the alternative locations of the mod is vital for CKAN (not sure why, but I guess you have the reason), then at least it makes sense to limit this check to the KSPAssembly annotated modules. Doing such a check for the dependency DLLs is a very error prone approach. Not to mention that people usually don't control the names of such libraries, and they simply won't be able to solve the issue.

HebaruSan commented 2 years ago

Is it still saying this, yes or no?

DLL for module KIS found at GameData/KIS2/Plugins/KIS.Unity.dll, but it's not where CKAN would install it. Aborting to prevent multiple copies of the same mod being installed. To install this module, uninstall it manually and try again.
Error during installation!
ihsoft commented 2 years ago

Changing the name of the module turned out to be a great deal to Unity: it resets all the prefabs if you rename a dependency DLL (surprise-suprise). I don't think I'll be able to spend time to it in a timespan of the nearest 2-3 weeks.

ihsoft commented 2 years ago

I've found a way to patch the Unity project to replace the dependency DLL. With name KIS_Unity.DLL CKAN works fine. So, the issue is solved for me now. I still believe the whole approach with the name format needs to be refined, but it's up to you.

HebaruSan commented 2 years ago

Another affected use case:

If you manually install some older versions of JNSQ, they bundle Kopernicus.Parser.dll, which is detected as a manually installed Kopernicus (with a version string of "Parser").