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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Jan 26, 2022

Problems

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

image

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):

image

(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)

@HebaruSan HebaruSan added Enhancement New features or functionality Core (ckan.dll) Issues affecting the core part of CKAN Pull request Netkan Issues affecting the netkan data labels Jan 26, 2022
@HebaruSan HebaruSan requested a review from DasSkelett January 26, 2022 00:35
@HebaruSan

This comment was marked as resolved.

@HebaruSan HebaruSan added the In progress We're still working on this label Mar 22, 2022
@HebaruSan HebaruSan removed the request for review from DasSkelett March 22, 2022 17:07
@HebaruSan HebaruSan force-pushed the feature/autodetect-cfg branch from fe804fe to 6d5cd63 Compare March 25, 2022 17:25
@HebaruSan HebaruSan requested a review from DasSkelett March 25, 2022 17:25
@HebaruSan HebaruSan removed the In progress We're still working on this label Mar 25, 2022
@HebaruSan HebaruSan force-pushed the feature/autodetect-cfg branch from 6d5cd63 to 6bf11e7 Compare March 25, 2022 21:19
@HebaruSan

This comment was marked as resolved.

@HebaruSan HebaruSan force-pushed the feature/autodetect-cfg branch 2 times, most recently from 9db004e to c65336e Compare March 25, 2022 21:35
@HebaruSan

This comment was marked as resolved.

@HebaruSan HebaruSan force-pushed the feature/autodetect-cfg branch 9 times, most recently from 119cd6b to 5245d8f Compare March 28, 2022 03:46
@HebaruSan HebaruSan force-pushed the feature/autodetect-cfg branch 2 times, most recently from cae4671 to baef48b Compare March 28, 2022 16:18
@HebaruSan

This comment was marked as resolved.

@HebaruSan

This comment was marked as resolved.

@DasSkelett
Copy link
Member

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
Copy link
Member

DasSkelett commented Apr 13, 2022

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
Copy link
Member Author

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.

@HebaruSan

This comment was marked as resolved.

@HebaruSan HebaruSan force-pushed the feature/autodetect-cfg branch 2 times, most recently from 199d4c8 to a666a17 Compare April 18, 2022 03:08
@HebaruSan HebaruSan added the In progress We're still working on this label Apr 22, 2022
@HebaruSan

This comment was marked as resolved.

@HebaruSan HebaruSan force-pushed the feature/autodetect-cfg branch 4 times, most recently from ee28353 to 4e52071 Compare April 29, 2022 18:30
@HebaruSan HebaruSan force-pushed the feature/autodetect-cfg branch 2 times, most recently from 24bb533 to 6d6f181 Compare May 1, 2022 13:32
@JonnyOThan
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

JonnyOThan commented Nov 22, 2023

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
Copy link
Member Author

HebaruSan commented Nov 22, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core (ckan.dll) Issues affecting the core part of CKAN Enhancement New features or functionality In progress We're still working on this Netkan Issues affecting the netkan data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants