From e6b1945de027b2c042aa30f2abf8707bd8711f5f Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Tue, 15 Nov 2022 13:48:40 -0400 Subject: [PATCH] Rediff patches Add 0003-Make-Default-Required-or-Required-Hidden-compilation.patch: Add 0002-Make-lokid-rpc-setting-required-in-SN-mode.patch: Add 0001-Fix-missing-option-names.patch: --- .../0001-Fix-missing-option-names.patch | 65 +++++ ...okid-rpc-setting-required-in-SN-mode.patch | 226 +++++++++++++++ ...uired-or-Required-Hidden-compilation.patch | 266 ++++++++++++++++++ debian/patches/series | 3 + 4 files changed, 560 insertions(+) create mode 100644 debian/patches/0001-Fix-missing-option-names.patch create mode 100644 debian/patches/0002-Make-lokid-rpc-setting-required-in-SN-mode.patch create mode 100644 debian/patches/0003-Make-Default-Required-or-Required-Hidden-compilation.patch create mode 100644 debian/patches/series diff --git a/debian/patches/0001-Fix-missing-option-names.patch b/debian/patches/0001-Fix-missing-option-names.patch new file mode 100644 index 000000000..89132a5db --- /dev/null +++ b/debian/patches/0001-Fix-missing-option-names.patch @@ -0,0 +1,65 @@ +From: Jason Rhinelander +Date: Tue, 15 Nov 2022 13:11:11 -0400 +Subject: Fix missing option names + +At some point between 0.9.9 and 0.9.10 we removed the printing of option +names when a value doesn't have a default, but this means the config is +littered with things like: + + # This option sets the greater foo value. + +with no actual option name printed out when there is no default. + +This fixes it by always printing the option name in such a case, just +with an empty value, e.g.: + + # This option sets the greater foo value. + #big-foo= +--- + llarp/config/definition.cpp | 23 +++++++++++++++-------- + 1 file changed, 15 insertions(+), 8 deletions(-) + +diff --git a/llarp/config/definition.cpp b/llarp/config/definition.cpp +index aa199eb..7617760 100644 +--- a/llarp/config/definition.cpp ++++ b/llarp/config/definition.cpp +@@ -152,10 +152,8 @@ namespace llarp + const std::string& section, std::vector 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()) diff --git a/debian/patches/0002-Make-lokid-rpc-setting-required-in-SN-mode.patch b/debian/patches/0002-Make-lokid-rpc-setting-required-in-SN-mode.patch new file mode 100644 index 000000000..5941c5167 --- /dev/null +++ b/debian/patches/0002-Make-lokid-rpc-setting-required-in-SN-mode.patch @@ -0,0 +1,226 @@ +From: Jason Rhinelander +Date: Tue, 15 Nov 2022 13:14:15 -0400 +Subject: Make [lokid]:rpc setting required in SN mode + +When running as a service node we can't do anything without a lokid rpc +URL, and we don't necessarily have a good default for it. + +This makes it required so that we fail with an appropriate error message +(rather than connect timeouts) if it is not specified. +--- + llarp/config/config.cpp | 1 + + test/config/test_llarp_config_values.cpp | 49 ++++++++++++++------------------ + 2 files changed, 23 insertions(+), 27 deletions(-) + +diff --git a/llarp/config/config.cpp b/llarp/config/config.cpp +index aa02da5..67d92fa 100644 +--- a/llarp/config/config.cpp ++++ b/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", +diff --git a/test/config/test_llarp_config_values.cpp b/test/config/test_llarp_config_values.cpp +index 8276cc5..f0e9ebe 100644 +--- a/test/config/test_llarp_config_values.cpp ++++ b/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 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 + )"; diff --git a/debian/patches/0003-Make-Default-Required-or-Required-Hidden-compilation.patch b/debian/patches/0003-Make-Default-Required-or-Required-Hidden-compilation.patch new file mode 100644 index 000000000..edd5b0cb1 --- /dev/null +++ b/debian/patches/0003-Make-Default-Required-or-Required-Hidden-compilation.patch @@ -0,0 +1,266 @@ +From: Jason Rhinelander +Date: Tue, 15 Nov 2022 13:15:54 -0400 +Subject: Make Default&Required or Required&Hidden compilation failures + +Default & Required makes no sense: if we have a default it makes no +sense to make it required. The previous behaviour when this was +specified was to force an (uncommented) value in the config with the +value, but this was only used in the test suite. + +Required & Hidden makes no sense either: if it's required to be +specified we definitely don't want to hide it from the generated config +file. + +These are now compile-time failures. +--- + llarp/config/definition.hpp | 10 ++++ + test/config/test_llarp_config_definition.cpp | 11 ++--- + test/config/test_llarp_config_output.cpp | 68 ++++++++++++++++------------ + 3 files changed, 53 insertions(+), 36 deletions(-) + +diff --git a/llarp/config/definition.hpp b/llarp/config/definition.hpp +index 8f9225c..6cacd31 100644 +--- a/llarp/config/definition.hpp ++++ b/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 || config::is_default) || ...); ++ constexpr bool has_required = ++ (std::is_same_v, config::Required_t> || ...); ++ constexpr bool has_hidden = ++ (std::is_same_v, 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(opts)), ...); + (extractAcceptor(std::forward(opts)), ...); + (extractComments(std::forward(opts)), ...); +diff --git a/test/config/test_llarp_config_definition.cpp b/test/config/test_llarp_config_definition.cpp +index e3edec2..80005d5 100644 +--- a/test/config/test_llarp_config_definition.cpp ++++ b/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 def("foo", "bar", Required, Default{1}, [&](int arg) { ++ llarp::OptionDefinition 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>( + "router", + "threads", +- Default{1}, + Required)); + + CHECK_THROWS(config.validateRequiredFields()); +@@ -186,13 +185,13 @@ TEST_CASE("ConfigDefinition section test", "[config]") + config.defineOption(std::make_unique>( + "foo", + "bar", +- Required, +- Default{1})); ++ Required ++ )); + config.defineOption(std::make_unique>( + "goo", + "bar", +- Required, +- Default{1})); ++ Required ++ )); + + CHECK_THROWS(config.validateRequiredFields()); + +diff --git a/test/config/test_llarp_config_output.cpp b/test/config/test_llarp_config_output.cpp +index 13eb64c..5ba47bf 100644 +--- a/test/config/test_llarp_config_output.cpp ++++ b/test/config/test_llarp_config_output.cpp +@@ -8,37 +8,37 @@ TEST_CASE("ConfigDefinition simple generate test", "[config]") + { + llarp::ConfigDefinition config{true}; + +- config.defineOption("foo", "bar", Required, Default{1}); ++ config.defineOption("foo", "bar", Required); + config.defineOption("foo", "baz", Default{2}); +- config.defineOption(std::make_unique>( +- "foo", "quux", Required, Default{"hello"})); ++ config.defineOption( ++ std::make_unique>("foo", "quux", Default{"hello"})); + +- config.defineOption("argle", "bar", RelayOnly, Required, Default{3}); ++ config.defineOption("argle", "bar", RelayOnly, Required); + config.defineOption("argle", "baz", Default{4}); +- config.defineOption("argle", "quux", Required, Default{"the quick brown fox"}); ++ config.defineOption("argle", "quux", Default{"the quick brown fox"}); + +- config.defineOption("not", "for-routers", ClientOnly, Required, Default{1}); ++ config.defineOption("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("foo", "bar", Required, Default{1}); ++ config.defineOption("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>( +- "foo", "bar", Required, Default{1})); ++ config.defineOption(std::make_unique>("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("foo", "bar", Required, Default{1}); ++ config.defineOption("foo", "bar", Default{1}); + +- config.defineOption("foo", "far", Default{"abc"}, ++ config.defineOption( ++ "foo", ++ "far", ++ Default{"abc"}, + Comment{ +- "Fill in the missing values:", +- "___defg", ++ "Fill in the missing values:", ++ "___defg", + }); + + config.defineOption("client", "omg", ClientOnly, Default{1}, Comment{"hi"}); + config.defineOption("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("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("foo", "bar", Required, Default{1}); ++ config.defineOption("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("foo", "bar", Required, Default{1}); ++ config.defineOption("foo", "bar", Default{1}); + + config.addOptionComments("foo", "baz", {"foo baz option comment"}); +- config.defineOption("foo", "baz", Required, Default{1}); ++ config.defineOption("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"); + } +- diff --git a/debian/patches/series b/debian/patches/series new file mode 100644 index 000000000..a7d3414c0 --- /dev/null +++ b/debian/patches/series @@ -0,0 +1,3 @@ +0001-Fix-missing-option-names.patch +0002-Make-lokid-rpc-setting-required-in-SN-mode.patch +0003-Make-Default-Required-or-Required-Hidden-compilation.patch