From 363d66021e6b32746d7d2011fb81d2d0674cb4a7 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Wed, 4 Nov 2020 21:38:20 +0000 Subject: [PATCH 1/3] Fix working with filesystem parent paths. I found two separate bugs while working with parent paths: 1. Asking for the first parent path of an absolute path would add a duplicate separator at the beginning. I fixed this by checking to see if the separator already exists, and skiping if there is already one. 2. Asking for the grandparent path of an absolute path would return a relative path, which is very incorrect. The problem here turned out to be that we were not propagating empty vector pieces from a child to a parent, and so the grandparent would miss out that it was supposed to be absolute. Fix this by always adding all pieces to the parents, even when they are empty. I also added tests for the above cases. Signed-off-by: Chris Lalancette --- include/rcpputils/filesystem_helper.hpp | 22 +++++++++++++++------- test/test_filesystem_helper.cpp | 12 ++++++++++++ 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/include/rcpputils/filesystem_helper.hpp b/include/rcpputils/filesystem_helper.hpp index 68bb080d..f6cad70b 100644 --- a/include/rcpputils/filesystem_helper.hpp +++ b/include/rcpputils/filesystem_helper.hpp @@ -32,9 +32,8 @@ /*! \file filesystem_helper.hpp * \brief Cross-platform filesystem helper functions and additional emulation of [std::filesystem](https://en.cppreference.com/w/cpp/filesystem). * - * If std::filesystem is not available the necessary functions are emulated. * Note: Once std::filesystem is supported on all ROS2 platforms, this class - * can be deprecated in favor of the built-in functionality. + * can be deprecated/removed in favor of the built-in functionality. */ #ifndef RCPPUTILS__FILESYSTEM_HELPER_HPP_ @@ -220,7 +219,7 @@ class path bool is_absolute() const { return path_.size() > 0 && - (path_.compare(0, 1, std::string(1, kPreferredSeparator)) == 0 || + (path_[0] == kPreferredSeparator || this->is_absolute_with_drive_letter()); } @@ -277,10 +276,13 @@ class path path parent; for (auto it = this->cbegin(); it != --this->cend(); ++it) { - if (!parent.empty() || it->empty()) { - parent /= *it; - } else { + if (parent.empty() && !this->is_absolute()) { + // This handles the case where we are dealing with a relative path; + // we don't want a separator at the beginning, so just copy the piece + // directly. parent = *it; + } else { + parent /= *it; } } return parent; @@ -356,7 +358,13 @@ class path this->path_ = other.path_; this->path_as_vector_ = other.path_as_vector_; } else { - this->path_ += kPreferredSeparator + other.string(); + if (this->path_[this->path_.length() - 1] != kPreferredSeparator) { + // This ensures that we don't put duplicate separators into the path; + // this can happen, for instance, on absolute paths where the first + // item in the vector is the empty string. + this->path_ += kPreferredSeparator; + } + this->path_ += other.string(); this->path_as_vector_.insert( std::end(this->path_as_vector_), std::begin(other.path_as_vector_), std::end(other.path_as_vector_)); diff --git a/test/test_filesystem_helper.cpp b/test/test_filesystem_helper.cpp index a83047f0..3c3bcb75 100644 --- a/test/test_filesystem_helper.cpp +++ b/test/test_filesystem_helper.cpp @@ -392,3 +392,15 @@ TEST(TestFilesystemHelper, get_cwd) auto p = rcpputils::fs::current_path(); EXPECT_EQ(expected_dir, p.string()); } + +TEST(TestFilesystemHelper, parent_absolute_path) +{ + rcpputils::fs::path path("/home/foo/bar/baz"); + ASSERT_EQ(path.string(), "/home/foo/bar/baz"); + + rcpputils::fs::path parent = path.parent_path(); + ASSERT_EQ(parent.string(), "/home/foo/bar"); + + rcpputils::fs::path grandparent = parent.parent_path(); + ASSERT_EQ(grandparent.string(), "/home/foo"); +} From 49aed47e6e7610f7de9e4f65e8fe370a9f8852f0 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 6 Nov 2020 15:40:52 +0000 Subject: [PATCH 2/3] Make sure to split *after* transforming to preferred separator. Signed-off-by: Chris Lalancette --- include/rcpputils/filesystem_helper.hpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/rcpputils/filesystem_helper.hpp b/include/rcpputils/filesystem_helper.hpp index f6cad70b..08c0672e 100644 --- a/include/rcpputils/filesystem_helper.hpp +++ b/include/rcpputils/filesystem_helper.hpp @@ -103,10 +103,11 @@ class path * \param p A string path split by the platform's string path separator. */ path(const std::string & p) // NOLINT(runtime/explicit): this is a conversion constructor - : path_(p), path_as_vector_(split(p, kPreferredSeparator)) + : path_(p) { std::replace(path_.begin(), path_.end(), '\\', kPreferredSeparator); std::replace(path_.begin(), path_.end(), '/', kPreferredSeparator); + path_as_vector_ = split(path_, kPreferredSeparator); } /** From 869b78c9ce5a7def73962955d6f1cb265cf9761d Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 9 Nov 2020 19:16:35 +0000 Subject: [PATCH 3/3] More fixes for Windows. Signed-off-by: Chris Lalancette --- include/rcpputils/filesystem_helper.hpp | 8 ++++---- test/test_filesystem_helper.cpp | 27 ++++++++++++++++++++++--- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/include/rcpputils/filesystem_helper.hpp b/include/rcpputils/filesystem_helper.hpp index 08c0672e..8877df1e 100644 --- a/include/rcpputils/filesystem_helper.hpp +++ b/include/rcpputils/filesystem_helper.hpp @@ -277,10 +277,10 @@ class path path parent; for (auto it = this->cbegin(); it != --this->cend(); ++it) { - if (parent.empty() && !this->is_absolute()) { - // This handles the case where we are dealing with a relative path; - // we don't want a separator at the beginning, so just copy the piece - // directly. + if (parent.empty() && (!this->is_absolute() || this->is_absolute_with_drive_letter())) { + // This handles the case where we are dealing with a relative path or + // the Windows drive letter; in both cases we don't want a separator at + // the beginning, so just copy the piece directly. parent = *it; } else { parent /= *it; diff --git a/test/test_filesystem_helper.cpp b/test/test_filesystem_helper.cpp index 3c3bcb75..8f4c7820 100644 --- a/test/test_filesystem_helper.cpp +++ b/test/test_filesystem_helper.cpp @@ -396,11 +396,32 @@ TEST(TestFilesystemHelper, get_cwd) TEST(TestFilesystemHelper, parent_absolute_path) { rcpputils::fs::path path("/home/foo/bar/baz"); - ASSERT_EQ(path.string(), "/home/foo/bar/baz"); + if (is_win32) { + ASSERT_EQ(path.string(), "\\home\\foo\\bar\\baz"); + } else { + ASSERT_EQ(path.string(), "/home/foo/bar/baz"); + } rcpputils::fs::path parent = path.parent_path(); - ASSERT_EQ(parent.string(), "/home/foo/bar"); + if (is_win32) { + ASSERT_EQ(parent.string(), "\\home\\foo\\bar"); + } else { + ASSERT_EQ(parent.string(), "/home/foo/bar"); + } rcpputils::fs::path grandparent = parent.parent_path(); - ASSERT_EQ(grandparent.string(), "/home/foo"); + if (is_win32) { + ASSERT_EQ(grandparent.string(), "\\home\\foo"); + } else { + ASSERT_EQ(grandparent.string(), "/home/foo"); + } + + if (is_win32) { + rcpputils::fs::path win_drive_letter("C:\\home\\foo\\bar"); + ASSERT_EQ(win_drive_letter.string(), "C:\\home\\foo\\bar"); + rcpputils::fs::path win_parent = win_drive_letter.parent_path(); + ASSERT_EQ(win_parent.string(), "C:\\home\\foo"); + rcpputils::fs::path win_grandparent = win_parent.parent_path(); + ASSERT_EQ(win_grandparent.string(), "C:\\home"); + } }