From 172a7f8a8e3c90edeab415b3a7e6fcbedce95ab8 Mon Sep 17 00:00:00 2001 From: Benjamin Doherty Date: Wed, 8 Aug 2018 00:05:08 -0700 Subject: [PATCH 1/5] Fix getCanonicalPath on Windows --- libs/utils/CMakeLists.txt | 16 +++++-- libs/utils/src/Path.cpp | 13 +++--- libs/utils/src/win32/Path.cpp | 8 ---- libs/utils/test/test_WinPath.cpp | 78 ++++++++++++++++++++++++++++++++ 4 files changed, 95 insertions(+), 20 deletions(-) create mode 100644 libs/utils/test/test_WinPath.cpp diff --git a/libs/utils/CMakeLists.txt b/libs/utils/CMakeLists.txt index 2ec533940318..196576ed019b 100644 --- a/libs/utils/CMakeLists.txt +++ b/libs/utils/CMakeLists.txt @@ -91,7 +91,8 @@ install(FILES ${DIST_HDRS} DESTINATION include/${TARGET}) # ================================================================================================== # Test executables # ================================================================================================== -add_executable(test_${TARGET} + +set(TEST_SRCS test/test_algorithm.cpp test/test_Allocators.cpp test/test_bitset.cpp @@ -102,11 +103,16 @@ add_executable(test_${TARGET} test/test_JobSystem.cpp test/test_StructureOfArrays.cpp test/test_utils_main.cpp - test/test_Zip2Iterator.cpp) + test/test_Zip2Iterator.cpp +) -# The Path tests are platform-specific, and fail on Windows -if (NOT WIN32) - list(APPEND SRCS test/test_Path.cpp) +# The Path tests are platform-specific +if (WIN32) + list(APPEND TEST_SRCS test/test_WinPath.cpp) +else() + list(APPEND TEST_SRCS test/test_Path.cpp) endif() +add_executable(test_${TARGET} ${TEST_SRCS}) + target_link_libraries(test_${TARGET} PRIVATE gtest utils tsl math) diff --git a/libs/utils/src/Path.cpp b/libs/utils/src/Path.cpp index 35f90ba83e42..b365fb285911 100644 --- a/libs/utils/src/Path.cpp +++ b/libs/utils/src/Path.cpp @@ -179,24 +179,24 @@ std::vector Path::split() const { return segments; } -#if !defined(WIN32) std::string Path::getCanonicalPath(const std::string& path) { if (path.empty()) return ""; std::vector segments; // If the path starts with a / we must preserve it - bool starts_with_slash = path.front() == '/'; + bool starts_with_slash = path.front() == SEPARATOR; // If the path does not end with a / we need to remove the // extra / added by the join process - bool ends_with_slash = path.back() == '/'; + bool ends_with_slash = path.back() == SEPARATOR; size_t current; ssize_t next = -1; do { current = size_t(next + 1); - next = path.find_first_of("/", current); + // Handle both Unix and Windows style separators + next = path.find_first_of("/\\", current); std::string segment(path.substr(current, next - current)); size_t size = segment.length(); @@ -230,11 +230,11 @@ std::string Path::getCanonicalPath(const std::string& path) { // the end that might need to be removed std::stringstream clean_path; std::copy(segments.begin(), segments.end(), - std::ostream_iterator(clean_path, "/")); + std::ostream_iterator(clean_path, SEPARATOR_STR)); std::string new_path = clean_path.str(); if (starts_with_slash && new_path.empty()) { - new_path = "/"; + new_path = SEPARATOR_STR; } if (!ends_with_slash && new_path.length() > 1) { @@ -243,7 +243,6 @@ std::string Path::getCanonicalPath(const std::string& path) { return new_path; } -#endif bool Path::mkdirRecursive() const { if (isEmpty()) { diff --git a/libs/utils/src/win32/Path.cpp b/libs/utils/src/win32/Path.cpp index 528cd0a462c0..f0239edb7439 100644 --- a/libs/utils/src/win32/Path.cpp +++ b/libs/utils/src/win32/Path.cpp @@ -63,12 +63,4 @@ std::vector Path::listContents() const { return directory_contents; } -std::string Path::getCanonicalPath(const std::string& path) { - if (path.empty()) return ""; - - char canonized[MAX_PATH]; - PathCanonicalize(canonized, path.c_str()); - return canonized; -} - } // namespace utils diff --git a/libs/utils/test/test_WinPath.cpp b/libs/utils/test/test_WinPath.cpp new file mode 100644 index 000000000000..81e7dea2fe3a --- /dev/null +++ b/libs/utils/test/test_WinPath.cpp @@ -0,0 +1,78 @@ +/* + * Copyright (C) 2018 The Android Open Source Project + * + * 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 +#include +#include + +#ifndef PATH_MAX // should be in +#define PATH_MAX 4096 +#endif + +using namespace utils; + +TEST(PathTest, Sanitization) { + std::string r; + + // An empty path remains empty + r = Path::getCanonicalPath(""); + EXPECT_EQ("", r); + + // A single / is preserved + r = Path::getCanonicalPath("\\"); + EXPECT_EQ("\\", r); + + // Unix style paths are converted to Windows style + r = Path::getCanonicalPath("out/./././bin/foo/../../bar"); + EXPECT_EQ("out\\bar", r); + + // A mix of Unix style paths and Windows style + r = Path::getCanonicalPath("out/.\\././bin/foo\\../..\\bar"); + EXPECT_EQ("out\\bar", r); + + // Disk designation + r = Path::getCanonicalPath("C:\\out\\bin"); + EXPECT_EQ("C:\\out\\bin", r); + + // Collapse .. with disk designation + r = Path::getCanonicalPath("C:\\out\\bin\\..\\foo"); + EXPECT_EQ("C:\\out\\foo", r); + + // Collapse multiple .. with disk designation + r = Path::getCanonicalPath("C:\\out\\bin\\..\\..\\foo"); + EXPECT_EQ("C:\\foo", r); + + // Collapse . with disk designation + r = Path::getCanonicalPath("C:\\out\\.\\foo"); + EXPECT_EQ("C:\\out\\foo", r); + + // Collapse multiple . with disk designation + r = Path::getCanonicalPath("C:\\out\\.\\.\\foo"); + EXPECT_EQ("C:\\out\\foo", r); + + // Collapse multiple . and .. with disk designation + r = Path::getCanonicalPath("C:\\out\\bin\\.\\..\\..\\foo"); + EXPECT_EQ("C:\\foo", r); + + // make sure it works with several ../ + r = Path::getCanonicalPath("..\\..\\bin"); + EXPECT_EQ("..\\..\\bin", r); +} From b2f43f1af10038852d31bf63d51aa909a8d573cd Mon Sep 17 00:00:00 2001 From: Benjamin Doherty Date: Wed, 8 Aug 2018 10:11:49 -0700 Subject: [PATCH 2/5] Fix isAbsolutePath on Windows --- libs/utils/include/utils/Path.h | 4 +- libs/utils/src/Path.cpp | 7 ++ libs/utils/src/win32/Path.cpp | 136 ++++++++++++++++--------------- libs/utils/test/test_WinPath.cpp | 15 ++++ 4 files changed, 93 insertions(+), 69 deletions(-) diff --git a/libs/utils/include/utils/Path.h b/libs/utils/include/utils/Path.h index a2f36bc6beea..9fe5d687062f 100644 --- a/libs/utils/include/utils/Path.h +++ b/libs/utils/include/utils/Path.h @@ -154,9 +154,7 @@ class Path { * @return true if this abstract pathname is not empty * and starts with a leading '/', false otherwise */ - bool isAbsolute() const { - return !isEmpty() && m_path.front() == '/'; - } + bool isAbsolute() const; /** * Splits this object's abstract pathname in a vector of file diff --git a/libs/utils/src/Path.cpp b/libs/utils/src/Path.cpp index b365fb285911..4dbf9dce3115 100644 --- a/libs/utils/src/Path.cpp +++ b/libs/utils/src/Path.cpp @@ -104,6 +104,13 @@ Path Path::getAbsolutePath() const { return getCurrentDirectory().concat(*this); } + +#if !defined(WIN32) +bool Path::isAbsolute() const { + return !isEmpty() && m_path.front() == '/'; +} +#endif + Path Path::getParent() const { if (isEmpty()) return ""; diff --git a/libs/utils/src/win32/Path.cpp b/libs/utils/src/win32/Path.cpp index f0239edb7439..1975cbc09f77 100644 --- a/libs/utils/src/win32/Path.cpp +++ b/libs/utils/src/win32/Path.cpp @@ -1,66 +1,70 @@ -/* - * Copyright (C) 2018 The Android Open Source Project - * - * 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 -#include -#include -#include - -namespace utils { - -bool Path::mkdir() const { - return _mkdir(m_path.c_str()) == 0; -} - -Path Path::getCurrentExecutable() { - // First, need to establish resource path. - TCHAR path[MAX_PATH + 1]; - Path result; - - GetModuleFileName(NULL, path, MAX_PATH + 1); - result.setPath(path); - - return result; -} - -std::vector Path::listContents() const { - // Return an empty vector if the path doesn't exist or is not a directory - if (!isDirectory() || !exists()) { - return {}; - } - - TCHAR dirName[MAX_PATH]; - StringCchCopy(dirName, MAX_PATH, c_str()); - - WIN32_FIND_DATA findData; - HANDLE find = FindFirstFile(dirName, &findData); - - std::vector directory_contents; - do - { - if (findData.cFileName[0] != '.') { - directory_contents.push_back(concat(findData.cFileName)); - } - } while (FindNextFile(find, &findData) != 0); - - return directory_contents; -} - -} // namespace utils +/* + * Copyright (C) 2018 The Android Open Source Project + * + * 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 +#include +#include +#include + +namespace utils { + +bool Path::mkdir() const { + return _mkdir(m_path.c_str()) == 0; +} + +Path Path::getCurrentExecutable() { + // First, need to establish resource path. + TCHAR path[MAX_PATH + 1]; + Path result; + + GetModuleFileName(NULL, path, MAX_PATH + 1); + result.setPath(path); + + return result; +} + +std::vector Path::listContents() const { + // Return an empty vector if the path doesn't exist or is not a directory + if (!isDirectory() || !exists()) { + return {}; + } + + TCHAR dirName[MAX_PATH]; + StringCchCopy(dirName, MAX_PATH, c_str()); + + WIN32_FIND_DATA findData; + HANDLE find = FindFirstFile(dirName, &findData); + + std::vector directory_contents; + do + { + if (findData.cFileName[0] != '.') { + directory_contents.push_back(concat(findData.cFileName)); + } + } while (FindNextFile(find, &findData) != 0); + + return directory_contents; +} + +bool Path::isAbsolute() const { + return !PathIsRelative(m_path.c_str()); +} + +} // namespace utils diff --git a/libs/utils/test/test_WinPath.cpp b/libs/utils/test/test_WinPath.cpp index 81e7dea2fe3a..e80aa9f7c348 100644 --- a/libs/utils/test/test_WinPath.cpp +++ b/libs/utils/test/test_WinPath.cpp @@ -76,3 +76,18 @@ TEST(PathTest, Sanitization) { r = Path::getCanonicalPath("..\\..\\bin"); EXPECT_EQ("..\\..\\bin", r); } + +TEST(PathTest, AbsolutePath) { + Path cwd = Path::getCurrentDirectory(); + + Path p; + p = Path("C:\\out\\blue\\bin"); + EXPECT_TRUE(p.isAbsolute()); + + p = p.getAbsolutePath(); + EXPECT_EQ("C:\\out\\blue\\bin", p.getPath()); + + p = Path("../bin").getAbsolutePath(); + EXPECT_NE(cwd, p); + EXPECT_TRUE(p.isAbsolute()); +} From 5f2574493cd7df5400f34e126f37e87fd0c31d6a Mon Sep 17 00:00:00 2001 From: Benjamin Doherty Date: Wed, 8 Aug 2018 10:18:06 -0700 Subject: [PATCH 3/5] Add Split test --- libs/utils/test/test_WinPath.cpp | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/libs/utils/test/test_WinPath.cpp b/libs/utils/test/test_WinPath.cpp index e80aa9f7c348..b4fdf28e3d4d 100644 --- a/libs/utils/test/test_WinPath.cpp +++ b/libs/utils/test/test_WinPath.cpp @@ -29,7 +29,7 @@ using namespace utils; -TEST(PathTest, Sanitization) { +TEST(WinPathTest, Sanitization) { std::string r; // An empty path remains empty @@ -77,7 +77,7 @@ TEST(PathTest, Sanitization) { EXPECT_EQ("..\\..\\bin", r); } -TEST(PathTest, AbsolutePath) { +TEST(WinPathTest, AbsolutePath) { Path cwd = Path::getCurrentDirectory(); Path p; @@ -91,3 +91,29 @@ TEST(PathTest, AbsolutePath) { EXPECT_NE(cwd, p); EXPECT_TRUE(p.isAbsolute()); } + +TEST(WinPathTest, Split) { + std::vector segments; + + segments = Path("").split(); + EXPECT_EQ(0, segments.size()); + + segments = Path("\\").split(); + EXPECT_EQ(1, segments.size()); + EXPECT_EQ("\\", segments[0]); + + segments = Path("\\out\\blue\\bin").split(); + EXPECT_EQ(4, segments.size()); + EXPECT_EQ("\\", segments[0]); + EXPECT_EQ("out", segments[1]); + EXPECT_EQ("blue", segments[2]); + EXPECT_EQ("bin", segments[3]); + + segments = Path("/out\\foo/blue\\bin/").split(); + EXPECT_EQ(5, segments.size()); + EXPECT_EQ("\\", segments[0]); + EXPECT_EQ("out", segments[1]); + EXPECT_EQ("foo", segments[2]); + EXPECT_EQ("blue", segments[3]); + EXPECT_EQ("bin", segments[4]); +} From 64549eecabf1d2c0a6624c2c29e8d1da48dac21a Mon Sep 17 00:00:00 2001 From: Benjamin Doherty Date: Wed, 8 Aug 2018 10:29:02 -0700 Subject: [PATCH 4/5] Add concatenation tests --- libs/utils/test/test_WinPath.cpp | 49 ++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/libs/utils/test/test_WinPath.cpp b/libs/utils/test/test_WinPath.cpp index b4fdf28e3d4d..8ecb1969e306 100644 --- a/libs/utils/test/test_WinPath.cpp +++ b/libs/utils/test/test_WinPath.cpp @@ -116,4 +116,53 @@ TEST(WinPathTest, Split) { EXPECT_EQ("foo", segments[2]); EXPECT_EQ("blue", segments[3]); EXPECT_EQ("bin", segments[4]); + + segments = Path("C:\\out\\foo/blue\\bin/").split(); + EXPECT_EQ(5, segments.size()); + EXPECT_EQ("C:", segments[0]); + EXPECT_EQ("out", segments[1]); + EXPECT_EQ("foo", segments[2]); + EXPECT_EQ("blue", segments[3]); + EXPECT_EQ("bin", segments[4]); +} + +TEST(WinPathTest, Concatenate) { + Path root("C:\\Volumes\\Replicant\\blue"); + + Path r; + r = root.concat(""); + EXPECT_EQ("C:\\Volumes\\Replicant\\blue", r.getPath()); + + r = root.concat("C:\\out\\bin"); + EXPECT_EQ("C:\\out\\bin", r.getPath()); + + r = root.concat("out\\bin"); + EXPECT_EQ("C:\\Volumes\\Replicant\\blue\\out\\bin", r.getPath()); + + r = root.concat("."); + EXPECT_EQ("C:\\Volumes\\Replicant\\blue", r.getPath()); + + r = root.concat(".."); + EXPECT_EQ("C:\\Volumes\\Replicant", r.getPath()); + + r = root.concat("C:\\"); + EXPECT_EQ("C:\\", r.getPath()); + + r = root.concat("..\\remote-blue"); + EXPECT_EQ("C:\\Volumes\\Replicant\\remote-blue", r.getPath()); + + r = root.concat("..\\remote-blue"); + EXPECT_EQ(r, root + Path("../remote-blue")); + EXPECT_EQ(r, root + "../remote-blue"); + + r = "C:\\out\\bin"; + r.concatToSelf("../bin"); + EXPECT_EQ("C:\\out\\bin", r.getPath()); + + r += "./resources"; + EXPECT_EQ("C:\\out\\bin\\resources", r.getPath()); + + // Unix-style separators work too + r = root.concat("out/bin/foo/bar"); + EXPECT_EQ("C:\\Volumes\\Replicant\\blue\\out\\bin\\foo\\bar", r.getPath()); } From 77a3d2c75d1a1aa51f4c9318876a596ab57a55df Mon Sep 17 00:00:00 2001 From: Benjamin Doherty Date: Wed, 8 Aug 2018 11:42:27 -0700 Subject: [PATCH 5/5] Handle splits correctly with disk designations --- libs/utils/src/Path.cpp | 22 +++++++++++----- libs/utils/test/test_WinPath.cpp | 45 +++++++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 8 deletions(-) diff --git a/libs/utils/src/Path.cpp b/libs/utils/src/Path.cpp index 4dbf9dce3115..d8e5f3b34333 100644 --- a/libs/utils/src/Path.cpp +++ b/libs/utils/src/Path.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #if defined(WIN32) # include @@ -116,13 +117,14 @@ Path Path::getParent() const { std::string result; - // if our path starts with '/', keep the '/' - if (m_path.front() == SEPARATOR) { - result.append(SEPARATOR_STR); + std::vector segments(split()); + + // if our path is absolute with a single segment, + // be sure to keep the prefix component + if (!isAbsolute() || segments.size() > 1) { + segments.pop_back(); // peel the last one } - std::vector segments(split()); - segments.pop_back(); // peel the last one for (auto const& s : segments) { result.append(s).append(SEPARATOR_STR); } @@ -168,11 +170,17 @@ std::vector Path::split() const { std::vector segments; if (isEmpty()) return segments; - if (m_path.front() == SEPARATOR) segments.push_back(SEPARATOR_STR); - size_t current; ssize_t next = -1; + // Matches a leading disk designator (C:\), forward slash (/), or back slash (\) + const static std::regex driveDesignationRegex(R"_regex(^([a-zA-Z]:\\|\\|\/))_regex"); + std::smatch match; + if (std::regex_search(m_path, match, driveDesignationRegex)) { + segments.push_back(match[0]); + next = match[0].length() - 1; + } + do { current = size_t(next + 1); next = m_path.find_first_of(SEPARATOR_STR, current); diff --git a/libs/utils/test/test_WinPath.cpp b/libs/utils/test/test_WinPath.cpp index 8ecb1969e306..8478ce548b09 100644 --- a/libs/utils/test/test_WinPath.cpp +++ b/libs/utils/test/test_WinPath.cpp @@ -48,6 +48,10 @@ TEST(WinPathTest, Sanitization) { r = Path::getCanonicalPath("out/.\\././bin/foo\\../..\\bar"); EXPECT_EQ("out\\bar", r); + // Disk designation + r = Path::getCanonicalPath("C:\\"); + EXPECT_EQ("C:\\", r); + // Disk designation r = Path::getCanonicalPath("C:\\out\\bin"); EXPECT_EQ("C:\\out\\bin", r); @@ -102,6 +106,10 @@ TEST(WinPathTest, Split) { EXPECT_EQ(1, segments.size()); EXPECT_EQ("\\", segments[0]); + segments = Path("C:\\").split(); + EXPECT_EQ(1, segments.size()); + EXPECT_EQ("C:\\", segments[0]); + segments = Path("\\out\\blue\\bin").split(); EXPECT_EQ(4, segments.size()); EXPECT_EQ("\\", segments[0]); @@ -119,7 +127,7 @@ TEST(WinPathTest, Split) { segments = Path("C:\\out\\foo/blue\\bin/").split(); EXPECT_EQ(5, segments.size()); - EXPECT_EQ("C:", segments[0]); + EXPECT_EQ("C:\\", segments[0]); EXPECT_EQ("out", segments[1]); EXPECT_EQ("foo", segments[2]); EXPECT_EQ("blue", segments[3]); @@ -166,3 +174,38 @@ TEST(WinPathTest, Concatenate) { r = root.concat("out/bin/foo/bar"); EXPECT_EQ("C:\\Volumes\\Replicant\\blue\\out\\bin\\foo\\bar", r.getPath()); } + +TEST(PathTest, GetParent) { + std::string r; + Path p("C:\\out\\bin"); + r = p.getParent(); + EXPECT_EQ("C:\\out\\", r); + + p = "C:\\out\\bin\\"; + r = p.getParent(); + EXPECT_EQ("C:\\out\\", r); + + p = "out\\bin"; + r = p.getParent(); + EXPECT_EQ("out\\", r); + + p = "out\\bin\\"; + r = p.getParent(); + EXPECT_EQ("out\\", r); + + p = "out"; + r = p.getParent(); + EXPECT_EQ("", r); + + p = "C:\\out"; + r = p.getParent(); + EXPECT_EQ("C:\\", r); + + p = ""; + r = p.getParent(); + EXPECT_EQ("", r); + + p = "C:\\"; + r = p.getParent(); + EXPECT_EQ("C:\\", r); +}