From 7df86863fae2290c2332aec34380f07b4e6f2ded Mon Sep 17 00:00:00 2001 From: Benjamin Morgan Date: Thu, 28 Sep 2023 15:49:20 +0200 Subject: [PATCH] engine: Fix command execution Fix several severe bugs with command execution. BREAKING CHANGE: Command JSON deserialization renames `output` key to `log_output` and the enum value `normal` is renamed to `on_error`. As the `output` option was poorly documented, it is unlikely to have been used and therefore this change should have low impact. --- docs/reference/actions.rst | 4 +- engine/src/utility/command.cpp | 41 ++++++------- engine/src/utility/command.hpp | 2 +- engine/tests/test_engine_json_schema.json | 72 +++++++++++------------ runtime/include/cloe/utility/command.hpp | 18 +++--- 5 files changed, 69 insertions(+), 68 deletions(-) diff --git a/docs/reference/actions.rst b/docs/reference/actions.rst index 1e924e880..53d94d0e8 100644 --- a/docs/reference/actions.rst +++ b/docs/reference/actions.rst @@ -200,7 +200,7 @@ Parameter Required Type Description ================== ========== ============== ================================== ``command`` yes string command to execute in shell ``mode`` no string one of ``sync``, ``async``, ``detach`` -``output`` no string one of ``never``, ``normal``, ``always`` +``log_output`` no string one of ``never``, ``on_error``, ``always`` ``ignore_failure`` no bool whether to ignore execution failure ================== ========== ============== ================================== @@ -212,7 +212,7 @@ Parameter Required Type Description ``path`` yes string command to execute directly ``args`` no array arguments to pass to path ``mode`` no string one of ``sync``, ``async``, ``detach`` -``output`` no string one of ``never``, ``normal``, ``always`` +``log_output`` no string one of ``never``, ``on_error``, ``always`` ``ignore_failure`` no bool whether to ignore execution failure ================== ========== ============== ================================== diff --git a/engine/src/utility/command.cpp b/engine/src/utility/command.cpp index 6b6cbf78a..6de1a7586 100644 --- a/engine/src/utility/command.cpp +++ b/engine/src/utility/command.cpp @@ -30,6 +30,9 @@ namespace engine { CommandResult CommandExecuter::run_and_release(const cloe::Command& cmd) const { namespace bp = boost::process; + // Verbosity is Never(0), OnFailure(1), Always(2) + auto verbose = static_cast(cmd.verbosity()); + CommandResult r; r.name = cmd.executable().filename().native(); r.command = cmd.command(); @@ -40,7 +43,9 @@ CommandResult CommandExecuter::run_and_release(const cloe::Command& cmd) const { return r; } - logger()->info("Run: {}", r.command); + if (verbose > 1) { + logger()->info("Run: {}", r.command); + } try { if (!cmd.is_sync()) { r.child = bp::child(cmd.executable(), cmd.args()); @@ -56,20 +61,19 @@ CommandResult CommandExecuter::run_and_release(const cloe::Command& cmd) const { // was only found by rummaging through the source code. Ridiculous. r.child = bp::child(cmd.executable(), cmd.args(), (bp::std_out & bp::std_err) > is); - if (cmd.verbosity() != cloe::Command::Verbosity::Never) { - std::string line; - bool log_output = logger_ && cmd.verbosity() == cloe::Command::Verbosity::Always; - while (r.child->running() && std::getline(is, line)) { - if (log_output) { - logger()->debug("{}:{} | {}", r.name, r.child->id(), line); - } - r.output.push_back(line); + std::string line; + // After finished running output the rest of the lines. + while (std::getline(is, line)) { + if (verbose > 1) { + logger()->debug("{}:{} | {}", r.name, r.child->id(), line); } + r.output.push_back(line); } + r.child->wait(); r.exit_code = r.child->exit_code(); - if (*r.exit_code != 0) { + if (*r.exit_code != 0 && (verbose > 0 || !cmd.ignore_failure())) { logger()->error("Error running: {}", r.command); if (!r.output.empty()) { std::stringstream s; @@ -84,20 +88,17 @@ CommandResult CommandExecuter::run_and_release(const cloe::Command& cmd) const { } } } - } catch (std::system_error& e) { - logger()->error("Error running: {}", r.command); - logger()->error("> Message: {}", e.what()); + } catch (std::runtime_error& e) { + if (verbose > 0 || !cmd.ignore_failure()) { + logger()->error("Error running: {}", r.command); + logger()->error("> Message: {}", e.what()); - if (!cmd.ignore_failure()) { - throw cloe::ConcludedError{e}; + if (!cmd.ignore_failure()) { + throw cloe::ConcludedError{e}; + } } r.error = e; - } catch (std::runtime_error& e) { - // TODO(ben): When does this happen? - if (!cmd.ignore_failure()) { - throw cloe::ConcludedError{e}; - } } return r; diff --git a/engine/src/utility/command.hpp b/engine/src/utility/command.hpp index 666b7b7e6..eb4b4c373 100644 --- a/engine/src/utility/command.hpp +++ b/engine/src/utility/command.hpp @@ -41,7 +41,7 @@ struct CommandResult { std::string command; boost::optional child; boost::optional exit_code; - boost::optional error; + boost::optional error; std::vector output; }; diff --git a/engine/tests/test_engine_json_schema.json b/engine/tests/test_engine_json_schema.json index 5bda290ce..3c1a90679 100644 --- a/engine/tests/test_engine_json_schema.json +++ b/engine/tests/test_engine_json_schema.json @@ -472,6 +472,15 @@ "description": "whether to ignore execution failure", "type": "boolean" }, + "log_output": { + "description": "how to log command output", + "enum": [ + "never", + "on_error", + "always" + ], + "type": "string" + }, "mode": { "description": "synchronization mode to use", "enum": [ @@ -481,15 +490,6 @@ ], "type": "string" }, - "output": { - "description": "how to log command output", - "enum": [ - "never", - "normal", - "always" - ], - "type": "string" - }, "path": { "comment": "path should be executable", "description": "path to executable", @@ -514,6 +514,15 @@ "description": "whether to ignore execution failure", "type": "boolean" }, + "log_output": { + "description": "how to log command output", + "enum": [ + "never", + "on_error", + "always" + ], + "type": "string" + }, "mode": { "description": "synchronization mode to use", "enum": [ @@ -522,15 +531,6 @@ "detach" ], "type": "string" - }, - "output": { - "description": "how to log command output", - "enum": [ - "never", - "normal", - "always" - ], - "type": "string" } }, "required": [ @@ -565,6 +565,15 @@ "description": "whether to ignore execution failure", "type": "boolean" }, + "log_output": { + "description": "how to log command output", + "enum": [ + "never", + "on_error", + "always" + ], + "type": "string" + }, "mode": { "description": "synchronization mode to use", "enum": [ @@ -574,15 +583,6 @@ ], "type": "string" }, - "output": { - "description": "how to log command output", - "enum": [ - "never", - "normal", - "always" - ], - "type": "string" - }, "path": { "comment": "path should be executable", "description": "path to executable", @@ -607,6 +607,15 @@ "description": "whether to ignore execution failure", "type": "boolean" }, + "log_output": { + "description": "how to log command output", + "enum": [ + "never", + "on_error", + "always" + ], + "type": "string" + }, "mode": { "description": "synchronization mode to use", "enum": [ @@ -615,15 +624,6 @@ "detach" ], "type": "string" - }, - "output": { - "description": "how to log command output", - "enum": [ - "never", - "normal", - "always" - ], - "type": "string" } }, "required": [ diff --git a/runtime/include/cloe/utility/command.hpp b/runtime/include/cloe/utility/command.hpp index bd6e71e63..fdcc56b4b 100644 --- a/runtime/include/cloe/utility/command.hpp +++ b/runtime/include/cloe/utility/command.hpp @@ -64,9 +64,9 @@ class Command : public Confable { * Logging verbosity. */ enum class Verbosity { - Never, /// never log anything - Normal, /// log combined error when an error occurs - Always, /// log combined output + Never, /// never log anything + OnError, /// log combined error when an error occurs + Always, /// log combined output }; Command() = default; @@ -103,12 +103,12 @@ class Command : public Confable { */ std::string command() const; - Verbosity verbosity() const { return output_; } + Verbosity verbosity() const { return log_output_; } Command verbosity(Verbosity v) && { set_verbosity(v); return std::move(*this); } - void set_verbosity(Verbosity v) { output_ = v; } + void set_verbosity(Verbosity v) { log_output_ = v; } Mode mode() const { return mode_; } Command mode(Mode m) && { @@ -142,13 +142,13 @@ class Command : public Confable { {"path", make_schema(&executable_, "path to executable").require().not_empty().executable()}, {"args", make_schema(&args_, "arguments to executable")}, {"mode", make_schema(&mode_, "synchronization mode to use")}, - {"output", make_schema(&output_, "how to log command output")}, + {"log_output", make_schema(&log_output_, "how to log command output")}, {"ignore_failure", make_schema(&ignore_failure_, "whether to ignore execution failure")}, }, Struct{ {"command", make_schema(&command_, "command to execute within shell").require().not_empty()}, {"mode", make_schema(&mode_, "synchronization mode to use")}, - {"output", make_schema(&output_, "how to log command output")}, + {"log_output", make_schema(&log_output_, "how to log command output")}, {"ignore_failure", make_schema(&ignore_failure_, "whether to ignore execution failure")}, }, String{&command_, "command to execute within shell"}.not_empty(), @@ -163,7 +163,7 @@ class Command : public Confable { std::vector args_; std::string command_; Mode mode_ = Mode::Sync; - Verbosity output_ = Verbosity::Always; + Verbosity log_output_ = Verbosity::Always; bool ignore_failure_ = false; }; @@ -176,7 +176,7 @@ ENUM_SERIALIZATION(Command::Mode, ({ ENUM_SERIALIZATION(Command::Verbosity, ({ {Command::Verbosity::Never, "never"}, - {Command::Verbosity::Normal, "normal"}, + {Command::Verbosity::OnError, "on_error"}, {Command::Verbosity::Always, "always"}, })) // clang-format on