Make the single argument constructor inherit the env

include/gz/utils/Subprocess.hh

@@ -19,8 +19,12 @@
 #define GZ_UTILS__SUBPROCESS_HH_
 
 #include "detail/subprocess.h"
+#include "gz/utils/Environment.hh"
+
+#include <iostream>
 #include <string>
 #include <vector>
+#include <utility>
 
 #include <gz/utils/detail/subprocess.h>
 
@@ -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<std::string> &_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<std::string> &_commandLine,
-                     const std::vector<std::string> &_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<std::string> environmentStr;
+    std::vector<const char*> environmentCstr;
     std::vector<const char*> commandLineCstr;
+
     for (const auto &val : this->commandLine)
     {
       commandLineCstr.push_back(val.c_str());
     }
     commandLineCstr.push_back(nullptr);
 
-    std::vector<const char*> 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,15 +162,15 @@ class Subprocess
 
   public: bool Alive()
   {
-    if (this->process)
+    if (this->process != nullptr)
       return subprocess_alive(this->process);
     else
       return false;
   }
 
   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<std::string> commandLine;
 
-  protected: std::vector<std::string> environment;
+  protected: EnvironmentMap environment;
+
+  protected: bool inheritEnvironment;
 
   protected: subprocess_s * process {nullptr};
 };

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))
   {
   }
 
@@ -34,13 +34,11 @@ class OutputSink
     if (dest == "cout" || dest == "both")
     {
       std::cout << val << std::endl;
-
     }
     else if (dest == "cerr" || dest == "both")
     {
       std::cerr << val << std::endl;
     }
-    return;
   }
 
   private: std::string dest;
@@ -60,6 +58,10 @@ int main(int argc, char **argv)
   int iter_ms = 0;
   app.add_option("--iteration-ms", iter_ms, "length of one iteration");
 
+  bool environment = false;
+  app.add_flag("--environment", environment,
+               "print the environment variables");
+
   CLI11_PARSE(app, argc, argv);
 
   auto sink = OutputSink(output);
@@ -69,10 +71,9 @@ 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))
+  if (environment)
   {
-    sink.Write("ENV_VAR=" + env_var);
+    sink.Write(gz::utils::printenv());
   }
   return 0;
 }

test/integration/subprocess_TEST.cc

@@ -21,6 +21,7 @@
 
 #include <chrono>
 #include <thread>
+#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,52 @@ 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", "--environment"});
     // 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", "--environment"}, {});
     // 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", "--environment"}, {
+      {"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"));
   }
 }