Fix env behavior to return true on empty vars

include/ignition/common/Util.hh

@@ -215,12 +215,41 @@ namespace ignition
     #endif
 
     /// \brief Find the environment variable '_name' and return its value.
+    ///
+    /// \TODO(mjcarroll): Deprecate and remove in tick-tock.
+    ///
     /// \param[in] _name Name of the environment variable.
     /// \param[out] _value Value if the variable was found.
     /// \return True if the variable was found or false otherwise.
     bool IGNITION_COMMON_VISIBLE env(
         const std::string &_name, std::string &_value);
 
+    /// \brief Find the environment variable '_name' and return its value.
+    /// \param[in] _name Name of the environment variable.
+    /// \param[out] _value Value if the variable was found.
+    /// \param[in] _allowEmpty Allow set-but-empty variables.
+    ///           (Unsupported on Windows)
+    /// \return True if the variable was found or false otherwise.
+    bool IGNITION_COMMON_VISIBLE env(
+        const std::string &_name, std::string &_value,
+        bool _allowEmpty);
+
+    /// \brief Set the environment variable '_name'.
+    ///
+    /// Note that on Windows setting an empty string (_value=="")
+    /// is the equivalent of unsetting the variable.
+    ///
+    /// \param[in] _name Name of the environment variable.
+    /// \param[in] _value Value of the variable to be set.
+    /// \return True if the variable was set or false otherwise.
+    bool IGNITION_COMMON_VISIBLE setenv(
+        const std::string &_name, const std::string &_value);
+
+    /// \brief Unset the environment variable '_name'.
+    /// \param[in] _name Name of the environment variable.
+    /// \return True if the variable was unset or false otherwise.
+    bool IGNITION_COMMON_VISIBLE unsetenv(const std::string &_name);
+
     /// \brief Get a UUID
     /// \return A UUID string
     std::string IGNITION_COMMON_VISIBLE uuid();

src/Util.cc

@@ -31,6 +31,7 @@
 #include <sstream>
 
 #include <ignition/common/config.hh>
+#include <ignition/common/Console.hh>
 #include <ignition/common/SystemPaths.hh>
 #include <ignition/common/Util.hh>
 #include <ignition/common/Uuid.hh>
@@ -321,28 +322,103 @@ ignition::common::SystemPaths *ignition::common::systemPaths()
 
 /////////////////////////////////////////////////
 bool ignition::common::env(const std::string &_name, std::string &_value)
+{
+  return env(_name, _value, false);
+}
+
+/////////////////////////////////////////////////
+bool ignition::common::env(const std::string &_name,
+                           std::string &_value,
+                           bool _allowEmpty)
 {
   std::string v;
+  bool valid = false;
 #ifdef _WIN32
   const DWORD buffSize = 32767;
   static char buffer[buffSize];
   if (GetEnvironmentVariable(_name.c_str(), buffer, buffSize))
   {
     v = buffer;
   }
+
+  if (!v.empty())
+  {
+    valid = true;
+  }
+
+  if (_allowEmpty)
+  {
+    ignwarn << "Reading environment variable with _allowEmpty==true"
+            << " is unsupported on Windows.\n";
+  }
+
 #else
   const char *cvar = std::getenv(_name.c_str());
-  if (cvar)
+  if (cvar != nullptr)
+  {
     v = cvar;
+    valid = true;
+
+    if (v[0] == '\0' && !_allowEmpty)
+    {
+      valid = false;
+    }
+  }
 #endif
-  if (!v.empty())
+  if (valid)
   {
     _value = v;
     return true;
   }
   return false;
 }
 
+/////////////////////////////////////////////////
+bool ignition::common::setenv(const std::string &_name,
+                              const std::string &_value)
+{
+#ifdef _WIN32
+  if (0 != _putenv_s(_name.c_str(), _value.c_str()))
+  {
+    ignwarn << "Failed to set environment variable: "
+            << "[" << _name << "]"
+            << strerror(errno) << std::endl;
+    return false;
+  }
+#else
+  if (0 != ::setenv(_name.c_str(), _value.c_str(), true))
+  {
+    ignwarn << "Failed to set environment variable: "
+            << "[" << _name << "]"
+            << strerror(errno) << std::endl;
+    return false;
+  }
+#endif
+  return true;
+}
+/////////////////////////////////////////////////
+bool ignition::common::unsetenv(const std::string &_name)
+{
+#ifdef _WIN32
+  if (0 != _putenv_s(_name.c_str(), ""))
+  {
+    ignwarn << "Failed to unset environment variable: "
+            << "[" << _name << "]"
+            << strerror(errno) << std::endl;
+    return false;
+  }
+#else
+  if (0 != ::unsetenv(_name.c_str()))
+  {
+    ignwarn << "Failed to unset environment variable: "
+            << "[" << _name << "]"
+            << strerror(errno) << std::endl;
+    return false;
+  }
+#endif
+  return true;
+}
+
 /////////////////////////////////////////////////
 std::string ignition::common::sha1(void const *_buffer, std::size_t _byteCount)
 {

src/Util_TEST.cc

@@ -172,6 +172,109 @@ TEST(Util_TEST, emptyENV)
   EXPECT_TRUE(var.empty());
 }
 
+/////////////////////////////////////////////////
+TEST(Util_TEST, envSet)
+{
+  const auto key = "IGN_ENV_SET";
+  ASSERT_TRUE(common::setenv(key, "VALUE"));
+
+  // Check set var
+  {
+    std::string value;
+    EXPECT_TRUE(common::env(key, value));
+    EXPECT_FALSE(value.empty());
+    EXPECT_EQ("VALUE", value);
+  }
+
+  // Check set var with allowEmpty
+  {
+    std::string value;
+    EXPECT_TRUE(common::env(key, value, true));
+    EXPECT_FALSE(value.empty());
+    EXPECT_EQ("VALUE", value);
+  }
+
+  // Check set var without allowEmpty
+  {
+    std::string value;
+    EXPECT_TRUE(common::env(key, value, false));
+    EXPECT_FALSE(value.empty());
+    EXPECT_EQ("VALUE", value);
+  }
+
+  ASSERT_TRUE(common::unsetenv(key));
+}
+
+/////////////////////////////////////////////////
+TEST(Util_TEST, envUnset)
+{
+  const auto key = "IGN_ENV_UNSET";
+  ASSERT_TRUE(common::unsetenv(key));
+
+  // Check unset var (default)
+  {
+    std::string value;
+    EXPECT_FALSE(common::env(key, value));
+    EXPECT_TRUE(value.empty());
+  }
+
+  // Check unset var with allowEmpty
+  {
+    std::string value;
+    EXPECT_FALSE(common::env(key, value, true));
+    EXPECT_TRUE(value.empty());
+  }
+
+  // Check unset var without allowEmpty
+  {
+    std::string value;
+    EXPECT_FALSE(common::env(key, value, false));
+    EXPECT_TRUE(value.empty());
+  }
+  ASSERT_TRUE(common::unsetenv(key));
+}
+
+/////////////////////////////////////////////////
+TEST(Util_TEST, envSetEmpty)
+{
+  const auto key = "IGN_ENV_SET_EMPTY";
+
+  ASSERT_TRUE(common::setenv(key, ""));
+  ASSERT_FALSE(common::setenv("", ""));
+
+  // Check set empty var (default)
+  {
+    std::string value;
+    EXPECT_FALSE(common::env(key, value));
+    EXPECT_TRUE(value.empty());
+  }
+
+#ifdef _WIN32
+  {
+    // This will warn on Windows, but return false
+    std::string value;
+    EXPECT_FALSE(common::env(key, value, true));
+    EXPECT_TRUE(value.empty());
+  }
+#else
+  {
+    // This will not warn and return true on Linux,
+    // as empty environment variables are allowed.
+    std::string value;
+    EXPECT_TRUE(common::env(key, value, true));
+    EXPECT_TRUE(value.empty());
+  }
+#endif
+
+  // Check set empty var without allowEmpty
+  {
+    std::string value;
+    EXPECT_FALSE(common::env(key, value, false));
+    EXPECT_TRUE(value.empty());
+  }
+  ASSERT_TRUE(common::unsetenv(key));
+}
+
 /////////////////////////////////////////////////
 TEST(Util_TEST, findFile)
 {