Skip to content

Commit 60e4559

Browse files
cassavacassava
committed
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.
1 parent 0fcb758 commit 60e4559

File tree

5 files changed

+69
-68
lines changed

5 files changed

+69
-68
lines changed

‎docs/reference/actions.rst

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ Parameter Required Type Description
200200
================== ========== ============== ==================================
201201
``command`` yes string command to execute in shell
202202
``mode`` no string one of ``sync``, ``async``, ``detach``
203-
``output`` no string one of ``never``, ``normal``, ``always``
203+
``log_output`` no string one of ``never``, ``on_error``, ``always``
204204
``ignore_failure`` no bool whether to ignore execution failure
205205
================== ========== ============== ==================================
206206

@@ -212,7 +212,7 @@ Parameter Required Type Description
212212
``path`` yes string command to execute directly
213213
``args`` no array arguments to pass to path
214214
``mode`` no string one of ``sync``, ``async``, ``detach``
215-
``output`` no string one of ``never``, ``normal``, ``always``
215+
``log_output`` no string one of ``never``, ``on_error``, ``always``
216216
``ignore_failure`` no bool whether to ignore execution failure
217217
================== ========== ============== ==================================
218218

‎engine/src/utility/command.cpp

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@ namespace engine {
3030
CommandResult CommandExecuter::run_and_release(const cloe::Command& cmd) const {
3131
namespace bp = boost::process;
3232

33+
// Verbosity is Never(0), OnFailure(1), Always(2)
34+
auto verbose = static_cast<int>(cmd.verbosity());
35+
3336
CommandResult r;
3437
r.name = cmd.executable().filename().native();
3538
r.command = cmd.command();
@@ -40,7 +43,9 @@ CommandResult CommandExecuter::run_and_release(const cloe::Command& cmd) const {
4043
return r;
4144
}
4245

43-
logger()->info("Run: {}", r.command);
46+
if (verbose > 1) {
47+
logger()->info("Run: {}", r.command);
48+
}
4449
try {
4550
if (!cmd.is_sync()) {
4651
r.child = bp::child(cmd.executable(), cmd.args());
@@ -56,20 +61,19 @@ CommandResult CommandExecuter::run_and_release(const cloe::Command& cmd) const {
5661
// was only found by rummaging through the source code. Ridiculous.
5762
r.child = bp::child(cmd.executable(), cmd.args(), (bp::std_out & bp::std_err) > is);
5863

59-
if (cmd.verbosity() != cloe::Command::Verbosity::Never) {
60-
std::string line;
61-
bool log_output = logger_ && cmd.verbosity() == cloe::Command::Verbosity::Always;
62-
while (r.child->running() && std::getline(is, line)) {
63-
if (log_output) {
64-
logger()->debug("{}:{} | {}", r.name, r.child->id(), line);
65-
}
66-
r.output.push_back(line);
64+
std::string line;
65+
// After finished running output the rest of the lines.
66+
while (std::getline(is, line)) {
67+
if (verbose > 1) {
68+
logger()->debug("{}:{} | {}", r.name, r.child->id(), line);
6769
}
70+
r.output.push_back(line);
6871
}
72+
6973
r.child->wait();
7074
r.exit_code = r.child->exit_code();
7175

72-
if (*r.exit_code != 0) {
76+
if (*r.exit_code != 0 && (verbose > 0 || !cmd.ignore_failure())) {
7377
logger()->error("Error running: {}", r.command);
7478
if (!r.output.empty()) {
7579
std::stringstream s;
@@ -84,20 +88,17 @@ CommandResult CommandExecuter::run_and_release(const cloe::Command& cmd) const {
8488
}
8589
}
8690
}
87-
} catch (std::system_error& e) {
88-
logger()->error("Error running: {}", r.command);
89-
logger()->error("> Message: {}", e.what());
91+
} catch (std::runtime_error& e) {
92+
if (verbose > 0 || !cmd.ignore_failure()) {
93+
logger()->error("Error running: {}", r.command);
94+
logger()->error("> Message: {}", e.what());
9095

91-
if (!cmd.ignore_failure()) {
92-
throw cloe::ConcludedError{e};
96+
if (!cmd.ignore_failure()) {
97+
throw cloe::ConcludedError{e};
98+
}
9399
}
94100

95101
r.error = e;
96-
} catch (std::runtime_error& e) {
97-
// TODO(ben): When does this happen?
98-
if (!cmd.ignore_failure()) {
99-
throw cloe::ConcludedError{e};
100-
}
101102
}
102103

103104
return r;

‎engine/src/utility/command.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ struct CommandResult {
4141
std::string command;
4242
boost::optional<boost::process::child> child;
4343
boost::optional<int> exit_code;
44-
boost::optional<std::system_error> error;
44+
boost::optional<std::runtime_error> error;
4545
std::vector<std::string> output;
4646
};
4747

‎engine/tests/test_engine_json_schema.json

Lines changed: 36 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -472,6 +472,15 @@
472472
"description": "whether to ignore execution failure",
473473
"type": "boolean"
474474
},
475+
"log_output": {
476+
"description": "how to log command output",
477+
"enum": [
478+
"never",
479+
"on_error",
480+
"always"
481+
],
482+
"type": "string"
483+
},
475484
"mode": {
476485
"description": "synchronization mode to use",
477486
"enum": [
@@ -481,15 +490,6 @@
481490
],
482491
"type": "string"
483492
},
484-
"output": {
485-
"description": "how to log command output",
486-
"enum": [
487-
"never",
488-
"normal",
489-
"always"
490-
],
491-
"type": "string"
492-
},
493493
"path": {
494494
"comment": "path should be executable",
495495
"description": "path to executable",
@@ -514,6 +514,15 @@
514514
"description": "whether to ignore execution failure",
515515
"type": "boolean"
516516
},
517+
"log_output": {
518+
"description": "how to log command output",
519+
"enum": [
520+
"never",
521+
"on_error",
522+
"always"
523+
],
524+
"type": "string"
525+
},
517526
"mode": {
518527
"description": "synchronization mode to use",
519528
"enum": [
@@ -522,15 +531,6 @@
522531
"detach"
523532
],
524533
"type": "string"
525-
},
526-
"output": {
527-
"description": "how to log command output",
528-
"enum": [
529-
"never",
530-
"normal",
531-
"always"
532-
],
533-
"type": "string"
534534
}
535535
},
536536
"required": [
@@ -565,6 +565,15 @@
565565
"description": "whether to ignore execution failure",
566566
"type": "boolean"
567567
},
568+
"log_output": {
569+
"description": "how to log command output",
570+
"enum": [
571+
"never",
572+
"on_error",
573+
"always"
574+
],
575+
"type": "string"
576+
},
568577
"mode": {
569578
"description": "synchronization mode to use",
570579
"enum": [
@@ -574,15 +583,6 @@
574583
],
575584
"type": "string"
576585
},
577-
"output": {
578-
"description": "how to log command output",
579-
"enum": [
580-
"never",
581-
"normal",
582-
"always"
583-
],
584-
"type": "string"
585-
},
586586
"path": {
587587
"comment": "path should be executable",
588588
"description": "path to executable",
@@ -607,6 +607,15 @@
607607
"description": "whether to ignore execution failure",
608608
"type": "boolean"
609609
},
610+
"log_output": {
611+
"description": "how to log command output",
612+
"enum": [
613+
"never",
614+
"on_error",
615+
"always"
616+
],
617+
"type": "string"
618+
},
610619
"mode": {
611620
"description": "synchronization mode to use",
612621
"enum": [
@@ -615,15 +624,6 @@
615624
"detach"
616625
],
617626
"type": "string"
618-
},
619-
"output": {
620-
"description": "how to log command output",
621-
"enum": [
622-
"never",
623-
"normal",
624-
"always"
625-
],
626-
"type": "string"
627627
}
628628
},
629629
"required": [

‎runtime/include/cloe/utility/command.hpp

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,9 @@ class Command : public Confable {
6464
* Logging verbosity.
6565
*/
6666
enum class Verbosity {
67-
Never, /// never log anything
68-
Normal, /// log combined error when an error occurs
69-
Always, /// log combined output
67+
Never, /// never log anything
68+
OnError, /// log combined error when an error occurs
69+
Always, /// log combined output
7070
};
7171

7272
Command() = default;
@@ -103,12 +103,12 @@ class Command : public Confable {
103103
*/
104104
std::string command() const;
105105

106-
Verbosity verbosity() const { return output_; }
106+
Verbosity verbosity() const { return log_output_; }
107107
Command verbosity(Verbosity v) && {
108108
set_verbosity(v);
109109
return std::move(*this);
110110
}
111-
void set_verbosity(Verbosity v) { output_ = v; }
111+
void set_verbosity(Verbosity v) { log_output_ = v; }
112112

113113
Mode mode() const { return mode_; }
114114
Command mode(Mode m) && {
@@ -142,13 +142,13 @@ class Command : public Confable {
142142
{"path", make_schema(&executable_, "path to executable").require().not_empty().executable()},
143143
{"args", make_schema(&args_, "arguments to executable")},
144144
{"mode", make_schema(&mode_, "synchronization mode to use")},
145-
{"output", make_schema(&output_, "how to log command output")},
145+
{"log_output", make_schema(&log_output_, "how to log command output")},
146146
{"ignore_failure", make_schema(&ignore_failure_, "whether to ignore execution failure")},
147147
},
148148
Struct{
149149
{"command", make_schema(&command_, "command to execute within shell").require().not_empty()},
150150
{"mode", make_schema(&mode_, "synchronization mode to use")},
151-
{"output", make_schema(&output_, "how to log command output")},
151+
{"log_output", make_schema(&log_output_, "how to log command output")},
152152
{"ignore_failure", make_schema(&ignore_failure_, "whether to ignore execution failure")},
153153
},
154154
String{&command_, "command to execute within shell"}.not_empty(),
@@ -163,7 +163,7 @@ class Command : public Confable {
163163
std::vector<std::string> args_;
164164
std::string command_;
165165
Mode mode_ = Mode::Sync;
166-
Verbosity output_ = Verbosity::Always;
166+
Verbosity log_output_ = Verbosity::Always;
167167
bool ignore_failure_ = false;
168168
};
169169

@@ -176,7 +176,7 @@ ENUM_SERIALIZATION(Command::Mode, ({
176176

177177
ENUM_SERIALIZATION(Command::Verbosity, ({
178178
{Command::Verbosity::Never, "never"},
179-
{Command::Verbosity::Normal, "normal"},
179+
{Command::Verbosity::OnError, "on_error"},
180180
{Command::Verbosity::Always, "always"},
181181
}))
182182
// clang-format on

0 commit comments

Comments
 (0)