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

Warnings for missing swinfo.json deps #3827

Merged
merged 1 commit into from Apr 18, 2023

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Apr 18, 2023

Problem

See KSP-CKAN/KSP2-NetKAN#33, a dependency was missing from a KSP2 module. This dependency was documented (only) in the swinfo.json file, and the netkan didn't have it.

This could easily happen again with other mods, either when initially adding them or as new versions are released.

Cause

The inflator parses the swinfo.json file but ignores its dependencies property. I thought about automatically importing it into CkanModule.depends, but that would have trapped us if:

  • The identifier was different in CKAN (there's no guarantee of matches)
  • The swinfo.json was wrong

Changes

Now when we inflate a module with a swinfo.json file, if it has dependencies listed, we'll check them and log a warning for any identifier that isn't in the CkanModule.depends list. It's a warning instead of an error so a human can decide what to do.

$ netkan.exe --game KSP2 NetKAN/CheatsMenu.netkan
1788 [1] WARN CKAN.NetKAN.Transformers.SpaceWarpInfoTransformer (null) - Dependencies from swinfo.json missing from module: ShadowUtilityLIB

For now, min/max versions are ignored. From what I've seen, these tend to be filled in unnecessarily with non-useful data, so it would be a lot of noise / alert-fatigue to see these all if we logged them.

This way we will get a notification in Discord if a KSP2 module adds a new dependency and documents it in the swinfo.json file, or in the GitHub Action output in a pull request.

Considered and not done

In principle, this code belongs in a Validator file/class.

In practice, I started that and it got really messy. I had to duplicate essentially all of the logic of the Transformer class, including retrieving the file from the ZIP and checking the remote copy. The last straw was when I had to add an IGithubApi object to CkanValidator, which really does not belong there.

Since I was ending up with an extended copy of the Transformer class, I thought it would be cleaner to extend the original. We can look into adding some sort of shared retrieval and caching for swinfo.json data in the future and revisit this then.

This means these warnings will only fire for .netkan files, not for validating .ckan files. I think that's fine for now.

@HebaruSan HebaruSan added Enhancement Easy This is easy to fix Netkan Issues affecting the netkan data Relationships Issues affecting depends, recommends, etc. labels Apr 18, 2023
@HebaruSan
Copy link
Member Author

HebaruSan commented Apr 18, 2023

Self-reviewing so I can get a sense of what's out there in the already indexed mods...

... nothing, apparently!

@HebaruSan HebaruSan merged commit 4aa4db6 into KSP-CKAN:master Apr 18, 2023
12 checks passed
@HebaruSan HebaruSan deleted the feature/swinfo-dep-warning branch April 18, 2023 16:43
@HebaruSan
Copy link
Member Author

This worked, but we didn't catch the message (eventually addressed in KSP-CKAN/KSP2-NetKAN#44):

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy This is easy to fix Enhancement Netkan Issues affecting the netkan data Relationships Issues affecting depends, recommends, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant