diff --git a/include/gz/utils/Subprocess.hh b/include/gz/utils/Subprocess.hh index 787daf5..dee00fd 100644 --- a/include/gz/utils/Subprocess.hh +++ b/include/gz/utils/Subprocess.hh @@ -19,8 +19,12 @@ #define GZ_UTILS__SUBPROCESS_HH_ #include "detail/subprocess.h" +#include "gz/utils/Environment.hh" + +#include #include #include +#include #include @@ -32,46 +36,80 @@ namespace gz namespace utils { +/// \brief Create a RAII-type object that encapsulates a subprocess. class Subprocess { + /// \brief Constructor + /// + /// This variant will spawn a subprocess that inherits the environment + /// from the calling process. + /// + /// \param[in] _commandLine set of arguments starting with an executable + /// used to spawn the subprocess + public: explicit Subprocess(const std::vector &_commandLine): + commandLine(_commandLine), + environment({}), + inheritEnvironment(true) + { + this->Create(); + } + + /// \brief Constructor + /// + /// This variant will spawn a subprocess that uses the user-specified + /// environment + /// + /// \param[in] _commandLine set of arguments starting with an executable + /// used to spawn the subprocess + /// \param[in] _environment environment variables to set in the spawned + /// subprocess public: Subprocess(const std::vector &_commandLine, - const std::vector &_environment = {}): + gz::utils::EnvironmentMap _environment): commandLine(_commandLine), - environment(_environment) + environment(std::move(_environment)), + inheritEnvironment(false) { this->Create(); } private: void Create() { - if (this->process) + if (this->process != nullptr) return; this->process = new subprocess_s; + std::vector environmentStr; + std::vector environmentCstr; std::vector commandLineCstr; + for (const auto &val : this->commandLine) { commandLineCstr.push_back(val.c_str()); } commandLineCstr.push_back(nullptr); - std::vector environmentCstr; - for (const auto &val : this->environment) + if (!this->inheritEnvironment) { - environmentCstr.push_back(val.c_str()); + for (const auto &[key, val] : this->environment) + { + environmentStr.push_back(key + "=" + val); + environmentCstr.push_back(environmentStr.back().c_str()); + } + environmentCstr.push_back(nullptr); } - environmentCstr.push_back(nullptr); int ret = -1; - if (this->environment.size()) + if (!this->inheritEnvironment) { ret = subprocess_create_ex(commandLineCstr.data(), 0, environmentCstr.data(), this->process); } else { - ret = subprocess_create(commandLineCstr.data(), 0, this->process); + ret = subprocess_create(commandLineCstr.data(), + subprocess_option_inherit_environment, + this->process); } if (0 != ret) @@ -84,21 +122,21 @@ class Subprocess public: ~Subprocess() { - if (this->process) + if (this->process != nullptr) subprocess_destroy(this->process); delete this->process; } public: std::string Stdout() { - std::string result{""}; - if (this->process) + std::string result; + if (this->process != nullptr) { - auto p_stdout = subprocess_stdout(this->process); + auto *p_stdout = subprocess_stdout(this->process); char buffer[128]; while (!feof(p_stdout)) { - if (fgets(buffer, 128, p_stdout) != NULL) + if (fgets(buffer, 128, p_stdout) != nullptr) result += buffer; } } @@ -107,14 +145,14 @@ class Subprocess public: std::string Stderr() { - std::string result{""}; - if (this->process) + std::string result; + if (this->process != nullptr) { - auto p_stdout = subprocess_stderr(this->process); + auto *p_stdout = subprocess_stderr(this->process); char buffer[128]; while (!feof(p_stdout)) { - if (fgets(buffer, 128, p_stdout) != NULL) + if (fgets(buffer, 128, p_stdout) != nullptr) result += buffer; } } @@ -124,7 +162,7 @@ class Subprocess public: bool Alive() { - if (this->process) + if (this->process != nullptr) return subprocess_alive(this->process); else return false; @@ -132,7 +170,7 @@ class Subprocess public: bool Terminate() { - if (this->process) + if (this->process != nullptr) return subprocess_terminate(this->process) != 0; else return false; @@ -141,7 +179,7 @@ class Subprocess public: int Join() { int return_code = -1; - if (this->process) + if (this->process != nullptr) { auto ret = subprocess_join(this->process, &return_code); if (ret != 0) @@ -155,7 +193,9 @@ class Subprocess protected: std::vector commandLine; - protected: std::vector environment; + protected: EnvironmentMap environment; + + protected: bool inheritEnvironment; protected: subprocess_s * process {nullptr}; }; diff --git a/test/integration/subprocess/subprocess_main.cc b/test/integration/subprocess/subprocess_main.cc index 436172e..e903e3e 100644 --- a/test/integration/subprocess/subprocess_main.cc +++ b/test/integration/subprocess/subprocess_main.cc @@ -24,8 +24,8 @@ class OutputSink { - public: OutputSink(const std::string &_dest): - dest(_dest) + public: explicit OutputSink(std::string _dest): + dest(std::move(_dest)) { } @@ -40,7 +40,6 @@ class OutputSink { std::cerr << val << std::endl; } - return; } private: std::string dest; @@ -69,10 +68,6 @@ int main(int argc, char **argv) std::this_thread::sleep_for(std::chrono::milliseconds(iter_ms)); } - std::string env_var; - if(gz::utils::env("ENV_VAR", env_var)) - { - sink.Write("ENV_VAR=" + env_var); - } + sink.Write(gz::utils::printenv()); return 0; } diff --git a/test/integration/subprocess_TEST.cc b/test/integration/subprocess_TEST.cc index 32c6ed4..ef539d9 100644 --- a/test/integration/subprocess_TEST.cc +++ b/test/integration/subprocess_TEST.cc @@ -21,6 +21,7 @@ #include #include +#include "gz/utils/Environment.hh" using Subprocess = gz::utils::Subprocess; @@ -33,6 +34,28 @@ TEST(Subprocess, CreateInvalid) EXPECT_FALSE(proc.Alive()); } +///////////////////////////////////////////////// +TEST(Subprocess, CreateInvalidSpaces) +{ + // Test if a user passes a string with spaces, rather than + // a vector of strings + std::string command(kExecutablePath); + command.append(" --help"); + auto proc = Subprocess({command}); + + // Block until the executable is done + auto ret = proc.Join(); + EXPECT_EQ(-1, ret); + + EXPECT_FALSE(proc.Alive()); + + auto cout = proc.Stdout(); + auto cerr = proc.Stdout(); + + EXPECT_TRUE(cout.empty()); + EXPECT_TRUE(cerr.empty()); +} + ///////////////////////////////////////////////// TEST(Subprocess, CreateValid) { @@ -94,38 +117,50 @@ TEST(Subprocess, Cerr) ///////////////////////////////////////////////// TEST(Subprocess, Environment) { + ASSERT_TRUE(gz::utils::clearenv()); + ASSERT_TRUE(gz::utils::setenv("GZ_FOO_KEY", "GZ_FOO_VAL")); + ASSERT_TRUE(gz::utils::setenv("GZ_BAR_KEY", "GZ_BAR_VAL")); + ASSERT_TRUE(gz::utils::setenv("GZ_BAZ_KEY", "GZ_BAZ_VAL")); + { - auto proc = Subprocess({kExecutablePath, "--output=cout"}, - {"ENV_VAR=foobar"}); + // Default behavior is to inherit the environment + auto proc = Subprocess({kExecutablePath, "--output=cout"}); // Block until the executable is done auto ret = proc.Join(); EXPECT_EQ(0u, ret); auto cout = proc.Stdout(); - EXPECT_NE(std::string::npos, cout.find("ENV_VAR=foobar")); + EXPECT_NE(std::string::npos, cout.find("GZ_FOO_KEY=GZ_FOO_VAL")); + EXPECT_NE(std::string::npos, cout.find("GZ_BAR_KEY=GZ_BAR_VAL")); + EXPECT_NE(std::string::npos, cout.find("GZ_BAZ_KEY=GZ_BAZ_VAL")); } { - auto proc = Subprocess({kExecutablePath, "--output=cerr"}, - {"ENV_VAR=foobar2"}); + // Passing an empty map as the second arg clears the environment + auto proc = Subprocess({kExecutablePath, "--output=cout"}, {}); // Block until the executable is done auto ret = proc.Join(); EXPECT_EQ(0u, ret); - auto cerr = proc.Stderr(); - EXPECT_NE(std::string::npos, cerr.find("ENV_VAR=foobar2")); + auto cout = proc.Stdout(); + EXPECT_EQ(std::string::npos, cout.find("GZ_FOO_KEY=GZ_FOO_VAL")); + EXPECT_EQ(std::string::npos, cout.find("GZ_BAR_KEY=GZ_BAR_VAL")); + EXPECT_EQ(std::string::npos, cout.find("GZ_BAZ_KEY=GZ_BAZ_VAL")); } { - auto proc = Subprocess({kExecutablePath}, - {"ENV_VAR=foobar3"}); + // Passing a map sets those variables, clearing the rest + auto proc = Subprocess({kExecutablePath, "--output=cout"}, { + {"QUX_KEY", "QUX_VAL"} + }); // Block until the executable is done auto ret = proc.Join(); EXPECT_EQ(0u, ret); auto cout = proc.Stdout(); - auto cerr = proc.Stderr(); - EXPECT_EQ(std::string::npos, cerr.find("ENV_VAR=foobar3")); - EXPECT_EQ(std::string::npos, cout.find("ENV_VAR=foobar3")); + EXPECT_EQ(std::string::npos, cout.find("GZ_FOO_KEY=GZ_FOO_VAL")); + EXPECT_EQ(std::string::npos, cout.find("GZ_BAR_KEY=GZ_BAR_VAL")); + EXPECT_EQ(std::string::npos, cout.find("GZ_BAZ_KEY=GZ_BAZ_VAL")); + EXPECT_NE(std::string::npos, cout.find("QUX_KEY=QUX_VAL")); } }