From 712bcaad54e99a9ab6a831c62b36eb5f7df9491b Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Tue, 31 Aug 2021 13:16:29 -0500 Subject: [PATCH 1/9] Add TempDirectory Signed-off-by: Michael Carroll --- include/ignition/common/TempDirectory.hh | 106 ++++++++++++ src/TempDirectory.cc | 206 +++++++++++++++++++++++ src/TempDirectory_TEST.cc | 116 +++++++++++++ 3 files changed, 428 insertions(+) create mode 100644 include/ignition/common/TempDirectory.hh create mode 100644 src/TempDirectory.cc create mode 100644 src/TempDirectory_TEST.cc diff --git a/include/ignition/common/TempDirectory.hh b/include/ignition/common/TempDirectory.hh new file mode 100644 index 000000000..6734ab303 --- /dev/null +++ b/include/ignition/common/TempDirectory.hh @@ -0,0 +1,106 @@ +/* + * Copyright 2021 Open Source Robotics Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#ifndef IGNITION_COMMON_TEMPDIRECTORY_HH_ +#define IGNITION_COMMON_TEMPDIRECTORY_HH_ + +#include +#include + +#include +#include + +namespace ignition +{ + namespace common + { + class TempDirectoryPrivate; + + /// \brief Return the path to a directory suitable for temporary files. + /// + /// Calls std::filesystem::temp_directory_path, refer to the standard + /// documentation for your platform for behaviors. + /// \return A directory suitable for temporary files. + std::string IGNITION_COMMON_VISIBLE tempDirectoryPath(); + + /// \brief Create a directory in the tempDirectoryPath by expanding + /// a name template + /// + /// On execution, will create the directory: + /// "_parentPath"/"_baseName" + "XXXXXX", where XXXXXX will be filled + /// out by an OS-appropriate method (eg mkdtmp/_mktemp_s) + /// + /// \param[in] _baseName String to be prepended to the expanded template + /// \param[in] _parentPath Location to create the directory + /// \param[in] _warningOp Allow or suppress filesystem warnings + /// \return Path to newly-created temporary directory + std::string IGNITION_COMMON_VISIBLE createTempDirectory( + const std::string &_baseName, + const std::string &_parentPath, + const FilesystemWarningOp _warningOp = FSWO_LOG_WARNINGS); + + /// \class TempDirectory TempDirectory.hh ignitin/common/TempDirectory.hh + /// \brief Create a temporary directory in the OS temp location. + class IGNITION_COMMON_VISIBLE TempDirectory + { + /// \brief Create a directory in the tempDirectoryPath by expanding + /// a name template. This directory can also be automatically cleaned + /// up when the object goes out of scope. + /// + /// The TempDirectory will have the form $TMPDIR/_subdir/_prefixXXXXX/ + /// + /// \param[in] _prefix String to be expanded for the template + /// \param[in] _subDir Subdirectory in OS $TMPDIR, if desired + /// \param[in] _cleanup True to indicate that the filesystem should + /// be cleaned as part of the destructor + public: TempDirectory(const std::string &_prefix = "temp_dir", + const std::string &_subDir = "ignition", + bool _cleanup = true); + + /// \brief Destroy the temporary directory, removing from filesystem + /// if cleanup is true. + public: ~TempDirectory(); + + /// \brief Indicate if the TempDirectory object is in a valid state + /// and that the folder exists on the filesystem + /// \return true if the TempDirectory is valid + public: bool Valid() const; + + /// \brief Set if the folder on disk should be cleaned. + /// + /// This is useful if you wish to clean by default during a test, but + /// retain the contents of the TempDirectory if the test fails. + /// \param[in] _cleanup True to indicate that the filesystem should + /// be cleaned as part of the destructor + public: void Cleanup(bool _cleanup); + + /// \brief Retrieve the current cleanup flag state + /// \return true if filesystem cleanup will occur + public: bool Cleanup() const; + + /// \brief Retrieve the fully-expanded temporary directory path + /// \return the temporary directory path + public: std::string Path() const; + + IGN_COMMON_WARN_IGNORE__DLL_INTERFACE_MISSING + private: std::unique_ptr dataPtr; + IGN_COMMON_WARN_RESUME__DLL_INTERFACE_MISSING + }; + } // namespace common +} // namespace ignition +#endif // IGNITION_COMMON_TEMPDIRECTORY_HH_ + diff --git a/src/TempDirectory.cc b/src/TempDirectory.cc new file mode 100644 index 000000000..12bc53da4 --- /dev/null +++ b/src/TempDirectory.cc @@ -0,0 +1,206 @@ +/* + * Copyright 2021 Open Source Robotics Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include +#include + +#include + +#ifdef _WIN32 +#include +#include +#include +#include +#endif + +namespace fs = std::filesystem; + +using namespace ignition; +using namespace common; + +///////////////////////////////////////////////// +// Return true if success, false if error +inline bool fs_warn(const std::string &_fcn, + const std::error_code &_ec, + const FilesystemWarningOp &_warningOp = FSWO_LOG_WARNINGS) +{ + + if (_ec) + { + if (FSWO_LOG_WARNINGS == _warningOp) + { + ignwarn << "Failed ignition::common::" << _fcn + << " (ec: " << _ec << " " << _ec.message() << ")\n"; + } + return false; + } + return true; +} + +///////////////////////////////////////////////// +std::string ignition::common::tempDirectoryPath() +{ + std::error_code ec; + auto ret = fs::temp_directory_path(ec); + + if (!fs_warn("tempDirectoryPath", ec)) + { + ret = ""; + } + + return ret.string(); +} + +///////////////////////////////////////////////// +/// \brief Internal method for createTempDirectory +/// +/// This is primarily to scope the "throw" behavior from when this +/// was copied from rclcpp. +std::string createTempDirectory( + const std::string &_baseName, + const std::string &_parentPath) +{ + fs::path parentPath(_parentPath); + fs::path templatePath = _baseName + "XXXXXX"; + + std::string fullTemplateStr = (parentPath / templatePath).string(); + if (!createDirectories(parentPath.string())) { + std::error_code ec{errno, std::system_category()}; + errno = 0; + throw std::system_error(ec, "could not create the parent directory"); + } + +#ifdef _WIN32 + errno_t errcode = _mktemp_s(&fullTemplateStr[0], fullTemplateStr.size() + 1); + if (errcode) { + std::error_code ec(static_cast(errcode), std::system_category()); + throw std::system_error(ec, + "could not format the temp directory name template"); + } + const fs::path finalPath{fullTemplateStr}; + if (!createDirectories(finalPath.string())) { + std::error_code ec(static_cast(GetLastError()), + std::system_category()); + throw std::system_error(ec, "could not create the temp directory"); + } +#else + const char * dirName = mkdtemp(&fullTemplateStr[0]); + if (dirName == nullptr) { + std::error_code ec{errno, std::system_category()}; + errno = 0; + throw std::system_error(ec, + "could not format or create the temp directory"); + } + const fs::path finalPath{dirName}; +#endif + + return finalPath.string(); +} + +///////////////////////////////////////////////// +std::string ignition::common::createTempDirectory( + const std::string &_baseName, + const std::string &_parentPath, + const FilesystemWarningOp _warningOp) +{ + std::string ret; + try { + ret = ::createTempDirectory(_baseName, _parentPath); + } catch (const std::system_error &ex) { + ret = ""; + if(FSWO_LOG_WARNINGS == _warningOp) + { + ignwarn << "Failed to create temp directory: " << ex.what() << "\n"; + } + } + return ret; +} + +class ignition::common::TempDirectoryPrivate +{ + /// \brief Current working directory before creation of temporary dir. + public: std::string oldPath {""}; + + /// \brief Path of the temporary directory + public: std::string path {""}; + + /// \brief True if the temporary directory exists + public: bool isValid {false}; + + /// \brief True if the temporary directory should be cleaned up from + /// disk when the object goes out of scope. + public: bool doCleanup {true}; +}; + +///////////////////////////////////////////////// +TempDirectory::TempDirectory(const std::string &_prefix, + const std::string &_subDir, + bool _cleanup): + dataPtr(std::make_unique()) +{ + + this->dataPtr->oldPath = common::cwd(); + this->dataPtr->doCleanup = _cleanup; + + auto tempPath = common::tempDirectoryPath(); + if (!_subDir.empty()) + { + tempPath = common::joinPaths(tempPath, _subDir); + } + + this->dataPtr->path = common::createTempDirectory(_prefix, tempPath); + std::cerr << "Created temporary directory: " << this->dataPtr->path << std::endl; + if (!this->dataPtr->path.empty()) + { + this->dataPtr->isValid = true; + common::chdir(this->dataPtr->path); + } +} + +///////////////////////////////////////////////// +TempDirectory::~TempDirectory() +{ + common::chdir(this->dataPtr->oldPath); + if (this->dataPtr->isValid && this->dataPtr->doCleanup) + { + common::removeAll(this->dataPtr->path); + } +} + +///////////////////////////////////////////////// +bool TempDirectory::Valid() const +{ + return this->dataPtr->isValid; +} + +///////////////////////////////////////////////// +void TempDirectory::Cleanup(bool _cleanup) +{ + this->dataPtr->doCleanup = _cleanup; +} + +///////////////////////////////////////////////// +bool TempDirectory::Cleanup() const +{ + return this->dataPtr->doCleanup; +} + +///////////////////////////////////////////////// +std::string TempDirectory::Path() const +{ + return this->dataPtr->path; +} diff --git a/src/TempDirectory_TEST.cc b/src/TempDirectory_TEST.cc new file mode 100644 index 000000000..b054c621c --- /dev/null +++ b/src/TempDirectory_TEST.cc @@ -0,0 +1,116 @@ +/* + * Copyright (C) 2021 Open Source Robotics Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include + +#include +#include + +#include + + +///////////////////////////////////////////////// +TEST(TempDirectory, tempDirectoryPath) +{ + // OS TMPDIR should never be empty + ASSERT_FALSE(ignition::common::tempDirectoryPath().empty()); +} + +///////////////////////////////////////////////// +TEST(TempDirectory, createTempDirectory) +{ + // OS TMPDIR should never be empty + auto tmpDir = ignition::common::tempDirectoryPath(); + ASSERT_FALSE(tmpDir.empty()); + + // Nominal case + // eg /tmp/fooXXXXXX + auto tmp = ignition::common::createTempDirectory("foo", tmpDir); + ASSERT_FALSE(tmp.empty()); + EXPECT_NE(std::string::npos, tmp.find("foo")); + EXPECT_NE(std::string::npos, tmp.find(tmpDir)); + + // Should also work for subdirectories + // eg /tmp/bar/fooXXXXXX + tmpDir = ignition::common::joinPaths(tmpDir, "bar"); + auto tmp2 = ignition::common::createTempDirectory("bar", tmpDir); + ASSERT_FALSE(tmp2.empty()); +} + +///////////////////////////////////////////////// +TEST(TempDirectory, createTempDirectory_emptybase) +{ + // OS TMPDIR should never be empty + auto tmpDir = ignition::common::tempDirectoryPath(); + ASSERT_FALSE(tmpDir.empty()); + + // Create with an empty basename (eg /tmp/XXXXXX) + auto tmp = ignition::common::createTempDirectory("", tmpDir); + ASSERT_FALSE(tmp.empty()); + EXPECT_EQ(std::string::npos, tmp.find("foo")); + EXPECT_NE(std::string::npos, tmp.find(tmpDir)); +} + +///////////////////////////////////////////////// +TEST(TempDirectory, TempDirectory) +{ + std::string path; + { + ignition::common::TempDirectory tmp("temp_dir", "ignition", true); + EXPECT_TRUE(tmp.Valid()); + EXPECT_TRUE(tmp.Cleanup()); + path = tmp.Path(); + EXPECT_FALSE(path.empty()); + EXPECT_TRUE(ignition::common::exists(path)); + } + EXPECT_FALSE(ignition::common::exists(path)); +} + +///////////////////////////////////////////////// +TEST(TempDirectory, TempDirectory_noclean) +{ + std::string path; + { + ignition::common::TempDirectory tmp("temp_dir", "ignition", false); + EXPECT_TRUE(tmp.Valid()); + EXPECT_FALSE(tmp.Cleanup()); + path = tmp.Path(); + EXPECT_FALSE(path.empty()); + EXPECT_TRUE(ignition::common::exists(path)); + } + EXPECT_TRUE(ignition::common::exists(path)); + EXPECT_TRUE(ignition::common::removeDirectory(path)); +} + +///////////////////////////////////////////////// +TEST(TempDirectory, TempDirectory_noclean_later) +{ + std::string path; + { + ignition::common::TempDirectory tmp("temp_dir", "ignition", true); + EXPECT_TRUE(tmp.Valid()); + EXPECT_TRUE(tmp.Cleanup()); + path = tmp.Path(); + EXPECT_FALSE(path.empty()); + EXPECT_TRUE(ignition::common::exists(path)); + tmp.Cleanup(false); + EXPECT_FALSE(tmp.Cleanup()); + } + EXPECT_TRUE(ignition::common::exists(path)); + EXPECT_TRUE(ignition::common::removeDirectory(path)); +} + From c379ef312ebc58090f3b0902fd44e970dfd99452 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Tue, 31 Aug 2021 14:17:23 -0500 Subject: [PATCH 2/9] Use TempDirectory in tests Signed-off-by: Michael Carroll --- src/Console.cc | 2 +- src/Console_TEST.cc | 19 ++++++++-------- src/SystemPaths_TEST.cc | 16 ++++++++++++++ src/TempDirectory.cc | 49 +++++++++++++++++++++++++++-------------- test/test_config.h.in | 49 ++++++++++++----------------------------- 5 files changed, 73 insertions(+), 62 deletions(-) diff --git a/src/Console.cc b/src/Console.cc index 0e8b1fbbb..ff013fe23 100644 --- a/src/Console.cc +++ b/src/Console.cc @@ -252,7 +252,7 @@ void FileLogger::Init(const std::string &_directory, if (isDirectory(logPath)) this->logDirectory = logPath; else - this->logDirectory = logPath.substr(0, logPath.rfind(separator(""))); + this->logDirectory = common::parentPath(logPath); this->initialized = true; diff --git a/src/Console_TEST.cc b/src/Console_TEST.cc index 2cc3805fd..28b05668c 100644 --- a/src/Console_TEST.cc +++ b/src/Console_TEST.cc @@ -33,23 +33,21 @@ const int g_messageRepeat = 4; class Console_TEST : public ::testing::Test { protected: virtual void SetUp() { - // Set IGN_HOMEDIR and store it - common::testing::TestSetHomePath(this->logBasePath); + this->temp = std::make_unique( + "test", "ign_common", true); + ASSERT_TRUE(this->temp->Valid()); + common::setenv(IGN_HOMEDIR, this->temp->Path()); } /// \brief Clear out all the directories we produced during this test. - public: virtual ~Console_TEST() + public: virtual void TearDown() { + ignLogClose(); EXPECT_TRUE(ignition::common::unsetenv(IGN_HOMEDIR)); - - if (ignition::common::isDirectory(this->logBasePath)) - { - ignLogClose(); - EXPECT_TRUE(ignition::common::removeAll(this->logBasePath)); - } } - private: std::string logBasePath; + /// \brief Temporary directory to run test in + private: std::unique_ptr temp; }; std::string GetLogContent(const std::string &_filename) @@ -58,6 +56,7 @@ std::string GetLogContent(const std::string &_filename) std::string path; EXPECT_TRUE(ignition::common::env(IGN_HOMEDIR, path)); path = ignition::common::joinPaths(path, _filename); + EXPECT_TRUE(ignition::common::exists(path)); // Open the log file, and read back the string std::ifstream ifs(path.c_str(), std::ios::in); diff --git a/src/SystemPaths_TEST.cc b/src/SystemPaths_TEST.cc index f20878fb0..a17950baa 100644 --- a/src/SystemPaths_TEST.cc +++ b/src/SystemPaths_TEST.cc @@ -26,6 +26,7 @@ #include "ignition/common/Util.hh" #include "ignition/common/StringUtils.hh" #include "ignition/common/SystemPaths.hh" +#include "ignition/common/TempDirectory.hh" #ifdef _WIN32 #define snprintf _snprintf @@ -36,11 +37,22 @@ using namespace ignition; const char kPluginPath[] = "IGN_PLUGIN_PATH"; const char kFilePath[] = "IGN_FILE_PATH"; +class TestTempDirectory : public ignition::common::TempDirectory +{ + public: TestTempDirectory(): + ignition::common::TempDirectory("systempaths", "ign_common", true) + { + } +}; + class SystemPathsFixture : public ::testing::Test { // Documentation inherited public: virtual void SetUp() { + this->temp = std::make_unique(); + ASSERT_TRUE(this->temp->Valid()); + common::env(kPluginPath, this->backupPluginPath); common::unsetenv(kPluginPath); @@ -59,6 +71,7 @@ class SystemPathsFixture : public ::testing::Test { common::setenv(kPluginPath, this->backupPluginPath); common::setenv(kFilePath, this->backupFilePath); + this->temp.reset(); } /// \brief Backup of plugin paths to be restored after the test @@ -69,6 +82,9 @@ class SystemPathsFixture : public ::testing::Test /// \brief Root of filesystem according to each platform public: std::string filesystemRoot; + + /// \brief Temporary directory to execute test in + public: std::unique_ptr temp; }; std::string SystemPathsJoin(const std::vector &_paths) diff --git a/src/TempDirectory.cc b/src/TempDirectory.cc index 12bc53da4..d18f9ca19 100644 --- a/src/TempDirectory.cc +++ b/src/TempDirectory.cc @@ -18,8 +18,6 @@ #include #include -#include - #ifdef _WIN32 #include #include @@ -27,8 +25,6 @@ #include #endif -namespace fs = std::filesystem; - using namespace ignition; using namespace common; @@ -51,18 +47,41 @@ inline bool fs_warn(const std::string &_fcn, return true; } + +///////////////////////////////////////////////// +std::string temp_directory_path(std::error_code& _err) +{ + _err = std::error_code(); +#ifdef _WIN32 + TCHAR temp_path[MAX_PATH]; + DWORD size = GetTempPathA(MAX_PATH, temp_path); + if (size > MAX_PATH || size == 0) { + _err = std::error_code( + static_cast(GetLastError()), std::system_category()); + } + temp_path[size] = '\0'; +#else + std::string temp_path; + if(!ignition::common::env("TMPDIR", temp_path)) + { + temp_path = "/tmp"; + } +#endif + return std::string(temp_path); +} + ///////////////////////////////////////////////// std::string ignition::common::tempDirectoryPath() { std::error_code ec; - auto ret = fs::temp_directory_path(ec); + auto ret = temp_directory_path(ec); if (!fs_warn("tempDirectoryPath", ec)) { ret = ""; } - return ret.string(); + return ret; } ///////////////////////////////////////////////// @@ -74,11 +93,11 @@ std::string createTempDirectory( const std::string &_baseName, const std::string &_parentPath) { - fs::path parentPath(_parentPath); - fs::path templatePath = _baseName + "XXXXXX"; + std::string parentPath(_parentPath); + std::string templatePath = _baseName + "XXXXXX"; - std::string fullTemplateStr = (parentPath / templatePath).string(); - if (!createDirectories(parentPath.string())) { + std::string fullTemplateStr = joinPaths(parentPath, templatePath); + if (!createDirectories(parentPath)) { std::error_code ec{errno, std::system_category()}; errno = 0; throw std::system_error(ec, "could not create the parent directory"); @@ -91,8 +110,8 @@ std::string createTempDirectory( throw std::system_error(ec, "could not format the temp directory name template"); } - const fs::path finalPath{fullTemplateStr}; - if (!createDirectories(finalPath.string())) { + const std::string finalPath{fullTemplateStr}; + if (!createDirectories(finalPath)) { std::error_code ec(static_cast(GetLastError()), std::system_category()); throw std::system_error(ec, "could not create the temp directory"); @@ -105,10 +124,10 @@ std::string createTempDirectory( throw std::system_error(ec, "could not format or create the temp directory"); } - const fs::path finalPath{dirName}; + const std::string finalPath{dirName}; #endif - return finalPath.string(); + return finalPath; } ///////////////////////////////////////////////// @@ -161,9 +180,7 @@ TempDirectory::TempDirectory(const std::string &_prefix, { tempPath = common::joinPaths(tempPath, _subDir); } - this->dataPtr->path = common::createTempDirectory(_prefix, tempPath); - std::cerr << "Created temporary directory: " << this->dataPtr->path << std::endl; if (!this->dataPtr->path.empty()) { this->dataPtr->isValid = true; diff --git a/test/test_config.h.in b/test/test_config.h.in index 0a4616552..aea2bf931 100644 --- a/test/test_config.h.in +++ b/test/test_config.h.in @@ -13,6 +13,7 @@ #include #include "ignition/common/Console.hh" #include "ignition/common/Filesystem.hh" +#include "ignition/common/TempDirectory.hh" #include "ignition/common/Util.hh" #define PROJECT_BINARY_PATH "${PROJECT_BINARY_DIR}" @@ -59,37 +60,9 @@ namespace ignition } else { - _tmpDir = common::joinPaths("${PROJECT_BINARY_DIR}", "tmp"); - return true; - } - } - - /// \brief Method to retrieve temporary home directory for tests - /// - /// This will update the contents of the home directory path variable - /// (HOME on Linux/MacOS, HOMEPATH on Windows) to this newly-set - /// directory - /// This additionally sets the HOME and HOMEPATH environment variables - /// - /// \param[inout] _homeDir Full path to the home directory - /// \return True if directory is set correctly, false otherwise - bool TestSetHomePath(std::string &_homeDir) - { - if (common::env("TEST_UNDECLARED_OUTPUTS_DIR", _homeDir)) - { - return ignition::common::setenv(IGN_HOMEDIR, _homeDir); - } - else - { - if (TestTmpPath(_homeDir)) - { - // Set both for linux and windows - return ignition::common::setenv(IGN_HOMEDIR, _homeDir); - } - else - { - return false; - } + _tmpDir = common::createTempDirectory("ignition", + common::tempDirectoryPath()); + return !_tmpDir.empty(); } } @@ -143,11 +116,14 @@ namespace ignition std::string testCaseName = testInfo->test_case_name(); this->logFilename = testCaseName + "_" + testName + ".log"; - common::testing::TestSetHomePath(this->logBasePath); + this->temp = std::make_unique( + "test", "ign_common", true); + ASSERT_TRUE(this->temp->Valid()); + common::setenv(IGN_HOMEDIR, this->temp->Path()); // Initialize Console - ignLogInit(common::joinPaths(this->logBasePath, "test_logs"), - this->logFilename); + ignLogInit(common::joinPaths(this->temp->Path(), "test_logs"), + this->logFilename); ignition::common::Console::SetVerbosity(4); @@ -184,7 +160,7 @@ namespace ignition public: virtual ~AutoLogFixture() { ignLogClose(); - ignition::common::removeAll(this->logBasePath); + EXPECT_TRUE(ignition::common::unsetenv(IGN_HOMEDIR)); } /// \brief String with the full path of the logfile @@ -195,6 +171,9 @@ namespace ignition /// \brief String with the base path to log directory private: std::string logBasePath; + + /// \brief Temporary directory to run test in + private: std::unique_ptr temp; }; } // namespace testing } // namespace common From 2f143abd6e466ae157d5f44694d4fba44dafc8af Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Fri, 10 Sep 2021 13:48:37 -0500 Subject: [PATCH 3/9] Add note about `temp_directory_path` Signed-off-by: Michael Carroll --- src/TempDirectory.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/TempDirectory.cc b/src/TempDirectory.cc index d18f9ca19..524804d37 100644 --- a/src/TempDirectory.cc +++ b/src/TempDirectory.cc @@ -49,6 +49,9 @@ inline bool fs_warn(const std::string &_fcn, ///////////////////////////////////////////////// +// Helper implementation of std::filesystem::temp_directory_path +// https://en.cppreference.com/w/cpp/filesystem/temp_directory_path +// \TODO(anyone) remove when using `std::filesystem` in C++17 and greater. std::string temp_directory_path(std::error_code& _err) { _err = std::error_code(); From 105a1ccee3a7526e5ef7b271a5588ff8abb8ccb4 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Fri, 10 Sep 2021 14:00:28 -0500 Subject: [PATCH 4/9] Address reviewer feedback Signed-off-by: Michael Carroll --- include/ignition/common/TempDirectory.hh | 8 ++++++-- src/TempDirectory.cc | 13 ++++++++----- src/TempDirectory_TEST.cc | 16 ++++++++-------- 3 files changed, 22 insertions(+), 15 deletions(-) diff --git a/include/ignition/common/TempDirectory.hh b/include/ignition/common/TempDirectory.hh index 6734ab303..66ac3012f 100644 --- a/include/ignition/common/TempDirectory.hh +++ b/include/ignition/common/TempDirectory.hh @@ -55,6 +55,10 @@ namespace ignition /// \class TempDirectory TempDirectory.hh ignitin/common/TempDirectory.hh /// \brief Create a temporary directory in the OS temp location. + /// Upon construction, the current working directory will be set to this + /// new temporary directory. + /// Upon destruction, the current working directory will be restored to the + /// location when the TempDirectory object was constructed. class IGNITION_COMMON_VISIBLE TempDirectory { /// \brief Create a directory in the tempDirectoryPath by expanding @@ -86,11 +90,11 @@ namespace ignition /// retain the contents of the TempDirectory if the test fails. /// \param[in] _cleanup True to indicate that the filesystem should /// be cleaned as part of the destructor - public: void Cleanup(bool _cleanup); + public: void DoCleanup(bool _doCleanup); /// \brief Retrieve the current cleanup flag state /// \return true if filesystem cleanup will occur - public: bool Cleanup() const; + public: bool DoCleanup() const; /// \brief Retrieve the fully-expanded temporary directory path /// \return the temporary directory path diff --git a/src/TempDirectory.cc b/src/TempDirectory.cc index 524804d37..575ea08c9 100644 --- a/src/TempDirectory.cc +++ b/src/TempDirectory.cc @@ -100,7 +100,8 @@ std::string createTempDirectory( std::string templatePath = _baseName + "XXXXXX"; std::string fullTemplateStr = joinPaths(parentPath, templatePath); - if (!createDirectories(parentPath)) { + if (!createDirectories(parentPath)) + { std::error_code ec{errno, std::system_category()}; errno = 0; throw std::system_error(ec, "could not create the parent directory"); @@ -142,7 +143,9 @@ std::string ignition::common::createTempDirectory( std::string ret; try { ret = ::createTempDirectory(_baseName, _parentPath); - } catch (const std::system_error &ex) { + } + catch (const std::system_error &ex) + { ret = ""; if(FSWO_LOG_WARNINGS == _warningOp) { @@ -208,13 +211,13 @@ bool TempDirectory::Valid() const } ///////////////////////////////////////////////// -void TempDirectory::Cleanup(bool _cleanup) +void TempDirectory::DoCleanup(bool _doCleanup) { - this->dataPtr->doCleanup = _cleanup; + this->dataPtr->doCleanup = _doCleanup; } ///////////////////////////////////////////////// -bool TempDirectory::Cleanup() const +bool TempDirectory::DoCleanup() const { return this->dataPtr->doCleanup; } diff --git a/src/TempDirectory_TEST.cc b/src/TempDirectory_TEST.cc index b054c621c..164dff716 100644 --- a/src/TempDirectory_TEST.cc +++ b/src/TempDirectory_TEST.cc @@ -52,7 +52,7 @@ TEST(TempDirectory, createTempDirectory) } ///////////////////////////////////////////////// -TEST(TempDirectory, createTempDirectory_emptybase) +TEST(TempDirectory, createTempDirectoryEmptyBase) { // OS TMPDIR should never be empty auto tmpDir = ignition::common::tempDirectoryPath(); @@ -72,7 +72,7 @@ TEST(TempDirectory, TempDirectory) { ignition::common::TempDirectory tmp("temp_dir", "ignition", true); EXPECT_TRUE(tmp.Valid()); - EXPECT_TRUE(tmp.Cleanup()); + EXPECT_TRUE(tmp.DoCleanup()); path = tmp.Path(); EXPECT_FALSE(path.empty()); EXPECT_TRUE(ignition::common::exists(path)); @@ -81,13 +81,13 @@ TEST(TempDirectory, TempDirectory) } ///////////////////////////////////////////////// -TEST(TempDirectory, TempDirectory_noclean) +TEST(TempDirectory, TempDirectoryNoClean) { std::string path; { ignition::common::TempDirectory tmp("temp_dir", "ignition", false); EXPECT_TRUE(tmp.Valid()); - EXPECT_FALSE(tmp.Cleanup()); + EXPECT_FALSE(tmp.DoCleanup()); path = tmp.Path(); EXPECT_FALSE(path.empty()); EXPECT_TRUE(ignition::common::exists(path)); @@ -97,18 +97,18 @@ TEST(TempDirectory, TempDirectory_noclean) } ///////////////////////////////////////////////// -TEST(TempDirectory, TempDirectory_noclean_later) +TEST(TempDirectory, TempDirectoryNoCleanLater) { std::string path; { ignition::common::TempDirectory tmp("temp_dir", "ignition", true); EXPECT_TRUE(tmp.Valid()); - EXPECT_TRUE(tmp.Cleanup()); + EXPECT_TRUE(tmp.DoCleanup()); path = tmp.Path(); EXPECT_FALSE(path.empty()); EXPECT_TRUE(ignition::common::exists(path)); - tmp.Cleanup(false); - EXPECT_FALSE(tmp.Cleanup()); + tmp.DoCleanup(false); + EXPECT_FALSE(tmp.DoCleanup()); } EXPECT_TRUE(ignition::common::exists(path)); EXPECT_TRUE(ignition::common::removeDirectory(path)); From 3f0f9354aaff6c56c4778b1f4958f2d7b3d01a0b Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Mon, 13 Sep 2021 07:57:00 -0500 Subject: [PATCH 5/9] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Alejandro Hernández Cordero Signed-off-by: Michael Carroll --- include/ignition/common/TempDirectory.hh | 4 ++-- src/TempDirectory.cc | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/include/ignition/common/TempDirectory.hh b/include/ignition/common/TempDirectory.hh index 66ac3012f..378881451 100644 --- a/include/ignition/common/TempDirectory.hh +++ b/include/ignition/common/TempDirectory.hh @@ -62,7 +62,7 @@ namespace ignition class IGNITION_COMMON_VISIBLE TempDirectory { /// \brief Create a directory in the tempDirectoryPath by expanding - /// a name template. This directory can also be automatically cleaned + /// a name template. This directory can also be automatically cleaned /// up when the object goes out of scope. /// /// The TempDirectory will have the form $TMPDIR/_subdir/_prefixXXXXX/ @@ -88,7 +88,7 @@ namespace ignition /// /// This is useful if you wish to clean by default during a test, but /// retain the contents of the TempDirectory if the test fails. - /// \param[in] _cleanup True to indicate that the filesystem should + /// \param[in] _doCleanup True to indicate that the filesystem should /// be cleaned as part of the destructor public: void DoCleanup(bool _doCleanup); diff --git a/src/TempDirectory.cc b/src/TempDirectory.cc index 575ea08c9..31b67b867 100644 --- a/src/TempDirectory.cc +++ b/src/TempDirectory.cc @@ -34,7 +34,6 @@ inline bool fs_warn(const std::string &_fcn, const std::error_code &_ec, const FilesystemWarningOp &_warningOp = FSWO_LOG_WARNINGS) { - if (_ec) { if (FSWO_LOG_WARNINGS == _warningOp) From 8d048492c16fd7d1c69a489b7bddf9e8456b1f91 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Mon, 13 Sep 2021 08:04:14 -0500 Subject: [PATCH 6/9] Switch to IGN_UTILS_IMPL_PTR Signed-off-by: Michael Carroll --- include/ignition/common/TempDirectory.hh | 6 +++--- src/CMakeLists.txt | 3 +++ src/TempDirectory.cc | 5 +++-- src/TempDirectory_TEST.cc | 5 +---- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/include/ignition/common/TempDirectory.hh b/include/ignition/common/TempDirectory.hh index 378881451..b66c691e5 100644 --- a/include/ignition/common/TempDirectory.hh +++ b/include/ignition/common/TempDirectory.hh @@ -23,6 +23,7 @@ #include #include +#include namespace ignition { @@ -100,9 +101,8 @@ namespace ignition /// \return the temporary directory path public: std::string Path() const; - IGN_COMMON_WARN_IGNORE__DLL_INTERFACE_MISSING - private: std::unique_ptr dataPtr; - IGN_COMMON_WARN_RESUME__DLL_INTERFACE_MISSING + /// \brief Private data pointer. + IGN_UTILS_IMPL_PTR(dataPtr) }; } // namespace common } // namespace ignition diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 90e333f1e..7b2974194 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -17,6 +17,9 @@ target_link_libraries(${PROJECT_LIBRARY_TARGET_NAME} target_include_directories(${PROJECT_LIBRARY_TARGET_NAME} PRIVATE ${ignition-math${IGN_MATH_VER}_INCLUDE_DIRS}) +target_include_directories(${PROJECT_LIBRARY_TARGET_NAME} PUBLIC + ${ignition-utils${IGN_UTILS_VER}_INCLUDE_DIRS}) + # Handle non-Windows configuration settings if(NOT WIN32) # Link the libraries that we don't expect to find on Windows diff --git a/src/TempDirectory.cc b/src/TempDirectory.cc index 31b67b867..61f7ec807 100644 --- a/src/TempDirectory.cc +++ b/src/TempDirectory.cc @@ -154,7 +154,8 @@ std::string ignition::common::createTempDirectory( return ret; } -class ignition::common::TempDirectoryPrivate + +class ignition::common::TempDirectory::Implementation { /// \brief Current working directory before creation of temporary dir. public: std::string oldPath {""}; @@ -174,7 +175,7 @@ class ignition::common::TempDirectoryPrivate TempDirectory::TempDirectory(const std::string &_prefix, const std::string &_subDir, bool _cleanup): - dataPtr(std::make_unique()) + dataPtr(ignition::utils::MakeImpl()) { this->dataPtr->oldPath = common::cwd(); diff --git a/src/TempDirectory_TEST.cc b/src/TempDirectory_TEST.cc index 164dff716..a0c3ad7bb 100644 --- a/src/TempDirectory_TEST.cc +++ b/src/TempDirectory_TEST.cc @@ -17,11 +17,8 @@ #include -#include #include - -#include - +#include ///////////////////////////////////////////////// TEST(TempDirectory, tempDirectoryPath) From 386d21d0885ae28e9f502347952b0d048995e69d Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Mon, 13 Sep 2021 08:05:44 -0500 Subject: [PATCH 7/9] Alphebetize Signed-off-by: Michael Carroll --- src/TempDirectory.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/TempDirectory.cc b/src/TempDirectory.cc index 61f7ec807..c4dd57061 100644 --- a/src/TempDirectory.cc +++ b/src/TempDirectory.cc @@ -16,13 +16,14 @@ */ #include + #include #ifdef _WIN32 -#include #include #include #include +#include #endif using namespace ignition; From fe0372f015dbe3c2bc1152f249f87680ab4c857e Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Mon, 13 Sep 2021 10:32:28 -0500 Subject: [PATCH 8/9] Update src/TempDirectory.cc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Michael Carroll Co-authored-by: Alejandro Hernández Cordero --- src/TempDirectory.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/TempDirectory.cc b/src/TempDirectory.cc index c4dd57061..6201c6f89 100644 --- a/src/TempDirectory.cc +++ b/src/TempDirectory.cc @@ -47,7 +47,6 @@ inline bool fs_warn(const std::string &_fcn, return true; } - ///////////////////////////////////////////////// // Helper implementation of std::filesystem::temp_directory_path // https://en.cppreference.com/w/cpp/filesystem/temp_directory_path From 3a6da4e3f1e7b257183630f3a2db79c38c17788b Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Mon, 13 Sep 2021 13:38:19 -0500 Subject: [PATCH 9/9] windows.h included first Signed-off-by: Michael Carroll --- src/TempDirectory.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/TempDirectory.cc b/src/TempDirectory.cc index 6201c6f89..8e44b38fc 100644 --- a/src/TempDirectory.cc +++ b/src/TempDirectory.cc @@ -20,10 +20,10 @@ #include #ifdef _WIN32 +#include #include #include #include -#include #endif using namespace ignition;