From 917faea7458195ae34351b12bc636a0027126b4b Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 1 Nov 2023 14:06:52 +0000 Subject: [PATCH 01/15] Add new functions for manipulating the environment This adds functions for using collections with the environment. It allows for conveniently retreiving and setting the environment variables via a map, rather than individually. This also adds a clearenv and printenv for completeness. Signed-off-by: Michael Carroll --- include/gz/utils/Environment.hh | 44 ++++++++++++++++++++- src/Environment.cc | 70 +++++++++++++++++++++++++++++++++ src/Environment_TEST.cc | 43 +++++++++++++++++++- 3 files changed, 155 insertions(+), 2 deletions(-) diff --git a/include/gz/utils/Environment.hh b/include/gz/utils/Environment.hh index 8ac06a4..a6dfa5f 100644 --- a/include/gz/utils/Environment.hh +++ b/include/gz/utils/Environment.hh @@ -22,6 +22,7 @@ #include #include +#include namespace gz { @@ -66,7 +67,48 @@ bool GZ_UTILS_VISIBLE setenv( /// \return True if the variable was unset or false otherwise. bool GZ_UTILS_VISIBLE unsetenv(const std::string &_name); -} +/// \brief Unset all environment variables +/// +/// Note: This function is not thread-safe and should not be called +/// concurrently with `env` or `setenv` +/// +/// \return True if the environment was unset or false otherwise. +bool GZ_UTILS_VISIBLE clearenv(); + +/// \brief Type alias for a collection of environment variables +using EnvironmentMap = std::unordered_map; + +/// \brief Retrieve all current environment variables +/// +/// Note: This function is not thread-safe and should not be called +/// concurrently with `setenv` or `unsetenv` +/// +/// \return A collection of current environment variables +EnvironmentMap GZ_UTILS_VISIBLE env(); + +/// \brief Set the environment variable '_name'. +/// +/// Note: On Windows setting an empty string (_value=="") +/// is the equivalent of unsetting the variable. +// +/// Note: This function is not thread-safe and should not be called +/// concurrently with `env` or `unsetenv` +/// +/// \param[in] _vars Collection of environment variables to set +/// \return True if all variables were set or false otherwise. +bool GZ_UTILS_VISIBLE setenv(const EnvironmentMap &_vars); + +/// \brief Print the entire current environment to a string +/// +/// This prints each variable in the form KEY=VALUE\n +/// +/// Note: This function is not thread-safe and should not be called +/// concurrently with `setenv` or `unsetenv` +/// +/// \return A string containing all environment variables +std::string printenv(); + +} // namespace GZ_UTILS_VERSION_NAMESPACE } // namespace utils } // namespace gz diff --git a/src/Environment.cc b/src/Environment.cc index 63e0746..3300845 100644 --- a/src/Environment.cc +++ b/src/Environment.cc @@ -20,6 +20,8 @@ #include #include +extern char ** environ; + namespace gz { namespace utils @@ -98,6 +100,74 @@ bool unsetenv(const std::string &_name) #endif return true; } + +///////////////////////////////////////////////// +bool clearenv() +{ + ::clearenv(); + return true; + /* + bool success = true; + for (const auto &[key, value] : env()) + { + success &= unsetenv(key); + } + return success; + */ } + +///////////////////////////////////////////////// +EnvironmentMap env() +{ + // Portable method for reading environment variables + // Ref: https://stackoverflow.com/a/71483564/460065 + char **currentEnv {nullptr}; +#if defined(WIN) && (_MSC_VER >= 1900) + currentEnv = *__p_environ(); +#else + currentEnv = environ; +#endif + + // In the case that clearenv() was just called + // currentEnv will be nullptr + if (currentEnv == nullptr) + return {}; + + EnvironmentMap ret; + for (; *currentEnv; ++currentEnv) + { + std::string var(*currentEnv); + auto key = var.substr(0, var.find('=')); + var.erase(0, var.find('=') + 1); + ret[key] = var; + } + return ret; } + +///////////////////////////////////////////////// +bool setenv(const EnvironmentMap &_vars) +{ + bool success = true; + for (const auto &[key, value] : _vars) + { + success &= setenv(key, value); + } + return success; +} + +///////////////////////////////////////////////// +std::string printenv() +{ + std::string ret; + for (const auto &[key, value] : env()) + { + ret.append(key); + ret.append("="); + ret.append(value); + ret.append("\n"); + } + return ret; } +} // namespace GZ_UTILS_VERSION_NAMESPACE +} // namespace utils +} // namespace gz diff --git a/src/Environment_TEST.cc b/src/Environment_TEST.cc index 8168153..24da134 100644 --- a/src/Environment_TEST.cc +++ b/src/Environment_TEST.cc @@ -26,6 +26,8 @@ using namespace gz; ///////////////////////////////////////////////// TEST(Environment, emptyENV) { + gz::utils::clearenv(); + std::string var; EXPECT_FALSE(utils::env("!!SHOULD_NOT_EXIST!!", var)); EXPECT_TRUE(var.empty()); @@ -34,6 +36,8 @@ TEST(Environment, emptyENV) ///////////////////////////////////////////////// TEST(Environment, envSet) { + gz::utils::clearenv(); + const auto key = "GZ_ENV_SET"; ASSERT_TRUE(utils::setenv(key, "VALUE")); @@ -67,6 +71,8 @@ TEST(Environment, envSet) ///////////////////////////////////////////////// TEST(Environment, envUnset) { + gz::utils::clearenv(); + const auto key = "GZ_ENV_UNSET"; ASSERT_TRUE(utils::unsetenv(key)); @@ -94,8 +100,10 @@ TEST(Environment, envUnset) } ///////////////////////////////////////////////// -TEST(Util_TEST, envSetEmpty) +TEST(Environment, envSetEmpty) { + gz::utils::clearenv(); + const auto key = "GZ_ENV_SET_EMPTY"; ASSERT_TRUE(utils::setenv(key, "")); @@ -133,3 +141,36 @@ TEST(Util_TEST, envSetEmpty) } ASSERT_TRUE(utils::unsetenv(key)); } + +///////////////////////////////////////////////// +TEST(Environment, envGetCollection) +{ + gz::utils::clearenv(); + auto currentEnv = gz::utils::env(); + EXPECT_EQ(currentEnv.size(), 0); + + 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")); + + currentEnv = gz::utils::env(); + EXPECT_EQ(currentEnv.size(), 3); + + EXPECT_EQ(currentEnv["GZ_FOO_KEY"], "GZ_FOO_VAL"); + EXPECT_EQ(currentEnv["GZ_BAR_KEY"], "GZ_BAR_VAL"); + EXPECT_EQ(currentEnv["GZ_BAZ_KEY"], "GZ_BAZ_VAL"); +} + +///////////////////////////////////////////////// +TEST(Environment, printenv) +{ + gz::utils::clearenv(); + EXPECT_EQ(gz::utils::printenv(), ""); + + 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")); + + EXPECT_EQ(gz::utils::printenv(), + "GZ_BAZ_KEY=GZ_BAZ_VAL\nGZ_BAR_KEY=GZ_BAR_VAL\nGZ_FOO_KEY=GZ_FOO_VAL\n"); +} From 65f81d74d49616a374301a58a610420181d0d8fe Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 1 Nov 2023 15:22:19 +0000 Subject: [PATCH 02/15] Fix Windows and macOS clearenv implementation Signed-off-by: Michael Carroll --- include/gz/utils/Environment.hh | 4 ++-- src/Environment.cc | 14 ++++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/include/gz/utils/Environment.hh b/include/gz/utils/Environment.hh index a6dfa5f..9a7a062 100644 --- a/include/gz/utils/Environment.hh +++ b/include/gz/utils/Environment.hh @@ -106,8 +106,8 @@ bool GZ_UTILS_VISIBLE setenv(const EnvironmentMap &_vars); /// concurrently with `setenv` or `unsetenv` /// /// \return A string containing all environment variables -std::string printenv(); - +/// NOLINTNEXTLINE - This is incorrectly parsed as a global variable +std::string GZ_UTILS_VISIBLE printenv(); } // namespace GZ_UTILS_VERSION_NAMESPACE } // namespace utils } // namespace gz diff --git a/src/Environment.cc b/src/Environment.cc index 3300845..7c2f02f 100644 --- a/src/Environment.cc +++ b/src/Environment.cc @@ -104,16 +104,22 @@ bool unsetenv(const std::string &_name) ///////////////////////////////////////////////// bool clearenv() { - ::clearenv(); - return true; - /* bool success = true; +#if __linux__ + if (0 != ::clearenv()) + { + success = false; + } +#else + // Windows and macOS don't have clearenv + // so iterate and clear one-by-one for (const auto &[key, value] : env()) { success &= unsetenv(key); } +#endif return success; - */ + } ///////////////////////////////////////////////// From 55451cc21474ea8f2f862fef80c299b08ddcfc84 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 1 Nov 2023 16:07:56 +0000 Subject: [PATCH 03/15] Sort the environment variables Signed-off-by: Michael Carroll --- src/Environment.cc | 10 +++++++++- src/Environment_TEST.cc | 3 ++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Environment.cc b/src/Environment.cc index 7c2f02f..b18824f 100644 --- a/src/Environment.cc +++ b/src/Environment.cc @@ -17,8 +17,10 @@ #include +#include #include #include +#include extern char ** environ; @@ -165,7 +167,13 @@ bool setenv(const EnvironmentMap &_vars) std::string printenv() { std::string ret; - for (const auto &[key, value] : env()) + // Variables are in an unordered_map as we generally don't + // care, but for printing sort for consistent display + auto currentEnv = env(); + auto sorted = std::vector>( + currentEnv.begin(), currentEnv.end()); + std::sort(sorted.begin(), sorted.end()); + for (const auto &[key, value] : sorted) { ret.append(key); ret.append("="); diff --git a/src/Environment_TEST.cc b/src/Environment_TEST.cc index 24da134..6d09923 100644 --- a/src/Environment_TEST.cc +++ b/src/Environment_TEST.cc @@ -171,6 +171,7 @@ TEST(Environment, printenv) ASSERT_TRUE(gz::utils::setenv("GZ_BAR_KEY", "GZ_BAR_VAL")); ASSERT_TRUE(gz::utils::setenv("GZ_BAZ_KEY", "GZ_BAZ_VAL")); + // Always returned in sorted order EXPECT_EQ(gz::utils::printenv(), - "GZ_BAZ_KEY=GZ_BAZ_VAL\nGZ_BAR_KEY=GZ_BAR_VAL\nGZ_FOO_KEY=GZ_FOO_VAL\n"); + "GZ_BAR_KEY=GZ_BAR_VAL\nGZ_BAZ_KEY=GZ_BAZ_VAL\nGZ_FOO_KEY=GZ_FOO_VAL\n"); } From 9d7eb95eb76d65e9d9ef17785e600adc2005fb3d Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 1 Nov 2023 16:30:22 +0000 Subject: [PATCH 04/15] Avoid __p_error on Windows Signed-off-by: Michael Carroll --- src/Environment.cc | 45 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/src/Environment.cc b/src/Environment.cc index b18824f..a0f62ba 100644 --- a/src/Environment.cc +++ b/src/Environment.cc @@ -127,21 +127,47 @@ bool clearenv() ///////////////////////////////////////////////// EnvironmentMap env() { - // Portable method for reading environment variables - // Ref: https://stackoverflow.com/a/71483564/460065 - char **currentEnv {nullptr}; -#if defined(WIN) && (_MSC_VER >= 1900) - currentEnv = *__p_environ(); -#else - currentEnv = environ; -#endif + EnvironmentMap ret; +#ifdef _WIN32 + DWORD environment_block_size = GetEnvironmentBlockSize(); + + // Allocate a buffer to store the environment block. + LPCH env_buf = (LPCH)malloc(environment_block_size); + if (env_buf == nullptr) { + return {}; + } + // Get the environment block. + if (!GetEnvironmentVariables(env_buf, environment_block_size)) { + free(env_buf); + return {}; + } + // Parse the environment block into an unordered_map. + LPCH env_var = env_buf; + while (*env_var != '\0') { + // Split the environment variable into a key-value pair. + char* equal_sign = strchr(env_var, '='); + if (equal_sign == nullptr) { + continue; + } + + // Get the key and value of the environment variable. + std::string key(env_var, equal_sign - env_var); + std::string value(equal_sign + 1); + + // Add the key-value pair to the unordered_map. + ret[key] = value; + // Advance to the next environment variable. + env_var = equal_sign + 1; + } + free(env_buf); +#else + char **currentEnv = environ; // In the case that clearenv() was just called // currentEnv will be nullptr if (currentEnv == nullptr) return {}; - EnvironmentMap ret; for (; *currentEnv; ++currentEnv) { std::string var(*currentEnv); @@ -149,6 +175,7 @@ EnvironmentMap env() var.erase(0, var.find('=') + 1); ret[key] = var; } +#endif return ret; } From c0f05423790f437b5d6fce989694e686f1207b17 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 1 Nov 2023 16:38:29 +0000 Subject: [PATCH 05/15] Fix headers Signed-off-by: Michael Carroll --- src/Environment.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Environment.cc b/src/Environment.cc index a0f62ba..ba4a239 100644 --- a/src/Environment.cc +++ b/src/Environment.cc @@ -22,7 +22,14 @@ #include #include +#ifdef _WIN32 +#include +#include +#endif + +#ifndef _WIN32 extern char ** environ; +#endif namespace gz { From 1756d83b07e28ea0a9f3f55bbc27f7551a4912b8 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 1 Nov 2023 16:50:50 +0000 Subject: [PATCH 06/15] Back to original implementation Signed-off-by: Michael Carroll --- src/Environment.cc | 49 ++++++++-------------------------------------- 1 file changed, 8 insertions(+), 41 deletions(-) diff --git a/src/Environment.cc b/src/Environment.cc index ba4a239..e3108e5 100644 --- a/src/Environment.cc +++ b/src/Environment.cc @@ -22,12 +22,7 @@ #include #include -#ifdef _WIN32 -#include -#include -#endif - -#ifndef _WIN32 +#ifndef _win32 extern char ** environ; #endif @@ -134,47 +129,20 @@ bool clearenv() ///////////////////////////////////////////////// EnvironmentMap env() { - EnvironmentMap ret; + // Portable method for reading environment variables + // Ref: https://stackoverflow.com/a/71483564/460065 + char **currentEnv {nullptr}; #ifdef _WIN32 - DWORD environment_block_size = GetEnvironmentBlockSize(); - - // Allocate a buffer to store the environment block. - LPCH env_buf = (LPCH)malloc(environment_block_size); - if (env_buf == nullptr) { - return {}; - } - // Get the environment block. - if (!GetEnvironmentVariables(env_buf, environment_block_size)) { - free(env_buf); - return {}; - } - // Parse the environment block into an unordered_map. - LPCH env_var = env_buf; - while (*env_var != '\0') { - // Split the environment variable into a key-value pair. - char* equal_sign = strchr(env_var, '='); - if (equal_sign == nullptr) { - continue; - } - - // Get the key and value of the environment variable. - std::string key(env_var, equal_sign - env_var); - std::string value(equal_sign + 1); - - // Add the key-value pair to the unordered_map. - ret[key] = value; - - // Advance to the next environment variable. - env_var = equal_sign + 1; - } - free(env_buf); + currentEnv = *__p_environ(); #else - char **currentEnv = environ; + currentEnv = environ; +#endif // In the case that clearenv() was just called // currentEnv will be nullptr if (currentEnv == nullptr) return {}; + EnvironmentMap ret; for (; *currentEnv; ++currentEnv) { std::string var(*currentEnv); @@ -182,7 +150,6 @@ EnvironmentMap env() var.erase(0, var.find('=') + 1); ret[key] = var; } -#endif return ret; } From 72b193b88af9734fdafdc70b6e74af7b388be177 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 1 Nov 2023 19:21:29 +0000 Subject: [PATCH 07/15] Fix Windows implementation Signed-off-by: Michael Carroll --- src/Environment.cc | 42 ++++++++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/src/Environment.cc b/src/Environment.cc index e3108e5..18858a5 100644 --- a/src/Environment.cc +++ b/src/Environment.cc @@ -22,7 +22,12 @@ #include #include -#ifndef _win32 +#ifdef _WIN32 +#include +#include +#endif + +#ifndef _WIN32 extern char ** environ; #endif @@ -129,27 +134,40 @@ bool clearenv() ///////////////////////////////////////////////// EnvironmentMap env() { - // Portable method for reading environment variables - // Ref: https://stackoverflow.com/a/71483564/460065 - char **currentEnv {nullptr}; + EnvironmentMap ret; + + // Helper function to split KEY=VAL + auto split = [](const std::string &_inp) + { + return std::make_pair( + _inp.substr(0, _inp.find('=')), + _inp.substr(_inp.find('='))); + }; + #ifdef _WIN32 - currentEnv = *__p_environ(); + LPCH currentEnv = GetEnvironmentStrings(); + if (currentEnv == nullptr) + return {}; + + LPCH env_var = env_buf; + while (*env_var != '\0') + { + ret.emplace(split(*env_var)); + env_var += strlen(env_var) + 1; + } + FreeEnvironmentStrings(currentEnv); #else - currentEnv = environ; -#endif + char **currentEnv = environ; // In the case that clearenv() was just called // currentEnv will be nullptr if (currentEnv == nullptr) return {}; - EnvironmentMap ret; for (; *currentEnv; ++currentEnv) { - std::string var(*currentEnv); - auto key = var.substr(0, var.find('=')); - var.erase(0, var.find('=') + 1); - ret[key] = var; + ret.emplace(split(*currentEnv)); } +#endif return ret; } From e153ac9760d5390bc93b8f46c7d1264b419de65e Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 1 Nov 2023 19:37:23 +0000 Subject: [PATCH 08/15] Off by one Signed-off-by: Michael Carroll --- src/Environment.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Environment.cc b/src/Environment.cc index 18858a5..e06ab88 100644 --- a/src/Environment.cc +++ b/src/Environment.cc @@ -141,7 +141,7 @@ EnvironmentMap env() { return std::make_pair( _inp.substr(0, _inp.find('=')), - _inp.substr(_inp.find('='))); + _inp.substr(_inp.find('=') + 1)); }; #ifdef _WIN32 From 0d549f83ddc9c4af756e2292de53271816e87d57 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 1 Nov 2023 20:20:26 +0000 Subject: [PATCH 09/15] One more try Signed-off-by: Michael Carroll --- src/Environment.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Environment.cc b/src/Environment.cc index e06ab88..0aede95 100644 --- a/src/Environment.cc +++ b/src/Environment.cc @@ -149,10 +149,10 @@ EnvironmentMap env() if (currentEnv == nullptr) return {}; - LPCH env_var = env_buf; + LPCH env_var = currentEnv; while (*env_var != '\0') { - ret.emplace(split(*env_var)); + ret.emplace(split(env_var)); env_var += strlen(env_var) + 1; } FreeEnvironmentStrings(currentEnv); From b9fc005feafa29154a8ba7b5094b5ed64f6bf749 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Thu, 2 Nov 2023 14:48:00 +0000 Subject: [PATCH 10/15] Turns out I was making this too complicated Signed-off-by: Michael Carroll --- src/Environment.cc | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/src/Environment.cc b/src/Environment.cc index 0aede95..4d86e0b 100644 --- a/src/Environment.cc +++ b/src/Environment.cc @@ -144,20 +144,12 @@ EnvironmentMap env() _inp.substr(_inp.find('=') + 1)); }; + char **currentEnv = nullptr; #ifdef _WIN32 - LPCH currentEnv = GetEnvironmentStrings(); - if (currentEnv == nullptr) - return {}; - - LPCH env_var = currentEnv; - while (*env_var != '\0') - { - ret.emplace(split(env_var)); - env_var += strlen(env_var) + 1; - } - FreeEnvironmentStrings(currentEnv); + currentEnv = *__p__environ(); #else - char **currentEnv = environ; + currentEnv = environ; +#endif // In the case that clearenv() was just called // currentEnv will be nullptr if (currentEnv == nullptr) @@ -167,7 +159,6 @@ EnvironmentMap env() { ret.emplace(split(*currentEnv)); } -#endif return ret; } From a0a1c47cd03ec85b610ec229f29f064359629f58 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Tue, 31 Oct 2023 22:41:01 +0000 Subject: [PATCH 11/15] Make the single argument constructor inherit the env Signed-off-by: Michael Carroll --- include/gz/utils/Subprocess.hh | 84 ++++++++++++++----- .../integration/subprocess/subprocess_main.cc | 11 +-- test/integration/subprocess_TEST.cc | 59 ++++++++++--- 3 files changed, 112 insertions(+), 42 deletions(-) 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")); } } From 8b36418550dea46bc590cb624c5ad1eafd779b01 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Tue, 7 Nov 2023 22:42:22 +0000 Subject: [PATCH 12/15] Tweak subprocess test Signed-off-by: Michael Carroll --- test/integration/subprocess/subprocess_main.cc | 10 ++++++++-- test/integration/subprocess_TEST.cc | 8 +++++--- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/test/integration/subprocess/subprocess_main.cc b/test/integration/subprocess/subprocess_main.cc index e903e3e..d3afa05 100644 --- a/test/integration/subprocess/subprocess_main.cc +++ b/test/integration/subprocess/subprocess_main.cc @@ -34,7 +34,6 @@ class OutputSink if (dest == "cout" || dest == "both") { std::cout << val << std::endl; - } else if (dest == "cerr" || dest == "both") { @@ -59,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); @@ -68,6 +71,9 @@ int main(int argc, char **argv) std::this_thread::sleep_for(std::chrono::milliseconds(iter_ms)); } - sink.Write(gz::utils::printenv()); + if (environment) + { + sink.Write(gz::utils::printenv()); + } return 0; } diff --git a/test/integration/subprocess_TEST.cc b/test/integration/subprocess_TEST.cc index ef539d9..911ac60 100644 --- a/test/integration/subprocess_TEST.cc +++ b/test/integration/subprocess_TEST.cc @@ -124,7 +124,7 @@ TEST(Subprocess, Environment) { // Default behavior is to inherit the environment - auto proc = Subprocess({kExecutablePath, "--output=cout"}); + auto proc = Subprocess({kExecutablePath, "--output=cout", "--environment"}); // Block until the executable is done auto ret = proc.Join(); EXPECT_EQ(0u, ret); @@ -137,7 +137,8 @@ TEST(Subprocess, Environment) { // Passing an empty map as the second arg clears the environment - auto proc = Subprocess({kExecutablePath, "--output=cout"}, {}); + auto proc = Subprocess( + {kExecutablePath, "--output=cout", "--environment"}, {}); // Block until the executable is done auto ret = proc.Join(); EXPECT_EQ(0u, ret); @@ -150,7 +151,8 @@ TEST(Subprocess, Environment) { // Passing a map sets those variables, clearing the rest - auto proc = Subprocess({kExecutablePath, "--output=cout"}, { + auto proc = Subprocess( + {kExecutablePath, "--output=cout", "--environment"}, { {"QUX_KEY", "QUX_VAL"} }); // Block until the executable is done From 4280e468ae5255989fb947ea81ea4bd0f072653b Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 8 Nov 2023 15:52:56 +0000 Subject: [PATCH 13/15] Add helper functions and delegating constructor Signed-off-by: Michael Carroll --- include/gz/utils/Environment.hh | 18 ++++++++++ include/gz/utils/Subprocess.hh | 16 +++++++++ src/Environment.cc | 54 ++++++++++++++++++----------- src/Environment_TEST.cc | 36 +++++++++++++++++++ test/integration/subprocess_TEST.cc | 44 ++++++++++++++++++++--- 5 files changed, 144 insertions(+), 24 deletions(-) diff --git a/include/gz/utils/Environment.hh b/include/gz/utils/Environment.hh index 9a7a062..c282920 100644 --- a/include/gz/utils/Environment.hh +++ b/include/gz/utils/Environment.hh @@ -23,6 +23,7 @@ #include #include +#include namespace gz { @@ -78,6 +79,22 @@ bool GZ_UTILS_VISIBLE clearenv(); /// \brief Type alias for a collection of environment variables using EnvironmentMap = std::unordered_map; +/// \brief Type alias for a collection of environment variables +/// Each entry is of the form KEY=VAL +using EnvironmentStrings = std::vector; + +/// \brief Convert a vector of environment variables to a map +/// +/// \param[in] _envStrings Vector collection of environment variables. +/// \return Mapped collection of environment variables. +EnvironmentMap GZ_UTILS_VISIBLE envStringsToMap(const EnvironmentStrings &_envStrings); + +/// \brief Convert a map of environment variables to a vector +/// +/// \param[in] _envMap Collection of mapped environment variables. +/// \return Vector collection of environment variables. +EnvironmentStrings GZ_UTILS_VISIBLE envMapToStrings(const EnvironmentMap &_envMap); + /// \brief Retrieve all current environment variables /// /// Note: This function is not thread-safe and should not be called @@ -108,6 +125,7 @@ bool GZ_UTILS_VISIBLE setenv(const EnvironmentMap &_vars); /// \return A string containing all environment variables /// NOLINTNEXTLINE - This is incorrectly parsed as a global variable std::string GZ_UTILS_VISIBLE printenv(); + } // namespace GZ_UTILS_VERSION_NAMESPACE } // namespace utils } // namespace gz diff --git a/include/gz/utils/Subprocess.hh b/include/gz/utils/Subprocess.hh index dee00fd..843879d 100644 --- a/include/gz/utils/Subprocess.hh +++ b/include/gz/utils/Subprocess.hh @@ -72,6 +72,22 @@ class Subprocess 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 gz::utils::EnvironmentStrings &_environment): + Subprocess(_commandLine, gz::utils::envStringsToMap(_environment)) + { + } + + private: void Create() { if (this->process != nullptr) diff --git a/src/Environment.cc b/src/Environment.cc index 4d86e0b..a5b47e8 100644 --- a/src/Environment.cc +++ b/src/Environment.cc @@ -136,14 +136,6 @@ EnvironmentMap env() { EnvironmentMap ret; - // Helper function to split KEY=VAL - auto split = [](const std::string &_inp) - { - return std::make_pair( - _inp.substr(0, _inp.find('=')), - _inp.substr(_inp.find('=') + 1)); - }; - char **currentEnv = nullptr; #ifdef _WIN32 currentEnv = *__p__environ(); @@ -155,11 +147,12 @@ EnvironmentMap env() if (currentEnv == nullptr) return {}; + std::vector envStrings; for (; *currentEnv; ++currentEnv) { - ret.emplace(split(*currentEnv)); + envStrings.emplace_back(*currentEnv); } - return ret; + return envStringsToMap(envStrings); } ///////////////////////////////////////////////// @@ -174,20 +167,41 @@ bool setenv(const EnvironmentMap &_vars) } ///////////////////////////////////////////////// -std::string printenv() +EnvironmentMap envStringsToMap(const EnvironmentStrings &_envStrings) { - std::string ret; - // Variables are in an unordered_map as we generally don't - // care, but for printing sort for consistent display - auto currentEnv = env(); + EnvironmentMap ret; + for (const auto &pair : _envStrings) + { + auto eqPos = pair.find('='); + if (eqPos != std::string::npos) + { + ret.emplace(pair.substr(0, eqPos), pair.substr(eqPos + 1)); + } + } + return ret; +} + +///////////////////////////////////////////////// +EnvironmentStrings envMapToStrings(const EnvironmentMap &_envMap) +{ + EnvironmentStrings ret; auto sorted = std::vector>( - currentEnv.begin(), currentEnv.end()); + _envMap.begin(), _envMap.end()); std::sort(sorted.begin(), sorted.end()); - for (const auto &[key, value] : sorted) + for (auto [key, value] : sorted) + { + ret.push_back(key + "=" + value); + } + return ret; +} + +///////////////////////////////////////////////// +std::string printenv() +{ + std::string ret; + for (const auto &entry : envMapToStrings(env())) { - ret.append(key); - ret.append("="); - ret.append(value); + ret.append(entry); ret.append("\n"); } return ret; diff --git a/src/Environment_TEST.cc b/src/Environment_TEST.cc index 6d09923..496dddb 100644 --- a/src/Environment_TEST.cc +++ b/src/Environment_TEST.cc @@ -175,3 +175,39 @@ TEST(Environment, printenv) EXPECT_EQ(gz::utils::printenv(), "GZ_BAR_KEY=GZ_BAR_VAL\nGZ_BAZ_KEY=GZ_BAZ_VAL\nGZ_FOO_KEY=GZ_FOO_VAL\n"); } + +///////////////////////////////////////////////// +TEST(Environment, envStringsToMap) +{ + gz::utils::EnvironmentStrings strings; + strings.emplace_back("GZ_FOO_KEY=GZ_FOO_VAL"); + strings.emplace_back("GZ_BAR_KEY=GZ_BAR_VAL"); + strings.emplace_back("GZ_BAZ_KEY=GZ_BAZ_VAL"); + strings.emplace_back("BAD_KEY"); + + { + auto envMap = gz::utils::envStringsToMap(strings); + EXPECT_EQ(3u, envMap.size()); + EXPECT_EQ("GZ_FOO_VAL", envMap["GZ_FOO_KEY"]); + EXPECT_EQ("GZ_BAR_VAL", envMap["GZ_BAR_KEY"]); + EXPECT_EQ("GZ_BAZ_VAL", envMap["GZ_BAZ_KEY"]); + } +} + +///////////////////////////////////////////////// +TEST(Environment, envMapToStrings) +{ + gz::utils::EnvironmentMap envMap; + envMap.insert({{"GZ_FOO_KEY"}, {"GZ_FOO_VAL"}}); + envMap.insert({{"GZ_BAR_KEY"}, {"GZ_BAR_VAL"}}); + envMap.insert({{"GZ_BAZ_KEY"}, {"GZ_BAZ_VAL"}}); + + { + auto envStrings = gz::utils::envMapToStrings(envMap); + + EXPECT_EQ(3u, envStrings.size()); + EXPECT_EQ("GZ_BAR_KEY=GZ_BAR_VAL", envStrings[0]); + EXPECT_EQ("GZ_BAZ_KEY=GZ_BAZ_VAL", envStrings[1]); + EXPECT_EQ("GZ_FOO_KEY=GZ_FOO_VAL", envStrings[2]); + } +} diff --git a/test/integration/subprocess_TEST.cc b/test/integration/subprocess_TEST.cc index 911ac60..47c8976 100644 --- a/test/integration/subprocess_TEST.cc +++ b/test/integration/subprocess_TEST.cc @@ -138,7 +138,23 @@ TEST(Subprocess, Environment) { // Passing an empty map as the second arg clears the environment auto proc = Subprocess( - {kExecutablePath, "--output=cout", "--environment"}, {}); + {kExecutablePath, "--output=cout", "--environment"}, + gz::utils::EnvironmentMap()); + // Block until the executable is done + auto ret = proc.Join(); + EXPECT_EQ(0u, ret); + + 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")); + } + + { + // Passing an empty vector as the second arg clears the environment + auto proc = Subprocess( + {kExecutablePath, "--output=cout", "--environment"}, + gz::utils::EnvironmentStrings()); // Block until the executable is done auto ret = proc.Join(); EXPECT_EQ(0u, ret); @@ -152,9 +168,10 @@ TEST(Subprocess, Environment) { // Passing a map sets those variables, clearing the rest auto proc = Subprocess( - {kExecutablePath, "--output=cout", "--environment"}, { - {"QUX_KEY", "QUX_VAL"} - }); + {kExecutablePath, "--output=cout", "--environment"}, + gz::utils::EnvironmentMap{ + {"QUX_KEY", "QUX_VAL"} + }); // Block until the executable is done auto ret = proc.Join(); EXPECT_EQ(0u, ret); @@ -165,4 +182,23 @@ TEST(Subprocess, Environment) EXPECT_EQ(std::string::npos, cout.find("GZ_BAZ_KEY=GZ_BAZ_VAL")); EXPECT_NE(std::string::npos, cout.find("QUX_KEY=QUX_VAL")); } + + { + // Passing a map sets those variables, clearing the rest + auto proc = Subprocess( + {kExecutablePath, "--output=cout", "--environment"}, + gz::utils::EnvironmentStrings{ + {"QUX_KEY=QUX_VAL"} + }); + // Block until the executable is done + auto ret = proc.Join(); + EXPECT_EQ(0u, ret); + + 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")); + EXPECT_NE(std::string::npos, cout.find("QUX_KEY=QUX_VAL")); + } + } From 2ffb5f46c2dac545828c82e7cd5babe68189e65f Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 8 Nov 2023 15:57:32 +0000 Subject: [PATCH 14/15] Lint Signed-off-by: Michael Carroll --- include/gz/utils/Environment.hh | 9 +++++---- src/Environment.cc | 1 - 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/gz/utils/Environment.hh b/include/gz/utils/Environment.hh index c282920..3302f88 100644 --- a/include/gz/utils/Environment.hh +++ b/include/gz/utils/Environment.hh @@ -85,15 +85,17 @@ using EnvironmentStrings = std::vector; /// \brief Convert a vector of environment variables to a map /// -/// \param[in] _envStrings Vector collection of environment variables. +/// \param[in] _envStrings Vector collection of environment variables /// \return Mapped collection of environment variables. -EnvironmentMap GZ_UTILS_VISIBLE envStringsToMap(const EnvironmentStrings &_envStrings); +EnvironmentMap GZ_UTILS_VISIBLE envStringsToMap( + const EnvironmentStrings &_envStrings); /// \brief Convert a map of environment variables to a vector /// /// \param[in] _envMap Collection of mapped environment variables. /// \return Vector collection of environment variables. -EnvironmentStrings GZ_UTILS_VISIBLE envMapToStrings(const EnvironmentMap &_envMap); +EnvironmentStrings GZ_UTILS_VISIBLE envMapToStrings( + const EnvironmentMap &_envMap); /// \brief Retrieve all current environment variables /// @@ -125,7 +127,6 @@ bool GZ_UTILS_VISIBLE setenv(const EnvironmentMap &_vars); /// \return A string containing all environment variables /// NOLINTNEXTLINE - This is incorrectly parsed as a global variable std::string GZ_UTILS_VISIBLE printenv(); - } // namespace GZ_UTILS_VERSION_NAMESPACE } // namespace utils } // namespace gz diff --git a/src/Environment.cc b/src/Environment.cc index a5b47e8..d477414 100644 --- a/src/Environment.cc +++ b/src/Environment.cc @@ -128,7 +128,6 @@ bool clearenv() } #endif return success; - } ///////////////////////////////////////////////// From a81031c4726ec4249fa62c6db52ffd6f178cc2ad Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 8 Nov 2023 16:16:36 +0000 Subject: [PATCH 15/15] Use helper function Signed-off-by: Michael Carroll --- include/gz/utils/Subprocess.hh | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/include/gz/utils/Subprocess.hh b/include/gz/utils/Subprocess.hh index 843879d..f7f9fab 100644 --- a/include/gz/utils/Subprocess.hh +++ b/include/gz/utils/Subprocess.hh @@ -95,7 +95,7 @@ class Subprocess this->process = new subprocess_s; - std::vector environmentStr; + auto environmentStr = gz::utils::envMapToStrings(this->environment); std::vector environmentCstr; std::vector commandLineCstr; @@ -107,10 +107,9 @@ class Subprocess if (!this->inheritEnvironment) { - for (const auto &[key, val] : this->environment) + for (const auto &val : environmentStr) { - environmentStr.push_back(key + "=" + val); - environmentCstr.push_back(environmentStr.back().c_str()); + environmentCstr.push_back(val.c_str()); } environmentCstr.push_back(nullptr); }