Implement basic config loading and parsing

[michalhosna]

Based on #10. Replaces existing CLI flags with a config file.
I decided to go with reflect-cpp as it is "self-documenting" with possible JSON schema generation.
I'll follow up with implementing the rest of #10 after the basics are agreed upon here.

Stands on top of #28 to use the automatic formatting.
Fixes #22

---

This change is 

[mondain]

@mondain reviewed 9 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on afrind).

[afrind]

@afrind made 19 comments.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on michalhosna).

---

include/o_rly/config/config.h line 13 at r1 (raw file):

#include "o_rly/config/string_literal.h"

namespace openmoq::o_rly::config {

Do we like deeply nested namespaces? I 'm ok with it, just want to ask

---

include/o_rly/config/config.h line 19 at r1 (raw file):

  static constexpr uint16_t kDefaultPort = 9668;

  rfl::Description<str_concat<"Bind address (default: \"", kDefaultAddress, "\")">(),

I don't really know what rfl does (I guess I know who to ask lol).

---

include/o_rly/config/config.h line 20 at r1 (raw file):

  rfl::Description<str_concat<"Bind address (default: \"", kDefaultAddress, "\")">(),
                   std::optional<std::string>>

If this represents the parsed config, do we want to hold this in e.g. a folly::SocketAddress instead of a string (which may or may not be an actual address)?

Or at least, when this is passed around, is it assume that it's been validated?

---

include/o_rly/config/config.h line 37 at r1 (raw file):

};

struct TlsCredentials {

There's a lot to TLS config besides certificates (eg. credentials). Do you envision a larger struct encapsulating this or should we rename for future growth?

---

include/o_rly/config/config.h line 43 at r1 (raw file):

  rfl::Description<"Path to TLS private key file", std::optional<std::string>> key_file;
  rfl::Description<
      str_concat<"Skip TLS, insecure mode (default: ", bool_to_str<kDefaultInsecure>(), ")">(),

At least in moxygen, this doesn't mean "skip tls" but instead it means "use a default certificate/key that is compiled into the code"

---

include/o_rly/config/config.h line 47 at r1 (raw file):

      insecure;

  bool insecureOrDefault() const { return insecure.value().value_or(kDefaultInsecure); }

Isn't another way to handle this by declaring it as bool insecure{kDefulatInsecure}? or does the rfl thing require everything to be optional?

---

include/o_rly/config/config.h line 51 at r1 (raw file):

struct ListenerConfig {
  static constexpr char kDefaultEndpoint[] = "/moq-relay";

std::string_view?

---

include/o_rly/config/string_literal.h line 3 at r1 (raw file):

#pragma once

// Compile-time string utilities for building rfl::Description strings

My goodness. This is required to make rfl happy because it has it's own string types?

---

scripts/config-schema-to-markdown.sh line 3 at r1 (raw file):

#!/usr/bin/env bash
# Generate markdown config reference from o-rly JSON schema on stdin.
# Usage: o_rly dump-config-schema | scripts/gen-config-reference.sh

Can you paste / link the output of this script?

---

src/main.cpp line 17 at r1 (raw file):

// On success returns LoadResult for "serve" mode.
// On error returns an exit code (for early-exit subcommands or failures).
folly::Expected<openmoq::o_rly::config::LoadResult, int> handleSubcommand(int argc, char* argv[]) {

What do you think about moving this into another file if you think there may be a lot of subcommands? I don't want to clutter main too much?

Also I wonder if we should build a dispatch table of command -> handler rather than this cascading if. This is ok for now though.

---

src/main.cpp line 18 at r1 (raw file):

// On error returns an exit code (for early-exit subcommands or failures).
folly::Expected<openmoq::o_rly::config::LoadResult, int> handleSubcommand(int argc, char* argv[]) {
  std::string subcommand = "serve";

Let's use symbolic constants for subcommands (kServeCommand, kValidateConfigCommand) etc

---

src/main.cpp line 29 at r1 (raw file):

  if (subcommand != "serve" && subcommand != "validate-config") {
    std::cerr << "Unknown subcommand: " << subcommand << "\n";

I think these are probably appropriate straight uses of std::cerr rather than our logging framework.

---

src/config/loader.cpp line 14 at r1 (raw file):

namespace {

std::string readFileContents(const std::string& path) {

Unsure if you do or do not want additional folly deps, but there's helpers for things like this - see folly::File and folly::readFile (see https://github.com/facebook/folly/blob/main/folly/FileUtil.h)

---

src/config/loader.cpp line 37 at r1 (raw file):

  // Matches reflect-cpp v0.18.0 error format for NoExtraFields violations.
  // Update if upgrading reflect-cpp changes the error message format.
  std::regex fieldRegex(R"(Value named '([^']+)' not used)");

std::regex is notoriously slow, so we should use it cautiously.

---

src/config/loader.cpp line 64 at r1 (raw file):

  lr.config = std::move(*result);

  auto unknownWarnings = detectUnknownFields(content);

Wait, you already called yaml::read and now detectUnknownFields calls it again. Can you combine into a single read() call or am I missing something?

---

src/config/loader.cpp line 106 at r1 (raw file):

  // Listener must have udp configured
  if (!listener.udp.value().has_value()) {

Again don't know rfl, but what is the type of udp() here? If it was itself optional then we don't need value().has_value() which looks silly.

---

src/config/loader.cpp line 113 at r1 (raw file):

    auto sock = listener.udp.value()->socketOrDefault();
    uint16_t port = sock.portOrDefault();
    if (port == 0) {

Eventually we will want to allow port 0 for integration tests (pick an available one), but this is ok for now.

---

src/config/loader.cpp line 147 at r1 (raw file):

}

std::vector<std::string> warnConfig(const Config& config) {

Thoughts on combining the config validation into a single pass that adds both warnings and errors? I think it might be easier if we only had one place in the code where we think about e.g. listener config

---

src/config/loader.cpp line 175 at r1 (raw file):

}

std::string moqtVersionsToString(const ListenerConfig& listener) {

See folly::join

[michalhosna]

Just a partial submission. I'll do the rest tomorrow

@michalhosna made 12 comments.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on afrind).

---

include/o_rly/config/config.h line 13 at r1 (raw file):

Previously, afrind wrote…

Do we like deeply nested namespaces? I 'm ok with it, just want to ask

🤷 I didn't put much thought into it; I just started with something—let's call it strawman. Happy to change either way.

---

include/o_rly/config/config.h line 19 at r1 (raw file):

A C++20 library for fast serialization, deserialization and validation using reflection. Supports JSON, Avro, BSON, Cap'n Proto, CBOR, CSV, flexbuffers, msgpack, parquet, TOML, UBJSON, XML, YAML / msgpack.org[C++20]

It actually uses yaml-cpp for parsing here. The main point is having validation and documentation in a single place.

Given that we are trying to hit a moving target, as we are not yet sure what kind of configuration, scoping, MOQT features, filters, etc. will need what kind of documentation. Therefore, the configuration will evolve quite a bit. Having a strong documentation and validation story seemed important to me.

Not that rfl is perfect, but the best I know about in cpp land.

---

include/o_rly/config/config.h line 20 at r1 (raw file):

Previously, afrind wrote…

If this represents the parsed config, do we want to hold this in e.g. a folly::SocketAddress instead of a string (which may or may not be an actual address)?

Or at least, when this is passed around, is it assume that it's been validated?

This needs to map 1:1 to the YAML file. It's not really meant to be passed around.

It would be nice if one could have the same structure representing the config file and pass it around. But usually those two tasks require different tradeoffs.

E.g. Folly::SocketAddress represents address and port. But we have those two as different fields in the YAML.

We could merge them. Not a big deal here, but generally it is good to be able to decouple the config for human readability and internal representation for correctness.

I tried to keep this PR simple, but there will 100% need to be another layer that provides validated structures with nice concrete types for passing around as the need comes.

I am generally more a fan of implementing stuff once it's needed and not trying to do too much (over-)engineering beforehand, but I'm happy to adapt to a different style.

---

include/o_rly/config/config.h line 37 at r1 (raw file):

Previously, afrind wrote…

There's a lot to TLS config besides certificates (eg. credentials). Do you envision a larger struct encapsulating this or should we rename for future growth?

I put the first name that came to mind here; renaming is cheap in the future. I do expect to rewrite this completely.

Do you want to rename it now? Any concrete suggestions? TlsConfig?

---

include/o_rly/config/config.h line 43 at r1 (raw file):

Previously, afrind wrote…

At least in moxygen, this doesn't mean "skip tls" but instead it means "use a default certificate/key that is compiled into the code"

Fixed

---

include/o_rly/config/config.h line 47 at r1 (raw file):

Previously, afrind wrote…

Isn't another way to handle this by declaring it as bool insecure{kDefulatInsecure}? or does the rfl thing require everything to be optional?

The std::optional wrapper is what tells reflect-cpp the field can be omitted from the YAML.
Changing to bool would make the field required regardless of the initializer, as reflect-cpp would just see the bool type.

---

include/o_rly/config/config.h line 51 at r1 (raw file):

Previously, afrind wrote…

std::string_view?

That won't work with rfl for the compile-time string in the documentation.

Starting to get feeling you won't like rfl much. But I don't think keeping documentation separate and updating it manually will be better.

---

include/o_rly/config/string_literal.h line 3 at r1 (raw file):

Previously, afrind wrote…

My goodness. This is required to make rfl happy because it has it's own string types?

It's only so I can concatenate strings with constants.
I want to print defaults in the description and have them defined only once so the documentation cannot drift.

It's gnarly, but as I expect the configuration file itself will get gnarly down the line, I think it's a fair tradeoff to have this.

---

scripts/config-schema-to-markdown.sh line 3 at r1 (raw file):
The JSON scheme can be consumed by multiple tools; this is just a vibe-coded helper to have something humanly readable in-tree

---

o-rly Configuration Reference

Auto-generated from JSON schema. Do not edit manually.

cache

Relay cache settings (default: true)

Field	Type	Description
enabled	boolean	Enable relay cache (default: true)
max_groups_per_track	integer	Max cached groups per track, ignored when disabled (default: 3)
max_tracks	integer	Max cached tracks, ignored when disabled (default: 100)

listeners

Listener definitions (currently exactly one supported)

Field	Type	Description
endpoint	string	WebTransport endpoint path (default: "/moq-relay")
moqt_versions	integer[]	MOQT draft versions (empty = all supported)
name	string	Listener name
tls_credentials.cert_file	string	Path to TLS certificate file
tls_credentials.insecure	boolean	Insecure mode, use default compiled-in certificate (default: false)
tls_credentials.key_file	string	Path to TLS private key file
udp.socket.address	string	Bind address (default: "::")
udp.socket.port	integer	Listen port, 1-65535 (default: 9668)

---

src/main.cpp line 29 at r1 (raw file):

Previously, afrind wrote…

I think these are probably appropriate straight uses of std::cerr rather than our logging framework.

Yep

FYI You can mark comments as informational in reviewable, so that I have to ack them, but it's clear its just a FYI.

---

src/config/loader.cpp line 113 at r1 (raw file):

Previously, afrind wrote…

Eventually we will want to allow port 0 for integration tests (pick an available one), but this is ok for now.

We'll then need a way to get the assigned port out, I expect. I would rather not deal with that now. Or does Moxigen Folly already solve that?

[afrind]

@afrind reviewed all commit messages, made 8 comments, and resolved 10 discussions.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on michalhosna).

---

include/o_rly/config/config.h line 13 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

🤷 I didn't put much thought into it; I just started with something—let's call it strawman. Happy to change either way.

Keep for now

---

include/o_rly/config/config.h line 19 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

A C++20 library for fast serialization, deserialization and validation using reflection. Supports JSON, Avro, BSON, Cap'n Proto, CBOR, CSV, flexbuffers, msgpack, parquet, TOML, UBJSON, XML, YAML / msgpack.org[C++20]

It actually uses yaml-cpp for parsing here. The main point is having validation and documentation in a single place.

Given that we are trying to hit a moving target, as we are not yet sure what kind of configuration, scoping, MOQT features, filters, etc. will need what kind of documentation. Therefore, the configuration will evolve quite a bit. Having a strong documentation and validation story seemed important to me.

Not that rfl is perfect, but the best I know about in cpp land.

Ok, seems like it might be heavyweight but it's fine. Let's go.

---

include/o_rly/config/config.h line 20 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

This needs to map 1:1 to the YAML file. It's not really meant to be passed around.

It would be nice if one could have the same structure representing the config file and pass it around. But usually those two tasks require different tradeoffs.

E.g. Folly::SocketAddress represents address and port. But we have those two as different fields in the YAML.

We could merge them. Not a big deal here, but generally it is good to be able to decouple the config for human readability and internal representation for correctness.

I tried to keep this PR simple, but there will 100% need to be another layer that provides validated structures with nice concrete types for passing around as the need comes.

I am generally more a fan of implementing stuff once it's needed and not trying to do too much (over-)engineering beforehand, but I'm happy to adapt to a different style.

There's folly::IPAddress too. But string works for now -- unclear if we want non-IP addresses (eg unix sock?)

---

include/o_rly/config/config.h line 37 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

I put the first name that came to mind here; renaming is cheap in the future. I do expect to rewrite this completely.

Do you want to rename it now? Any concrete suggestions? TlsConfig?

Kind of like TLSConfig better, but not blocking on this.

---

include/o_rly/config/config.h line 47 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

The std::optional wrapper is what tells reflect-cpp the field can be omitted from the YAML.
Changing to bool would make the field required regardless of the initializer, as reflect-cpp would just see the bool type.

Hmm, that's unfortunate -- we want things optional in the config but always have a value in the structure.

We could make a macro like

#define ORLY_CONFIG(DESC, TYPE, NAME, DEFAULT) \
  rfl::Description<DESC, std::optional<TYPE>> _ ## NAME; \
  TYPE NAME() { return _ ## NAME.value_or(DEFAULT); }

or both ORLY_OPTIONAL_CONFIG (with default) and ORLY_REQUIRED_CONFIG (without)

---

include/o_rly/config/config.h line 51 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

That won't work with rfl for the compile-time string in the documentation.

Starting to get feeling you won't like rfl much. But I don't think keeping documentation separate and updating it manually will be better.

Ah, I see rfl requires this specific type.

---

include/o_rly/config/string_literal.h line 3 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

It's only so I can concatenate strings with constants.
I want to print defaults in the description and have them defined only once so the documentation cannot drift.

It's gnarly, but as I expect the configuration file itself will get gnarly down the line, I think it's a fair tradeoff to have this.

I'm going to look away

---

src/config/loader.cpp line 113 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

We'll then need a way to get the assigned port out, I expect. I would rather not deal with that now. Or does Moxigen Folly already solve that?

In some integration tests that launch the binary independently, we [don't laugh] print it to stdout.

But we also have a framework where you can run our proxy as a thread inside another program. This is useful as an C++ integration test that has the clients in some threads, servers in another, and proxy is right there -- you get the bound address with a getAddress API.

[michalhosna]

I split off ParsedConfig to represent the structures required for file parsing and config that represents parsed and validated config.
I should have done this from the start.

It's quite a different approach, so it may be wise to not review the diff to the previous revision of this PR, which is the default, but review the diff to main. Time to flex reviewable features.

Also, I rebased this on main with different formatting. With reviewable, this should not clutter the diff much,

@michalhosna made 13 comments.
Reviewable status: 1 of 16 files reviewed, 9 unresolved discussions (waiting on afrind and mondain).

---

include/o_rly/config/config.h line 20 at r1 (raw file):

Previously, afrind wrote…

There's folly::IPAddress too. But string works for now -- unclear if we want non-IP addresses (eg unix sock?)

See the new file structure for folly::IPAddress.

Does QUIC over unix sockets make sense? I am not sure, probably not? So that would be raw moqt over unix sockets? What about datagrams?

I had this line of thought and decided that it doesn't need resolving now 😄

---

include/o_rly/config/config.h line 37 at r1 (raw file):

Previously, afrind wrote…

Kind of like TLSConfig better, but not blocking on this.

Changed to TLS config and just tls in the yaml.

---

include/o_rly/config/config.h line 47 at r1 (raw file):

Previously, afrind wrote…

Hmm, that's unfortunate -- we want things optional in the config but always have a value in the structure.

We could make a macro like

#define ORLY_CONFIG(DESC, TYPE, NAME, DEFAULT) \
  rfl::Description<DESC, std::optional<TYPE>> _ ## NAME; \
  TYPE NAME() { return _ ## NAME.value_or(DEFAULT); }

or both ORLY_OPTIONAL_CONFIG (with default) and ORLY_REQUIRED_CONFIG (without)

What about optional fields that doesn't have a default? Which I expect would be the majority.

And do the macros even help?

 rfl::Description<"Path to TLS private key file", std::optional<std::string>> key_file;

vs

 ORLY_OPTIONAL_CONFIG_NO_DEFAULT("Path to TLS private key file", std::optional<std::string>>, key_file);

I think the explicitness and wordiness here is actually better then having a level of indirection.

What do you think with the current split of concerns between Config and ParsedConfig?

---

src/main.cpp line 17 at r1 (raw file):

Previously, afrind wrote…

What do you think about moving this into another file if you think there may be a lot of subcommands? I don't want to clutter main too much?

Also I wonder if we should build a dispatch table of command -> handler rather than this cascading if. This is ok for now though.

I am not sure if we plan to have a lot of subcommands.

Currently its like, "serve" = please continue in the main. Other commands = quick exit.

Using a dispatch table in a different file breaks this "please continue". Should the rest of the main be moved into some kind of serve handler and make the main essentially empty? It makes sense, but also it's maybe too drastic given that that's the main way.

Happy to do it if you thinks its better. I wasn't even sure if subcommands are the way to go. Is a serve good default name?

---

src/main.cpp line 18 at r1 (raw file):

Previously, afrind wrote…

Let's use symbolic constants for subcommands (kServeCommand, kValidateConfigCommand) etc

Done.

---

src/config/loader.cpp line 14 at r1 (raw file):

Previously, afrind wrote…

Unsure if you do or do not want additional folly deps, but there's helpers for things like this - see folly::File and folly::readFile (see https://github.com/facebook/folly/blob/main/folly/FileUtil.h)

Done.

---

src/config/loader.cpp line 37 at r1 (raw file):

Previously, afrind wrote…

std::regex is notoriously slow, so we should use it cautiously.

Dropped this whole code for now

---

src/config/loader.cpp line 64 at r1 (raw file):

Previously, afrind wrote…

Wait, you already called yaml::read and now detectUnknownFields calls it again. Can you combine into a single read() call or am I missing something?

I wanted to always warn on unknown fields and only hard error in strict mode. The RFL doesn't support this. Its either ignore or hard error. Most libraries don't support it, given that you need a wrapper type for that and warnings don't have a clear structure.

But maybe I am too cautious. I simplified it to strict mode with hard error and ignore during non strict. Given that we provide the validation subcommand, it should be fine.

---

src/config/loader.cpp line 106 at r1 (raw file):

Previously, afrind wrote…

Again don't know rfl, but what is the type of udp() here? If it was itself optional then we don't need value().has_value() which looks silly.

The type of UDP is rfl::Description.

This really is only meant as a "parser generator".

---

src/config/loader.cpp line 113 at r1 (raw file):

But we also have a framework where you can run our proxy as a thread inside another program.

Then again, this is for parsing, not passing around.

I am going to do an intermediate config structure so this is clearer.

---

src/config/loader.cpp line 147 at r1 (raw file):

Previously, afrind wrote…

Thoughts on combining the config validation into a single pass that adds both warnings and errors? I think it might be easier if we only had one place in the code where we think about e.g. listener config

See comment above https://reviewable.io/reviews/openmoq/o-rly/29#-OmqJZAmDEY__fBd3T71

---

src/config/loader.cpp line 175 at r1 (raw file):

Previously, afrind wrote…

See folly::join

Done.

[afrind]

@afrind reviewed 15 files and all commit messages, made 7 comments, and resolved 7 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on michalhosna).

---

include/o_rly/config/config.h line 20 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

See the new file structure for folly::IPAddress.

Does QUIC over unix sockets make sense? I am not sure, probably not? So that would be raw moqt over unix sockets? What about datagrams?

I had this line of thought and decided that it doesn't need resolving now 😄

we'll have a tcp fallback for moqt soon (see QMUX)

---

include/o_rly/config/config.h line 47 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

What about optional fields that doesn't have a default? Which I expect would be the majority.

And do the macros even help?

 rfl::Description<"Path to TLS private key file", std::optional<std::string>> key_file;

vs

 ORLY_OPTIONAL_CONFIG_NO_DEFAULT("Path to TLS private key file", std::optional<std::string>>, key_file);

I think the explicitness and wordiness here is actually better then having a level of indirection.

What do you think with the current split of concerns between Config and ParsedConfig?

No they key difference is that the std::optional<> piece is added automatically by the macro if the config is optional. So it would be:

ORLY_OPTIONAL_CONFIG("Path to TLS private key file", std::string, key_file, "")`

---

src/main.cpp line 17 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

I am not sure if we plan to have a lot of subcommands.

Currently its like, "serve" = please continue in the main. Other commands = quick exit.

Using a dispatch table in a different file breaks this "please continue". Should the rest of the main be moved into some kind of serve handler and make the main essentially empty? It makes sense, but also it's maybe too drastic given that that's the main way.

Happy to do it if you thinks its better. I wasn't even sure if subcommands are the way to go. Is a serve good default name?

I'd rather move this out to the config library and keep main clean. processConfigCommand(command)

For the "default" command -- pass empty string?

---

CMakeLists.txt line 173 at r2 (raw file):

  # override the feature so CMake doesn't warn about mixed link strategies.
  set_property(TARGET o_rly_config_test PROPERTY
    LINK_LIBRARY_OVERRIDE "WHOLE_ARCHIVE,gflags_nothreads_static"

Ew. Can we file an issue in the appropriate place here? Is o-rly still using getdeps?

---

src/main.cpp line 83 at r2 (raw file):

  const auto& cache = resolved.cache;

  return std::visit(

I don't love this use of variant. But I don't think this is the long term play so I can live with it in the name of progress.

---

src/config/config_resolver.cpp line 95 at r2 (raw file):

}

Config resolveConfig(const ParsedConfig& config) {

This creates boilerplate that is easily forgotten and a pain in the butt. e.g. I added my config struct but not the file, or the file but not the struct, and missed this key step. Ask AI if there's a way to autogenerate both the Config struct itself and this function?

---

tests/config/test_utils.h line 12 at r2 (raw file):

// RAII helper: writes YAML content to a unique temp file, removes it on destruction.
class TempYamlFile {

Pretty sure there's a folly helper for this too...BUT it currently uses boost::filesystem (BOO).

[michalhosna]

@michalhosna made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on afrind).

---

CMakeLists.txt line 173 at r2 (raw file):

Previously, afrind wrote…

Ew. Can we file an issue in the appropriate place here? Is o-rly still using getdeps?

Honestly, I am completely lost in the current state of the build system of the dependencies. I am happy to have a project that builds, and I really don't want to have to go dig given the time crunch.

It builds; it doesn't pollute the compiler output with new garbage. I despise C++ ecosystem standard of having a wall of warnings during the compile that everyone ignores.

I am happy to accept any suggestions on how to solve this better. I wasn't even sure if I should for the new dependencies here use FetchContent, submodules, different way. H

Honestly, I really don't care that much. Get me a script that builds and doesn't make me look at C++'s horrible deps & build ecosystem again 😄 (just to be clear, I feel the same way with python, but they got uv now)

---

include/o_rly/config/config.h line 47 at r1 (raw file):

Previously, afrind wrote…

No they key difference is that the std::optional<> piece is added automatically by the macro if the config is optional. So it would be:

ORLY_OPTIONAL_CONFIG("Path to TLS private key file", std::string, key_file, "")`

My mistake, I always was thinking about.

 rfl::Description<"Path to TLS private key file", std::optional<std::string>> key_file;
 ORLY_OPTIONAL_CONFIG_NO_DEFAULT("Path to TLS private key file", std::string>, key_file);

My problem with your suggestion is that "" default is not the same as not having a default. I generally want to know default vs explicit set down the line.

---

src/main.cpp line 17 at r1 (raw file):

Previously, afrind wrote…

I'd rather move this out to the config library and keep main clean. processConfigCommand(command)

For the "default" command -- pass empty string?

Now I am lost. What do you actually suggest/want?

---

src/config/config_resolver.cpp line 95 at r2 (raw file):
I don't think this is a boilerplate. And I don't think there can be a way to autogenerate.

You need to express your config constraints somewhere. Config constraints can be fairly complicated, and you need to be able to validate and express complicated stuff.

For example, if we have multiple listeners that are named, then services can limit on which listeners they are available. You have to be able to check that the services list valid listeners. This must be a step after the parsing, and its something that cannot be reasonably expressed in something like JSON schema or other config DSL.

I added my config struct but not the file, or the file but not the struct, and missed this key step

The type system should have helped you not miss this step. Your newly added structure will always be missing on one side.

If you added only to the parser and not to the Config, you simply won't have the field in the application. You should be able to notice that.

If you add only to Config, and forget here, it won't compile.

---

I am used to this pattern of two pass config loading. First parse, and do checks that can be easily done at that step (required fields, required format of those fields, allowed keys etc.) and then have a somewhat manually crafted mapping function (this one) that does the second level of validation and also crafts nice types and structures.

Do you have a different pattern you can recommend?

[michalhosna]

I added the docs, dropped defaults, and reorganized the code.

@michalhosna made 3 comments.
Reviewable status: 6 of 22 files reviewed, 4 unresolved discussions (waiting on afrind).

---

include/o_rly/config/config.h line 47 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

My mistake, I always was thinking about.

 rfl::Description<"Path to TLS private key file", std::optional<std::string>> key_file;
 ORLY_OPTIONAL_CONFIG_NO_DEFAULT("Path to TLS private key file", std::string>, key_file);

My problem with your suggestion is that "" default is not the same as not having a default. I generally want to know default vs explicit set down the line.

I created a separate PR to show the diff: #36

---

src/main.cpp line 17 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

Now I am lost. What do you actually suggest/want?

Discussed elsewhere, I moved the config subcommands to the config tree. And kept the rest as a default here.

[afrind]

@afrind partially reviewed 16 files and all commit messages, made 5 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on michalhosna).

---

CMakeLists.txt line 173 at r2 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

Honestly, I am completely lost in the current state of the build system of the dependencies. I am happy to have a project that builds, and I really don't want to have to go dig given the time crunch.

It builds; it doesn't pollute the compiler output with new garbage. I despise C++ ecosystem standard of having a wall of warnings during the compile that everyone ignores.

I am happy to accept any suggestions on how to solve this better. I wasn't even sure if I should for the new dependencies here use FetchContent, submodules, different way. H

Honestly, I really don't care that much. Get me a script that builds and doesn't make me look at C++'s horrible deps & build ecosystem again 😄 (just to be clear, I feel the same way with python, but they got uv now)

Please file an issue somewhere so we don't forget to clean this up.

---

include/o_rly/config/config.h line 47 at r1 (raw file):

know default vs explicit set down the line.

You mean it matters if someone sets port=443 vs not setting port, even though the app sees port=443 in either case? I'm not sure why, but I'm not lying in the road here.

---

src/config/config_resolver.cpp line 95 at r2 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

I don't think this is a boilerplate. And I don't think there can be a way to autogenerate.

You need to express your config constraints somewhere. Config constraints can be fairly complicated, and you need to be able to validate and express complicated stuff.

For example, if we have multiple listeners that are named, then services can limit on which listeners they are available. You have to be able to check that the services list valid listeners. This must be a step after the parsing, and its something that cannot be reasonably expressed in something like JSON schema or other config DSL.

I added my config struct but not the file, or the file but not the struct, and missed this key step

The type system should have helped you not miss this step. Your newly added structure will always be missing on one side.

If you added only to the parser and not to the Config, you simply won't have the field in the application. You should be able to notice that.

If you add only to Config, and forget here, it won't compile.

---

I am used to this pattern of two pass config loading. First parse, and do checks that can be easily done at that step (required fields, required format of those fields, allowed keys etc.) and then have a somewhat manually crafted mapping function (this one) that does the second level of validation and also crafts nice types and structures.

Do you have a different pattern you can recommend?

There are downsides to this approach (have to change 3 things: two structs, and resolve function; plus validation). It's a lot for simple configs, but for others that have meaningful validation perhaps it's not that much more. My goal would be to touch things as few times as possible, but I think the upsides of your approach are fine. Let's roll.

---

src/main.cpp line 67 at r3 (raw file):

  }

  auto result = cfg::handleConfigSubcommand(subcommand, FLAGS_config, FLAGS_strict_config, argv[0]);

Just to be clear, serve as a config subcommand loads the config? Might be good to have a comment.

---

src/config/config_resolver.cpp line 77 at r3 (raw file):

  if (!errors.empty()) {
    std::ostringstream oss;

I think you could folly::join here

[michalhosna]

@michalhosna made 1 comment and resolved 1 discussion.
Reviewable status: 21 of 22 files reviewed, all discussions resolved (waiting on afrind).

---

CMakeLists.txt line 173 at r2 (raw file):

Previously, afrind wrote…

Please file an issue somewhere so we don't forget to clean this up.

#41

[michalhosna]

@michalhosna reviewed 2 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on afrind).