Detect manually installed mods via cfg file :FOR[identifier] clauses

[HebaruSan]

Problems

If you manually install JNSQ and ReStock and all their dependencies, CKAN detects the dependencies but not JNSQ or ReStock:

All non-plugin mods are affected similarly, as are some plugin mods.

Causes

In the case of JNSQ, this is because it has no plugins, and the code to detect manually installed mods only looks at DLLs. For ReStock it's because the capitalization of Restock.dll doesn't match the identifier.

Background

Module Manager's :FOR[identifier] clause can be used in a cfg file to declare that a mod is installed, to satisfy MM's :NEEDS[identifier] clauses for other mods:

ConfigNode:FOR[identifier]
{
}

Both JNSQ and ReStock use this syntax.

Changes

Now when we scan the game dir for manually installed mods, in addition to DLLs, we also consider .cfg files, but instead of just inspecting the filename as we do for DLLs, we look for :FOR[identifier] clauses in the file's contents. This allows us to detect manual installations of JNSQ and ReStock (and probably many other non-plugin mods):

(I even verified that you can "upgrade" these to CKAN-managed status. CKAN spends a very long time confirming that all of JNSQ's 3.68 GiB of files are identical, but it does eventually finish and prompt the user for permission to replace them. #3512 probably helped with that.)

The file scan should be pretty robust because it is done with a full parser of ModuleManager-augmented config node syntax. For example, it won't be fooled if a UI string says ":FOR[identifier]" somewhere; only real, valid uses of :FOR[] will be used. Tests for the parser are included.

The parser uses https://github.com/acple/ParsecSharp , which requires C# 7.3 or later, so I decided to upgrade to C# 8.0, which requires .NET Framework 4.6.1. .NET Core recognizes a LangVersion of 8.0 but not 8, so we had to go with 8.0, but unfortunately Framework 5.20 recognizes 8 but not 8.0, so we've dropped 5.20 from the builds.

Since this syntax is specific to KSP, the code to parse it is under the Core/Games/KerbalSpaceProgram directory and works via the IGame interface to allow other syntaxes to be parsed for other games.

Netkan is updated to use the parser (with results of parsing cached for 15 minutes to avoid repeat processing, which might improve performance if retrieving the file contents from the ZIP is a bottleneck) in the places where it examines cfg files currently:

• Localizations deduced from the real syntax rather than a regex kludge
• ModuleManager warnings now check the real syntax instead of a regex kludge
• The validation check for "manual installations won't be detected" is now suppressed for mods that contain a cfg file with a :FOR[identifier] clause with a matching identifier (such as JNSQ and ReStock)

[DasSkelett]

If we did that, would we want to host it in the KSP-CKAN organization, or should it be a personal repo of mine?

Under KSP-CKAN would be totally fine with me 👍

[DasSkelett]

So the command my IDE issues to start the client with debugging is

/usr/bin/mono-sgen --debug --debugger-agent=transport=dt_socket,server=y,suspend=y,address=127.0.0.1:55555,setpgid=y ./_build/out/CKAN-GUI/Debug/bin/CKAN-GUI.exe

To run it manually you need to replace suspend=y with suspend=n so it doesn't wait for a debugger to attach at launch.

But it does replicate the crash, without the --debugger-agent part it doesn't.

I don't really know what to do now. It's most likely a mono bug, and while we could report it it's unlikely to receive any attention (they don#t even merge ready-made severe bug fixes anymore).
We could report it to the ParsecSharp author, but they might be happier if we give them a smaller reproducer.

But I don't want to merge it without it being fixed, as it would break debugging completely for the entire application.

[HebaruSan]

Hmm, your guess about AggressiveInlining/NoInlining is a good one, so if we don't use that in any of the publicly accessible members of the parser DLL, maybe a Nuget package could isolate this problem to itself internally and protect dependencies from it.

[JonnyOThan]

Just browsing PRs...this one sounds pretty interesting, is there anything I can do to help? Is this ready to go but just needs merging?

Another thought I had along similar lines (and maybe should be a separate issue) is that many mods have a DLL but it's named differently from the metadata identifier. We could add something to the metadata that provides the name of the DLL that should be considered for auto-detection.

[HebaruSan]

Just browsing PRs...this one sounds pretty interesting, is there anything I can do to help? Is this ready to go but just needs merging?

I have two outstanding concerns:

• Performance
  Parsing every unregistered .cfg file is potentially a lot more work (compared to just checking filenames) and could slow down the client. I don't want to merge this and then get a hundred "CKAN is much slower" bug reports.
• Bad data
  Misusing :FOR[] is very common, which means we'd be giving badly coded mods the opportunity to mess with CKAN's behavior. This seems very risky unless we have some way to deal with that, at least limit the damage that can be done if we can't make it perfect.

I do still think there's potential value here, which is why this isn't closed yet. But these risks feel hard to quantify and potentially serious, and I'd like to think of ways to address them merging.

Another thought I had along similar lines (and maybe should be a separate issue) is that many mods have a DLL but it's named differently from the metadata identifier. We could add something to the metadata that provides the name of the DLL that should be considered for auto-detection.

I had this in the back of my mind as a "someday" project. It could also be a way to detect more mods if it can check for non-DLL files.

[JonnyOThan]

I had this in the back of my mind as a "someday" project. It could also be a way to detect more mods if it can check for non-DLL files.

Oh...this kind of seems like a better solve for the whole thing, right? Maybe not quite as automatic, but I'm sure you could analyze the files that a mod installs and make some good guesses about which ones are THE file that defines the mod.

[HebaruSan]

Oh...this kind of seems like a better solve for the whole thing, right? Maybe not quite as automatic, but I'm sure you could analyze the files that a mod installs and make some good guesses about which ones are THE file that defines the mod.

Possibly, yes. Another reason this PR is in stasis; if we try that approach and it works well, then it could make sense to close this instead of merging.