Consider changing clap to configure_me

[Kixunil]

I noticed there's a bunch of code manually implementing conversions between different config structs. Also, I don't see where config field of Opts is used. Is it used at all?

My crate configure_me happens to deal with all the complexities of loading the configuration from one or more files, environment variables and arguments and merging it as appropriate. It was developed mainly with long-running daemons in mind (that's why it currently lacks subcommands, but has a good support for config files), so it might be more suitable for a project like this. It was also successfully used in electrs.

I also plan for it to support interface files which, if proven, should make connecting daemons such as this one to other services (most notably bitcoind) very easy.

As the author of configure_me, I'm willing to do the PR, just asking beforehand to not waste time if you don't find it interesting. I noticed this crate has a cli tool that uses subcommands, so maybe I will have to implement subcommand support first, unless you're fine with doing it manually at first.

[dr-orlovsky]

Thanks for suggestion! I've been seeing configure_me in electrs and was impressed by it; even more impressive that it's your work!

What I'd like to consider before adopting it. The current model that we follow — to adopt command-line options + environment using (yet unreleased) Clap 3.0 with custom derive — you may see how it works in https://github.com/LNP-BP/lnp-node/blob/aa20f431fd5f6753049eb45011b1c04b1b048daa/src/wired/config.rs#L23-L54: quite rust-native way, without pre-build code generation.

However, with this, we have config file yet uncovered: https://github.com/LNP-BP/lnp-node/blob/aa20f431fd5f6753049eb45011b1c04b1b048daa/src/bin/wired.rs#L26

While it would be good to cover this via configure_me, but it would be great if we will be able to maintain the same macro-derive style of Clap 3.0 as in the given sample file + reduce the amount generated code at pre-built step.

In general, I did differentiate Opts concept from Config: Opts are stuff that is read from Env/Cli args, while config is the thing that is read from config file modified by opts at launch time. My point in doing this was the following: we need config structure since not all of the parameters can be specified via environment and command-line arguments. Thus we need a config file and default set of configuration.

May you consider the next version of config_me with this information?

[Kixunil]

Yeah, the issue of derives came from several people several times. I'd definitely love to do it using derives, there's even an issue still open: Kixunil/configure_me#3

I currently see these obstacles around man page generation and other tooling (there's experimental debconf script generator already and I'd love to implement shell completions too):

• Man page generation was previously a part of the build script, but now it's moved to a separate tool to not mix up compilation with documenting. I was against this design (let people use cargo only, no makefiles) for some time, but eventually I started to like it more (separation of concerns).
• If I wanted to provide man page & co for derives, I'd either:
  • have to access files from within proc macros, this
    • goes against the design above
    • is extremely uncommon in Rust ecosystem (didn't see it anywhere at all)
    • might get impossible if syscalls from proc macros get disabled (I've read about some plans to do it)
  • scan the whole source tree from external tools, parse it and find all occurrences of my derive. I like this approach a bit, but see other problems with it:
    • Implementing seems to be very complex. I think I'd have to rely on someone else doing bulk of work, as I don't have too much time to implement such big things myself. According to Document *how to* "Bring proc-macro-like functionality to other contexts" dtolnay/proc-macro2#213 this feature doesn't exist yet.
    • obviously, when Rust adds a new syntax, the parsing instantly breaks, this doesn't happen with toml. To be fair, syn crate seems to be pretty up to date with the new stuff, so maybe not too hard to keep it up to date
    • what about other macros? If someone generates the struct with another macro, then the parser can't see it. Expanding the code doesn't help because it'd also expand my derive, losing the structural information.
    • I'm not sure how it'd work with interface files, which by definition have to be language-independent file format. I'm not pessimistic here, though.
    • scanning the whole tree might be slow in case of big projects (not sure).
  • do hacks like requiring people to have the struct in a single file that doesn't contain anything else (besides use ...;, I guess), this would be fairly easy, but lead to people being confused why man page generation doesn't work or even forgetting about it completely if they didn't attempt to generate man page at all. Then someone else would find it impossible to generate man page.
  • another hack would be to embed man page to the code and then the user could make the program itself output the man page. But what about other tools? I don't want to bake debconf script into every program...

I'm quite sad about this situation as I'd love to see less boilerplate (build.rs, which fortunately is currently only 3 lines), and proper IDE support. But considering the issues above, using toml seems to be a more useful trade-off to me. I hope you can see the reasons now. If you have a solution, I definitely want to know about it.

Regarding reducing the amount of generated code, what changes would you like to see? I guess I could move a bit more stuff into shared library, but I don't see how to achieve significant reduction. Most of the complexity comes from making sure that each field has a strong rustic type and well-understandable, human-readable error messages. I'm a huge fan of strong types and they were one of the primary design goals (clap had only stringly API when I started writing configure_me). Reducing the code would also make the generator simpler, so I'm definitely interested in improvements in this area.

[dr-orlovsky]

@Kixunil Let's do it! I am persuaded. Let's change Clap in wired (and the rest of future daemons will follow) with configure_me. I will still ask to leave Clap in lnp-cli though...

Will much appreciate a PR!

[Kixunil]

Great, I plan to look t it soon. Having clap in lnp-cli for now is reasonable. Keeping track of separate dependencies might not be entirely great, so it may motivate me to add the necessary features to configure_me sooner. :)

[Kixunil]

So I have something that passes typecheck. I have some linker errors so I can't test it yet. But anyway, there's one more issue: connect argument isn't implemented because configure_me can't accept arrays from command line and env vars yet. (in other words, it can accept them from config thanks to serde) Should I:

• Go ahead and ignore connect, since it isn't used anyway
• Disable arguments and env vars for the time being, only config file will be used for the time being
• Wait until arrays (multiple repeated arguments?) are implemented in configure_me

?

[dr-orlovsky]

I think we can ignore connect for now and bring it back later when configure_me will support arrays from cli args

[Kixunil]

When writing Kixunil/configure_me#38 I realized there's a quite easy workaround for multiple arguments - by making a wrapper implementing ParseArg by inserting and having a custom merge function. The effect of that would be that instead of replacing file configuration with arguments, it'd only extend it. It'd also merge all instances of connect if given in different files, without ability to remove them.

If you like this approach, I can write it.

[dr-orlovsky]

It seems fine, however not sure what did you mean by "instead of replacing file configuration with arguments, it'd only extend it". Does it mean that current version can use only config file or args, and not both of them? Even if yes, for sure it would be better to move to merge procedure

[Kixunil]

It can, the difference is what happens if an option is set in config file and arg. Normally arg will just overwrite whatever is int the file, but with custom merge_fn, the function defines the behavior. So for instance, we can write:

fn merge_vec(mut a: Vec<String>, b: Vec<String>) -> Vec<String> {
    a.extend(b);
    a
}

And this will cause the vecs to merge. This is handy in some scenarios (e.g. btc-rpc-proxy can have different users specified in different files), but makes it unable to use argument to remove an existing entry from the collection.

[Kixunil]

I implemented it. Was pretty easy, but required better support in rust-lnpbp. Once that is merged, I think we're pretty much done with this issue.