Merge pull request #2055 from jagerman/fix-option-names

Fix missing option names, make [lokid]:rpc required

llarp/config/config.cpp

@@ -1179,6 +1179,7 @@ namespace llarp
         "lokid",
         "rpc",
         RelayOnly,
+        Required,
         Comment{
             "oxenmq control address for for communicating with oxend. Depends on oxend's",
             "lmq-local-control configuration option. By default this value should be",

llarp/config/definition.cpp

@@ -152,10 +152,8 @@ namespace llarp
       const std::string& section, std::vector<std::string> comments)
   {
     auto& sectionComments = m_sectionComments[section];
-    for (size_t i = 0; i < comments.size(); ++i)
-    {
-      sectionComments.emplace_back(std::move(comments[i]));
-    }
+    for (auto& c : comments)
+      sectionComments.emplace_back(std::move(c));
   }
 
   void
@@ -198,13 +196,22 @@ namespace llarp
         if (useValues and def->getNumberFound() > 0)
         {
           for (const auto& val : def->valuesAsString())
-            fmt::format_to(sect_append, "\n{}={}\n", name, val);
+            fmt::format_to(sect_append, "\n{}={}", name, val);
+          *sect_append = '\n';
         }
-        else if (not(def->hidden and not has_comment))
+        else if (not def->hidden)
         {
-          for (const auto& val : def->defaultValuesAsString())
-            fmt::format_to(sect_append, "\n{}{}={}\n", def->required ? "" : "#", name, val);
+          if (auto defaults = def->defaultValuesAsString(); not defaults.empty())
+            for (const auto& val : defaults)
+              fmt::format_to(sect_append, "\n{}{}={}", def->required ? "" : "#", name, val);
+          else
+            // We have no defaults so we append it as "#opt-name=" so that we show the option name,
+            // and make it simple to uncomment and edit to the desired value.
+            fmt::format_to(sect_append, "\n#{}=", name);
+          *sect_append = '\n';
         }
+        else if (has_comment)
+          *sect_append = '\n';
       });
 
       if (sect_str.empty())

llarp/config/definition.hpp

@@ -202,6 +202,16 @@ namespace llarp
     OptionDefinition(std::string section_, std::string name_, Options&&... opts)
         : OptionDefinitionBase(section_, name_, opts...)
     {
+      constexpr bool has_default =
+          ((config::is_default_array<Options> || config::is_default<Options>) || ...);
+      constexpr bool has_required =
+          (std::is_same_v<config::remove_cvref_t<Options>, config::Required_t> || ...);
+      constexpr bool has_hidden =
+          (std::is_same_v<config::remove_cvref_t<Options>, config::Hidden_t> || ...);
+      static_assert(
+          not(has_default and has_required), "Default{...} and Required are mutually exclusive");
+      static_assert(not(has_hidden and has_required), "Hidden and Required are mutually exclusive");
+
       (extractDefault(std::forward<Options>(opts)), ...);
       (extractAcceptor(std::forward<Options>(opts)), ...);
       (extractComments(std::forward<Options>(opts)), ...);

test/config/test_llarp_config_definition.cpp

@@ -104,7 +104,7 @@ TEST_CASE("OptionDefinition acceptor throws test", "[config]")
 TEST_CASE("OptionDefinition tryAccept missing option test", "[config]")
 {
   int unset = -1;
-  llarp::OptionDefinition<int> def("foo", "bar", Required, Default{1}, [&](int arg) {
+  llarp::OptionDefinition<int> def("foo", "bar", Required, [&](int arg) {
     (void)arg;
     unset = 0; // should never be called
   });
@@ -170,7 +170,6 @@ TEST_CASE("ConfigDefinition required test", "[config]")
   config.defineOption(std::make_unique<llarp::OptionDefinition<int>>(
             "router",
             "threads",
-            Default{1},
             Required));
 
   CHECK_THROWS(config.validateRequiredFields());
@@ -186,13 +185,13 @@ TEST_CASE("ConfigDefinition section test", "[config]")
   config.defineOption(std::make_unique<llarp::OptionDefinition<int>>(
             "foo",
             "bar",
-            Required,
-            Default{1}));
+            Required
+            ));
   config.defineOption(std::make_unique<llarp::OptionDefinition<int>>(
             "goo",
             "bar",
-            Required,
-            Default{1}));
+            Required
+            ));
 
   CHECK_THROWS(config.validateRequiredFields());
 

test/config/test_llarp_config_output.cpp

@@ -8,37 +8,37 @@ TEST_CASE("ConfigDefinition simple generate test", "[config]")
 {
   llarp::ConfigDefinition config{true};
 
-  config.defineOption<int>("foo", "bar", Required, Default{1});
+  config.defineOption<int>("foo", "bar", Required);
   config.defineOption<int>("foo", "baz", Default{2});
-  config.defineOption(std::make_unique<llarp::OptionDefinition<std::string>>(
-            "foo", "quux", Required, Default{"hello"}));
+  config.defineOption(
+      std::make_unique<llarp::OptionDefinition<std::string>>("foo", "quux", Default{"hello"}));
 
-  config.defineOption<int>("argle", "bar", RelayOnly, Required, Default{3});
+  config.defineOption<int>("argle", "bar", RelayOnly, Required);
   config.defineOption<int>("argle", "baz", Default{4});
-  config.defineOption<std::string>("argle", "quux", Required, Default{"the quick brown fox"});
+  config.defineOption<std::string>("argle", "quux", Default{"the quick brown fox"});
 
-  config.defineOption<int>("not", "for-routers", ClientOnly, Required, Default{1});
+  config.defineOption<int>("not", "for-routers", ClientOnly, Default{1});
 
   std::string output = config.generateINIConfig();
 
   CHECK(output == R"raw([foo]
 
 
-bar=1
+#bar=
 
 #baz=2
 
-quux=hello
+#quux=hello
 
 
 [argle]
 
 
-bar=3
+#bar=
 
 #baz=4
 
-quux=the quick brown fox
+#quux=the quick brown fox
 )raw");
 }
 
@@ -46,16 +46,24 @@ TEST_CASE("ConfigDefinition useValue test", "[config]")
 {
   llarp::ConfigDefinition config{true};
 
-  config.defineOption<int>("foo", "bar", Required, Default{1});
+  config.defineOption<int>("foo", "bar", Default{1});
 
-  constexpr auto expected = "[foo]\n\n\nbar=1\n";
+  constexpr auto expected = R"raw([foo]
+
+
+#bar=1
+)raw";
 
   CHECK(config.generateINIConfig(false) == expected);
   CHECK(config.generateINIConfig(true) == expected);
 
   config.addConfigValue("foo", "bar", "2");
 
-  constexpr auto expectedWhenValueProvided = "[foo]\n\n\nbar=2\n";
+  constexpr auto expectedWhenValueProvided = R"raw([foo]
+
+
+bar=2
+)raw";
 
   CHECK(config.generateINIConfig(false) == expected);
   CHECK(config.generateINIConfig(true) == expectedWhenValueProvided);
@@ -67,8 +75,7 @@ TEST_CASE("ConfigDefinition section comments test")
 
   config.addSectionComments("foo", {"test comment"});
   config.addSectionComments("foo", {"test comment 2"});
-  config.defineOption(std::make_unique<llarp::OptionDefinition<int>>(
-            "foo", "bar", Required, Default{1}));
+  config.defineOption(std::make_unique<llarp::OptionDefinition<int>>("foo", "bar", Default{1}));
 
   std::string output = config.generateINIConfig();
 
@@ -77,7 +84,7 @@ TEST_CASE("ConfigDefinition section comments test")
 # test comment 2
 
 
-bar=1
+#bar=1
 )raw");
 }
 
@@ -87,18 +94,21 @@ TEST_CASE("ConfigDefinition option comments test")
 
   config.addOptionComments("foo", "bar", {"test comment 1"});
   config.addOptionComments("foo", "bar", {"test comment 2"});
-  config.defineOption<int>("foo", "bar", Required, Default{1});
+  config.defineOption<int>("foo", "bar", Default{1});
 
-  config.defineOption<std::string>("foo", "far", Default{"abc"},
+  config.defineOption<std::string>(
+      "foo",
+      "far",
+      Default{"abc"},
       Comment{
-        "Fill in the missing values:",
-        "___defg",
+          "Fill in the missing values:",
+          "___defg",
       });
 
   config.defineOption<int>("client", "omg", ClientOnly, Default{1}, Comment{"hi"});
   config.defineOption<int>("relay", "ftw", RelayOnly, Default{1}, Comment{"bye"});
 
-  // has comment, so still gets shown.
+  // has comment, but is hidden: we show only the comment but not the value/default.
   config.defineOption<int>("foo", "old-bar", Hidden, Default{456});
   config.addOptionComments("foo", "old-bar", {"old bar option"});
 
@@ -112,14 +122,13 @@ TEST_CASE("ConfigDefinition option comments test")
 
 # test comment 1
 # test comment 2
-bar=1
+#bar=1
 
 # Fill in the missing values:
 # ___defg
 #far=abc
 
 # old bar option
-#old-bar=456
 
 
 [relay]
@@ -139,7 +148,7 @@ TEST_CASE("ConfigDefinition empty comments test")
 
   config.addOptionComments("foo", "bar", {"option comment"});
   config.addOptionComments("foo", "bar", {""});
-  config.defineOption<int>("foo", "bar", Required, Default{1});
+  config.defineOption<int>("foo", "bar", Default{1});
 
   std::string output = config.generateINIConfig();
 
@@ -150,7 +159,7 @@ TEST_CASE("ConfigDefinition empty comments test")
 
 # option comment
 # 
-bar=1
+#bar=1
 )raw");
 }
 
@@ -161,10 +170,10 @@ TEST_CASE("ConfigDefinition multi option comments")
   config.addSectionComments("foo", {"foo section comment"});
 
   config.addOptionComments("foo", "bar", {"foo bar option comment"});
-  config.defineOption<int>("foo", "bar", Required, Default{1});
+  config.defineOption<int>("foo", "bar", Default{1});
 
   config.addOptionComments("foo", "baz", {"foo baz option comment"});
-  config.defineOption<int>("foo", "baz", Required, Default{1});
+  config.defineOption<int>("foo", "baz", Default{1});
 
   std::string output = config.generateINIConfig();
 
@@ -173,10 +182,9 @@ TEST_CASE("ConfigDefinition multi option comments")
 
 
 # foo bar option comment
-bar=1
+#bar=1
 
 # foo baz option comment
-baz=1
+#baz=1
 )raw");
 }
-

test/config/test_llarp_config_values.cpp

@@ -75,6 +75,8 @@ run_config_test(mocks::Network env, std::string_view ini_str)
     throw std::runtime_error{"non zero return"};
 }
 
+const std::string ini_minimal = "[lokid]\nrpc=ipc://dummy\n";
+
 TEST_CASE("service node bind section on valid network", "[config]")
 {
   std::unordered_multimap<std::string, llarp::IPRange> env{
@@ -92,15 +94,14 @@ TEST_CASE("service node bind section on valid network", "[config]")
     REQUIRE(not mock_net.IsBogon(*maybe_addr));
   }
 
-  SECTION("empty config")
+  SECTION("minimal config")
   {
-    std::string_view ini_str = "";
-    REQUIRE_NOTHROW(run_config_test(env, ini_str));
+    REQUIRE_NOTHROW(run_config_test(env, ini_minimal));
   }
 
   SECTION("explicit bind via ifname")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [bind]
 mock0=443
 )";
@@ -108,7 +109,7 @@ mock0=443
   }
   SECTION("explicit bind via ip address")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [bind]
 inbound=1.1.1.1:443
 )";
@@ -116,7 +117,7 @@ inbound=1.1.1.1:443
   }
   SECTION("explicit bind via ip address with old syntax")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [bind]
 1.1.1.1=443
 )";
@@ -125,7 +126,7 @@ inbound=1.1.1.1:443
   }
   SECTION("ip spoof fails")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [router]
 public-ip=8.8.8.8
 public-port=443
@@ -136,7 +137,7 @@ inbound=1.1.1.1:443
   }
   SECTION("explicit bind via ifname but fails from non existing ifname")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [bind]
 ligma0=443
 )";
@@ -145,7 +146,7 @@ ligma0=443
 
   SECTION("explicit bind via ifname but fails from using loopback")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [bind]
 lo=443
 )";
@@ -154,7 +155,7 @@ lo=443
 
   SECTION("explicit bind via explicit loopback")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [bind]
 inbound=127.0.0.1:443
 )";
@@ -162,7 +163,7 @@ inbound=127.0.0.1:443
   }
   SECTION("public ip provided but no bind section")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [router]
 public-ip=1.1.1.1
 public-port=443
@@ -171,7 +172,7 @@ public-port=443
   }
   SECTION("public ip provided with ip in bind section")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [router]
 public-ip=1.1.1.1
 public-port=443
@@ -196,7 +197,7 @@ TEST_CASE("service node bind section on nat network", "[config]")
 
   SECTION("public ip provided via inbound directive")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [router]
 public-ip=1.1.1.1
 public-port=443
@@ -209,7 +210,7 @@ inbound=10.1.1.1:443
 
   SECTION("public ip provided with bind via ifname")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [router]
 public-ip=1.1.1.1
 public-port=443
@@ -222,7 +223,7 @@ mock0=443
 
   SECTION("public ip provided bind via wildcard ip")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [router]
 public-ip=1.1.1.1
 public-port=443
@@ -232,7 +233,6 @@ inbound=0.0.0.0:443
 )";
     REQUIRE_THROWS(run_config_test(env, ini_str));
   }
-
 }
 
 TEST_CASE("service node bind section with multiple public ip", "[config]")
@@ -242,14 +242,9 @@ TEST_CASE("service node bind section with multiple public ip", "[config]")
       {"mock0", llarp::IPRange::FromIPv4(2, 1, 1, 1, 32)},
       {"lo", llarp::IPRange::FromIPv4(127, 0, 0, 1, 8)},
   };
-  SECTION("empty config")
-  {
-    std::string_view ini_str = "";
-    REQUIRE_NOTHROW(run_config_test(env, ini_str));
-  }
   SECTION("with old style wildcard for inbound and no public ip, fails")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [bind]
 0.0.0.0=443
 )";
@@ -257,7 +252,7 @@ TEST_CASE("service node bind section with multiple public ip", "[config]")
   }
   SECTION("with old style wildcard for outbound")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [bind]
 *=1443
 )";
@@ -265,7 +260,7 @@ TEST_CASE("service node bind section with multiple public ip", "[config]")
   }
   SECTION("with wildcard via inbound directive no public ip given, fails")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [bind]
 inbound=0.0.0.0:443
 )";
@@ -274,7 +269,7 @@ inbound=0.0.0.0:443
   }
   SECTION("with wildcard via inbound directive primary public ip given")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [router]
 public-ip=1.1.1.1
 public-port=443
@@ -286,7 +281,7 @@ inbound=0.0.0.0:443
   }
   SECTION("with wildcard via inbound directive secondary public ip given")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [router]
 public-ip=2.1.1.1
 public-port=443
@@ -298,7 +293,7 @@ inbound=0.0.0.0:443
   }
   SECTION("with bind via interface name")
   {
-    std::string_view ini_str = R"(
+    auto ini_str = ini_minimal + R"(
 [bind]
 mock0=443
 )";