From f10377d8aab83c07f88849b86e15d4b0f25be361 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Tue, 31 Aug 2021 13:16:29 -0500 Subject: [PATCH 01/15] Remove FilesystemBoost in favor of std::filesystem This is intended to be a drop-in replacement for the functionality that is currently in FilesystemBoost, as is demonstrated via no test updates. Signed-off-by: Michael Carroll --- include/ignition/common/Filesystem.hh | 3 - src/CMakeLists.txt | 8 +- src/DirIter.cc | 84 ++++ src/Filesystem.cc | 563 ++++++++--------------- src/FilesystemBoost.cc | 638 -------------------------- src/TempDirectory.cc | 60 +-- 6 files changed, 303 insertions(+), 1053 deletions(-) create mode 100644 src/DirIter.cc delete mode 100644 src/FilesystemBoost.cc diff --git a/include/ignition/common/Filesystem.hh b/include/ignition/common/Filesystem.hh index aa36f0a6f..daf20a9a4 100644 --- a/include/ignition/common/Filesystem.hh +++ b/include/ignition/common/Filesystem.hh @@ -264,9 +264,6 @@ namespace ignition /// \param[in] _in Directory to iterate over. public: explicit DirIter(const std::string &_in); - /// \brief Destructor - public: ~DirIter(); - /// \brief Dereference operator; returns current directory record. /// \return A string representing the entire path of the directory /// record. diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index b36e0af24..62cc2ef15 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -8,15 +8,21 @@ ign_create_core_library( SOURCES ${sources} CXX_STANDARD 17) +if (CMAKE_CXX_COMPILER_ID STREQUAL GNU) + set(CXX_FILESYSTEM_LIBRARIES stdc++fs) +else() + set(CXX_FILESYSTEM_LIBRARIES) +endif() + # Link the libraries that we always need target_link_libraries(${PROJECT_LIBRARY_TARGET_NAME} PRIVATE ${DL_TARGET} + ${CXX_FILESYSTEM_LIBRARIES} PUBLIC ignition-utils${IGN_UTILS_VER}::ignition-utils${IGN_UTILS_VER} ) - # This is required by the WorkerPool::WaitForResults(const Time &_timeout) # TODO(anyone): IGN_DEPRECATED(4). Remove this part when the method is removed target_include_directories(${PROJECT_LIBRARY_TARGET_NAME} PRIVATE diff --git a/src/DirIter.cc b/src/DirIter.cc new file mode 100644 index 000000000..c0314f2e9 --- /dev/null +++ b/src/DirIter.cc @@ -0,0 +1,84 @@ +/* + * 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 "ignition/common/Filesystem.hh" + +namespace fs = std::filesystem; + +namespace ignition +{ +namespace common +{ + +class DirIter::Implementation +{ + public: fs::directory_iterator it; +}; + +////////////////////////////////////////////////// +DirIter::DirIter(): + dataPtr(ignition::utils::MakeImpl()) +{ + this->dataPtr->it = fs::directory_iterator(); +} + +////////////////////////////////////////////////// +DirIter::DirIter(const std::string &_in): + DirIter() +{ + try + { + this->dataPtr->it = fs::directory_iterator(_in); + } + catch (const fs::filesystem_error &ex) + { + } +} + +////////////////////////////////////////////////// +std::string DirIter::operator*() const +{ + return this->dataPtr->it->path().string(); +} + +////////////////////////////////////////////////// +const DirIter &DirIter::operator++() +{ + this->dataPtr->it++; + return *this; +} + +////////////////////////////////////////////////// +bool DirIter::operator!=(const DirIter &_other) const +{ + return (this->dataPtr->it != _other.dataPtr->it); +} + +////////////////////////////////////////////////// +void DirIter::Next() {} + +////////////////////////////////////////////////// +void DirIter::SetInternalEmpty() {} + +////////////////////////////////////////////////// +void DirIter::CloseHandle() {} + +} // namespace common +} // namespace ignition + diff --git a/src/Filesystem.cc b/src/Filesystem.cc index cc213f350..69af52817 100644 --- a/src/Filesystem.cc +++ b/src/Filesystem.cc @@ -15,16 +15,10 @@ * */ -#include - -#ifdef __linux__ -#include -#endif - -#include #include #include #include +#include #include #include #include @@ -36,466 +30,289 @@ #include #include #include +#include "ignition/common/Filesystem.hh" + +namespace fs = std::filesystem; #ifndef _WIN32 -#include -#include -#include +static const char preferred_separator = fs::path::preferred_separator; #else -#include -#include -#include "win_dirent.h" -#include "PrintWindowsSystemWarning.hh" +static const char preferred_separator = '\\'; #endif -#include "ignition/common/Filesystem.hh" - -#ifdef _WIN32 -# define IGN_PATH_MAX _MAX_PATH -#elif defined(PATH_MAX) -# define IGN_PATH_MAX PATH_MAX -#elif defined(_XOPEN_PATH_MAX) -# define IGN_PATH_MAX _XOPEN_PATH_MAX -#else -# define IGN_PATH_MAX _POSIX_PATH_MAX -#endif +// Return true if success, false if error +inline bool fs_warn(const std::string &_fcn, + const std::error_code &_ec, + const ignition::common::FilesystemWarningOp &_warningOp = + ignition::common::FSWO_LOG_WARNINGS) +{ -namespace igncmn = ignition::common; -using namespace ignition; -using namespace igncmn; + if (_ec) + { + if (ignition::common::FSWO_LOG_WARNINGS == _warningOp) + { + ignwarn << "Failed ignition::common::" << _fcn + << " (ec: " << _ec << " " << _ec.message() << ")\n"; + } + return false; + } + return true; +} ///////////////////////////////////////////////// -bool ignition::common::isFile(const std::string &_path) +bool ignition::common::exists(const std::string &_path) { - std::ifstream f(_path); - return (!isDirectory(_path)) && f.good(); + return fs::exists(_path); } ///////////////////////////////////////////////// -bool ignition::common::removeDirectory(const std::string &_path, - const FilesystemWarningOp _warningOp) +bool ignition::common::isDirectory(const std::string &_path) { - bool removed = false; - if (ignition::common::isDirectory(_path)) - { -#ifdef _WIN32 - removed = RemoveDirectory(_path.c_str()); - if (!removed && FSWO_LOG_WARNINGS == _warningOp) - { - ignition::common::PrintWindowsSystemWarning( - "Failed to remove directory [" + _path + "]"); - } -#else - removed = (rmdir(_path.c_str()) == 0); - if (!removed) - { - // A sym link would end up here - removed = (std::remove(_path.c_str()) == 0); - } - - if (!removed && FSWO_LOG_WARNINGS == _warningOp) - { - ignwarn << "Failed to remove directory [" + _path + "]: " - << std::strerror(errno) << "\n"; - } -#endif - } - else if (_warningOp) - { - ignwarn << "The path [" << _path << "] does not refer to a directory\n"; - } - - return removed; + return fs::is_directory(_path); } ///////////////////////////////////////////////// -bool ignition::common::removeFile(const std::string &_existingFilename, - const FilesystemWarningOp _warningOp) +bool ignition::common::isFile(const std::string &_path) { - const bool removed = (std::remove(_existingFilename.c_str()) == 0); - if (!removed && FSWO_LOG_WARNINGS == _warningOp) - { - ignwarn << "Failed to remove file [" << _existingFilename << "]: " - << std::strerror(errno) << "\n"; - } + return fs::is_regular_file(_path); +} - return removed; +///////////////////////////////////////////////// +bool ignition::common::createDirectory(const std::string &_path) +{ + std::error_code ec; + fs::create_directory(_path, ec); + return fs_warn("createDirectory", ec); } ///////////////////////////////////////////////// -bool ignition::common::removeDirectoryOrFile( - const std::string &_path, - const FilesystemWarningOp _warningOp) +bool ignition::common::createDirectories(const std::string &_path) { - if (ignition::common::isDirectory(_path)) - { - return ignition::common::removeDirectory(_path, _warningOp); - } - else if (ignition::common::isFile(_path)) - { - return ignition::common::removeFile(_path, _warningOp); - } - else if (FSWO_LOG_WARNINGS == _warningOp) - { - ignwarn << "The path [" << _path << "] does not refer to a " - << "directory nor to a file\n"; - } - return false; + std::error_code ec; + // Disregard return here because it may return false if the + // directory is not actually created (already exists) + fs::create_directories(_path, ec); + return fs_warn("createDirectories", ec); } ///////////////////////////////////////////////// -bool ignition::common::removeAll(const std::string &_path, - const FilesystemWarningOp _warningOp) +std::string const ignition::common::separator(std::string const &_s) { - if (ignition::common::isDirectory(_path)) - { - DIR *dir = opendir(_path.c_str()); - if (dir) - { - struct dirent *p; - while ((p=readdir(dir))) - { - // Skip special files. - if (!std::strcmp(p->d_name, ".") || !std::strcmp(p->d_name, "..")) - continue; - - const auto removed = ignition::common::removeAll( - ignition::common::joinPaths(_path, p->d_name), _warningOp); - if (!removed) - return false; - } - } - closedir(dir); - } + fs::path path(_s); + return (_s / fs::path("")).string(); +} - return ignition::common::removeDirectoryOrFile(_path, _warningOp); +///////////////////////////////////////////////// +void ignition::common::changeFromUnixPath(std::string &_path) { + + std::replace(_path.begin(), _path.end(), '/', preferred_separator); } ///////////////////////////////////////////////// -bool ignition::common::moveFile(const std::string &_existingFilename, - const std::string &_newFilename, - const FilesystemWarningOp _warningOp) +std::string ignition::common::copyFromUnixPath(const std::string &_path) { - if (!copyFile(_existingFilename, _newFilename, _warningOp)) - return false; - - if (removeFile(_existingFilename, _warningOp)) - return true; + std::string copy = _path; + changeFromUnixPath(copy); + return copy; +} - // The original file could not be removed, which means we are not - // able to "move" it (we can only copy it, apparently). Since this - // function is meant to move files, and we have failed to move the - // file, we should remove the copy that we made earlier. - removeFile(_newFilename, _warningOp); +///////////////////////////////////////////////// +void ignition::common::changeToUnixPath(std::string &_path) { + std::replace(_path.begin(), _path.end(), preferred_separator, '/'); +} - return false; +///////////////////////////////////////////////// +std::string ignition::common::copyToUnixPath(const std::string &_path) +{ + std::string copy = _path; + changeToUnixPath(copy); + return copy;; } ///////////////////////////////////////////////// std::string ignition::common::absPath(const std::string &_path) { - std::string result; + return fs::absolute(_path).string(); +} - // cppcheck-suppress ConfigurationNotChecked - char path[IGN_PATH_MAX] = ""; -#ifdef _WIN32 - if (GetFullPathName(_path.c_str(), IGN_PATH_MAX, &path[0], nullptr) != 0) -#else - if (realpath(_path.c_str(), &path[0]) != nullptr) -#endif - result = path; - else if (!_path.empty()) - { - // If _path is an absolute path, then return _path. - // An absolute path on Windows is a character followed by a colon and a - // backslash. - if (_path.compare(0, 1, "/") == 0 || _path.compare(1, 3, ":\\") == 0) - result = _path; - // Otherwise return the current working directory with _path appended. - else - result = joinPaths(ignition::common::cwd(), _path); - } +///////////////////////////////////////////////// +std::string ignition::common::joinPaths( + const std::string &_path1, const std::string &_path2) +{ + fs::path p1{_path1}; + fs::path p2{_path2}; - ignition::common::replaceAll(result, result, "//", "/"); + bool is_url = false; - return result; -} + if (_path1.find("://") == std::string::npos) + p1 = p1.lexically_normal(); + else + is_url = true; -// This is help function to handle windows paths, there are a mix between '/' -// and backslashes. -// joinPaths uses the system separator, in Windows this generate some issues -// with URIs -std::string checkWindowsPath(const std::string _path) -{ - if (_path.empty()) - return _path; + if (_path2.find("://") == std::string::npos) + p2 = p2.lexically_normal(); + else + is_url = true; - // Check if this is a http or https, if so change backslashes generated by - // jointPaths to '/' - if ((_path.size() > 7 && 0 == _path.compare(0, 7, "http://")) || - (_path.size() > 8 && 0 == _path.compare(0, 8, "https://"))) + if (p2.string()[0] == preferred_separator) { - return std::regex_replace(_path, std::regex(R"(\\)"), "/"); + p2 = fs::path{p2.string().substr(1)}; } - // This is a Windows path, convert all '/' into backslashes - std::string result = std::regex_replace(_path, std::regex(R"(/)"), "\\"); - std::string drive_letters; + auto ret = (p1 / p2); - // only Windows contains absolute paths starting with drive letters - if (result.length() > 3 && 0 == result.compare(1, 2, ":\\")) - { - drive_letters = result.substr(0, 3); - result = result.substr(3); - } - result = drive_letters + std::regex_replace( - result, std::regex("[<>:\"|?*]"), ""); - return result; + if (!is_url) + ret = ret.lexically_normal(); + + return ret.string(); } -////////////////////////////////////////////////// -std::string ignition::common::joinPaths(const std::string &_path1, - const std::string &_path2) +///////////////////////////////////////////////// +std::string ignition::common::cwd() { + std::error_code ec; + auto curdir = fs::current_path(ec); - /// This function is used to avoid duplicated path separators at the - /// beginning/end of the string, and between the two paths being joined. - /// \param[in] _path This is the string to sanitize. - /// \param[in] _stripLeading True if the leading separator should be - /// removed. - auto sanitizeSlashes = [](const std::string &_path, - bool _stripLeading = false) + if (!fs_warn("cwd", ec)) { - // Shortcut - if (_path.empty()) - return _path; - - std::string result = _path; - - // Use the appropriate character for each platform. -#ifndef _WIN32 - char replacement = '/'; -#else - char replacement = '\\'; -#endif + curdir = ""; + } - // Sanitize the start of the path. - size_t index = 0; - size_t leadingIndex = _stripLeading ? 0 : 1; - for (; result[index] == replacement; ++index) - { - } - if (index > leadingIndex) - result.erase(leadingIndex, index-leadingIndex); + return curdir.string(); +} - // Sanitize the end of the path. - index = result.length()-1; - for (; result[index] == replacement; --index) - { - } - index += 1; - if (index < result.length()-1) - result.erase(index+1); - return result; - }; +///////////////////////////////////////////////// +bool ignition::common::chdir(const std::string &_dir) +{ + std::error_code ec; + fs::current_path(_dir, ec); + return fs_warn("chdir", ec); +} - std::string path; -#ifndef _WIN32 - path = sanitizeSlashes(sanitizeSlashes(separator(_path1)) + - sanitizeSlashes(_path2, true)); -#else // _WIN32 - std::string path1 = sanitizeSlashes(checkWindowsPath(_path1)); - std::string path2 = sanitizeSlashes(checkWindowsPath(_path2), true); - std::vector combined(path1.length() + path2.length() + 2); - if (::PathCombineA(combined.data(), path1.c_str(), path2.c_str()) != NULL) - { - path = sanitizeSlashes(checkWindowsPath(std::string(combined.data()))); - } - else - { - path = sanitizeSlashes(checkWindowsPath(separator(path1) + path2)); - } -#endif // _WIN32 - return path; +///////////////////////////////////////////////// +std::string ignition::common::basename(const std::string &_path) +{ + fs::path p(_path); + // Maintain compatibility with ign-common + if (*_path.rbegin() == preferred_separator) + p = fs::path(_path.substr(0, _path.size()-1)); + return p.filename().string(); } ///////////////////////////////////////////////// std::string ignition::common::parentPath(const std::string &_path) { - std::string result; - - size_t last_sep = _path.find_last_of(separator("")); - // If slash is the last character, find its parent directory - if (last_sep == _path.length() - 1) - last_sep = _path.substr(0, last_sep).find_last_of(separator("")); - - result = _path.substr(0, last_sep); - - return result; + fs::path p(_path); + // Maintain compatibility with ign-common + if (*_path.rbegin() == preferred_separator) + p = fs::path(_path.substr(0, _path.size()-1)); + return p.parent_path().string(); } ///////////////////////////////////////////////// -bool ignition::common::copyFile(const std::string &_existingFilename, - const std::string &_newFilename, - const FilesystemWarningOp _warningOp) +bool ignition::common::copyFile( + const std::string &_existingFilename, + const std::string &_newFilename, + const FilesystemWarningOp _warningOp) { - std::string absExistingFilename = - ignition::common::absPath(_existingFilename); - std::string absNewFilename = ignition::common::absPath(_newFilename); - - if (absExistingFilename == absNewFilename) - return false; + const auto copyOptions = fs::copy_options::overwrite_existing; + std::error_code ec; + auto ret = fs::copy_file(_existingFilename, _newFilename, copyOptions, ec); + return ret && fs_warn("copyFile", ec, _warningOp); +} -#ifdef _WIN32 - const bool copied = CopyFile(absExistingFilename.c_str(), - absNewFilename.c_str(), false); +///////////////////////////////////////////////// +bool ignition::common::copyDirectory( + const std::string &_existingDirname, + const std::string &_newDirname, + const FilesystemWarningOp _warningOp) +{ + const auto copyOptions = fs::copy_options::recursive + | fs::copy_options::overwrite_existing; - if (!copied && FSWO_LOG_WARNINGS == _warningOp) + // std::filesystem won't create intermediate directories + // before copying, this maintains compatibility with ignition behavior. + if (!ignition::common::createDirectories(_newDirname)) { - ignition::common::PrintWindowsSystemWarning( - "Failed to copy file [" + absExistingFilename - + "] to [" + absNewFilename + "]"); + return false; } - return copied; -#else - bool result = false; - std::ifstream in(absExistingFilename.c_str(), std::ifstream::binary); - - if (in.good()) - { - std::ofstream out(absNewFilename.c_str(), - std::ifstream::trunc | std::ifstream::binary); - if (out.good()) - { - out << in.rdbuf(); - result = ignition::common::isFile(absNewFilename); - } - else if (FSWO_LOG_WARNINGS == _warningOp) - { - ignwarn << "Failed to create file [" << absNewFilename << "]: " - << std::strerror(errno) << "\n"; - } - out.close(); - } - else if (FSWO_LOG_WARNINGS == _warningOp) - { - ignwarn << "Failed to open file [" << absExistingFilename << "]: " - << std::strerror(errno) << "\n"; - } - in.close(); + std::error_code ec; + fs::copy(_existingDirname, _newDirname, copyOptions, ec); + return fs_warn("copyDirectory", ec, _warningOp); +} - return result; -#endif +///////////////////////////////////////////////// +bool ignition::common::moveFile( + const std::string &_existingFilename, + const std::string &_newFilename, + const FilesystemWarningOp _warningOp) +{ + std::error_code ec; + fs::rename(_existingFilename, _newFilename, ec); + return fs_warn("moveFile", ec, _warningOp); } ///////////////////////////////////////////////// -bool ignition::common::copyDirectory(const std::string &_existingDirname, - const std::string &_newDirname, - const FilesystemWarningOp _warningOp) +bool ignition::common::removeDirectory( + const std::string &_path, + const FilesystemWarningOp _warningOp) { - // Check whether source directory exists - if (!exists(_existingDirname) || !isDirectory(_existingDirname)) + if (!isDirectory(_path)) { if (FSWO_LOG_WARNINGS == _warningOp) { - ignwarn << "Source directory [" << _existingDirname - << "] does not exist or is not a directory" << std::endl; + ignwarn << "Cannot remove, not a directory [" << _path << "]\n"; } + return false; } - if (exists(_newDirname)) - { - if (!removeAll(_newDirname, _warningOp)) - { - if (FSWO_LOG_WARNINGS == _warningOp) - { - ignwarn << "Unable to remove existing destination directory [" - << _newDirname << "]\n"; - } - return false; - } - } - // Create the destination directory - if (!createDirectories(_newDirname)) + return removeDirectoryOrFile(_path, _warningOp); +} + +///////////////////////////////////////////////// +bool ignition::common::removeFile( + const std::string &_existingFilename, + const FilesystemWarningOp _warningOp) +{ + if (!isFile(_existingFilename)) { if (FSWO_LOG_WARNINGS == _warningOp) { - ignwarn << "Unable to create the destination directory [" - << _newDirname << "], please check the permission\n"; + ignwarn << "Cannot remove, not a file [" << _existingFilename << "]\n"; } return false; } - // Start copy from source to destination directory - for (DirIter file(_existingDirname); file != DirIter(); ++file) - { - std::string current(*file); - if (isDirectory(current)) - { - // Copy recursively - if (!copyDirectory(current, joinPaths(_newDirname, basename(current)), - _warningOp)) - { - if (FSWO_LOG_WARNINGS == _warningOp) - { - ignwarn << "Unable to copy directory to [" - << joinPaths(_newDirname, basename(current)) << "]\n"; - } - return false; - } - } - else - { - if (!copyFile(current, joinPaths(_newDirname, basename(current)), - _warningOp)) - { - if (FSWO_LOG_WARNINGS == _warningOp) - { - ignwarn << "Unable to copy file to [" - << joinPaths(_newDirname, basename(current)) << "]\n"; - } - return false; - } - } - } - return true; + return removeDirectoryOrFile(_existingFilename, _warningOp); } ///////////////////////////////////////////////// -bool ignition::common::createDirectories(const std::string &_path) +bool ignition::common::removeDirectoryOrFile( + const std::string &_path, + const FilesystemWarningOp _warningOp) { - size_t index = 0; - while (index < _path.size()) - { - size_t end = _path.find(separator(""), index+1); - std::string dir = _path.substr(0, end); - if (!exists(dir)) - { -#ifdef _WIN32 - dir = checkWindowsPath(dir); - if (_mkdir(dir.c_str()) != 0) - { -#else - // cppcheck-suppress ConfigurationNotChecked - if (mkdir(dir.c_str(), S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH) != 0) - { -#endif - ignerr << "Failed to create directory [" + dir + "]: " - << std::strerror(errno) << std::endl; - return false; - } - } - index = end; - } + std::error_code ec; + auto ret = fs::remove(_path, ec); + fs_warn("removeDirectoryOrFile", ec, _warningOp); + return ret; +} - return true; +///////////////////////////////////////////////// +bool ignition::common::removeAll( + const std::string &_path, + const FilesystemWarningOp _warningOp) +{ + std::error_code ec; + fs::remove_all(_path, ec); + return fs_warn("removeAll", ec, _warningOp); } -////////////////////////////////////////////////// -std::string ignition::common::uniqueFilePath(const std::string &_pathAndName, - const std::string &_extension) +///////////////////////////////////////////////// +std::string ignition::common::uniqueFilePath( + const std::string &_pathAndName, + const std::string &_extension) { std::string result = _pathAndName + "." + _extension; int count = 1; @@ -510,7 +327,7 @@ std::string ignition::common::uniqueFilePath(const std::string &_pathAndName, return result; } -////////////////////////////////////////////////// +///////////////////////////////////////////////// std::string ignition::common::uniqueDirectoryPath(const std::string &_dir) { std::string result = _dir; diff --git a/src/FilesystemBoost.cc b/src/FilesystemBoost.cc deleted file mode 100644 index 12e5cabc5..000000000 --- a/src/FilesystemBoost.cc +++ /dev/null @@ -1,638 +0,0 @@ -/* - * Copyright 2002-2009, 2014 Beman Dawes - * Copyright 2001 Dietmar Kuehl - * - * Distributed under the Boost Software License, Version 1.0. - * - * Boost Software License - Version 1.0 - August 17th, 2003 - * - * Permission is hereby granted, free of charge, to any person or organization - * obtaining a copy of the software and accompanying documentation covered by - * this license (the "Software") to use, reproduce, display, distribute, - * execute, and transmit the Software, and to prepare derivative works of the - * Software, and to permit third-parties to whom the Software is furnished to - * do so, all subject to the following: - * - * The copyright notices in the Software and this entire statement, including - * the above license grant, this restriction and the following disclaimer, - * must be included in all copies of the Software, in whole or in part, and - * all derivative works of the Software, unless such copies or derivative - * works are solely in the form of machine-executable object code generated by - * a source language processor. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE, TITLE AND NON-INFRINGEMENT. IN NO EVENT - * SHALL THE COPYRIGHT HOLDERS OR ANYONE DISTRIBUTING THE SOFTWARE BE LIABLE - * FOR ANY DAMAGES OR OTHER LIABILITY, WHETHER IN CONTRACT, TORT OR OTHERWISE, - * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER - * DEALINGS IN THE SOFTWARE. - */ - -/* - * Most of this code was borrowed from Boost in - * libs/filesystem/src/operations.cpp and - * libs/filesystem/include/boost/filesystem/operations.hpp. - */ - -#include -#include -#include -#include - -#ifndef _WIN32 -#include -#include -#include -#include -#include -#include -#include -#include -#else -#include -#include -#include -#endif - -#include "ignition/common/Console.hh" -#include "ignition/common/Filesystem.hh" - -namespace ignition -{ - namespace common - { - /// \internal - /// \brief Private data for the DirIter class. - class DirIter::Implementation - { - /// \def current - /// \brief The current directory item. - public: std::string current; - - /// \def dirname - /// \brief The original path to the directory. - public: std::string dirname; - - /// \def handle - /// \brief Opaque handle for holding the directory iterator. - public: void *handle; - - /// \def end - /// \brief Private variable to indicate whether the iterator has reached - /// the end. - public: bool end; - }; - -#ifndef _WIN32 - - static const char preferred_separator = '/'; - - ////////////////////////////////////////////////// - bool exists(const std::string &_path) - { - struct stat path_stat; - return ::stat(_path.c_str(), &path_stat) == 0; - } - - ////////////////////////////////////////////////// - bool isDirectory(const std::string &_path) - { - struct stat path_stat; - - if (::stat(_path.c_str(), &path_stat) != 0) - { - return false; - } - - return S_ISDIR(path_stat.st_mode); - } - - ////////////////////////////////////////////////// - bool createDirectory(const std::string &_path) - { - return ::mkdir(_path.c_str(), S_IRWXU|S_IRWXG|S_IRWXO) == 0; - } - - ////////////////////////////////////////////////// - std::string cwd() - { - std::string cur; - // loop 'til buffer large enough - for (int32_t path_max = 128;; path_max *= 2) - { - std::vector buf(path_max); - - if (::getcwd(buf.data(), buf.size()) == 0) - { - if (errno != ERANGE) - { - break; - } - } - else - { - char resolved[PATH_MAX]; - - if (realpath(buf.data(), resolved) != nullptr) - { - cur = std::string(resolved); - } - break; - } - } - return cur; - } - - ////////////////////////////////////////////////// - bool chdir(const std::string& _dir) - { - return ::chdir(_dir.c_str()) == 0; - } - - ////////////////////////////////////////////////// - DirIter::DirIter(const std::string &_in) - : dataPtr(ignition::utils::MakeImpl()) - { - this->dataPtr->dirname = _in; - - this->dataPtr->current = ""; - - this->dataPtr->handle = opendir(_in.c_str()); - - this->dataPtr->end = false; - - if (this->dataPtr->handle == nullptr) - { - this->dataPtr->end = true; - } - else - { - Next(); - } - } - - ////////////////////////////////////////////////// - void DirIter::Next() - { - while (true) - { - struct dirent *entry = - readdir(reinterpret_cast(this->dataPtr->handle)); // NOLINT - if (!entry) - { - this->dataPtr->end = true; - this->dataPtr->current = ""; - break; - } - - if ((strcmp(entry->d_name, ".") != 0) - && (strcmp(entry->d_name, "..") != 0)) - { - this->dataPtr->current = std::string(entry->d_name); - break; - } - } - } - - ////////////////////////////////////////////////// - void DirIter::CloseHandle() - { - closedir(reinterpret_cast(this->dataPtr->handle)); - } - -#else // Windows - - static const char preferred_separator = '\\'; - - ////////////////////////////////////////////////// - static bool not_found_error(int _errval) - { - return _errval == ERROR_FILE_NOT_FOUND - || _errval == ERROR_PATH_NOT_FOUND - || _errval == ERROR_INVALID_NAME // "tools/src/:sys:stat.h", "//foo" - || _errval == ERROR_INVALID_DRIVE // USB card reader with no card - || _errval == ERROR_NOT_READY // CD/DVD drive with no disc inserted - || _errval == ERROR_INVALID_PARAMETER // ":sys:stat.h" - || _errval == ERROR_BAD_PATHNAME // "//nosuch" on Win64 - || _errval == ERROR_BAD_NETPATH; // "//nosuch" on Win32 - } - - ////////////////////////////////////////////////// - static bool process_status_failure() - { - int errval(::GetLastError()); - - if (not_found_error(errval)) - { - return false; - } - else if ((errval == ERROR_SHARING_VIOLATION)) - { - return true; // odd, but this is what boost does - } - return false; - } - - struct handle_wrapper - { - HANDLE handle; - explicit handle_wrapper(HANDLE h) - : handle(h) {} - ~handle_wrapper() - { - if (handle != INVALID_HANDLE_VALUE) - { - ::CloseHandle(handle); - } - } - }; - - // REPARSE_DATA_BUFFER related definitions are in ntifs.h, which is part of - // the Windows Device Driver Kit. Since it's inconvenient, the definitions - // are provided here. http://msdn.microsoft.com/en-us/library/ms791514.aspx - - typedef struct _REPARSE_DATA_BUFFER { - ULONG ReparseTag; - USHORT ReparseDataLength; - USHORT Reserved; - union { - struct { - USHORT SubstituteNameOffset; - USHORT SubstituteNameLength; - USHORT PrintNameOffset; - USHORT PrintNameLength; - ULONG Flags; - WCHAR PathBuffer[1]; - /* Example of distinction between substitute and print names: - mklink /d ldrive c:\ - SubstituteName: c:\\??\ - PrintName: c:\ - */ - } SymbolicLinkReparseBuffer; - struct { - USHORT SubstituteNameOffset; - USHORT SubstituteNameLength; - USHORT PrintNameOffset; - USHORT PrintNameLength; - WCHAR PathBuffer[1]; - } MountPointReparseBuffer; - struct { - UCHAR DataBuffer[1]; - } GenericReparseBuffer; - }; - } REPARSE_DATA_BUFFER, *PREPARSE_DATA_BUFFER; - - ////////////////////////////////////////////////// - HANDLE create_file_handle(const std::string &_path, DWORD _dwDesiredAccess, - DWORD _dwShareMode, - LPSECURITY_ATTRIBUTES _lpSecurityAttributes, - DWORD _dwCreationDisposition, - DWORD _dwFlagsAndAttributes, - HANDLE _hTemplateFile) - { - return ::CreateFileA(_path.c_str(), _dwDesiredAccess, - _dwShareMode, _lpSecurityAttributes, - _dwCreationDisposition, _dwFlagsAndAttributes, - _hTemplateFile); - } - -#ifndef MAXIMUM_REPARSE_DATA_BUFFER_SIZE -#define MAXIMUM_REPARSE_DATA_BUFFER_SIZE (16 * 1024) -#endif - - ////////////////////////////////////////////////// - bool is_reparse_point_a_symlink(const std::string &_path) - { - handle_wrapper h(create_file_handle(_path, FILE_READ_EA, - FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, - nullptr, OPEN_EXISTING, - FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT, - nullptr)); - if (h.handle == INVALID_HANDLE_VALUE) - { - return false; - } - - std::vector buf(MAXIMUM_REPARSE_DATA_BUFFER_SIZE); - - // Query the reparse data - DWORD dwRetLen; - BOOL result = ::DeviceIoControl(h.handle, FSCTL_GET_REPARSE_POINT, - nullptr, 0, buf.data(), (DWORD)buf.size(), &dwRetLen, nullptr); - if (!result) - { - return false; - } - - return reinterpret_cast(&buf[0])->ReparseTag - == IO_REPARSE_TAG_SYMLINK - // Issue 9016 asked that NTFS directory junctions be recognized as - // directories. That is equivalent to recognizing them as symlinks, and - // then the normal symlink mechanism will recognize them as directories. - // - // Directory junctions are very similar to symlinks, but have some - // performance and other advantages over symlinks. They can be created - // from the command line with "mklink /j junction-name target-path". - || reinterpret_cast(&buf[0])->ReparseTag - == IO_REPARSE_TAG_MOUNT_POINT; // "directory junction" or "junction" - } - - ////////////////////////////////////////////////// - bool internal_check_path(const std::string &_path, DWORD &attr) - { - attr = ::GetFileAttributesA(_path.c_str()); - if (attr == INVALID_FILE_ATTRIBUTES) - { - return process_status_failure(); - } - - // reparse point handling; - // since GetFileAttributesW does not resolve symlinks, try to open file - // handle to discover if the file exists - if (attr & FILE_ATTRIBUTE_REPARSE_POINT) - { - handle_wrapper h( - create_file_handle( - _path, - 0, // dwDesiredAccess; attributes only - FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, - 0, // lpSecurityAttributes - OPEN_EXISTING, - FILE_FLAG_BACKUP_SEMANTICS, - 0)); // hTemplateFile - if (h.handle == INVALID_HANDLE_VALUE) - { - return process_status_failure(); - } - - if (!is_reparse_point_a_symlink(_path)) - { - return true; - } - } - - return true; - } - - ////////////////////////////////////////////////// - bool exists(const std::string &_path) - { - DWORD attr; - return internal_check_path(_path, attr); - } - - ////////////////////////////////////////////////// - bool isDirectory(const std::string &_path) - { - DWORD attr; - - if (internal_check_path(_path, attr)) - { - return (attr & FILE_ATTRIBUTE_DIRECTORY) != 0; - } - - return false; - } - - ////////////////////////////////////////////////// - bool createDirectory(const std::string &_path) - { - return ::CreateDirectoryA(_path.c_str(), 0) != 0; - } - - ////////////////////////////////////////////////// - std::string cwd() - { - DWORD sz; - if ((sz = ::GetCurrentDirectoryA(0, nullptr)) == 0) - { - sz = 1; - } - - std::vector buf(sz); - - if (::GetCurrentDirectoryA(sz, buf.data()) == 0) - { - // error - return std::string(""); - } - else - { - return buf.data(); - } - } - - ///////////////////////////////////////////////// - bool chdir(const std::string& _dir) - { - return ::SetCurrentDirectoryA(_dir.c_str()); - } - - ////////////////////////////////////////////////// - DirIter::DirIter(const std::string &_in) - : dataPtr(ignition::utils::MakeImpl()) - { - // use a form of search Sebastian Martel reports will work with Win98 - this->dataPtr->dirname = _in; - - this->dataPtr->current = ""; - - this->dataPtr->end = false; - - if (_in.empty()) - { - // To be compatible with Unix, if given an empty string, assume this - // is the end. - this->dataPtr->end = true; - return; - } - - std::string dirpath(_in); - dirpath += (dirpath.empty() - || (dirpath[dirpath.size()-1] != '\\' - && dirpath[dirpath.size()-1] != '/' - && dirpath[dirpath.size()-1] != ':'))? "\\*" : "*"; - - WIN32_FIND_DATAA data; - this->dataPtr->handle = ::FindFirstFileA(dirpath.c_str(), &data); - - // Signal EOF - if (this->dataPtr->handle == INVALID_HANDLE_VALUE) - { - this->dataPtr->handle = nullptr; - this->dataPtr->end = true; - return; - } - - this->dataPtr->current = std::string(data.cFileName); - - // Skip "." and ".." - while (this->dataPtr->current == "." || - this->dataPtr->current == "..") - { - this->Next(); - } - } - - ////////////////////////////////////////////////// - void DirIter::Next() - { - WIN32_FIND_DATAA data; - data.cFileName[0] = 0; - while (data.cFileName[0] == 0 || - std::string(data.cFileName) == "." || - std::string(data.cFileName) == "..") - { - auto found = ::FindNextFileA(this->dataPtr->handle, &data); - // Signal EOF - if (!found) - { - this->dataPtr->end = true; - this->dataPtr->current = ""; - return; - } - } - - this->dataPtr->current = std::string(data.cFileName); - } - - ////////////////////////////////////////////////// - void DirIter::CloseHandle() - { - ::FindClose(this->dataPtr->handle); - } - -#endif // _WIN32 - - ////////////////////////////////////////////////// - const std::string separator(const std::string &_p) - { - return _p + preferred_separator; - } - - ////////////////////////////////////////////////// - void changeFromUnixPath(std::string &_path) - { - // cppcheck-suppress knownConditionTrueFalse - // cppcheck-suppress unmatchedSuppression - if ('/' == preferred_separator) - return; - - std::replace(_path.begin(), _path.end(), '/', preferred_separator); - } - - ////////////////////////////////////////////////// - std::string copyFromUnixPath(const std::string &_path) - { - std::string copy = _path; - changeFromUnixPath(copy); - return copy; - } - - ////////////////////////////////////////////////// - void changeToUnixPath(std::string &_path) - { - // cppcheck-suppress knownConditionTrueFalse - // cppcheck-suppress unmatchedSuppression - if ('/' == preferred_separator) - return; - - std::replace(_path.begin(), _path.end(), preferred_separator, '/'); - } - - ////////////////////////////////////////////////// - std::string copyToUnixPath(const std::string &_path) - { - std::string copy = _path; - changeToUnixPath(copy); - return copy; - } - - ////////////////////////////////////////////////// - std::string basename(const std::string &_path) - { - bool last_was_slash = false; - std::string basename; - - basename.reserve(_path.length()); - - for (size_t i = 0; i < _path.length(); ++i) - { - if (_path[i] == preferred_separator) - { - if (i == (_path.length() - 1)) - { - // if this is the last character, then according to basename we - // should return the portion of the path *before* the slash, i.e. - // the one we were just building. However, as a special case, if - // basename is empty, we return just a "/". - if (basename.size() == 0) - { - basename.push_back(preferred_separator); - } - break; - } - - last_was_slash = true; - } - else - { - if (last_was_slash) - { - last_was_slash = false; - basename.clear(); - } - - basename.push_back(_path[i]); - } - } - - return basename; - } - - ////////////////////////////////////////////////// - DirIter::DirIter() : dataPtr(ignition::utils::MakeImpl()) - { - this->dataPtr->current = ""; - - this->dataPtr->dirname = ""; - - this->dataPtr->handle = nullptr; - - this->dataPtr->end = true; - } - - ////////////////////////////////////////////////// - std::string DirIter::operator*() const - { - return this->dataPtr->dirname + preferred_separator + - this->dataPtr->current; - } - - ////////////////////////////////////////////////// - // prefix operator; note that we don't support the postfix operator - // because it is complicated to do so - const DirIter& DirIter::operator++() - { - Next(); - return *this; - } - - ////////////////////////////////////////////////// - bool DirIter::operator!=(const DirIter &_other) const - { - return this->dataPtr->end != _other.dataPtr->end; - } - - ////////////////////////////////////////////////// - DirIter::~DirIter() - { - if (this->dataPtr->handle != nullptr) - { - CloseHandle(); - this->dataPtr->handle = nullptr; - } - } - } -} diff --git a/src/TempDirectory.cc b/src/TempDirectory.cc index bdc8a3840..da07f4d9b 100644 --- a/src/TempDirectory.cc +++ b/src/TempDirectory.cc @@ -19,6 +19,8 @@ #include +#include + #ifdef _WIN32 #include #include @@ -26,6 +28,8 @@ #include #endif +namespace fs = std::filesystem; + using namespace ignition; using namespace common; @@ -47,43 +51,18 @@ 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 -// \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(); -#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 = temp_directory_path(ec); + auto ret = fs::temp_directory_path(ec); if (!fs_warn("tempDirectoryPath", ec)) { ret = ""; } - return ret; + return ret.string(); } ///////////////////////////////////////////////// @@ -95,11 +74,11 @@ std::string createTempDirectory( const std::string &_baseName, const std::string &_parentPath) { - std::string parentPath(_parentPath); - std::string templatePath = _baseName + "XXXXXX"; + fs::path parentPath(_parentPath); + fs::path templatePath = _baseName + "XXXXXX"; - std::string fullTemplateStr = joinPaths(parentPath, templatePath); - if (!createDirectories(parentPath)) + std::string fullTemplateStr = (parentPath / templatePath).string(); + if (!createDirectories(parentPath.string())) { std::error_code ec{errno, std::system_category()}; errno = 0; @@ -108,29 +87,32 @@ std::string createTempDirectory( #ifdef _WIN32 errno_t errcode = _mktemp_s(&fullTemplateStr[0], fullTemplateStr.size() + 1); - if (errcode) { + 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 std::string finalPath{fullTemplateStr}; - if (!createDirectories(finalPath)) { + 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) { + 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 std::string finalPath{dirName}; + const fs::path finalPath{dirName}; #endif - return finalPath; + return finalPath.string(); } ///////////////////////////////////////////////// @@ -140,7 +122,8 @@ std::string ignition::common::createTempDirectory( const FilesystemWarningOp _warningOp) { std::string ret; - try { + try + { ret = ::createTempDirectory(_baseName, _parentPath); } catch (const std::system_error &ex) @@ -192,6 +175,7 @@ TempDirectory::TempDirectory(const std::string &_prefix, this->dataPtr->isValid = true; common::chdir(this->dataPtr->path); } + this->dataPtr->path = common::cwd(); } ///////////////////////////////////////////////// From 4daabdab669958a728ffc73c8d553a5e004d5700 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 22 Dec 2021 15:19:08 -0600 Subject: [PATCH 02/15] Fixup Window URL test Signed-off-by: Michael Carroll --- src/Filesystem.cc | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/Filesystem.cc b/src/Filesystem.cc index 69af52817..fe48c36de 100644 --- a/src/Filesystem.cc +++ b/src/Filesystem.cc @@ -161,10 +161,14 @@ std::string ignition::common::joinPaths( auto ret = (p1 / p2); - if (!is_url) - ret = ret.lexically_normal(); - - return ret.string(); + if (is_url) + { + return copyToUnixPath(ret.string()); + } + else + { + return ret.lexically_normal().string(); + } } ///////////////////////////////////////////////// From 68bce1228bff2172222e7ec7d97c7a2708c3f671 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 22 Dec 2021 16:29:11 -0600 Subject: [PATCH 03/15] Increase test timeout to reduce flakes Signed-off-by: Michael Carroll --- test/performance/logging.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/performance/logging.cc b/test/performance/logging.cc index d06868a23..0808d237a 100644 --- a/test/performance/logging.cc +++ b/test/performance/logging.cc @@ -38,7 +38,7 @@ void WriteToFile(std::string result_filename, std::string content) std::cerr << "Error writing to " << result_filename << std::endl; } out << content << std::flush; - std::cout << content; + //std::cout << content; } void MeasurePeakDuringLogWrites(const size_t id, std::vector &result) @@ -196,7 +196,7 @@ TEST_P(LoggingTest, RunThreads) } INSTANTIATE_TEST_SUITE_P(LoggingTest, LoggingTest, - ::testing::Values(1, 2, 4, 8, 16, 32)); + ::testing::Values(1, 2, 4, 8, 16)); ///////////////////////////////////////////////// // This test is valid (passes) if it runs without segfaults. From 50455b618e8aeabc5f1be5849a3415d1f136522d Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Thu, 6 Jan 2022 12:26:32 -0600 Subject: [PATCH 04/15] Remove empty functions Signed-off-by: Michael Carroll --- include/ignition/common/Filesystem.hh | 9 --------- src/DirIter.cc | 10 ---------- 2 files changed, 19 deletions(-) diff --git a/include/ignition/common/Filesystem.hh b/include/ignition/common/Filesystem.hh index daf20a9a4..fb765d523 100644 --- a/include/ignition/common/Filesystem.hh +++ b/include/ignition/common/Filesystem.hh @@ -279,15 +279,6 @@ namespace ignition /// \return true if the iterators are equal, false otherwise. public: bool operator!=(const DirIter &_other) const; - /// \brief Move to the next directory record, skipping . and .. records. - private: void Next(); - - /// \brief Set the internal variable to the empty string. - private: void SetInternalEmpty(); - - /// \brief Close an open directory handle. - private: void CloseHandle(); - /// \brief Pointer to private data. IGN_UTILS_IMPL_PTR(dataPtr) }; diff --git a/src/DirIter.cc b/src/DirIter.cc index c0314f2e9..eceee6b32 100644 --- a/src/DirIter.cc +++ b/src/DirIter.cc @@ -70,15 +70,5 @@ bool DirIter::operator!=(const DirIter &_other) const return (this->dataPtr->it != _other.dataPtr->it); } -////////////////////////////////////////////////// -void DirIter::Next() {} - -////////////////////////////////////////////////// -void DirIter::SetInternalEmpty() {} - -////////////////////////////////////////////////// -void DirIter::CloseHandle() {} - } // namespace common } // namespace ignition - From 7b0ddfc6fadb1fabfa9218c85e18a94049db2b22 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Thu, 6 Jan 2022 12:40:36 -0600 Subject: [PATCH 05/15] fs_warn -> fsWarn Signed-off-by: Michael Carroll --- src/Filesystem.cc | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/Filesystem.cc b/src/Filesystem.cc index fe48c36de..04db00081 100644 --- a/src/Filesystem.cc +++ b/src/Filesystem.cc @@ -41,12 +41,11 @@ static const char preferred_separator = '\\'; #endif // Return true if success, false if error -inline bool fs_warn(const std::string &_fcn, +inline bool fsWarn(const std::string &_fcn, const std::error_code &_ec, const ignition::common::FilesystemWarningOp &_warningOp = ignition::common::FSWO_LOG_WARNINGS) { - if (_ec) { if (ignition::common::FSWO_LOG_WARNINGS == _warningOp) @@ -82,7 +81,7 @@ bool ignition::common::createDirectory(const std::string &_path) { std::error_code ec; fs::create_directory(_path, ec); - return fs_warn("createDirectory", ec); + return fsWarn("createDirectory", ec); } ///////////////////////////////////////////////// @@ -92,7 +91,7 @@ bool ignition::common::createDirectories(const std::string &_path) // Disregard return here because it may return false if the // directory is not actually created (already exists) fs::create_directories(_path, ec); - return fs_warn("createDirectories", ec); + return fsWarn("createDirectories", ec); } ///////////////////////////////////////////////// @@ -177,7 +176,7 @@ std::string ignition::common::cwd() std::error_code ec; auto curdir = fs::current_path(ec); - if (!fs_warn("cwd", ec)) + if (!fsWarn("cwd", ec)) { curdir = ""; } @@ -190,7 +189,7 @@ bool ignition::common::chdir(const std::string &_dir) { std::error_code ec; fs::current_path(_dir, ec); - return fs_warn("chdir", ec); + return fsWarn("chdir", ec); } ///////////////////////////////////////////////// @@ -222,7 +221,7 @@ bool ignition::common::copyFile( const auto copyOptions = fs::copy_options::overwrite_existing; std::error_code ec; auto ret = fs::copy_file(_existingFilename, _newFilename, copyOptions, ec); - return ret && fs_warn("copyFile", ec, _warningOp); + return ret && fsWarn("copyFile", ec, _warningOp); } ///////////////////////////////////////////////// @@ -243,7 +242,7 @@ bool ignition::common::copyDirectory( std::error_code ec; fs::copy(_existingDirname, _newDirname, copyOptions, ec); - return fs_warn("copyDirectory", ec, _warningOp); + return fsWarn("copyDirectory", ec, _warningOp); } ///////////////////////////////////////////////// @@ -254,7 +253,7 @@ bool ignition::common::moveFile( { std::error_code ec; fs::rename(_existingFilename, _newFilename, ec); - return fs_warn("moveFile", ec, _warningOp); + return fsWarn("moveFile", ec, _warningOp); } ///////////////////////////////////////////////// @@ -299,7 +298,7 @@ bool ignition::common::removeDirectoryOrFile( { std::error_code ec; auto ret = fs::remove(_path, ec); - fs_warn("removeDirectoryOrFile", ec, _warningOp); + fsWarn("removeDirectoryOrFile", ec, _warningOp); return ret; } @@ -310,7 +309,7 @@ bool ignition::common::removeAll( { std::error_code ec; fs::remove_all(_path, ec); - return fs_warn("removeAll", ec, _warningOp); + return fsWarn("removeAll", ec, _warningOp); } ///////////////////////////////////////////////// From 811fbfa3e9809c53020cfdfa68d06ec4bd7dd903 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Thu, 6 Jan 2022 12:42:29 -0600 Subject: [PATCH 06/15] Document DirIter::Implementation Signed-off-by: Michael Carroll Co-authored-by: Louise Poubel --- src/DirIter.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/DirIter.cc b/src/DirIter.cc index eceee6b32..d8b19ab90 100644 --- a/src/DirIter.cc +++ b/src/DirIter.cc @@ -28,6 +28,7 @@ namespace common class DirIter::Implementation { + /// \brief Filesystem iterator that this class is wrapping public: fs::directory_iterator it; }; From a01e98d7d213ea00c7bf18b0c6b4dfca0658f742 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Thu, 6 Jan 2022 12:46:48 -0600 Subject: [PATCH 07/15] Remove preferred_separator Signed-off-by: Michael Carroll --- src/Filesystem.cc | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/Filesystem.cc b/src/Filesystem.cc index 04db00081..6e80b7f32 100644 --- a/src/Filesystem.cc +++ b/src/Filesystem.cc @@ -34,12 +34,7 @@ namespace fs = std::filesystem; -#ifndef _WIN32 -static const char preferred_separator = fs::path::preferred_separator; -#else -static const char preferred_separator = '\\'; -#endif - +///////////////////////////////////////////////// // Return true if success, false if error inline bool fsWarn(const std::string &_fcn, const std::error_code &_ec, @@ -104,7 +99,7 @@ std::string const ignition::common::separator(std::string const &_s) ///////////////////////////////////////////////// void ignition::common::changeFromUnixPath(std::string &_path) { - std::replace(_path.begin(), _path.end(), '/', preferred_separator); + std::replace(_path.begin(), _path.end(), '/', fs::path::preferred_separator); } ///////////////////////////////////////////////// @@ -117,7 +112,7 @@ std::string ignition::common::copyFromUnixPath(const std::string &_path) ///////////////////////////////////////////////// void ignition::common::changeToUnixPath(std::string &_path) { - std::replace(_path.begin(), _path.end(), preferred_separator, '/'); + std::replace(_path.begin(), _path.end(), fs::path::preferred_separator, '/'); } ///////////////////////////////////////////////// @@ -153,7 +148,7 @@ std::string ignition::common::joinPaths( else is_url = true; - if (p2.string()[0] == preferred_separator) + if (p2.string()[0] == fs::path::preferred_separator) { p2 = fs::path{p2.string().substr(1)}; } @@ -197,7 +192,7 @@ std::string ignition::common::basename(const std::string &_path) { fs::path p(_path); // Maintain compatibility with ign-common - if (*_path.rbegin() == preferred_separator) + if (*_path.rbegin() == fs::path::preferred_separator) p = fs::path(_path.substr(0, _path.size()-1)); return p.filename().string(); } @@ -207,7 +202,7 @@ std::string ignition::common::parentPath(const std::string &_path) { fs::path p(_path); // Maintain compatibility with ign-common - if (*_path.rbegin() == preferred_separator) + if (*_path.rbegin() == fs::path::preferred_separator) p = fs::path(_path.substr(0, _path.size()-1)); return p.parent_path().string(); } From 948a4b7a3aff62af12e2ce3a1066292f1e3eb209 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Tue, 11 Jan 2022 14:46:59 -0600 Subject: [PATCH 08/15] Fix for Windows Signed-off-by: Michael Carroll --- src/Filesystem.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Filesystem.cc b/src/Filesystem.cc index 6e80b7f32..b4b512ec3 100644 --- a/src/Filesystem.cc +++ b/src/Filesystem.cc @@ -98,8 +98,8 @@ std::string const ignition::common::separator(std::string const &_s) ///////////////////////////////////////////////// void ignition::common::changeFromUnixPath(std::string &_path) { - - std::replace(_path.begin(), _path.end(), '/', fs::path::preferred_separator); + std::replace(_path.begin(), _path.end(), '/', + static_cast(fs::path::preferred_separator)); } ///////////////////////////////////////////////// @@ -112,7 +112,8 @@ std::string ignition::common::copyFromUnixPath(const std::string &_path) ///////////////////////////////////////////////// void ignition::common::changeToUnixPath(std::string &_path) { - std::replace(_path.begin(), _path.end(), fs::path::preferred_separator, '/'); + std::replace(_path.begin(), _path.end(), + static_cast(fs::path::preferred_separator), '/'); } ///////////////////////////////////////////////// From 020abd3cefdb3f4fcc8b73fbf589c4240aee0440 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Fri, 28 Jan 2022 12:25:02 -0600 Subject: [PATCH 09/15] Prevent initialization/destruction order fiasco Signed-off-by: Michael Carroll --- include/ignition/common/SystemPaths.hh | 13 ------------ src/Filesystem.cc | 4 ++-- src/SystemPaths.cc | 26 +++++++++-------------- src/TempDirectory.cc | 1 + src/Util.cc | 29 +++++++++++++++++++------- 5 files changed, 34 insertions(+), 39 deletions(-) diff --git a/include/ignition/common/SystemPaths.hh b/include/ignition/common/SystemPaths.hh index 9b8224525..d368dc737 100644 --- a/include/ignition/common/SystemPaths.hh +++ b/include/ignition/common/SystemPaths.hh @@ -17,16 +17,6 @@ #ifndef IGNITION_COMMON_SYSTEMPATHS_HH_ #define IGNITION_COMMON_SYSTEMPATHS_HH_ -#include - -#ifdef _WIN32 - #include - #define GetCurrentDir _getcwd -#else - #include - #define GetCurrentDir getcwd -#endif - #include #include #include @@ -41,9 +31,6 @@ namespace ignition { namespace common { - // Forward declare private data class - class SystemPathsPrivate; - /// \class SystemPaths SystemPaths.hh ignition/common/SystemPaths.hh /// \brief Functions to handle getting system paths, keeps track of: /// \li SystemPaths#pluginPaths - plugin library paths diff --git a/src/Filesystem.cc b/src/Filesystem.cc index b4b512ec3..50f8b9dd8 100644 --- a/src/Filesystem.cc +++ b/src/Filesystem.cc @@ -98,7 +98,7 @@ std::string const ignition::common::separator(std::string const &_s) ///////////////////////////////////////////////// void ignition::common::changeFromUnixPath(std::string &_path) { - std::replace(_path.begin(), _path.end(), '/', + std::replace(_path.begin(), _path.end(), '/', static_cast(fs::path::preferred_separator)); } @@ -112,7 +112,7 @@ std::string ignition::common::copyFromUnixPath(const std::string &_path) ///////////////////////////////////////////////// void ignition::common::changeToUnixPath(std::string &_path) { - std::replace(_path.begin(), _path.end(), + std::replace(_path.begin(), _path.end(), static_cast(fs::path::preferred_separator), '/'); } diff --git a/src/SystemPaths.cc b/src/SystemPaths.cc index 20ed8db4c..4358dac55 100644 --- a/src/SystemPaths.cc +++ b/src/SystemPaths.cc @@ -25,12 +25,6 @@ #include #include -#ifndef _WIN32 -#include -#else -#include "win_dirent.h" -#endif - #include "ignition/common/Console.hh" #include "ignition/common/StringUtils.hh" #include "ignition/common/SystemPaths.hh" @@ -101,18 +95,11 @@ SystemPaths::SystemPaths() else fullPath = path; - DIR *dir = opendir(fullPath.c_str()); - if (!dir) + if (!exists(fullPath)) { -#ifdef _WIN32 - mkdir(fullPath.c_str()); -#else - // cppcheck-suppress ConfigurationNotChecked - mkdir(fullPath.c_str(), S_IRWXU | S_IRGRP | S_IROTH); -#endif + createDirectories(fullPath); } - else - closedir(dir); + this->dataPtr->logPath = fullPath; // Populate this->dataPtr->filePaths with values from the default @@ -403,6 +390,8 @@ std::string SystemPaths::FindFile(const std::string &_filename, // Try appending to local paths else { + ignerr << "Here: " << cwd() << " " << filename << std::endl; + auto cwdPath = joinPaths(cwd(), filename); if (_searchLocalPath && exists(cwdPath)) { @@ -465,9 +454,12 @@ std::string SystemPaths::FindFile(const std::string &_filename, std::string SystemPaths::LocateLocalFile(const std::string &_filename, const std::vector &_paths) { + std::cerr << "LocateLocalFile: " << _filename << "\n"; + std::string foundPath = ""; for (auto const &path : _paths) { + std::cout << "\tPath: " << path << "\n"; std::string checkPath = NormalizeDirectoryPath(path) + _filename; if (exists(checkPath)) { @@ -531,6 +523,8 @@ void SystemPaths::AddFindFileURICallback( ///////////////////////////////////////////////// std::list SystemPaths::PathsFromEnv(const std::string &_env) { + std::cerr << "PathsFromEnv" << std::endl; + std::list paths; std::string envPathsStr; diff --git a/src/TempDirectory.cc b/src/TempDirectory.cc index da07f4d9b..13385b337 100644 --- a/src/TempDirectory.cc +++ b/src/TempDirectory.cc @@ -19,6 +19,7 @@ #include +#include #include #ifdef _WIN32 diff --git a/src/Util.cc b/src/Util.cc index de13ac071..71ff0340e 100644 --- a/src/Util.cc +++ b/src/Util.cc @@ -36,6 +36,8 @@ #include #include +#include + #ifndef _WIN32 #include #include @@ -55,8 +57,19 @@ static const auto &ignstrtok = strtok_r; #endif -static std::unique_ptr gSystemPaths( - new ignition::common::SystemPaths); +///////////////////////////////////////////////// +/// \brief Global instance of system paths for helper functions below +/// +/// This uses the NeverDestroyed pattern to prevent static initialization and +/// destruction order fiasco issues. +ignition::common::SystemPaths* GetSystemPaths() +{ + static const + ignition::utils::NeverDestroyed paths{ + new ignition::common::SystemPaths() + }; + return paths.Access(); +} ///////////////////////////////////////////////// // Internal class for SHA1 computation @@ -286,26 +299,26 @@ std::string ignition::common::timeToIso( ///////////////////////////////////////////////// std::string ignition::common::logPath() { - return gSystemPaths->LogPath(); + return GetSystemPaths()->LogPath(); } ///////////////////////////////////////////////// void ignition::common::addSearchPathSuffix(const std::string &_suffix) { - gSystemPaths->AddSearchPathSuffix(_suffix); + GetSystemPaths()->AddSearchPathSuffix(_suffix); } ///////////////////////////////////////////////// std::string ignition::common::findFile(const std::string &_file) { - return gSystemPaths->FindFile(_file, true); + return GetSystemPaths()->FindFile(_file, true); } ///////////////////////////////////////////////// std::string ignition::common::findFile(const std::string &_file, const bool _searchLocalPath) { - return gSystemPaths->FindFile(_file, _searchLocalPath); + return GetSystemPaths()->FindFile(_file, _searchLocalPath); } ///////////////////////////////////////////////// @@ -328,13 +341,13 @@ std::string ignition::common::findFilePath(const std::string &_file) void ignition::common::addFindFileURICallback( std::function _cb) { - gSystemPaths->AddFindFileURICallback(_cb); + GetSystemPaths()->AddFindFileURICallback(_cb); } ///////////////////////////////////////////////// ignition::common::SystemPaths *ignition::common::systemPaths() { - return gSystemPaths.get(); + return GetSystemPaths(); } ///////////////////////////////////////////////// From 583b4211d42b7aabc78e8060528f897ddbc5e119 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Fri, 28 Jan 2022 14:22:29 -0600 Subject: [PATCH 10/15] Make macOS happy Signed-off-by: Michael Carroll --- src/TempDirectory.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/TempDirectory.cc b/src/TempDirectory.cc index 13385b337..cf65b3a64 100644 --- a/src/TempDirectory.cc +++ b/src/TempDirectory.cc @@ -19,7 +19,6 @@ #include -#include #include #ifdef _WIN32 @@ -27,6 +26,9 @@ #include #include #include +#else +#include +#include #endif namespace fs = std::filesystem; From c5bea16f0a4dda5249bfc5f64c8a2ddc59892141 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Fri, 28 Jan 2022 15:00:28 -0600 Subject: [PATCH 11/15] Cleaner static SystemPaths implementation Signed-off-by: Michael Carroll --- src/Util.cc | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/Util.cc b/src/Util.cc index 71ff0340e..d22ba30e1 100644 --- a/src/Util.cc +++ b/src/Util.cc @@ -62,12 +62,10 @@ /// /// This uses the NeverDestroyed pattern to prevent static initialization and /// destruction order fiasco issues. -ignition::common::SystemPaths* GetSystemPaths() +ignition::common::SystemPaths& GetSystemPaths() { - static const - ignition::utils::NeverDestroyed paths{ - new ignition::common::SystemPaths() - }; + static + ignition::utils::NeverDestroyed paths; return paths.Access(); } @@ -299,26 +297,26 @@ std::string ignition::common::timeToIso( ///////////////////////////////////////////////// std::string ignition::common::logPath() { - return GetSystemPaths()->LogPath(); + return GetSystemPaths().LogPath(); } ///////////////////////////////////////////////// void ignition::common::addSearchPathSuffix(const std::string &_suffix) { - GetSystemPaths()->AddSearchPathSuffix(_suffix); + GetSystemPaths().AddSearchPathSuffix(_suffix); } ///////////////////////////////////////////////// std::string ignition::common::findFile(const std::string &_file) { - return GetSystemPaths()->FindFile(_file, true); + return GetSystemPaths().FindFile(_file, true); } ///////////////////////////////////////////////// std::string ignition::common::findFile(const std::string &_file, const bool _searchLocalPath) { - return GetSystemPaths()->FindFile(_file, _searchLocalPath); + return GetSystemPaths().FindFile(_file, _searchLocalPath); } ///////////////////////////////////////////////// @@ -341,13 +339,13 @@ std::string ignition::common::findFilePath(const std::string &_file) void ignition::common::addFindFileURICallback( std::function _cb) { - GetSystemPaths()->AddFindFileURICallback(_cb); + GetSystemPaths().AddFindFileURICallback(_cb); } ///////////////////////////////////////////////// ignition::common::SystemPaths *ignition::common::systemPaths() { - return GetSystemPaths(); + return &GetSystemPaths(); } ///////////////////////////////////////////////// From dbdb0e9503b3404274d8d8eb63fff52d06c8563e Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Tue, 1 Feb 2022 12:55:03 -0600 Subject: [PATCH 12/15] Remove extra debug output Signed-off-by: Michael Carroll --- src/SystemPaths.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/SystemPaths.cc b/src/SystemPaths.cc index 4358dac55..0b7dd76dd 100644 --- a/src/SystemPaths.cc +++ b/src/SystemPaths.cc @@ -454,12 +454,9 @@ std::string SystemPaths::FindFile(const std::string &_filename, std::string SystemPaths::LocateLocalFile(const std::string &_filename, const std::vector &_paths) { - std::cerr << "LocateLocalFile: " << _filename << "\n"; - std::string foundPath = ""; for (auto const &path : _paths) { - std::cout << "\tPath: " << path << "\n"; std::string checkPath = NormalizeDirectoryPath(path) + _filename; if (exists(checkPath)) { @@ -523,8 +520,6 @@ void SystemPaths::AddFindFileURICallback( ///////////////////////////////////////////////// std::list SystemPaths::PathsFromEnv(const std::string &_env) { - std::cerr << "PathsFromEnv" << std::endl; - std::list paths; std::string envPathsStr; From 6ab5b607bd7d36acea1fec722965a13bb1d507dc Mon Sep 17 00:00:00 2001 From: Steve Peters Date: Tue, 1 Feb 2022 15:14:30 -0800 Subject: [PATCH 13/15] Remove one more debug message Signed-off-by: Steve Peters --- src/SystemPaths.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/SystemPaths.cc b/src/SystemPaths.cc index 0b7dd76dd..a8393d3eb 100644 --- a/src/SystemPaths.cc +++ b/src/SystemPaths.cc @@ -390,8 +390,6 @@ std::string SystemPaths::FindFile(const std::string &_filename, // Try appending to local paths else { - ignerr << "Here: " << cwd() << " " << filename << std::endl; - auto cwdPath = joinPaths(cwd(), filename); if (_searchLocalPath && exists(cwdPath)) { From 66f716ca291a170b261a49cdb9cad4d90cba2859 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 2 Feb 2022 15:38:29 -0600 Subject: [PATCH 14/15] Clarify discarding return on create_directories Signed-off-by: Michael Carroll --- src/Filesystem.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Filesystem.cc b/src/Filesystem.cc index 50f8b9dd8..c257a98b3 100644 --- a/src/Filesystem.cc +++ b/src/Filesystem.cc @@ -83,9 +83,10 @@ bool ignition::common::createDirectory(const std::string &_path) bool ignition::common::createDirectories(const std::string &_path) { std::error_code ec; + bool created = fs::create_directories(_path, ec); // Disregard return here because it may return false if the // directory is not actually created (already exists) - fs::create_directories(_path, ec); + (void) created; return fsWarn("createDirectories", ec); } From e404a8228daf6eb68d8560a0dce65e89415175dc Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Wed, 2 Feb 2022 17:02:22 -0600 Subject: [PATCH 15/15] Address reviewer feedback Signed-off-by: Michael Carroll --- src/Filesystem.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Filesystem.cc b/src/Filesystem.cc index c257a98b3..ba4fda7e8 100644 --- a/src/Filesystem.cc +++ b/src/Filesystem.cc @@ -83,9 +83,9 @@ bool ignition::common::createDirectory(const std::string &_path) bool ignition::common::createDirectories(const std::string &_path) { std::error_code ec; - bool created = fs::create_directories(_path, ec); - // Disregard return here because it may return false if the + // Disregard return of create_directories, because it may return false if the // directory is not actually created (already exists) + bool created = fs::create_directories(_path, ec); (void) created; return fsWarn("createDirectories", ec); } @@ -145,6 +145,9 @@ std::string ignition::common::joinPaths( else is_url = true; + // TODO(mjcarroll) Address the case that path2 is also a URI. + // It's likely not a valid scenario, but not currently covered by our test + // suite and doesn't return an error. if (_path2.find("://") == std::string::npos) p2 = p2.lexically_normal(); else