From 0b40729aec016bf835c111edc68787fcec75a418 Mon Sep 17 00:00:00 2001 From: Angela Chen Date: Thu, 10 Aug 2023 00:19:10 -0400 Subject: [PATCH 1/9] Add --config-json-path to accept flags specified in a JSON file. --- include/ppx/application.h | 1 + include/ppx/command_line_parser.h | 28 +++++++-- src/ppx/application.cpp | 9 ++- src/ppx/command_line_parser.cpp | 83 ++++++++++++++++++++++---- src/test/command_line_parser_test.cpp | 86 +++++++++++++++++++++++++++ 5 files changed, 188 insertions(+), 19 deletions(-) diff --git a/include/ppx/application.h b/include/ppx/application.h index 164a6d42e..93fa5c13f 100644 --- a/include/ppx/application.h +++ b/include/ppx/application.h @@ -252,6 +252,7 @@ struct StandardOptions #endif std::shared_ptr>> pAssetsPaths; + std::shared_ptr>> pConfigJsonPath; }; // ------------------------------------------------------------------------------------------------- diff --git a/include/ppx/command_line_parser.h b/include/ppx/command_line_parser.h index 653a6b890..e8e571a6e 100644 --- a/include/ppx/command_line_parser.h +++ b/include/ppx/command_line_parser.h @@ -15,6 +15,8 @@ #ifndef ppx_command_line_parser_h #define ppx_command_line_parser_h +#include "nlohmann/json.hpp" + #include #include #include @@ -101,8 +103,10 @@ class CliOptions private: // Adds new option if the option does not already exist // Otherwise, the new value is appended to the end of the vector of stored parameters for this option - void - AddOption(std::string_view optionName, std::string_view value); + void AddOption(std::string_view optionName, std::string_view valueStr); + + // Same as above, but appends an array of values at the same key + void AddOption(std::string_view optionName, const std::vector& valueArray); template T GetParsedOrDefault(std::string_view valueStr, const T& defaultValue) const @@ -132,8 +136,18 @@ class CliOptions } private: + struct string_hash + { + using hash_type = std::hash; + using is_transparent = void; + + size_t operator()(const char* str) const { return hash_type{}(str); } + size_t operator()(std::string_view str) const { return hash_type{}(str); } + size_t operator()(std::string const& str) const { return hash_type{}(str); } + }; + // All flag names (string) and parameters (vector of strings) specified on the command line - std::unordered_map> mAllOptions; + std::unordered_map, string_hash, std::equal_to<>> mAllOptions; friend class CommandLineParser; }; @@ -156,13 +170,19 @@ class CommandLineParser // and write the error to `out_error`. std::optional Parse(int argc, const char* argv[]); + // Adds all options specified within jsonConfig to mOpts. + std::optional AddJsonOptions(const nlohmann::json& jsonConfig); + const CliOptions& GetOptions() const { return mOpts; } std::string GetUsageMsg() const { return mUsageMsg; } void AppendUsageMsg(const std::string& additionalMsg) { mUsageMsg += additionalMsg; } private: - CliOptions mOpts; + // Adds an option to mOpts and handles the special --no-flag-name case. + // Expects option names without the "--" prefix. + std::optional AddOption(std::string_view optionName, std::string_view valueStr); + CliOptions mOpts; std::string mUsageMsg = R"( USAGE ============================== diff --git a/src/ppx/application.cpp b/src/ppx/application.cpp index 94a672847..1ba9daed1 100644 --- a/src/ppx/application.cpp +++ b/src/ppx/application.cpp @@ -806,13 +806,18 @@ void Application::SaveMetricsReportToDisk() void Application::InitStandardKnobs() { // Flag names in alphabetical order - std::vector defaultAssetsPaths = {}; + std::vector defaultEmptyList = {}; mStandardOpts.pAssetsPaths = - mKnobManager.CreateKnob>>("assets-path", defaultAssetsPaths); + mKnobManager.CreateKnob>>("assets-path", defaultEmptyList); mStandardOpts.pAssetsPaths->SetFlagDescription( "Add a path in front of the assets search path list."); mStandardOpts.pAssetsPaths->SetFlagParameters(""); + mStandardOpts.pConfigJsonPath = mKnobManager.CreateKnob>>("config-json-path", defaultEmptyList); + mStandardOpts.pAssetsPaths->SetFlagDescription( + "Additional commandline flags specified in a JSON file."); + mStandardOpts.pAssetsPaths->SetFlagParameters(""); + mStandardOpts.pDeterministic = mKnobManager.CreateKnob>("deterministic", false); mStandardOpts.pDeterministic->SetFlagDescription( diff --git a/src/ppx/command_line_parser.cpp b/src/ppx/command_line_parser.cpp index 19d31dbc0..dc8adb202 100644 --- a/src/ppx/command_line_parser.cpp +++ b/src/ppx/command_line_parser.cpp @@ -13,6 +13,7 @@ // limitations under the License. #include +#include #include #include #include @@ -58,11 +59,22 @@ void CliOptions::AddOption(std::string_view optionName, std::string_view valueSt { auto it = mAllOptions.find(optionName); if (it == mAllOptions.cend()) { - std::vector v{valueStr}; - mAllOptions.emplace(optionName, v); + std::vector v{std::string(valueStr)}; + mAllOptions.emplace(std::string(optionName), v); return; } - it->second.push_back(valueStr); + it->second.push_back(std::string(valueStr)); +} + +void CliOptions::AddOption(std::string_view optionName, const std::vector& valueArray) +{ + auto it = mAllOptions.find(optionName); + if (it == mAllOptions.cend()) { + mAllOptions.emplace(std::string(optionName), valueArray); + return; + } + auto storedValueArray = it->second; + storedValueArray.insert(storedValueArray.end(), valueArray.cbegin(), valueArray.cend()); } bool CliOptions::Parse(std::string_view valueStr, bool defaultValue) const @@ -118,26 +130,71 @@ std::optional CommandLineParser::Parse(int argc } name = name.substr(2); + std::string_view value = ""; if (i + 1 < args.size()) { - std::string_view nextElem = ppx::string_util::TrimBothEnds(args[i + 1]); + auto nextElem = ppx::string_util::TrimBothEnds(args[i + 1]); if (!StartsWithDoubleDash(nextElem)) { - // We found an option with a parameter. - mOpts.AddOption(name, nextElem); + // The next element is a parameter for the current option + value = nextElem; ++i; - continue; } } - // There is no parameter so it's likely a flag - if (name.substr(0, 3) == "no-") { - mOpts.AddOption(name.substr(3), "0"); + + if (auto error = AddOption(name, value)) { + return error; } - else { - // Do not assign "1" in case it's an option lacking a parameter - mOpts.AddOption(name, ""); + } + + std::vector configJsonPaths; + configJsonPaths = mOpts.GetOptionValueOrDefault("config-json-path", configJsonPaths); + for (const auto& jsonPath : configJsonPaths) { + std::ifstream f(jsonPath); + nlohmann::json data = nlohmann::json::parse(f); + if (auto error = AddJsonOptions(data)) { + return error; } } return std::nullopt; } +std::optional CommandLineParser::AddJsonOptions(const nlohmann::json& jsonConfig) +{ + std::stringstream ss; + for (auto it = jsonConfig.cbegin(); it != jsonConfig.cend(); ++it) { + ss << it.value(); + std::string value = ss.str(); + ss.str(""); + + if (value.length() > 0 && value.substr(0, 1) == "[") { + // Special case, arrays specified in JSON are added directly to mOpts to avoid inserting element by element + std::vector jsonStringArray; + for (const auto& elem : it.value()) { + ss << elem; + jsonStringArray.push_back(ss.str()); + ss.str(""); + } + mOpts.AddOption(it.key(), jsonStringArray); + } + else if (auto error = AddOption(it.key(), ppx::string_util::TrimBothEnds(value, " \t\""))) { + return error; + } + } + return std::nullopt; +} + +std::optional CommandLineParser::AddOption(std::string_view optionName, std::string_view valueStr) +{ + if (optionName.length() > 2 && optionName.substr(0, 3) == "no-") { + // Special case where "no-flag-name" syntax is used + if (valueStr.length() > 0) { + return "invalid prefix no- for option \"" + std::string(optionName) + "\" and value \"" + std::string(valueStr) + "\""; + } + optionName = optionName.substr(3); + valueStr = "0"; + } + mOpts.AddOption(optionName, valueStr); + return std::nullopt; +} + } // namespace ppx diff --git a/src/test/command_line_parser_test.cpp b/src/test/command_line_parser_test.cpp index 93d79b2f2..ad994554e 100644 --- a/src/test/command_line_parser_test.cpp +++ b/src/test/command_line_parser_test.cpp @@ -230,5 +230,91 @@ TEST(CommandLineParserTest, ExtraOptionsSuccessfullyParsed) EXPECT_TRUE(opts.HasExtraOption("extra-option-no-param")); } +TEST(CommandLineParserTest, JsonEmptyParsedSuccessfully) +{ + CommandLineParser parser; + nlohmann::json jsonConfig; + if (auto error = parser.AddJsonOptions(jsonConfig)) { + FAIL() << error->errorMsg; + } + auto opts = parser.GetOptions(); + EXPECT_EQ(opts.GetNumUniqueOptions(), 0); +} + +TEST(CommandLineParserTest, JsonSimpleParsedSuccessfully) +{ + CommandLineParser parser; + std::string jsonText = R"( + { + "a": true, + "b": false, + "c": 1.234, + "d": 5, + "e": "hello world" + } +)"; + nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); + if (auto error = parser.AddJsonOptions(jsonConfig)) { + FAIL() << error->errorMsg; + } + auto opts = parser.GetOptions(); + EXPECT_EQ(opts.GetNumUniqueOptions(), 5); + EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); + EXPECT_EQ(opts.GetOptionValueOrDefault("b", true), false); + EXPECT_FLOAT_EQ(opts.GetOptionValueOrDefault("c", 6.0f), 1.234f); + EXPECT_EQ(opts.GetOptionValueOrDefault("d", 0), 5); + EXPECT_EQ(opts.GetOptionValueOrDefault("e", "foo"), "hello world"); +} + +TEST(CommandLineParserTest, JsonWithIntArray) +{ + CommandLineParser parser; + std::string jsonText = R"( + { + "a": true, + "b": [1, 2, 3] + } +)"; + nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); + if (auto error = parser.AddJsonOptions(jsonConfig)) { + FAIL() << error->errorMsg; + } + auto opts = parser.GetOptions(); + EXPECT_EQ(opts.GetNumUniqueOptions(), 2); + EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); + EXPECT_TRUE(opts.HasExtraOption("b")); + std::vector defaultB = {100}; + std::vector gotB = opts.GetOptionValueOrDefault("b", defaultB); + EXPECT_EQ(gotB.size(), 3); + EXPECT_EQ(gotB.at(0), 1); + EXPECT_EQ(gotB.at(1), 2); + EXPECT_EQ(gotB.at(2), 3); +} + +TEST(CommandLineParserTest, JsonWithNestedStructure) +{ + CommandLineParser parser; + std::string jsonText = R"( + { + "a": true, + "b": { + "c" : 1, + "d" : 2 + } + } +)"; + nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); + if (auto error = parser.AddJsonOptions(jsonConfig)) { + FAIL() << error->errorMsg; + } + auto opts = parser.GetOptions(); + EXPECT_EQ(opts.GetNumUniqueOptions(), 2); + EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); + EXPECT_TRUE(opts.HasExtraOption("b")); + EXPECT_EQ(opts.GetOptionValueOrDefault("b", "default"), "{\"c\":1,\"d\":2}"); + EXPECT_FALSE(opts.HasExtraOption("c")); + EXPECT_FALSE(opts.HasExtraOption("d")); +} + } // namespace } // namespace ppx From c69b842f13f7bccdb7192bd127f57c828366376e Mon Sep 17 00:00:00 2001 From: Angela Chen Date: Thu, 10 Aug 2023 11:24:21 -0400 Subject: [PATCH 2/9] Remove custom hash for unordered map since not supported by CI android/linux builds * Make mAllOptions strings and not string_views * Make the functions that need to search mAllOptions take input parameters of const std::string& instead --- include/ppx/command_line_parser.h | 24 +++++++----------------- src/ppx/command_line_parser.cpp | 19 +++++++++++-------- 2 files changed, 18 insertions(+), 25 deletions(-) diff --git a/include/ppx/command_line_parser.h b/include/ppx/command_line_parser.h index e8e571a6e..c3097a164 100644 --- a/include/ppx/command_line_parser.h +++ b/include/ppx/command_line_parser.h @@ -46,7 +46,7 @@ class CliOptions public: CliOptions() = default; - bool HasExtraOption(std::string_view option) const { return mAllOptions.contains(option); } + bool HasExtraOption(const std::string& option) const { return mAllOptions.contains(option); } // Returns the number of unique options and flags that were specified on the commandline, // not counting multiple appearances of the same flag such as: --assets-path a --assets-path b @@ -57,7 +57,7 @@ class CliOptions // Warning: If this is called instead of the vector overload for multiple-value flags, // only the last value will be returned. template - T GetOptionValueOrDefault(std::string_view optionName, const T& defaultValue) const + T GetOptionValueOrDefault(const std::string& optionName, const T& defaultValue) const { auto it = mAllOptions.find(optionName); if (it == mAllOptions.cend()) { @@ -70,7 +70,7 @@ class CliOptions // Same as above, but intended for list flags that are specified on the command line // with multiple instances of the same flag template - std::vector GetOptionValueOrDefault(std::string_view optionName, const std::vector& defaultValues) const + std::vector GetOptionValueOrDefault(const std::string& optionName, const std::vector& defaultValues) const { auto it = mAllOptions.find(optionName); if (it == mAllOptions.cend()) { @@ -86,14 +86,14 @@ class CliOptions // Same as above, but intended for resolution flags that are specified on command line // with x - std::pair GetOptionValueOrDefault(std::string_view optionName, const std::pair& defaultValue) const; + std::pair GetOptionValueOrDefault(const std::string& optionName, const std::pair& defaultValue) const; // (WILL BE DEPRECATED, USE KNOBS INSTEAD) // Get the parameter value after converting it into the desired integral, // floating-point, or boolean type. If the value fails to be converted, // return the specified default value. template - T GetExtraOptionValueOrDefault(std::string_view optionName, const T& defaultValue) const + T GetExtraOptionValueOrDefault(const std::string& optionName, const T& defaultValue) const { static_assert(std::is_integral_v || std::is_floating_point_v || std::is_same_v, "GetExtraOptionValueOrDefault must be called with an integral, floating-point, boolean, or std::string type"); @@ -103,7 +103,7 @@ class CliOptions private: // Adds new option if the option does not already exist // Otherwise, the new value is appended to the end of the vector of stored parameters for this option - void AddOption(std::string_view optionName, std::string_view valueStr); + void AddOption(std::string_view optionName, std::string_view value); // Same as above, but appends an array of values at the same key void AddOption(std::string_view optionName, const std::vector& valueArray); @@ -136,18 +136,8 @@ class CliOptions } private: - struct string_hash - { - using hash_type = std::hash; - using is_transparent = void; - - size_t operator()(const char* str) const { return hash_type{}(str); } - size_t operator()(std::string_view str) const { return hash_type{}(str); } - size_t operator()(std::string const& str) const { return hash_type{}(str); } - }; - // All flag names (string) and parameters (vector of strings) specified on the command line - std::unordered_map, string_hash, std::equal_to<>> mAllOptions; + std::unordered_map> mAllOptions; friend class CommandLineParser; }; diff --git a/src/ppx/command_line_parser.cpp b/src/ppx/command_line_parser.cpp index dc8adb202..8e68ed039 100644 --- a/src/ppx/command_line_parser.cpp +++ b/src/ppx/command_line_parser.cpp @@ -38,7 +38,7 @@ bool StartsWithDoubleDash(std::string_view s) namespace ppx { -std::pair CliOptions::GetOptionValueOrDefault(std::string_view optionName, const std::pair& defaultValue) const +std::pair CliOptions::GetOptionValueOrDefault(const std::string& optionName, const std::pair& defaultValue) const { auto it = mAllOptions.find(optionName); if (it == mAllOptions.cend()) { @@ -55,22 +55,25 @@ std::pair CliOptions::GetOptionValueOrDefault(std::string_view optionN return std::make_pair(N, M); } -void CliOptions::AddOption(std::string_view optionName, std::string_view valueStr) +void CliOptions::AddOption(std::string_view optionName, std::string_view value) { - auto it = mAllOptions.find(optionName); + std::string optionNameStr{optionName}; + std::string valueStr{value}; + auto it = mAllOptions.find(optionNameStr); if (it == mAllOptions.cend()) { - std::vector v{std::string(valueStr)}; - mAllOptions.emplace(std::string(optionName), v); + std::vector v{valueStr}; + mAllOptions.emplace(optionName, v); return; } - it->second.push_back(std::string(valueStr)); + it->second.push_back(valueStr); } void CliOptions::AddOption(std::string_view optionName, const std::vector& valueArray) { - auto it = mAllOptions.find(optionName); + std::string optionNameStr{optionName}; + auto it = mAllOptions.find(optionNameStr); if (it == mAllOptions.cend()) { - mAllOptions.emplace(std::string(optionName), valueArray); + mAllOptions.emplace(optionNameStr, valueArray); return; } auto storedValueArray = it->second; From 7af7d2dcd2285e31d4f866dc9e89b2fe0a3589e0 Mon Sep 17 00:00:00 2001 From: Angela Chen Date: Fri, 11 Aug 2023 01:54:43 -0400 Subject: [PATCH 3/9] Add error for invalid config file name --- src/ppx/command_line_parser.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ppx/command_line_parser.cpp b/src/ppx/command_line_parser.cpp index 8e68ed039..9e768951f 100644 --- a/src/ppx/command_line_parser.cpp +++ b/src/ppx/command_line_parser.cpp @@ -151,7 +151,10 @@ std::optional CommandLineParser::Parse(int argc std::vector configJsonPaths; configJsonPaths = mOpts.GetOptionValueOrDefault("config-json-path", configJsonPaths); for (const auto& jsonPath : configJsonPaths) { - std::ifstream f(jsonPath); + std::ifstream f(jsonPath); + if (f.fail()) { + return "Invalid --config-json-path: " + jsonPath; + } nlohmann::json data = nlohmann::json::parse(f); if (auto error = AddJsonOptions(data)) { return error; From bcd797a2749ee342ec2ce3d2af9ff67ff1b68914 Mon Sep 17 00:00:00 2001 From: Angela Chen Date: Mon, 14 Aug 2023 12:03:45 -0400 Subject: [PATCH 4/9] Adding a couple of changes for config JSON file parsing * JSON file flags are lower priority than flags specified on the command line * Using JSON is_array and is_object functions * Using round bracket syntax for initalization * Fix Parse() function * Better error reporting for parsing the JSON file * Fixed flag description typo --- include/ppx/command_line_parser.h | 11 ++- src/ppx/application.cpp | 11 ++- src/ppx/command_line_parser.cpp | 79 +++++++++++++----- src/test/command_line_parser_test.cpp | 110 +++++++++++++++++++++++--- 4 files changed, 173 insertions(+), 38 deletions(-) diff --git a/include/ppx/command_line_parser.h b/include/ppx/command_line_parser.h index c3097a164..1c6a84b5d 100644 --- a/include/ppx/command_line_parser.h +++ b/include/ppx/command_line_parser.h @@ -124,9 +124,9 @@ class CliOptions T Parse(std::string_view valueStr, const T defaultValue) const { if constexpr (std::is_same_v) { - return static_cast(valueStr); + return std::string(valueStr); } - std::stringstream ss{static_cast(valueStr)}; + std::stringstream ss((std::string(valueStr))); T valueAsNum; ss >> valueAsNum; if (ss.fail()) { @@ -163,9 +163,11 @@ class CommandLineParser // Adds all options specified within jsonConfig to mOpts. std::optional AddJsonOptions(const nlohmann::json& jsonConfig); + std::string GetJsonConfigFlagName() const { return mJsonConfigFlagName; } const CliOptions& GetOptions() const { return mOpts; } std::string GetUsageMsg() const { return mUsageMsg; } - void AppendUsageMsg(const std::string& additionalMsg) { mUsageMsg += additionalMsg; } + + void AppendUsageMsg(const std::string& additionalMsg) { mUsageMsg += additionalMsg; } private: // Adds an option to mOpts and handles the special --no-flag-name case. @@ -173,7 +175,8 @@ class CommandLineParser std::optional AddOption(std::string_view optionName, std::string_view valueStr); CliOptions mOpts; - std::string mUsageMsg = R"( + std::string mJsonConfigFlagName = "config-json-path"; + std::string mUsageMsg = R"( USAGE ============================== Boolean options can be turned on with: diff --git a/src/ppx/application.cpp b/src/ppx/application.cpp index 1ba9daed1..c8cccb7d1 100644 --- a/src/ppx/application.cpp +++ b/src/ppx/application.cpp @@ -813,10 +813,13 @@ void Application::InitStandardKnobs() "Add a path in front of the assets search path list."); mStandardOpts.pAssetsPaths->SetFlagParameters(""); - mStandardOpts.pConfigJsonPath = mKnobManager.CreateKnob>>("config-json-path", defaultEmptyList); - mStandardOpts.pAssetsPaths->SetFlagDescription( - "Additional commandline flags specified in a JSON file."); - mStandardOpts.pAssetsPaths->SetFlagParameters(""); + mStandardOpts.pConfigJsonPath = mKnobManager.CreateKnob>>(mCommandLineParser.GetJsonConfigFlagName(), defaultEmptyList); + mStandardOpts.pConfigJsonPath->SetFlagDescription( + "Additional commandline flags specified in a JSON file. Values specified in JSON files are " + "always lower priority than those specified on the command line. Between different files, the " + "later ones take priority. For lists, the JSON values will come earlier in the array than the " + "command-line values"); + mStandardOpts.pConfigJsonPath->SetFlagParameters(""); mStandardOpts.pDeterministic = mKnobManager.CreateKnob>("deterministic", false); diff --git a/src/ppx/command_line_parser.cpp b/src/ppx/command_line_parser.cpp index 9e768951f..c3dda0b52 100644 --- a/src/ppx/command_line_parser.cpp +++ b/src/ppx/command_line_parser.cpp @@ -57,12 +57,12 @@ std::pair CliOptions::GetOptionValueOrDefault(const std::string& optio void CliOptions::AddOption(std::string_view optionName, std::string_view value) { - std::string optionNameStr{optionName}; - std::string valueStr{value}; + std::string optionNameStr(optionName); + std::string valueStr(value); auto it = mAllOptions.find(optionNameStr); if (it == mAllOptions.cend()) { - std::vector v{valueStr}; - mAllOptions.emplace(optionName, v); + std::vector v{std::move(valueStr)}; + mAllOptions.emplace(std::move(optionNameStr), std::move(v)); return; } it->second.push_back(valueStr); @@ -70,7 +70,7 @@ void CliOptions::AddOption(std::string_view optionName, std::string_view value) void CliOptions::AddOption(std::string_view optionName, const std::vector& valueArray) { - std::string optionNameStr{optionName}; + std::string optionNameStr(optionName); auto it = mAllOptions.find(optionNameStr); if (it == mAllOptions.cend()) { mAllOptions.emplace(optionNameStr, valueArray); @@ -125,7 +125,54 @@ std::optional CommandLineParser::Parse(int argc args.emplace_back(res->second); } - // Process arguments into either standalone flags or options with parameters. + // Special initial pass to identify JSON config files + for (size_t i = 0; i < args.size() - 1; ++i) { + std::string_view name = ppx::string_util::TrimBothEnds(args[i]); + if (name != "--" + mJsonConfigFlagName) { + continue; + } + name = name.substr(2); + std::string_view value = ppx::string_util::TrimBothEnds(args[i + 1]); + ++i; + + if (auto error = AddOption(name, value)) { + return error; + } + } + + // Flags inside JSON files are processed first + // These are always lower priority than flags on the command-line, will be "overwritten" + std::vector configJsonPaths; + configJsonPaths = mOpts.GetOptionValueOrDefault(mJsonConfigFlagName, configJsonPaths); + for (const auto& jsonPath : configJsonPaths) { + // Attempt to open JSON file at specified path + std::ifstream f(jsonPath); + if (f.fail()) { + return "Cannot locate file --" + mJsonConfigFlagName + ": " + jsonPath; + } + + // Expect that JSON file specifies a valid JSON object + PPX_LOG_INFO("Parsing JSON config file: " << jsonPath); + nlohmann::json data; + try { + data = nlohmann::json::parse(f); + } + catch (nlohmann::json::parse_error& e) { + PPX_LOG_ERROR("nlohmann::json::parse error: " << e.what() << '\n' + << "exception id: " << e.id << '\n' + << "byte position of error: " << e.byte); + } + if (!(data.is_object())) { + return "The following config file could not be parsed as a JSON object: " + jsonPath; + } + + // Attempt to add all options specified in this JSON object + if (auto error = AddJsonOptions(data)) { + return error; + } + } + + // Main pass, process arguments into either standalone flags or options with parameters. for (size_t i = 0; i < args.size(); ++i) { std::string_view name = ppx::string_util::TrimBothEnds(args[i]); if (!StartsWithDoubleDash(name)) { @@ -143,20 +190,12 @@ std::optional CommandLineParser::Parse(int argc } } - if (auto error = AddOption(name, value)) { - return error; + // Avoid re-adding option for JSON config file + if (name == mJsonConfigFlagName) { + continue; } - } - std::vector configJsonPaths; - configJsonPaths = mOpts.GetOptionValueOrDefault("config-json-path", configJsonPaths); - for (const auto& jsonPath : configJsonPaths) { - std::ifstream f(jsonPath); - if (f.fail()) { - return "Invalid --config-json-path: " + jsonPath; - } - nlohmann::json data = nlohmann::json::parse(f); - if (auto error = AddJsonOptions(data)) { + if (auto error = AddOption(name, value)) { return error; } } @@ -172,12 +211,12 @@ std::optional CommandLineParser::AddJsonOptions std::string value = ss.str(); ss.str(""); - if (value.length() > 0 && value.substr(0, 1) == "[") { + if ((it.value()).is_array()) { // Special case, arrays specified in JSON are added directly to mOpts to avoid inserting element by element std::vector jsonStringArray; for (const auto& elem : it.value()) { ss << elem; - jsonStringArray.push_back(ss.str()); + jsonStringArray.push_back(std::string(ppx::string_util::TrimBothEnds(ss.str(), " \t\""))); ss.str(""); } mOpts.AddOption(it.key(), jsonStringArray); diff --git a/src/test/command_line_parser_test.cpp b/src/test/command_line_parser_test.cpp index ad994554e..b2d2c0efd 100644 --- a/src/test/command_line_parser_test.cpp +++ b/src/test/command_line_parser_test.cpp @@ -250,7 +250,9 @@ TEST(CommandLineParserTest, JsonSimpleParsedSuccessfully) "b": false, "c": 1.234, "d": 5, - "e": "hello world" + "e": "helloworld", + "f": "hello world", + "g": "200x300" } )"; nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); @@ -258,12 +260,41 @@ TEST(CommandLineParserTest, JsonSimpleParsedSuccessfully) FAIL() << error->errorMsg; } auto opts = parser.GetOptions(); - EXPECT_EQ(opts.GetNumUniqueOptions(), 5); + EXPECT_EQ(opts.GetNumUniqueOptions(), 7); EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); EXPECT_EQ(opts.GetOptionValueOrDefault("b", true), false); EXPECT_FLOAT_EQ(opts.GetOptionValueOrDefault("c", 6.0f), 1.234f); EXPECT_EQ(opts.GetOptionValueOrDefault("d", 0), 5); - EXPECT_EQ(opts.GetOptionValueOrDefault("e", "foo"), "hello world"); + EXPECT_EQ(opts.GetOptionValueOrDefault("e", "foo"), "helloworld"); + EXPECT_EQ(opts.GetOptionValueOrDefault("f", "foo"), "hello world"); + std::pair gFlag = opts.GetOptionValueOrDefault("g", std::make_pair(1, 1)); + EXPECT_EQ(gFlag.first, 200); + EXPECT_EQ(gFlag.second, 300); +} + +TEST(CommandLineParserTest, JsonWithNestedStructure) +{ + CommandLineParser parser; + std::string jsonText = R"( + { + "a": true, + "b": { + "c" : 1, + "d" : 2 + } + } +)"; + nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); + if (auto error = parser.AddJsonOptions(jsonConfig)) { + FAIL() << error->errorMsg; + } + auto opts = parser.GetOptions(); + EXPECT_EQ(opts.GetNumUniqueOptions(), 2); + EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); + EXPECT_TRUE(opts.HasExtraOption("b")); + EXPECT_EQ(opts.GetOptionValueOrDefault("b", "default"), "{\"c\":1,\"d\":2}"); + EXPECT_FALSE(opts.HasExtraOption("c")); + EXPECT_FALSE(opts.HasExtraOption("d")); } TEST(CommandLineParserTest, JsonWithIntArray) @@ -291,16 +322,38 @@ TEST(CommandLineParserTest, JsonWithIntArray) EXPECT_EQ(gotB.at(2), 3); } -TEST(CommandLineParserTest, JsonWithNestedStructure) +TEST(CommandLineParserTest, JsonWithStrArray) { CommandLineParser parser; std::string jsonText = R"( { "a": true, - "b": { - "c" : 1, - "d" : 2 + "b": ["first", "second", "third"] + } +)"; + nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); + if (auto error = parser.AddJsonOptions(jsonConfig)) { + FAIL() << error->errorMsg; } + auto opts = parser.GetOptions(); + EXPECT_EQ(opts.GetNumUniqueOptions(), 2); + EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); + EXPECT_TRUE(opts.HasExtraOption("b")); + std::vector defaultB = {}; + std::vector gotB = opts.GetOptionValueOrDefault("b", defaultB); + EXPECT_EQ(gotB.size(), 3); + EXPECT_EQ(gotB.at(0), "first"); + EXPECT_EQ(gotB.at(1), "second"); + EXPECT_EQ(gotB.at(2), "third"); +} + +TEST(CommandLineParserTest, JsonWithHeterogeneousArray) +{ + CommandLineParser parser; + std::string jsonText = R"( + { + "a": true, + "b": [1, "2", {"c" : 3}, 4.0] } )"; nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); @@ -311,9 +364,46 @@ TEST(CommandLineParserTest, JsonWithNestedStructure) EXPECT_EQ(opts.GetNumUniqueOptions(), 2); EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); EXPECT_TRUE(opts.HasExtraOption("b")); - EXPECT_EQ(opts.GetOptionValueOrDefault("b", "default"), "{\"c\":1,\"d\":2}"); - EXPECT_FALSE(opts.HasExtraOption("c")); - EXPECT_FALSE(opts.HasExtraOption("d")); + std::vector defaultB; + std::vector gotB = opts.GetOptionValueOrDefault("b", defaultB); + EXPECT_EQ(gotB.size(), 4); + EXPECT_EQ(gotB.at(0), "1"); + EXPECT_EQ(gotB.at(1), "2"); + EXPECT_EQ(gotB.at(2), "{\"c\":3}"); + EXPECT_EQ(gotB.at(3), "4.0"); +} + +TEST(CommandLineParserTest, JsonVsCommandlinePriority) +{ + CommandLineParser parser; + std::string jsonText = R"( + { + "a": 1, + "b": ["one", "two", "three"] + } +)"; + nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); + if (auto error = parser.AddJsonOptions(jsonConfig)) { + FAIL() << error->errorMsg; + } + + const char* args[] = {"/path/to/executable", "--b", "four", "--a", "2", "--b", "five", "--a", "3"}; + if (auto error = parser.Parse(sizeof(args) / sizeof(args[0]), args)) { + FAIL() << error->errorMsg; + } + + auto opts = parser.GetOptions(); + EXPECT_EQ(opts.GetNumUniqueOptions(), 2); + EXPECT_EQ(opts.GetOptionValueOrDefault("a", 0), 3); + EXPECT_TRUE(opts.HasExtraOption("b")); + std::vector defaultB = {}; + std::vector gotB = opts.GetOptionValueOrDefault("b", defaultB); + EXPECT_EQ(gotB.size(), 5); + EXPECT_EQ(gotB.at(0), "one"); + EXPECT_EQ(gotB.at(1), "two"); + EXPECT_EQ(gotB.at(2), "three"); + EXPECT_EQ(gotB.at(3), "four"); + EXPECT_EQ(gotB.at(4), "five"); } } // namespace From 889f1f14f23726d2e1f04938adc9de97baec740d Mon Sep 17 00:00:00 2001 From: Angela Chen Date: Mon, 14 Aug 2023 16:20:43 -0400 Subject: [PATCH 5/9] Make only one initial pass, handling both '=' and --config-json-path --- src/ppx/command_line_parser.cpp | 44 ++++++++++++++++----------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/src/ppx/command_line_parser.cpp b/src/ppx/command_line_parser.cpp index c3dda0b52..827c2d9f9 100644 --- a/src/ppx/command_line_parser.cpp +++ b/src/ppx/command_line_parser.cpp @@ -73,7 +73,7 @@ void CliOptions::AddOption(std::string_view optionName, const std::vectorsecond; @@ -106,38 +106,41 @@ std::optional CommandLineParser::Parse(int argc return std::nullopt; } - // Split flag and parameters connected with '=' + // Initial pass: + // - Split flag and parameters connected with '=' + // - Identify JSON config files, add the option, and remove from argument list std::vector args; for (size_t i = 1; i < argc; ++i) { std::string_view argString(argv[i]); auto res = ppx::string_util::SplitInTwo(argString, '='); + + // Argument does not contain '=' if (res == std::nullopt) { + if (StartsWithDoubleDash(argString) && + argString == "--" + mJsonConfigFlagName && + i + 1 < argc && + !StartsWithDoubleDash(argv[i + 1])) { + mOpts.AddOption(mJsonConfigFlagName, ppx::string_util::TrimBothEnds(argv[i + 1])); + ++i; + continue; + } args.emplace_back(argString); continue; } + + // Argument contains '=' if (res->first.empty() || res->second.empty()) { return "Malformed flag with '=': \"" + std::string(argString) + "\""; } - else if (res->second.find('=') != std::string_view::npos) { + if (res->second.find('=') != std::string_view::npos) { return "Unexpected number of '=' symbols in the following string: \"" + std::string(argString) + "\""; } - args.emplace_back(res->first); - args.emplace_back(res->second); - } - - // Special initial pass to identify JSON config files - for (size_t i = 0; i < args.size() - 1; ++i) { - std::string_view name = ppx::string_util::TrimBothEnds(args[i]); - if (name != "--" + mJsonConfigFlagName) { + if (StartsWithDoubleDash(res->first) && res->first == "--" + mJsonConfigFlagName) { + mOpts.AddOption(mJsonConfigFlagName, ppx::string_util::TrimBothEnds(res->second)); continue; } - name = name.substr(2); - std::string_view value = ppx::string_util::TrimBothEnds(args[i + 1]); - ++i; - - if (auto error = AddOption(name, value)) { - return error; - } + args.emplace_back(res->first); + args.emplace_back(res->second); } // Flags inside JSON files are processed first @@ -190,11 +193,6 @@ std::optional CommandLineParser::Parse(int argc } } - // Avoid re-adding option for JSON config file - if (name == mJsonConfigFlagName) { - continue; - } - if (auto error = AddOption(name, value)) { return error; } From 4e8e56d1a85d162ddc179165dc5e3e59bb06365f Mon Sep 17 00:00:00 2001 From: Angela Chen Date: Mon, 14 Aug 2023 17:08:47 -0400 Subject: [PATCH 6/9] Cleaning up comments and refactoring a bit for clarity --- src/ppx/command_line_parser.cpp | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/src/ppx/command_line_parser.cpp b/src/ppx/command_line_parser.cpp index 827c2d9f9..305156c6a 100644 --- a/src/ppx/command_line_parser.cpp +++ b/src/ppx/command_line_parser.cpp @@ -106,15 +106,13 @@ std::optional CommandLineParser::Parse(int argc return std::nullopt; } - // Initial pass: - // - Split flag and parameters connected with '=' - // - Identify JSON config files, add the option, and remove from argument list + // With one initial pass: + // - Split any flag and parameters that are connected with '=' + // - Identify JSON config files, add that option, and remove it from the argument list std::vector args; for (size_t i = 1; i < argc; ++i) { std::string_view argString(argv[i]); auto res = ppx::string_util::SplitInTwo(argString, '='); - - // Argument does not contain '=' if (res == std::nullopt) { if (StartsWithDoubleDash(argString) && argString == "--" + mJsonConfigFlagName && @@ -127,8 +125,6 @@ std::optional CommandLineParser::Parse(int argc args.emplace_back(argString); continue; } - - // Argument contains '=' if (res->first.empty() || res->second.empty()) { return "Malformed flag with '=': \"" + std::string(argString) + "\""; } @@ -144,17 +140,15 @@ std::optional CommandLineParser::Parse(int argc } // Flags inside JSON files are processed first - // These are always lower priority than flags on the command-line, will be "overwritten" + // These are always lower priority than flags on the command-line std::vector configJsonPaths; configJsonPaths = mOpts.GetOptionValueOrDefault(mJsonConfigFlagName, configJsonPaths); for (const auto& jsonPath : configJsonPaths) { - // Attempt to open JSON file at specified path std::ifstream f(jsonPath); if (f.fail()) { return "Cannot locate file --" + mJsonConfigFlagName + ": " + jsonPath; } - // Expect that JSON file specifies a valid JSON object PPX_LOG_INFO("Parsing JSON config file: " << jsonPath); nlohmann::json data; try { @@ -169,7 +163,6 @@ std::optional CommandLineParser::Parse(int argc return "The following config file could not be parsed as a JSON object: " + jsonPath; } - // Attempt to add all options specified in this JSON object if (auto error = AddJsonOptions(data)) { return error; } @@ -205,10 +198,6 @@ std::optional CommandLineParser::AddJsonOptions { std::stringstream ss; for (auto it = jsonConfig.cbegin(); it != jsonConfig.cend(); ++it) { - ss << it.value(); - std::string value = ss.str(); - ss.str(""); - if ((it.value()).is_array()) { // Special case, arrays specified in JSON are added directly to mOpts to avoid inserting element by element std::vector jsonStringArray; @@ -218,8 +207,13 @@ std::optional CommandLineParser::AddJsonOptions ss.str(""); } mOpts.AddOption(it.key(), jsonStringArray); + continue; } - else if (auto error = AddOption(it.key(), ppx::string_util::TrimBothEnds(value, " \t\""))) { + + ss << it.value(); + std::string value = ss.str(); + ss.str(""); + if (auto error = AddOption(it.key(), ppx::string_util::TrimBothEnds(value, " \t\""))) { return error; } } @@ -229,7 +223,6 @@ std::optional CommandLineParser::AddJsonOptions std::optional CommandLineParser::AddOption(std::string_view optionName, std::string_view valueStr) { if (optionName.length() > 2 && optionName.substr(0, 3) == "no-") { - // Special case where "no-flag-name" syntax is used if (valueStr.length() > 0) { return "invalid prefix no- for option \"" + std::string(optionName) + "\" and value \"" + std::string(valueStr) + "\""; } From 334569b5b30ad51ddedad871ed4acac6a1c57a6d Mon Sep 17 00:00:00 2001 From: Angela Chen Date: Wed, 16 Aug 2023 14:04:11 -0400 Subject: [PATCH 7/9] Minor changes * Rename pConfigJsonPath to pConfigJsonPaths * Change commandline parsing to 2 initial passes for better readability --- include/ppx/application.h | 2 +- src/ppx/application.cpp | 6 +++--- src/ppx/command_line_parser.cpp | 30 +++++++++++++++--------------- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/include/ppx/application.h b/include/ppx/application.h index 93fa5c13f..71a2d8ed3 100644 --- a/include/ppx/application.h +++ b/include/ppx/application.h @@ -252,7 +252,7 @@ struct StandardOptions #endif std::shared_ptr>> pAssetsPaths; - std::shared_ptr>> pConfigJsonPath; + std::shared_ptr>> pConfigJsonPaths; }; // ------------------------------------------------------------------------------------------------- diff --git a/src/ppx/application.cpp b/src/ppx/application.cpp index c8cccb7d1..7e4e5efa6 100644 --- a/src/ppx/application.cpp +++ b/src/ppx/application.cpp @@ -813,13 +813,13 @@ void Application::InitStandardKnobs() "Add a path in front of the assets search path list."); mStandardOpts.pAssetsPaths->SetFlagParameters(""); - mStandardOpts.pConfigJsonPath = mKnobManager.CreateKnob>>(mCommandLineParser.GetJsonConfigFlagName(), defaultEmptyList); - mStandardOpts.pConfigJsonPath->SetFlagDescription( + mStandardOpts.pConfigJsonPaths = mKnobManager.CreateKnob>>(mCommandLineParser.GetJsonConfigFlagName(), defaultEmptyList); + mStandardOpts.pConfigJsonPaths->SetFlagDescription( "Additional commandline flags specified in a JSON file. Values specified in JSON files are " "always lower priority than those specified on the command line. Between different files, the " "later ones take priority. For lists, the JSON values will come earlier in the array than the " "command-line values"); - mStandardOpts.pConfigJsonPath->SetFlagParameters(""); + mStandardOpts.pConfigJsonPaths->SetFlagParameters(""); mStandardOpts.pDeterministic = mKnobManager.CreateKnob>("deterministic", false); diff --git a/src/ppx/command_line_parser.cpp b/src/ppx/command_line_parser.cpp index 305156c6a..192a59a1c 100644 --- a/src/ppx/command_line_parser.cpp +++ b/src/ppx/command_line_parser.cpp @@ -106,22 +106,12 @@ std::optional CommandLineParser::Parse(int argc return std::nullopt; } - // With one initial pass: - // - Split any flag and parameters that are connected with '=' - // - Identify JSON config files, add that option, and remove it from the argument list + // Initial pass to trim the name of executable and to split any flag and parameters that are connected with '=' std::vector args; for (size_t i = 1; i < argc; ++i) { std::string_view argString(argv[i]); auto res = ppx::string_util::SplitInTwo(argString, '='); if (res == std::nullopt) { - if (StartsWithDoubleDash(argString) && - argString == "--" + mJsonConfigFlagName && - i + 1 < argc && - !StartsWithDoubleDash(argv[i + 1])) { - mOpts.AddOption(mJsonConfigFlagName, ppx::string_util::TrimBothEnds(argv[i + 1])); - ++i; - continue; - } args.emplace_back(argString); continue; } @@ -131,14 +121,24 @@ std::optional CommandLineParser::Parse(int argc if (res->second.find('=') != std::string_view::npos) { return "Unexpected number of '=' symbols in the following string: \"" + std::string(argString) + "\""; } - if (StartsWithDoubleDash(res->first) && res->first == "--" + mJsonConfigFlagName) { - mOpts.AddOption(mJsonConfigFlagName, ppx::string_util::TrimBothEnds(res->second)); - continue; - } args.emplace_back(res->first); args.emplace_back(res->second); } + // Another pass to identify JSON config files, add that option, and remove it from the argument list + std::vector argsFiltered; + for (size_t i = 0; i < args.size(); ++i) { + bool nextArgumentIsParameter = (i + 1 < args.size()) && !StartsWithDoubleDash(args[i + 1]); + if ((args[i] == "--" + mJsonConfigFlagName) && nextArgumentIsParameter) { + mOpts.AddOption(mJsonConfigFlagName, ppx::string_util::TrimBothEnds(args[i + 1])); + ++i; + continue; + } + argsFiltered.emplace_back(args[i]); + } + args = argsFiltered; + argsFiltered.clear(); + // Flags inside JSON files are processed first // These are always lower priority than flags on the command-line std::vector configJsonPaths; From 655de0c5701c2b6c1af8153e94cd7d1f8a1c921a Mon Sep 17 00:00:00 2001 From: Angela Chen Date: Fri, 18 Aug 2023 11:14:28 -0400 Subject: [PATCH 8/9] Change priority for --assets-path to match flag descriptions. --- src/ppx/application.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ppx/application.cpp b/src/ppx/application.cpp index 7e4e5efa6..4b95fb574 100644 --- a/src/ppx/application.cpp +++ b/src/ppx/application.cpp @@ -672,8 +672,8 @@ void Application::DispatchConfig() auto assetPaths = mStandardOpts.pAssetsPaths->GetValue(); if (!assetPaths.empty()) { - // Insert at front, in reverse order, so we respect the command line ordering for priority. - for (auto it = assetPaths.rbegin(); it < assetPaths.rend(); it++) { + // Asset directories specified later have higher priority + for (auto it = assetPaths.begin(); it < assetPaths.end(); it++) { AddAssetDir(*it, /* insert_at_front= */ true); } } From dc32011ef464fb3971df38e6ea66558f43660377 Mon Sep 17 00:00:00 2001 From: Angela Chen Date: Fri, 18 Aug 2023 19:22:55 -0400 Subject: [PATCH 9/9] Make changes to allow subsequent json files to overwrite earlier ones, and command line flags to overwrite json files after that. --- include/ppx/command_line_parser.h | 15 ++-- src/ppx/application.cpp | 11 ++- src/ppx/command_line_parser.cpp | 27 +++++-- src/test/command_line_parser_test.cpp | 107 ++++++++++++-------------- 4 files changed, 82 insertions(+), 78 deletions(-) diff --git a/include/ppx/command_line_parser.h b/include/ppx/command_line_parser.h index 1c6a84b5d..8a54bd6c4 100644 --- a/include/ppx/command_line_parser.h +++ b/include/ppx/command_line_parser.h @@ -108,6 +108,9 @@ class CliOptions // Same as above, but appends an array of values at the same key void AddOption(std::string_view optionName, const std::vector& valueArray); + // For all options existing in newOptions, current entries in mAllOptions will be replaced by them + void OverwriteOptions(const CliOptions& newOptions); + template T GetParsedOrDefault(std::string_view valueStr, const T& defaultValue) const { @@ -160,8 +163,12 @@ class CommandLineParser // and write the error to `out_error`. std::optional Parse(int argc, const char* argv[]); - // Adds all options specified within jsonConfig to mOpts. - std::optional AddJsonOptions(const nlohmann::json& jsonConfig); + // Parses all options specified within jsonConfig and adds them to cliOptions. + std::optional ParseJson(CliOptions& cliOptions, const nlohmann::json& jsonConfig); + + // Parses an option, handles the special --no-flag-name case, then adds the option to cliOptions + // Expects option names without the "--" prefix. + std::optional ParseOption(CliOptions& cliOptions, std::string_view optionName, std::string_view valueStr); std::string GetJsonConfigFlagName() const { return mJsonConfigFlagName; } const CliOptions& GetOptions() const { return mOpts; } @@ -170,10 +177,6 @@ class CommandLineParser void AppendUsageMsg(const std::string& additionalMsg) { mUsageMsg += additionalMsg; } private: - // Adds an option to mOpts and handles the special --no-flag-name case. - // Expects option names without the "--" prefix. - std::optional AddOption(std::string_view optionName, std::string_view valueStr); - CliOptions mOpts; std::string mJsonConfigFlagName = "config-json-path"; std::string mUsageMsg = R"( diff --git a/src/ppx/application.cpp b/src/ppx/application.cpp index 4b95fb574..5045c9c00 100644 --- a/src/ppx/application.cpp +++ b/src/ppx/application.cpp @@ -672,8 +672,8 @@ void Application::DispatchConfig() auto assetPaths = mStandardOpts.pAssetsPaths->GetValue(); if (!assetPaths.empty()) { - // Asset directories specified later have higher priority - for (auto it = assetPaths.begin(); it < assetPaths.end(); it++) { + // Insert at front, in reverse order, so we respect the command line ordering for priority. + for (auto it = assetPaths.rbegin(); it < assetPaths.rend(); it++) { AddAssetDir(*it, /* insert_at_front= */ true); } } @@ -810,15 +810,14 @@ void Application::InitStandardKnobs() mStandardOpts.pAssetsPaths = mKnobManager.CreateKnob>>("assets-path", defaultEmptyList); mStandardOpts.pAssetsPaths->SetFlagDescription( - "Add a path in front of the assets search path list."); + "Add a path before the default assets folder in the search list."); mStandardOpts.pAssetsPaths->SetFlagParameters(""); mStandardOpts.pConfigJsonPaths = mKnobManager.CreateKnob>>(mCommandLineParser.GetJsonConfigFlagName(), defaultEmptyList); mStandardOpts.pConfigJsonPaths->SetFlagDescription( "Additional commandline flags specified in a JSON file. Values specified in JSON files are " - "always lower priority than those specified on the command line. Between different files, the " - "later ones take priority. For lists, the JSON values will come earlier in the array than the " - "command-line values"); + "always overwritten by those specified on the command line. Between different files, the " + "later ones take priority."); mStandardOpts.pConfigJsonPaths->SetFlagParameters(""); mStandardOpts.pDeterministic = diff --git a/src/ppx/command_line_parser.cpp b/src/ppx/command_line_parser.cpp index 192a59a1c..9987a110c 100644 --- a/src/ppx/command_line_parser.cpp +++ b/src/ppx/command_line_parser.cpp @@ -38,6 +38,13 @@ bool StartsWithDoubleDash(std::string_view s) namespace ppx { +void CliOptions::OverwriteOptions(const CliOptions& newOptions) +{ + for (auto& it : newOptions.mAllOptions) { + mAllOptions[it.first] = it.second; + } +} + std::pair CliOptions::GetOptionValueOrDefault(const std::string& optionName, const std::pair& defaultValue) const { auto it = mAllOptions.find(optionName); @@ -163,11 +170,14 @@ std::optional CommandLineParser::Parse(int argc return "The following config file could not be parsed as a JSON object: " + jsonPath; } - if (auto error = AddJsonOptions(data)) { + CliOptions jsonOptions; + if (auto error = ParseJson(jsonOptions, data)) { return error; } + mOpts.OverwriteOptions(jsonOptions); } + CliOptions commandlineOptions; // Main pass, process arguments into either standalone flags or options with parameters. for (size_t i = 0; i < args.size(); ++i) { std::string_view name = ppx::string_util::TrimBothEnds(args[i]); @@ -186,41 +196,42 @@ std::optional CommandLineParser::Parse(int argc } } - if (auto error = AddOption(name, value)) { + if (auto error = ParseOption(commandlineOptions, name, value)) { return error; } } + mOpts.OverwriteOptions(commandlineOptions); return std::nullopt; } -std::optional CommandLineParser::AddJsonOptions(const nlohmann::json& jsonConfig) +std::optional CommandLineParser::ParseJson(CliOptions& cliOptions, const nlohmann::json& jsonConfig) { std::stringstream ss; for (auto it = jsonConfig.cbegin(); it != jsonConfig.cend(); ++it) { if ((it.value()).is_array()) { - // Special case, arrays specified in JSON are added directly to mOpts to avoid inserting element by element + // Special case, arrays specified in JSON are added directly to cliOptions to avoid inserting element by element std::vector jsonStringArray; for (const auto& elem : it.value()) { ss << elem; jsonStringArray.push_back(std::string(ppx::string_util::TrimBothEnds(ss.str(), " \t\""))); ss.str(""); } - mOpts.AddOption(it.key(), jsonStringArray); + cliOptions.AddOption(it.key(), jsonStringArray); continue; } ss << it.value(); std::string value = ss.str(); ss.str(""); - if (auto error = AddOption(it.key(), ppx::string_util::TrimBothEnds(value, " \t\""))) { + if (auto error = ParseOption(cliOptions, it.key(), ppx::string_util::TrimBothEnds(value, " \t\""))) { return error; } } return std::nullopt; } -std::optional CommandLineParser::AddOption(std::string_view optionName, std::string_view valueStr) +std::optional CommandLineParser::ParseOption(CliOptions& cliOptions, std::string_view optionName, std::string_view valueStr) { if (optionName.length() > 2 && optionName.substr(0, 3) == "no-") { if (valueStr.length() > 0) { @@ -229,7 +240,7 @@ std::optional CommandLineParser::AddOption(std: optionName = optionName.substr(3); valueStr = "0"; } - mOpts.AddOption(optionName, valueStr); + cliOptions.AddOption(optionName, valueStr); return std::nullopt; } diff --git a/src/test/command_line_parser_test.cpp b/src/test/command_line_parser_test.cpp index b2d2c0efd..f38d49fd4 100644 --- a/src/test/command_line_parser_test.cpp +++ b/src/test/command_line_parser_test.cpp @@ -22,7 +22,7 @@ namespace { using ::testing::HasSubstr; -TEST(CommandLineParserTest, ZeroArguments) +TEST(CommandLineParserTest, Parse_ZeroArguments) { CommandLineParser parser; if (auto error = parser.Parse(0, nullptr)) { @@ -31,7 +31,7 @@ TEST(CommandLineParserTest, ZeroArguments) EXPECT_EQ(parser.GetOptions().GetNumUniqueOptions(), 0); } -TEST(CommandLineParserTest, FirstArgumentIgnored) +TEST(CommandLineParserTest, Parse_FirstArgumentIgnored) { CommandLineParser parser; const char* args[] = {"/path/to/executable"}; @@ -41,7 +41,7 @@ TEST(CommandLineParserTest, FirstArgumentIgnored) EXPECT_EQ(parser.GetOptions().GetNumUniqueOptions(), 0); } -TEST(CommandLineParserTest, BooleansSuccessfullyParsed) +TEST(CommandLineParserTest, Parse_Booleans) { CommandLineParser parser; const char* args[] = {"/path/to/executable", "--a", "--b", "1", "--c", "true", "--no-d", "--e", "0", "--f", "false"}; @@ -58,7 +58,7 @@ TEST(CommandLineParserTest, BooleansSuccessfullyParsed) EXPECT_EQ(gotOptions.GetOptionValueOrDefault("f", true), false); } -TEST(CommandLineParserTest, StringsSuccessfullyParsed) +TEST(CommandLineParserTest, Parse_Strings) { CommandLineParser parser; const char* args[] = {"/path/to/executable", "--a", "filename with spaces", "--b", "filenameWithoutSpaces", "--c", "filename,with/.punctuation,", "--d", "", "--e"}; @@ -74,7 +74,7 @@ TEST(CommandLineParserTest, StringsSuccessfullyParsed) EXPECT_EQ(gotOptions.GetOptionValueOrDefault("e", "foo"), ""); } -TEST(CommandLineParserTest, IntegersSuccessfullyParsed) +TEST(CommandLineParserTest, Parse_Integers) { CommandLineParser parser; const char* args[] = {"/path/to/executable", "--a", "0", "--b", "-5", "--c", "300", "--d", "0", "--e", "1000"}; @@ -90,7 +90,7 @@ TEST(CommandLineParserTest, IntegersSuccessfullyParsed) EXPECT_EQ(gotOptions.GetOptionValueOrDefault("e", -1), 1000); } -TEST(CommandLineParserTest, FloatsSuccessfullyParsed) +TEST(CommandLineParserTest, Parse_Floats) { CommandLineParser parser; const char* args[] = {"/path/to/executable", "--a", "1.0", "--b", "-6.5", "--c", "300"}; @@ -104,7 +104,7 @@ TEST(CommandLineParserTest, FloatsSuccessfullyParsed) EXPECT_EQ(gotOptions.GetOptionValueOrDefault("c", 0.0f), 300.0f); } -TEST(CommandLineParserTest, StringListSuccesfullyParsed) +TEST(CommandLineParserTest, Parse_StringList) { CommandLineParser parser; const char* args[] = {"/path/to/executable", "--a", "some-path", "--a", "some-other-path", "--a", "last-path"}; @@ -122,7 +122,7 @@ TEST(CommandLineParserTest, StringListSuccesfullyParsed) } } -TEST(CommandLineParserTest, ResolutionSuccesfullyParsed) +TEST(CommandLineParserTest, Parse_Resolution) { CommandLineParser parser; const char* args[] = {"/path/to/executable", "--a", "1000x2000"}; @@ -136,7 +136,7 @@ TEST(CommandLineParserTest, ResolutionSuccesfullyParsed) EXPECT_EQ(res.second, 2000); } -TEST(CommandLineParserTest, ResolutionSuccessfullyParsedButDefaulted) +TEST(CommandLineParserTest, Parse_ResolutionDefaulted) { CommandLineParser parser; const char* args[] = {"/path/to/executable", "--a", "1000X2000"}; @@ -150,7 +150,7 @@ TEST(CommandLineParserTest, ResolutionSuccessfullyParsedButDefaulted) EXPECT_EQ(res.second, 0); } -TEST(CommandLineParserTest, EqualSignsSuccessfullyParsed) +TEST(CommandLineParserTest, Parse_EqualSigns) { CommandLineParser parser; const char* args[] = {"/path/to/executable", "--a", "--b=5", "--c", "--d", "11"}; @@ -165,7 +165,7 @@ TEST(CommandLineParserTest, EqualSignsSuccessfullyParsed) EXPECT_EQ(gotOptions.GetOptionValueOrDefault("d", 0), 11); } -TEST(CommandLineParserTest, EqualSignsMultipleFailedParsed) +TEST(CommandLineParserTest, Parse_EqualSignsMultipleFail) { CommandLineParser parser; const char* args[] = {"/path/to/executable", "--a", "--b=5=8", "--c", "--d", "11"}; @@ -174,7 +174,7 @@ TEST(CommandLineParserTest, EqualSignsMultipleFailedParsed) EXPECT_THAT(error->errorMsg, HasSubstr("Unexpected number of '=' symbols in the following string")); } -TEST(CommandLineParserTest, EqualSignsMalformedFailedParsed) +TEST(CommandLineParserTest, Parse_EqualSignsMalformedFail) { CommandLineParser parser; const char* args[] = {"/path/to/executable", "--a", "--b=", "--c", "--d", "11"}; @@ -183,7 +183,7 @@ TEST(CommandLineParserTest, EqualSignsMalformedFailedParsed) EXPECT_THAT(error->errorMsg, HasSubstr("Malformed flag with '='")); } -TEST(CommandLineParserTest, LeadingParameterFailedParsed) +TEST(CommandLineParserTest, Parse_LeadingParameterFail) { CommandLineParser parser; const char* args[] = {"/path/to/executable", "10", "--a", "--b", "5", "--c", "--d", "11"}; @@ -192,7 +192,7 @@ TEST(CommandLineParserTest, LeadingParameterFailedParsed) EXPECT_THAT(error->errorMsg, HasSubstr("Invalid command-line option")); } -TEST(CommandLineParserTest, AdjacentParameterFailedParsed) +TEST(CommandLineParserTest, Parse_AdjacentParameterFail) { CommandLineParser parser; const char* args[] = {"/path/to/executable", "--a", "--b", "5", "8", "--c", "--d", "11"}; @@ -201,7 +201,7 @@ TEST(CommandLineParserTest, AdjacentParameterFailedParsed) EXPECT_THAT(error->errorMsg, HasSubstr("Invalid command-line option")); } -TEST(CommandLineParserTest, LastValueIsTaken) +TEST(CommandLineParserTest, Parse_LastValueIsTaken) { CommandLineParser parser; const char* args[] = {"/path/to/executable", "--a", "1", "--b", "1", "--a", "2", "--a", "3"}; @@ -214,7 +214,7 @@ TEST(CommandLineParserTest, LastValueIsTaken) EXPECT_EQ(gotOptions.GetOptionValueOrDefault("b", 0), 1); } -TEST(CommandLineParserTest, ExtraOptionsSuccessfullyParsed) +TEST(CommandLineParserTest, Parse_ExtraOptions) { CommandLineParser parser; const char* args[] = {"/path/to/executable", "--extra-option-bool", "true", "--extra-option-int", "123", "--extra-option-no-param", "--extra-option-str", "option string value"}; @@ -230,20 +230,21 @@ TEST(CommandLineParserTest, ExtraOptionsSuccessfullyParsed) EXPECT_TRUE(opts.HasExtraOption("extra-option-no-param")); } -TEST(CommandLineParserTest, JsonEmptyParsedSuccessfully) +TEST(CommandLineParserTest, ParseJson_Empty) { CommandLineParser parser; + CliOptions opts; nlohmann::json jsonConfig; - if (auto error = parser.AddJsonOptions(jsonConfig)) { + if (auto error = parser.ParseJson(opts, jsonConfig)) { FAIL() << error->errorMsg; } - auto opts = parser.GetOptions(); EXPECT_EQ(opts.GetNumUniqueOptions(), 0); } -TEST(CommandLineParserTest, JsonSimpleParsedSuccessfully) +TEST(CommandLineParserTest, ParseJson_Simple) { CommandLineParser parser; + CliOptions opts; std::string jsonText = R"( { "a": true, @@ -256,10 +257,9 @@ TEST(CommandLineParserTest, JsonSimpleParsedSuccessfully) } )"; nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); - if (auto error = parser.AddJsonOptions(jsonConfig)) { + if (auto error = parser.ParseJson(opts, jsonConfig)) { FAIL() << error->errorMsg; } - auto opts = parser.GetOptions(); EXPECT_EQ(opts.GetNumUniqueOptions(), 7); EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); EXPECT_EQ(opts.GetOptionValueOrDefault("b", true), false); @@ -272,9 +272,10 @@ TEST(CommandLineParserTest, JsonSimpleParsedSuccessfully) EXPECT_EQ(gFlag.second, 300); } -TEST(CommandLineParserTest, JsonWithNestedStructure) +TEST(CommandLineParserTest, ParseJson_NestedStructure) { CommandLineParser parser; + CliOptions opts; std::string jsonText = R"( { "a": true, @@ -285,10 +286,9 @@ TEST(CommandLineParserTest, JsonWithNestedStructure) } )"; nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); - if (auto error = parser.AddJsonOptions(jsonConfig)) { + if (auto error = parser.ParseJson(opts, jsonConfig)) { FAIL() << error->errorMsg; } - auto opts = parser.GetOptions(); EXPECT_EQ(opts.GetNumUniqueOptions(), 2); EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); EXPECT_TRUE(opts.HasExtraOption("b")); @@ -297,9 +297,10 @@ TEST(CommandLineParserTest, JsonWithNestedStructure) EXPECT_FALSE(opts.HasExtraOption("d")); } -TEST(CommandLineParserTest, JsonWithIntArray) +TEST(CommandLineParserTest, ParseJson_IntArray) { CommandLineParser parser; + CliOptions opts; std::string jsonText = R"( { "a": true, @@ -307,10 +308,9 @@ TEST(CommandLineParserTest, JsonWithIntArray) } )"; nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); - if (auto error = parser.AddJsonOptions(jsonConfig)) { + if (auto error = parser.ParseJson(opts, jsonConfig)) { FAIL() << error->errorMsg; } - auto opts = parser.GetOptions(); EXPECT_EQ(opts.GetNumUniqueOptions(), 2); EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); EXPECT_TRUE(opts.HasExtraOption("b")); @@ -322,9 +322,10 @@ TEST(CommandLineParserTest, JsonWithIntArray) EXPECT_EQ(gotB.at(2), 3); } -TEST(CommandLineParserTest, JsonWithStrArray) +TEST(CommandLineParserTest, ParseJson_StrArray) { CommandLineParser parser; + CliOptions opts; std::string jsonText = R"( { "a": true, @@ -332,10 +333,9 @@ TEST(CommandLineParserTest, JsonWithStrArray) } )"; nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); - if (auto error = parser.AddJsonOptions(jsonConfig)) { + if (auto error = parser.ParseJson(opts, jsonConfig)) { FAIL() << error->errorMsg; } - auto opts = parser.GetOptions(); EXPECT_EQ(opts.GetNumUniqueOptions(), 2); EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); EXPECT_TRUE(opts.HasExtraOption("b")); @@ -347,9 +347,10 @@ TEST(CommandLineParserTest, JsonWithStrArray) EXPECT_EQ(gotB.at(2), "third"); } -TEST(CommandLineParserTest, JsonWithHeterogeneousArray) +TEST(CommandLineParserTest, ParseJson_HeterogeneousArray) { CommandLineParser parser; + CliOptions opts; std::string jsonText = R"( { "a": true, @@ -357,10 +358,9 @@ TEST(CommandLineParserTest, JsonWithHeterogeneousArray) } )"; nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); - if (auto error = parser.AddJsonOptions(jsonConfig)) { + if (auto error = parser.ParseJson(opts, jsonConfig)) { FAIL() << error->errorMsg; } - auto opts = parser.GetOptions(); EXPECT_EQ(opts.GetNumUniqueOptions(), 2); EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); EXPECT_TRUE(opts.HasExtraOption("b")); @@ -373,37 +373,28 @@ TEST(CommandLineParserTest, JsonWithHeterogeneousArray) EXPECT_EQ(gotB.at(3), "4.0"); } -TEST(CommandLineParserTest, JsonVsCommandlinePriority) +TEST(CommandLineParserTest, ParseOption_Simple) { CommandLineParser parser; - std::string jsonText = R"( - { - "a": 1, - "b": ["one", "two", "three"] - } -)"; - nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); - if (auto error = parser.AddJsonOptions(jsonConfig)) { + CliOptions opts; + if (auto error = parser.ParseOption(opts, "flag-name", "true")) { FAIL() << error->errorMsg; } + EXPECT_EQ(opts.GetNumUniqueOptions(), 1); + EXPECT_TRUE(opts.HasExtraOption("flag-name")); + EXPECT_EQ(opts.GetOptionValueOrDefault("flag-name", false), true); +} - const char* args[] = {"/path/to/executable", "--b", "four", "--a", "2", "--b", "five", "--a", "3"}; - if (auto error = parser.Parse(sizeof(args) / sizeof(args[0]), args)) { +TEST(CommandLineParserTest, ParseOption_NoPrefix) +{ + CommandLineParser parser; + CliOptions opts; + if (auto error = parser.ParseOption(opts, "no-flag-name", "")) { FAIL() << error->errorMsg; } - - auto opts = parser.GetOptions(); - EXPECT_EQ(opts.GetNumUniqueOptions(), 2); - EXPECT_EQ(opts.GetOptionValueOrDefault("a", 0), 3); - EXPECT_TRUE(opts.HasExtraOption("b")); - std::vector defaultB = {}; - std::vector gotB = opts.GetOptionValueOrDefault("b", defaultB); - EXPECT_EQ(gotB.size(), 5); - EXPECT_EQ(gotB.at(0), "one"); - EXPECT_EQ(gotB.at(1), "two"); - EXPECT_EQ(gotB.at(2), "three"); - EXPECT_EQ(gotB.at(3), "four"); - EXPECT_EQ(gotB.at(4), "five"); + EXPECT_EQ(opts.GetNumUniqueOptions(), 1); + EXPECT_TRUE(opts.HasExtraOption("flag-name")); + EXPECT_EQ(opts.GetOptionValueOrDefault("flag-name", true), false); } } // namespace