Binaries panic when no config file is found

[airidaceae]

hearth-server currently panics where there is no config file. it should either make a new empty file (or something with reasonable defaults if we update the config file down the line) or exit cleanly

[marceline-cramer]

Changed the title because hearth-client also has this issue.

[airidaceae]

We've decided to make the config file not required and also give it a use.

for each option that exists within hearth (hearth-root, init, etc) we want to check the config file and the flags. If both are set, the flags get priority. If only one is set, that one is used. If neither are set a default value will be used. If it is not reasonable to have a default value for whatever reason (ex. hearth could not guess where you store the kindling repository) hearth will exit gracefully with message about what is missing.

This should make the first time experience for running hearth less painful while still keeping the usefulness of a config file for more advanced users.

[airidaceae]

There seems to be a few ways to go about this.
clap serde
clap config
those seem pretty out of date though. Im digging through stack overflow and github issues looking for the best solution right now. clap_serde is looking like the best option atm.

This is a problem I feel like should be solved but it sadly seems a bit hard to get good information on it.

[airidaceae]

Keeping good error messages is really important. Clap's built in error messages are really helpful. If we handle everything after claps parsing is finished we lose out on the really nice error messages that clap provides. There doesnt seem to be any support in clap itself for this.

[marceline-cramer]

Found some relative stuff:
https://stackoverflow.com/questions/55133351/is-there-a-way-to-get-clap-to-use-default-values-from-a-file
clap-rs/clap#2763

[marceline-cramer]

Also found this: https://docs.rs/config/0.13.3/config/
More clap-relevant info: clap-rs/clap#2763 (comment)

[marceline-cramer]

This seems really close to what we need: https://crates.io/crates/clap-serde-derive/

[airidaceae]

I like that. Still worried a bit about error messages but I'm honestly not sure if there's much we can do about that.

[airidaceae]

going to start trying to implement this now. We can see what the UX looks like when I'm done and decide from there.

[marceline-cramer]

Got it, sounds great.

[marceline-cramer]

By the way, with regards to #174, it seems like the initial issue (binaries panic with no config) and what we're using to solve this with layered config are not closely related.

I think that the initial mistake that led to this issue being created was making hearth-core responsible for loading configuration files. Wasm CI only uses hearth-wasm's run_wasm binary, and it doesn't access any other part of the runtime, so there's no need for actual config file reading.

I think that config file parsing should still be completely removed from the core. If plugins want to accept command line arguments, they should define a configuration structure using clap_serde_derive and then consume that in their new() functions. This way, we can specify config options in small binaries such as tests or run_wasm without having to create a configuration file, and the core doesn't need to be involved at all.

The binaries themselves are responsible for including those plugin config structs into their own config structs and then passing those to the plugin constructors.

tl;dr layering configs doesn't actually solve the problem blocking #174.

What do you think?

[airidaceae]

This issue was initially created because it was hard to run hearth for the first time. If the config file wasnt found when running either server or client it would panic. We wanted to make the config file optional and layering it was a pretty natural extension of that.

I wasnt aware that the config file loading was done in hearth-core and I agree that it would be a good idea to move that out of there.

I dont really get what plugins have to do with things right now.

[marceline-cramer]

Ah, hmmm, the core doesn't actually load the config file, but it DOES require a TOML table in RuntimeBuilder::new(), which the binaries have to parse, including run_wasm.

The idea with making the core participate in config loading was that individual plugins could all read from the configuration file (see RuntimeBuilder::load_config), although we don't actually ever do that right now. Whatever config system we come up with should have a way to easily add config options to plugins. For example, we could override audio devices or set renderer settings in the configuration file. Again, we don't use that right now, but that's why I initially deeply integrated the config system into the core.

Basically what I'm saying is that we should completely strip config stuff from the core API.

A temporary solution to unblock #174 would be just to pass in an empty TOML table to the runtime builder in the run_wasm binary, since it has no need for a config file. What do you think?

[airidaceae]

got it. How badly do we need #174 right now? This issue has fallen somewhat low on my priorities list so that may be worth doing.

[airidaceae]

clap serde derive looks pretty perfect. My one concern is that it is dependent on clap 4. We are currently using clap 3 after #20. In my admittedly limited research it does not seem like there is a reasonable way to get colored output in v4. Honestly though, I think that this may be worth losing out on the coloring for. Thoughts?

[airidaceae]

It seems possible to configure it with an unstable feature? may be more hassle than its worth
https://stackoverflow.com/questions/74068168/clap-rs-not-printing-colors-during-help

[marceline-cramer]

@airidaceae want to open a new issue to add layered config with everything we know now WRT clap_serde_derive and derived config types per runtime? We can open that now that #277 will close this.