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

Netkan warning when include_only doesn't match #3805

Merged
merged 1 commit into from
Mar 11, 2023

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Mar 9, 2023

Problem

See KSP-CKAN/NetKAN#9614; a mod using include_only was changed to contain different filenames than we were trying to include, and there was no inflation error or warning.

    include_only:
      - Assets
      - ASET_StckRplc_Mk1-2_Pod.cfg
      - RPM_ASET_StckRplc_Mk1-2_Pod_Patch.cfg

Cause

A different include_only list element (Assets) still matched, so there were still files being installed. (The other mods from that PR had no other include_only members, so they hit the "No files found" error).

Changes

Now the InstallsFilesValidator prints a warning if any include_only fails to match at least one installed file.

The logic isn't super sophisticated or water tight; it can miss things in very complex setups (e.g., if two stanzas have the same include_only and one has a match and the other doesn't, that file will count towards both). But it catches what we need it to:

NetKAN $ git checkout 30590d1fa
NetKAN $ netkan.exe NetKAN/MK12PodIVAReplacementbyASET.netkan
1976 [1] WARN CKAN.NetKAN.Validators.InstallsFilesValidator (null) - No matches for includes_only: ASET_StckRplc_Mk1-2_Pod.cfg, RPM_ASET_StckRplc_Mk1-2_Pod_Patch.cfg
2081 [1] WARN CKAN.NetKAN.Validators.ModuleManagerDependsValidator (null) - ModuleManager dependency may not be needed, no ModuleManager syntax found

I made this a warning instead of an error because include_only might conceivably be used to select things that might be in a mod, so human judgment should be applied to determine whether changes are needed. (And also because it's not water tight, so a false positive should not block merging.)

@HebaruSan HebaruSan added Enhancement New features or functionality Pull request Netkan Issues affecting the netkan data labels Mar 9, 2023
@HebaruSan HebaruSan requested a review from techman83 March 9, 2023 20:33
@HebaruSan HebaruSan force-pushed the fix/includes-only-validation branch from 6ce9188 to bd6b94d Compare March 9, 2023 20:37
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, makes sense!

@HebaruSan HebaruSan merged commit 5d7107b into KSP-CKAN:master Mar 11, 2023
@HebaruSan HebaruSan deleted the fix/includes-only-validation branch March 11, 2023 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New features or functionality Netkan Issues affecting the netkan data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants