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

Default install stanza multi-game support, catch missing install_to #3441

Merged
merged 2 commits into from
Sep 17, 2021

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Aug 28, 2021

Problem

In KSP-CKAN/NetKAN#8735, this metadata:

install:
  - find: GameData/InterstellarTechnologies
  - install_to: GameData

caused this error:

Error: 1459 [1] FATAL CKAN.NetKAN.Program (null) - Object reference not set to an instance of an object

Cause

install_to is required by the spec, and NetKAN.Validators.InstallValidator assumes install_to will be set. In this instance, it wasn't, because that YAML translates to:

  "install": [
    {
      "find": "GameData/InterstellarTechnologies"
    },
    {
      "install_to": "GameData"
    }
  ]

We should catch this explicitly rather than throwing a NRE.

Motivation

That PR's changes were essentially trying to make the default install stanza's behavior more explicit, so they've been set aside. However, in the process of explaining this I realized that the default install stanza is KSP1-specific: it hard-codes GameData. This would make default stanzas unusable for any future supported game that uses a different folder for mod data.

Changes

  • Now if install_to isn't set, we throw an explicit error saying that
  • Now the default install stanza uses the PrimaryModDirectoryRelative property of the module's game, which will be GameData for KSP1 and wherever mods are supposed to go instead for other games
  • The spec is updated to hopefully be clearer about how the default install stanza works

@HebaruSan HebaruSan added Bug Easy This is easy to fix Core (ckan.dll) Issues affecting the core part of CKAN Pull request Netkan Issues affecting the netkan data labels Aug 28, 2021
@DasSkelett
Copy link
Member

install_to is required by the spec

Interestingly, the client does work with a missing install_to in .ckan files, it fills in GameData when deserializing:

[JsonProperty("install_to", DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate)]
[DefaultValue("GameData")]
public string install_to;

(Just tested it, works indeed)

Also related to this comment, the null check is indeed always false:

// Make sure our install_to fields exists. We may be able to remove
// this check now that we're doing better json-fu above.
if (install_to == null)
{
throw new BadMetadataKraken(null, "Install stanzas must have an install_to");
}

I guess we could keep it, as valid metadata should always have an install_to. Or we could also try to use game.PrimaryModDirectoryRelative, but this would require serialization context work etc.. Maybe we should just not populate it at all and throw the hard BadMetadataKraken.

@HebaruSan
Copy link
Member Author

it fills in GameData when deserializing:
(Just tested it, works indeed)

Oh, I think that was done to save space in registry.json, since there were thousands of occurrences of install_to: GameData. So everyone's existing registry files have missing install_to properties, and we can't remove the defaulting now.

@DasSkelett
Copy link
Member

Oh wow, you're right, they are indeed left out in registry.json. Didn't realize that before.

@HebaruSan HebaruSan merged commit 0380d8e into KSP-CKAN:master Sep 17, 2021
@HebaruSan HebaruSan deleted the fix/default-install-multigame branch September 17, 2021 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Core (ckan.dll) Issues affecting the core part of CKAN Easy This is easy to fix Netkan Issues affecting the netkan data Pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants