From dfc3f10912833a0d1b25f92d1016d35efe82d239 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 21 Oct 2020 13:17:55 -0500 Subject: [PATCH] [ign-common3] Miscellaneous Cleanups (#107) * Use variable substitution to prevent forward port issues * Add string Join functionality * Replace all uses of env with Ignition functions Signed-off-by: Michael Carroll --- examples/CMakeLists.txt | 11 +-- include/ignition/common/StringUtils.hh | 24 +++++++ src/Filesystem_TEST.cc | 10 ++- src/StringUtils.cc | 24 +++++++ src/StringUtils_TEST.cc | 53 ++++++++++++++ src/SystemPaths_TEST.cc | 97 +++++++++----------------- 6 files changed, 144 insertions(+), 75 deletions(-) diff --git a/examples/CMakeLists.txt b/examples/CMakeLists.txt index 24b3db132..570041193 100644 --- a/examples/CMakeLists.txt +++ b/examples/CMakeLists.txt @@ -1,17 +1,18 @@ cmake_minimum_required(VERSION 2.8 FATAL_ERROR) # Find the ignition-common library -find_package(ignition-common4 QUIET REQUIRED COMPONENTS events profiler) +set(IGN_COMMON_VER 4) +find_package(ignition-common${IGN_COMMON_VER} QUIET REQUIRED COMPONENTS events profiler) add_executable(assert_example assert_example.cc) -target_link_libraries(assert_example ignition-common4::core) +target_link_libraries(assert_example ignition-common${IGN_COMMON_VER}::core) add_executable(console_example console.cc) -target_link_libraries(console_example ignition-common4::core) +target_link_libraries(console_example ignition-common${IGN_COMMON_VER}::core) add_executable(events_example events.cc) -target_link_libraries(events_example ignition-common4::events) +target_link_libraries(events_example ignition-common${IGN_COMMON_VER}::events) add_executable(profiler_example profiler.cc) -target_link_libraries(profiler_example ignition-common4::profiler) +target_link_libraries(profiler_example ignition-common${IGN_COMMON_VER}::profiler) target_compile_definitions(profiler_example PUBLIC "IGN_PROFILER_ENABLE=1") diff --git a/include/ignition/common/StringUtils.hh b/include/ignition/common/StringUtils.hh index 9836e40ff..6075fc935 100644 --- a/include/ignition/common/StringUtils.hh +++ b/include/ignition/common/StringUtils.hh @@ -33,6 +33,30 @@ namespace ignition std::vector IGNITION_COMMON_VISIBLE Split( const std::string &_orig, char _delim); + /// \brief Join a sequence of strings with a delimiter + /// + /// Note that this will skip any empty entries in the vector, + /// and will also not prepend or append the delimiter to the + /// final output string + /// + /// \param[in] _orig The input sequence of strings + /// \param[in] _delim a string delimiter to join the string with + /// \returns a single string composed of strings joined with the delimiter + std::string IGNITION_COMMON_VISIBLE Join( + const std::vector &_orig, const std::string &_delim); + + /// \brief Join a sequence of strings with a delimiter + /// + /// Note that this will skip any empty entries in the vector, + /// and will also not prepend or append the delimiter to the + /// final output string + /// + /// \param[in] _orig The input sequence of strings + /// \param[in] _delim a character to join the string with + /// \returns a single string composed of strings joined with the delimiter + std::string IGNITION_COMMON_VISIBLE Join( + const std::vector &_orig, char _delim); + /// \brief return true if string starts with another string /// \param[in] _s1 the string to check /// \param[in] _s2 the possible prefix diff --git a/src/Filesystem_TEST.cc b/src/Filesystem_TEST.cc index 031ef52c2..917b7a566 100644 --- a/src/Filesystem_TEST.cc +++ b/src/Filesystem_TEST.cc @@ -25,6 +25,8 @@ #include #include +#include "ignition/common/Util.hh" + // The symlink tests should always work on UNIX systems #define BUILD_SYMLINK_TESTS @@ -32,12 +34,8 @@ bool create_and_switch_to_temp_dir(std::string &_new_temp_path) { std::string tmppath; - const char *tmp = getenv("TMPDIR"); - if (tmp) - { - tmppath = std::string(tmp); - } - else + + if (!ignition::common::env("TMPDIR", tmppath)) { tmppath = std::string("/tmp"); } diff --git a/src/StringUtils.cc b/src/StringUtils.cc index f6c5434e0..ebd5562ba 100644 --- a/src/StringUtils.cc +++ b/src/StringUtils.cc @@ -38,6 +38,30 @@ namespace ignition return pieces; } + ////////////////////////////////////////////////// + std::string Join(const std::vector &_orig, + const std::string &_delim) + { + std::string ret = ""; + for (size_t ii = 0; ii < _orig.size(); ++ii) + { + if (_orig[ii].empty()) + continue; + + ret += _orig[ii]; + if (ii < _orig.size() - 1) + { + ret += _delim; + } + } + return ret; + } + ////////////////////////////////////////////////// + std::string Join(const std::vector &_orig, char _delim) + { + return Join(_orig, std::string(1, _delim)); + } + ////////////////////////////////////////////////// bool StartsWith(const std::string &_s1, const std::string &_s2) { diff --git a/src/StringUtils_TEST.cc b/src/StringUtils_TEST.cc index b8aedf993..5c7b31b48 100644 --- a/src/StringUtils_TEST.cc +++ b/src/StringUtils_TEST.cc @@ -70,6 +70,59 @@ TEST(StringUtils, SplitOneDelimiterAtEnd) EXPECT_EQ("", pieces[1]); } +///////////////////////////////////////////////// +TEST(Join, Simple) +{ + { + std::string delim = " "; + auto pieces = std::vector{"Hello", "World"}; + auto joined = common::Join(pieces, delim); + EXPECT_EQ("Hello World", joined); + } + + { + std::string delim = "x"; + auto pieces = std::vector{"Hello", "World"}; + auto joined = common::Join(pieces, delim); + EXPECT_EQ("HelloxWorld", joined); + } + + { + std::string delim = "x"; + auto pieces = std::vector{ + "foo", "bar", "baz", "bing"}; + auto joined = common::Join(pieces, delim); + EXPECT_EQ("fooxbarxbazxbing", joined); + } +} + +///////////////////////////////////////////////// +TEST(Join, EmptyDelim) +{ + std::string delim = ""; + auto pieces = std::vector{"Hello", "World"}; + auto joined = common::Join(pieces, delim); + EXPECT_EQ("HelloWorld", joined); +} + +///////////////////////////////////////////////// +TEST(Join, EmptyPart) +{ + std::string delim = " "; + auto pieces = std::vector{"Hello", "", "World"}; + auto joined = common::Join(pieces, delim); + EXPECT_EQ("Hello World", joined); +} + +///////////////////////////////////////////////// +TEST(Join, EmptyParts) +{ + std::string delim = "x"; + auto pieces = std::vector{}; + auto joined = common::Join(pieces, delim); + EXPECT_EQ("", joined); +} + ///////////////////////////////////////////////// TEST(StartsWith, NotInString) { diff --git a/src/SystemPaths_TEST.cc b/src/SystemPaths_TEST.cc index d540595a4..f695c0750 100644 --- a/src/SystemPaths_TEST.cc +++ b/src/SystemPaths_TEST.cc @@ -24,6 +24,7 @@ #include #include "ignition/common/Util.hh" +#include "ignition/common/StringUtils.hh" #include "ignition/common/SystemPaths.hh" #ifdef _WIN32 @@ -32,24 +33,19 @@ using namespace ignition; +const char kPluginPath[] = "IGN_PLUGIN_PATH"; +const char kFilePath[] = "IGN_FILE_PATH"; + class SystemPathsFixture : public ::testing::Test { // Documentation inherited public: virtual void SetUp() { - this->backupPluginPath = "IGN_PLUGIN_PATH="; - - if (getenv("IGN_PLUGIN_PATH")) - this->backupPluginPath += getenv("IGN_PLUGIN_PATH"); - - putenv(const_cast("IGN_PLUGIN_PATH=")); - - this->backupFilePath = "IGN_FILE_PATH="; + common::env(kPluginPath, this->backupPluginPath); + common::unsetenv(kPluginPath); - if (getenv("IGN_FILE_PATH")) - this->backupFilePath += getenv("IGN_FILE_PATH"); - - putenv(const_cast("IGN_FILE_PATH=")); + common::env(kFilePath, this->backupFilePath); + common::unsetenv(kFilePath); #ifdef _WIN32 this->filesystemRoot = "C:\\"; @@ -61,8 +57,8 @@ class SystemPathsFixture : public ::testing::Test // Documentation inherited public: virtual void TearDown() { - putenv(const_cast(this->backupPluginPath.c_str())); - putenv(const_cast(this->backupFilePath.c_str())); + common::setenv(kPluginPath, this->backupPluginPath); + common::setenv(kFilePath, this->backupFilePath); } /// \brief Backup of plugin paths to be restored after the test @@ -75,21 +71,18 @@ class SystemPathsFixture : public ::testing::Test public: std::string filesystemRoot; }; +std::string SystemPathsJoin(const std::vector &_paths) +{ + return common::Join(_paths, common::SystemPaths::Delimiter()); +} + ///////////////////////////////////////////////// TEST_F(SystemPathsFixture, SystemPaths) { - common::SystemPaths paths; - std::string env_str("IGN_PLUGIN_PATH=/tmp/plugin"); - env_str += ignition::common::SystemPaths::Delimiter(); - env_str += "/test/plugin/now"; - - // The char* passed to putenv will continue to be stored after the lifetime - // of this function, so we should not pass it a pointer which has an automatic - // lifetime. - static char env[1024]; - snprintf(env, sizeof(env), "%s", env_str.c_str()); - putenv(env); + common::setenv(kPluginPath, + SystemPathsJoin({"/tmp/plugin", "/test/plugin/now"})); + common::SystemPaths paths; const std::list pathList3 = paths.PluginPaths(); EXPECT_EQ(static_cast(2), pathList3.size()); EXPECT_STREQ("/tmp/plugin/", pathList3.front().c_str()); @@ -103,19 +96,12 @@ TEST_F(SystemPathsFixture, SystemPaths) ///////////////////////////////////////////////// TEST_F(SystemPathsFixture, FileSystemPaths) { - std::string env_str("IGN_FILE_PATH=/tmp/file"); - env_str += ignition::common::SystemPaths::Delimiter(); - env_str += "/test/file/now"; - - // The char* passed to putenv will continue to be stored after the lifetime - // of this function, so we should not pass it a pointer which has an automatic - // lifetime. - static char env[1024]; - snprintf(env, sizeof(env), "%s", env_str.c_str()); - putenv(env); + std::vector filePaths {"/tmp/file", + "/test/file/now"}; + common::setenv(kFilePath, SystemPathsJoin(filePaths)); common::SystemPaths paths; - EXPECT_EQ("IGN_FILE_PATH", paths.FilePathEnv()); + EXPECT_EQ(kFilePath, paths.FilePathEnv()); const std::list pathList3 = paths.FilePaths(); EXPECT_EQ(static_cast(2), pathList3.size()); EXPECT_STREQ("/tmp/file/", pathList3.front().c_str()); @@ -136,12 +122,10 @@ TEST_F(SystemPathsFixture, FileSystemPaths) EXPECT_EQ("", paths.FindFile("no_such_file")); EXPECT_EQ("", paths.FindFile("test_f1")); - env_str += ignition::common::SystemPaths::Delimiter(); - env_str += "test_dir1"; - snprintf(env, sizeof(env), "%s", env_str.c_str()); - putenv(env); - paths.SetFilePathEnv("IGN_FILE_PATH"); + filePaths.push_back("test_dir1"); + common::setenv(kFilePath, SystemPathsJoin(filePaths)); + paths.SetFilePathEnv(kFilePath); EXPECT_EQ(file1, paths.FindFile("test_f1")) << paths.FindFile("test_f1"); EXPECT_EQ(file1, paths.FindFile("model://test_f1")); } @@ -285,19 +269,14 @@ TEST_F(SystemPathsFixture, FindFileURI) EXPECT_EQ(file2, sp.FindFileURI("osrf://test_f2")); // URI + env var - static char env[1024]; - snprintf(env, sizeof(env), "%s", ("IGN_FILE_PATH=" + dir1).c_str()); - putenv(env); - - sp.SetFilePathEnv("IGN_FILE_PATH"); - EXPECT_EQ("IGN_FILE_PATH", sp.FilePathEnv()); + common::setenv(kFilePath, dir1); + sp.SetFilePathEnv(kFilePath); + EXPECT_EQ(kFilePath, sp.FilePathEnv()); EXPECT_EQ(file1, sp.FindFileURI("anything://test_f1")); EXPECT_NE(file2, sp.FindFileURI("anything://test_f2")); std::string newEnv{"IGN_NEW_FILE_PATH"}; - snprintf(env, sizeof(env), "%s", (newEnv + "=" + dir2).c_str()); - putenv(env); - + common::setenv(newEnv, dir2); sp.SetFilePathEnv(newEnv); EXPECT_EQ(newEnv, sp.FilePathEnv()); EXPECT_NE(file1, sp.FindFileURI("anything://test_f1")); @@ -422,20 +401,10 @@ TEST_F(SystemPathsFixture, NormalizeDirectoryPath) ////////////////////////////////////////////////// TEST_F(SystemPathsFixture, PathsFromEnv) { - std::string env_str = "IGN_PLUGIN_PATH=/tmp/plugin"; - env_str += ignition::common::SystemPaths::Delimiter(); - env_str += "/test/plugin/now/"; - env_str += ignition::common::SystemPaths::Delimiter(); - env_str += "/tmp/plugin"; - - // The char* passed to putenv will continue to be stored after the lifetime - // of this function, so we should not pass it a pointer which has an automatic - // lifetime. - static char env[1024]; - snprintf(env, sizeof(env), "%s", env_str.c_str()); - putenv(env); - - auto paths = ignition::common::SystemPaths::PathsFromEnv("IGN_PLUGIN_PATH"); + common::setenv(kPluginPath, + SystemPathsJoin({"/tmp/plugin", "/test/plugin/now/", "/tmp/plugin"})); + + auto paths = ignition::common::SystemPaths::PathsFromEnv(kPluginPath); EXPECT_EQ(paths.size(), 2u);