[stellar/stellar-core] Command line (#1732)

MonsieurNicolas commented on this pull request.

good change overall, just looking for better namespacing and clearer help strings

> @@ -2,8 +2,11 @@
// under the Apache License, Version 2.0. See the COPYING file at the root
// of this distribution or at http://www.apache.org/licenses/LICENSE-2.0

-#include «QuorumSetUtils.h»

+#include «scp/QuorumSetUtils.h»
+#include «history/HistoryManager.h»
+#include «main/Application.h»

let’s not add a dependency SCP -> application as SCP is more or less independent from the application

> @@ -217,15 +215,15 @@ readSecret(const std::string& prompt, bool force_tty)
}

void
-signtxn(std::string const& filename, bool base64)
+signtxn(std::string const& filename, std::string netId, bool base64)

thanks for getting rid of global variables!

> @@ -237,20 +237,30 @@ Config::loadQset(std::shared_ptr group, SCPQuorumSet& qset,
}

void
-Config::load(std::string const& filename)
+Config::load(std::string fileName)

if you want to make this change, why not making this a static function instead:

`Config Config::load(std::string const& filename)`

As we seem to override some fields of the config, it doesn’t really make sense to have this new member variable.

> {
VirtualClock clock(VirtualClock::REAL_TIME);
+ cfg.setNoListen();

why this change?

This causes the top level config to have some undefined values depending on which function gets called first.

If you really want to get those overrides done closer to app creation, clone and modify the config

> @@ -0,0 +1,29 @@
+#pragma once
+
+// Copyright 2018 Stellar Development Foundation and contributors. Licensed
+// under the Apache License, Version 2.0. See the COPYING file at the root
+// of this distribution or at http://www.apache.org/licenses/LICENSE-2.0
+
+#include «main/Application.h»
+
+namespace stellar
+{
+
+class CatchupConfiguration;
+
+bool checkInitialized(Application::pointer app);

better would be to namespace this as well: I don’t like that we’re polluting the global stellar namespace with utility functions only used by one file

> @@ -0,0 +1,921 @@
+// Copyright 2018 Stellar Development Foundation and contributors. Licensed
+// under the Apache License, Version 2.0. See the COPYING file at the root
+// of this distribution or at http://www.apache.org/licenses/LICENSE-2.0
+
+#define CATCH_CONFIG_RUNNER
+
+#include «main/Commands.h»

Use a better name than «Commands.cpp/h»: maybe CommandLine?

> @@ -2,8 +2,6 @@
// under the Apache License, Version 2.0. See the COPYING file at the root
// of this distribution or at http://www.apache.org/licenses/LICENSE-2.0

-#define CATCH_CONFIG_RUNNER

you should keep `CATCH_CONFIG_RUNNER` here as it just tells catch that this is the file that is used for injecting definitions.

> +#include «main/ApplicationUtils.h»
+#include «main/Config.h»
+#include «main/StellarCoreVersion.h»
+#include «main/dumpxdr.h»
+#include «main/fuzz.h»
+#include «scp/QuorumSetUtils.h»
+#include «test/test.h»
+#include «util/Logging.h»
+#include «util/optional.h»
+
+#include
+#include +#include +
+// must be included after catch.hpp
+#include «test/SimpleTestReporter.h»

let’s not move test (catch related) things here

> +#include «util/optional.h»
+
+#include
+#include
+
+namespace stellar
+{
+
+struct Args
+{
+ std::string mCommandName;
+ std::string mCommandDescription;
+ std::vector mArgs;
+};
+
+int runCatchup(Args const& args);

do we need all those functions (and the `Args` struct) to be exported? why not static and internal to the implementation?

> +
+ if (gVersionsToTest.empty())
+ {
+ gVersionsToTest.emplace_back(Config::CURRENT_LEDGER_PROTOCOL_VERSION);
+ }
+
+ auto r = session.run();
+ gTestRoots.clear();
+ gTestCfg->clear();
+ return r;
+}
+
+optional
+handleCommandLine(int argc, char* const* argv)
+{
+ std::vector commands;

maybe using the c++ 11 syntax for vector initialization would be a little better?
Something like:
«`c++
std::vector commands = {
{«catchup»,
«execute catchup from history archives without connecting to network»,
runCatchup},

{«check-quorum», «check quorum intersection from history»,
runCheckQuorum},

{«convert-id», «displays ID in all known forms», runConvertId},
«`

> + }
+
+ try
+ {
+ return {parseLedger(catchup.substr(0, separatorIndex)),
+ parseLedgerCount(catchup.substr(separatorIndex + 1))};
+ }
+ catch (std::exception& e)
+ {
+ throw std::runtime_error(errorMessage);
+ }
+}
+
+CommandExecutor::CommandExecutor(std::string exeName,
+ std::vector commands)
+ : mExeName{std::move(exeName)}, mCommands{std::move(commands)}

no need to put `std::move` here

> +void
+httpCommand(std::string const& command, unsigned short port)
+{
+ std::string ret;
+ std::ostringstream path;
+
+ path << "/"; + bool gotCommand = false; + + std::locale loc("C"); + + for (auto const& c : command) + { + if (gotCommand) + { + if (std::isalnum(c, loc)) this doesn't compile: you need to include `` in this file https://en.cppreference.com/w/cpp/locale/isalnum

> + optional mConfig;
+
+ bool
+ hasExitCode() const
+ {
+ return !!mExitCode;
+ }
+
+ int
+ exitCode() const
+ {
+ return *mExitCode;
+ }
+};
+
+struct Command

use less generic names than `Command` and `CommandExecutor`;

Maybe defining `Command` inside `CommandExecutor` may help with naming

> +#include «scp/QuorumSetUtils.h»
+#include «test/test.h»
+#include «util/Logging.h»
+#include «util/optional.h»
+
+#include
+#include +#include +
+// must be included after catch.hpp
+#include «test/SimpleTestReporter.h»
+
+namespace stellar
+{
+
+using namespace Catch;

let’s not include those namespaces like this.
I think you only need clara, so for brievity, you can do something like:
`namespace clara = Catch::clara;`
and then use it as `clara::XYZ`

> +
+// must be included after catch.hpp
+#include «test/SimpleTestReporter.h»
+
+namespace stellar
+{
+
+using namespace Catch;
+using namespace clara;
+
+namespace
+{
+
+struct Options
+{
+ optional mExitCode;

let’s not mix parameters and runtime:
we should just use exceptions instead of `mExitCode`.
It’s really only used to distinguish between a bad option and help, so if we make `help` return `1` then we don’t need this complexity.

> +#include +#include +
+// must be included after catch.hpp
+#include «test/SimpleTestReporter.h»
+
+namespace stellar
+{
+
+using namespace Catch;
+using namespace clara;
+
+namespace
+{
+
+struct Options

`Options` should really be defined inside the `CommandLine` namespace

> +class CommandExecutor
+{
+ public:
+ explicit CommandExecutor(std::string exeName,
+ std::vector commands);
+
+ optional execute(int argc, char* const* argv);
+ void writeToStream(std::ostream& os) const;
+
+ private:
+ std::string mExeName;
+ std::vector mCommands;
+};
+
+void
+writeWithTextFlow(std::ostream& os, std::string const& text)

I normally prefer static methods to properly namespace things

> + std::end(args.mArgs)});
+
+ if (!result)
+ {
+ valid = false;
+ errorMessage = result.errorMessage();
+ }
+ else
+ {
+ errorMessage = isValid();
+ valid = errorMessage.empty();
+ }
+ }
+ catch (std::exception& e)
+ {
+ // wrong parameter value was passed, we can just display help

I thought Clara doesn’t throw?

> +}
+
+Options
+handleCommonOptions(Args const& args, Parser parser,
+ std::function isValid)
+{
+ el::Level logLevel{el::Level::Info};
+ std::vector metrics;
+
+ auto commonOptions =
+ Opt(
+ [&](std::string const& arg) {
+ logLevel = Logging::getLLfromString(arg);
+ },
+ «LEVEL»)[«—ll»](«set the log level») |
+ Opt(metrics, «METRIC»)[«—metric»](«report metric METRIC on exit»);

`METRIC` -> `METRIC-NAME`

> + {
+ return options;
+ }
+
+ options.mLogLevel = logLevel;
+ options.mMetrics = std::move(metrics);
+ return options;
+}
+
+Options
+handleConfigurationOptions(Args const& args, Parser parser,
+ std::function isValid,
+ bool disableLogToFile = false)
+{
+ std::string configurationFile{«stellar-core.cfg»};
+ auto configurationOption = Opt(configurationFile, «FILE»)[«—conf»](

`FILE` -> `FILE-NAME`

> + «ledger>/, where destination »
+ «ledger is any valid number or ‘current’ and »
+ «ledger count is any valid number or ‘max'»;
+
+ auto separatorIndex = catchup.find(«/»);
+ if (separatorIndex == std::string::npos)
+ {
+ throw std::runtime_error(errorMessage);
+ }
+
+ try
+ {
+ return {parseLedger(catchup.substr(0, separatorIndex)),
+ parseLedgerCount(catchup.substr(separatorIndex + 1))};
+ }
+ catch (std::exception& e)

did you mean to also include the information included in `e` ? If not, remove its definition

> + .indent(2) +
+ clara::TextFlow::Spacer(4) +
+ clara::TextFlow::Column(command.mDescription)
+ .width(consoleWidth — 7 — commandWidth);
+ os << row << std::endl; + } +} +} + +int +runCatchup(Args const& args) +{ + std::string catchupString; + std::string outputFile; + + auto parser = Arg(catchupString, "CATCHUP").required() | `CATCHUP` -> `DESTINATION-LEDGER/LEDGER-COUNT`

> + clara::TextFlow::Spacer(4) +
+ clara::TextFlow::Column(command.mDescription)
+ .width(consoleWidth — 7 — commandWidth);
+ os << row << std::endl; + } +} +} + +int +runCatchup(Args const& args) +{ + std::string catchupString; + std::string outputFile; + + auto parser = Arg(catchupString, "CATCHUP").required() | + Opt(outputFile, "FILE")["--output-file"]("output file"); `FILE` -> `FILE-NAME`

> + }
+ });
+ if (options.hasExitCode())
+ {
+ return options.exitCode();
+ }
+
+ StrKeyUtils::logKey(std::cout, id);
+ return 0;
+}
+
+int
+runDumpXDR(Args const& args)
+{
+ std::string xdr;
+ auto parser = Parser{} | Arg(xdr, «XDR»).required();

`XDR` -> `FILE-NAME`

> + if (options.hasExitCode())
+ {
+ return options.exitCode();
+ }
+
+ dumpxdr(xdr);
+ return 0;
+}
+
+int
+runForceSCP(Args const& args)
+{
+ bool set = true;
+ auto parser =
+ Parser{} |
+ Opt(set, «FLAG»)[«—set»](

I think it would be better to have a flag called `reset`:
`Opt(reset)[«—set»]`
usage becomes `—reset` (no value needed)
and you just need to invoke `setForceSCPFlag` with `!reset`

> + auto options = handleConfigurationOptions(args, parser,
+ []() { return std::string{}; });
+ if (options.hasExitCode())
+ {
+ return options.exitCode();
+ }
+
+ setForceSCPFlag(*options.mConfig, set);
+ return 0;
+}
+
+int
+runFuzz(Args const& args)
+{
+ std::string fileName;
+ auto parser = Parser{} | Arg(fileName, «FILE»).required();

`FILE` -> `FILE-NAME`

> + }
+ });
+ if (options.hasExitCode())
+ {
+ return options.exitCode();
+ }
+
+ fuzz(fileName, options.mLogLevel, options.mMetrics);
+ return 0;
+}
+
+int
+runGenFuzz(Args const& args)
+{
+ std::string fileName;
+ auto parser = Parser{} | Arg(fileName, «FILE»).required();

`FILE` -> `FILE-NAME`

also, maybe you should have a helper function for the predicate: it’s quite a bit of copy/paste

> + auto options = handleConfigurationOptions(args, Parser{},
+ []() { return std::string{}; });
+ if (options.hasExitCode())
+ {
+ return options.exitCode();
+ }
+
+ inferQuorumAndWrite(*options.mConfig);
+ return 0;
+}
+
+int
+runLoadXDR(Args const& args)
+{
+ std::string xdr;
+ auto parser = Parser{} | Arg(xdr, «XDR»).required();

`XDR` -> `FILE-NAME`

> + auto options = handleConfigurationOptions(args, Parser{},
+ []() { return std::string{}; });
+ if (options.hasExitCode())
+ {
+ return options.exitCode();
+ }
+
+ initializeDatabase(*options.mConfig);
+ return 0;
+}
+
+int
+runNewHist(Args const& args)
+{
+ std::vector newHistories;
+ auto parser = Parser{} | Arg(newHistories, «HISTORY»).required();

`HISTORY` -> `HISTORY-LABEL`

> + []() { return std::string{}; });
+ if (options.hasExitCode())
+ {
+ return options.exitCode();
+ }
+
+ showOfflineInfo(*options.mConfig);
+ return 0;
+}
+
+int
+runPrintTransaction(Args const& args)
+{
+ std::string transaction;
+ bool base64{false};
+ auto parser = Arg(transaction, «TRANSACTION»).required() |

`TRANSACTION` -> `FILE-NAME`

> + });
+ if (options.hasExitCode())
+ {
+ return options.exitCode();
+ }
+
+ printtxn(transaction, base64);
+ return 0;
+}
+
+int
+runReportLastHistoryCheckpoint(Args const& args)
+{
+ std::string outputFile;
+ auto parser =
+ Parser{} | Opt(outputFile, «FILE»)[«—output-file»](«output file»);

`FILE` -> `FILE-NAME`

> + if (options.hasExitCode())
+ {
+ return options.exitCode();
+ }
+
+ priv2pub();
+ return 0;
+}
+
+int
+runSignTransaction(Args const& args)
+{
+ std::string transaction;
+ std::string netId;
+ bool base64;
+ auto parser = Arg(transaction, «TRANSACTION»).required() |

`TRANSACTION` -> `FILE-NAME`

> + {
+ return options.exitCode();
+ }
+
+ priv2pub();
+ return 0;
+}
+
+int
+runSignTransaction(Args const& args)
+{
+ std::string transaction;
+ std::string netId;
+ bool base64;
+ auto parser = Arg(transaction, «TRANSACTION»).required() |
+ Opt(netId, «STRING»)[«—netid»](«network ID used for signing»)

`STRING` -> `NETWORK-PASSPHRASE`

> + return 0;
+}
+
+int
+runVersion(Args const& args)
+{
+ std::cout << STELLAR_CORE_VERSION << std::endl; + return 0; +} + +int +runWriteQuorum(Args const& args) +{ + std::string outputFile; + auto parser = + Parser{} | Opt(outputFile, "FILE")["--output-file"]("output file"); `FILE` -> `FILE-NAME`

Добавить комментарий