Skip to content

Commit

Permalink
engine: Fix command execution
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@cassava
cassava committed Mar 28, 2024
1 parent c78dcaf commit 23a8b8b
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 66 deletions.
4 changes: 2 additions & 2 deletions docs/reference/actions.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
================== ========== ============== ==================================

Expand All @@ -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
================== ========== ============== ==================================

Expand Down
39 changes: 20 additions & 19 deletions engine/src/utility/command.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<int>(cmd.verbosity());

CommandResult r;
r.name = cmd.executable().filename().native();
r.command = cmd.command();
Expand All @@ -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());
Expand All @@ -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;
Expand All @@ -85,19 +89,16 @@ 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());
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;
Expand Down
72 changes: 36 additions & 36 deletions engine/tests/test_engine_json_schema.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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": [
Expand All @@ -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",
Expand All @@ -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": [
Expand All @@ -522,15 +531,6 @@
"detach"
],
"type": "string"
},
"output": {
"description": "how to log command output",
"enum": [
"never",
"normal",
"always"
],
"type": "string"
}
},
"required": [
Expand Down Expand Up @@ -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": [
Expand All @@ -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",
Expand All @@ -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": [
Expand All @@ -615,15 +624,6 @@
"detach"
],
"type": "string"
},
"output": {
"description": "how to log command output",
"enum": [
"never",
"normal",
"always"
],
"type": "string"
}
},
"required": [
Expand Down
18 changes: 9 additions & 9 deletions runtime/include/cloe/utility/command.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,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;
Expand Down Expand Up @@ -101,12 +101,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) && {
Expand Down Expand Up @@ -140,13 +140,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(),
Expand All @@ -161,7 +161,7 @@ class Command : public Confable {
std::vector<std::string> args_;
std::string command_;
Mode mode_ = Mode::Sync;
Verbosity output_ = Verbosity::Always;
Verbosity log_output_ = Verbosity::Always;
bool ignore_failure_ = false;
};

Expand All @@ -174,7 +174,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
Expand Down

0 comments on commit 23a8b8b

Please sign in to comment.