From 9483ecc32bdf510e81755918fa12fb37dac1c913 Mon Sep 17 00:00:00 2001 From: TP Boudreau Date: Wed, 15 Apr 2020 21:53:51 +0000 Subject: [PATCH 1/7] Enable long paths on Windows --- cpp/src/arrow/util/CMakeLists.txt | 1 + cpp/src/arrow/util/io_util.cc | 41 ++++++++++++++++---- cpp/src/arrow/util/io_util_test.cc | 49 +++++++++++++++++++++++- cpp/src/arrow/util/io_util_test.manifest | 9 +++++ 4 files changed, 92 insertions(+), 8 deletions(-) create mode 100644 cpp/src/arrow/util/io_util_test.manifest diff --git a/cpp/src/arrow/util/CMakeLists.txt b/cpp/src/arrow/util/CMakeLists.txt index 4c2ed4efa4fb5..208ef885e61a6 100644 --- a/cpp/src/arrow/util/CMakeLists.txt +++ b/cpp/src/arrow/util/CMakeLists.txt @@ -38,6 +38,7 @@ add_arrow_test(utility-test hashing_test.cc int_util_test.cc io_util_test.cc + io_util_test.manifest iterator_test.cc logging_test.cc parsing_util_test.cc diff --git a/cpp/src/arrow/util/io_util.cc b/cpp/src/arrow/util/io_util.cc index c7efa15dd24ec..6d2998eedcff7 100644 --- a/cpp/src/arrow/util/io_util.cc +++ b/cpp/src/arrow/util/io_util.cc @@ -837,8 +837,18 @@ Result FileOpenReadable(const PlatformFilename& file_name) { int fd, errno_actual; #if defined(_WIN32) SetLastError(0); - errno_actual = _wsopen_s(&fd, file_name.ToNative().c_str(), - _O_RDONLY | _O_BINARY | _O_NOINHERIT, _SH_DENYNO, _S_IREAD); + HANDLE file_handle = CreateFileW(file_name.ToNative().c_str(), GENERIC_READ, + FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, + OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); + + DWORD last_error = GetLastError(); + if (last_error == ERROR_SUCCESS) { + errno_actual = 0; + fd = _open_osfhandle((intptr_t)file_handle, _O_RDONLY | _O_BINARY | _O_NOINHERIT); + } else { + return IOErrorFromWinError(last_error, "Failed to open file '", file_name.ToString(), + "'"); + } #else fd = open(file_name.ToNative().c_str(), O_RDONLY); errno_actual = errno; @@ -868,23 +878,40 @@ Result FileOpenWritable(const PlatformFilename& file_name, bool write_only, #if defined(_WIN32) SetLastError(0); int oflag = _O_CREAT | _O_BINARY | _O_NOINHERIT; - int pmode = _S_IREAD | _S_IWRITE; + DWORD desired_access = GENERIC_WRITE; + DWORD share_mode = FILE_SHARE_READ | FILE_SHARE_WRITE; + DWORD creation_disposition = OPEN_ALWAYS; - if (truncate) { - oflag |= _O_TRUNC; - } if (append) { oflag |= _O_APPEND; } + if (truncate) { + oflag |= _O_TRUNC; + creation_disposition = CREATE_ALWAYS; + } + if (write_only) { oflag |= _O_WRONLY; } else { oflag |= _O_RDWR; + desired_access |= GENERIC_READ; } - errno_actual = _wsopen_s(&fd, file_name.ToNative().c_str(), oflag, _SH_DENYNO, pmode); + HANDLE file_handle = + CreateFileW(file_name.ToNative().c_str(), desired_access, share_mode, NULL, + creation_disposition, FILE_ATTRIBUTE_NORMAL, NULL); + DWORD last_error = GetLastError(); + if (last_error == ERROR_SUCCESS || + ((creation_disposition == OPEN_ALWAYS || creation_disposition == CREATE_ALWAYS) && + last_error == ERROR_ALREADY_EXISTS)) { + errno_actual = 0; + fd = _open_osfhandle((intptr_t)file_handle, oflag); + } else { + return IOErrorFromWinError(last_error, "Failed to open file '", file_name.ToString(), + "'"); + } #else int oflag = O_CREAT; diff --git a/cpp/src/arrow/util/io_util_test.cc b/cpp/src/arrow/util/io_util_test.cc index eb6ed0d9d92c4..a4bd5d484f731 100644 --- a/cpp/src/arrow/util/io_util_test.cc +++ b/cpp/src/arrow/util/io_util_test.cc @@ -429,7 +429,7 @@ TEST(TemporaryDir, Basics) { TEST(CreateDirTree, Basics) { std::unique_ptr temp_dir; PlatformFilename fn; - bool created; + bool created, deleted; ASSERT_OK_AND_ASSIGN(temp_dir, TemporaryDir::Make("io-util-test-")); @@ -446,6 +446,53 @@ TEST(CreateDirTree, Basics) { ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("EF")); ASSERT_OK_AND_ASSIGN(created, CreateDirTree(fn)); ASSERT_TRUE(created); + +#ifdef _WIN32 + const std::string KEY_NAME = R"(SYSTEM\CurrentControlSet\Control\FileSystem)"; + const std::string VALUE_NAME = "LongPathsEnabled"; + DWORD value = 0; + DWORD size = sizeof(value); + LSTATUS status = RegGetValueA(HKEY_LOCAL_MACHINE, KEY_NAME.c_str(), VALUE_NAME.c_str(), + RRF_RT_REG_DWORD, NULL, &value, &size); + bool test_long_paths = (status == ERROR_SUCCESS && value == 1); + if (!test_long_paths) { + ARROW_LOG(WARNING) + << "Tests for accessing files with long path names have been disabled. " + << "To enable these tests, set the value of " << VALUE_NAME + << " in registry key \\HKEY_LOCAL_MACHINE\\" << KEY_NAME + << " to 1 on the test host."; + } +#else + bool test_long_paths = true; +#endif + + if (test_long_paths) { + const std::string LONG_BASE = "xxx-io-util-test-dir-long"; + PlatformFilename long_path, long_filename; + int fd = -1; + std::stringstream fs; + fs << LONG_BASE; + for (int i = 0; i < 64; ++i) { + fs << "/123456789ABCDEF"; + } + ASSERT_OK_AND_ASSIGN( + long_path, PlatformFilename::FromString(fs.str())); // long_path length > 1024 + ASSERT_OK_AND_ASSIGN(created, CreateDirTree(long_path)); + ASSERT_TRUE(created); + AssertExists(long_path); + ASSERT_OK_AND_ASSIGN(long_filename, + PlatformFilename::FromString(fs.str() + "/file.txt")); + ASSERT_OK_AND_ASSIGN(fd, FileOpenWritable(long_filename)); + ASSERT_OK(FileClose(fd)); + AssertExists(long_filename); + fd = -1; + ASSERT_OK_AND_ASSIGN(fd, FileOpenReadable(long_filename)); + ASSERT_OK(FileClose(fd)); + ASSERT_OK_AND_ASSIGN(deleted, DeleteDirContents(long_path)); + ASSERT_TRUE(deleted); + ASSERT_OK_AND_ASSIGN(deleted, DeleteDirTree(long_path)); + ASSERT_TRUE(deleted); + } } TEST(ListDir, Basics) { diff --git a/cpp/src/arrow/util/io_util_test.manifest b/cpp/src/arrow/util/io_util_test.manifest new file mode 100644 index 0000000000000..4c44ebde2886c --- /dev/null +++ b/cpp/src/arrow/util/io_util_test.manifest @@ -0,0 +1,9 @@ + + + + + + true + + + \ No newline at end of file From 26f3ec43bc1fc5448581e45696be97cf0f0cd7d1 Mon Sep 17 00:00:00 2001 From: TP Boudreau Date: Fri, 17 Apr 2020 23:06:42 +0000 Subject: [PATCH 2/7] Fix assembly manifest injection in MSYS2 toolchain --- cpp/src/arrow/util/CMakeLists.txt | 13 +++++-- cpp/src/arrow/util/io_util.cc | 4 +-- cpp/src/arrow/util/io_util_test.cc | 5 ++- cpp/src/arrow/util/io_util_test.manifest | 27 ++++++++++++++- cpp/src/arrow/util/io_util_test.rc | 44 ++++++++++++++++++++++++ 5 files changed, 86 insertions(+), 7 deletions(-) create mode 100644 cpp/src/arrow/util/io_util_test.rc diff --git a/cpp/src/arrow/util/CMakeLists.txt b/cpp/src/arrow/util/CMakeLists.txt index 208ef885e61a6..377921b38cf7d 100644 --- a/cpp/src/arrow/util/CMakeLists.txt +++ b/cpp/src/arrow/util/CMakeLists.txt @@ -26,6 +26,16 @@ arrow_install_all_headers("arrow/util") # arrow_test_main # +if(WIN32) + if(MSVC) + set(IO_UTIL_TEST_SOURCES io_util_test.cc io_util_test.manifest) + else() + set(IO_UTIL_TEST_SOURCES io_util_test.cc io_util_test.rc) + endif() +else() + set(IO_UTIL_TEST_SOURCES io_util_test.cc) +endif() + add_arrow_test(utility-test SOURCES align_util_test.cc @@ -37,8 +47,7 @@ add_arrow_test(utility-test key_value_metadata_test.cc hashing_test.cc int_util_test.cc - io_util_test.cc - io_util_test.manifest + ${IO_UTIL_TEST_SOURCES} iterator_test.cc logging_test.cc parsing_util_test.cc diff --git a/cpp/src/arrow/util/io_util.cc b/cpp/src/arrow/util/io_util.cc index 6d2998eedcff7..636504c13d82f 100644 --- a/cpp/src/arrow/util/io_util.cc +++ b/cpp/src/arrow/util/io_util.cc @@ -903,9 +903,7 @@ Result FileOpenWritable(const PlatformFilename& file_name, bool write_only, creation_disposition, FILE_ATTRIBUTE_NORMAL, NULL); DWORD last_error = GetLastError(); - if (last_error == ERROR_SUCCESS || - ((creation_disposition == OPEN_ALWAYS || creation_disposition == CREATE_ALWAYS) && - last_error == ERROR_ALREADY_EXISTS)) { + if (last_error == ERROR_SUCCESS || last_error == ERROR_ALREADY_EXISTS) { errno_actual = 0; fd = _open_osfhandle((intptr_t)file_handle, oflag); } else { diff --git a/cpp/src/arrow/util/io_util_test.cc b/cpp/src/arrow/util/io_util_test.cc index a4bd5d484f731..09bf95c491028 100644 --- a/cpp/src/arrow/util/io_util_test.cc +++ b/cpp/src/arrow/util/io_util_test.cc @@ -429,7 +429,7 @@ TEST(TemporaryDir, Basics) { TEST(CreateDirTree, Basics) { std::unique_ptr temp_dir; PlatformFilename fn; - bool created, deleted; + bool created; ASSERT_OK_AND_ASSIGN(temp_dir, TemporaryDir::Make("io-util-test-")); @@ -447,6 +447,8 @@ TEST(CreateDirTree, Basics) { ASSERT_OK_AND_ASSIGN(created, CreateDirTree(fn)); ASSERT_TRUE(created); +#ifndef __APPLE__ + bool deleted; #ifdef _WIN32 const std::string KEY_NAME = R"(SYSTEM\CurrentControlSet\Control\FileSystem)"; const std::string VALUE_NAME = "LongPathsEnabled"; @@ -493,6 +495,7 @@ TEST(CreateDirTree, Basics) { ASSERT_OK_AND_ASSIGN(deleted, DeleteDirTree(long_path)); ASSERT_TRUE(deleted); } +#endif } TEST(ListDir, Basics) { diff --git a/cpp/src/arrow/util/io_util_test.manifest b/cpp/src/arrow/util/io_util_test.manifest index 4c44ebde2886c..b9c691c116ceb 100644 --- a/cpp/src/arrow/util/io_util_test.manifest +++ b/cpp/src/arrow/util/io_util_test.manifest @@ -1,4 +1,22 @@ + @@ -6,4 +24,11 @@ true - \ No newline at end of file + + + + + + + + diff --git a/cpp/src/arrow/util/io_util_test.rc b/cpp/src/arrow/util/io_util_test.rc new file mode 100644 index 0000000000000..c3236cb2b3f88 --- /dev/null +++ b/cpp/src/arrow/util/io_util_test.rc @@ -0,0 +1,44 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#ifndef CREATEPROCESS_MANIFEST_RESOURCE_ID +#define CREATEPROCESS_MANIFEST_RESOURCE_ID 1 +#endif +#ifndef RT_MANIFEST +#define RT_MANIFEST 24 +#endif + +CREATEPROCESS_MANIFEST_RESOURCE_ID RT_MANIFEST +BEGIN + "" + "" + "" + "" + "" + "true" + "" + "" + "" + "" + "" + "" + "" + "" + "" + "" +END + From 307d3fd15f5d9499f7d59573c6cb1c28468d2ed5 Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Mon, 20 Apr 2020 18:55:14 +0200 Subject: [PATCH 3/7] Fix CI failure on Windows Github Actions job --- .github/workflows/cpp.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/cpp.yml b/.github/workflows/cpp.yml index 287c0a581c28f..a4db36f52c6c2 100644 --- a/.github/workflows/cpp.yml +++ b/.github/workflows/cpp.yml @@ -228,7 +228,9 @@ jobs: run: ci/scripts/util_checkout.sh - name: Build shell: bash - run: ci/scripts/cpp_build.sh $(pwd) $(pwd)/build + run: | + export BOOST_ROOT=$BOOST_ROOT_1_72_0 + ci/scripts/cpp_build.sh $(pwd) $(pwd)/build - name: Test shell: bash run: ci/scripts/cpp_test.sh $(pwd) $(pwd)/build From bc41ab9954e86ae619ceee22a24887c1c51a941f Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Mon, 20 Apr 2020 19:16:08 +0200 Subject: [PATCH 4/7] Small changes, run test verbosely on GHA --- ci/scripts/cpp_test.sh | 2 + cpp/src/arrow/util/io_util.cc | 13 ++-- cpp/src/arrow/util/io_util_test.cc | 100 ++++++++++++++--------------- 3 files changed, 59 insertions(+), 56 deletions(-) diff --git a/ci/scripts/cpp_test.sh b/ci/scripts/cpp_test.sh index 21284eef0faef..ee7b201c8ac69 100755 --- a/ci/scripts/cpp_test.sh +++ b/ci/scripts/cpp_test.sh @@ -46,6 +46,8 @@ esac pushd ${build_dir} +ctest -V -R arrow-utility-test + ctest -L unittest --output-on-failure -j${n_jobs} if [ "${ARROW_FUZZING}" == "ON" ]; then diff --git a/cpp/src/arrow/util/io_util.cc b/cpp/src/arrow/util/io_util.cc index 636504c13d82f..2e589fd2696e0 100644 --- a/cpp/src/arrow/util/io_util.cc +++ b/cpp/src/arrow/util/io_util.cc @@ -844,10 +844,11 @@ Result FileOpenReadable(const PlatformFilename& file_name) { DWORD last_error = GetLastError(); if (last_error == ERROR_SUCCESS) { errno_actual = 0; - fd = _open_osfhandle((intptr_t)file_handle, _O_RDONLY | _O_BINARY | _O_NOINHERIT); + fd = _open_osfhandle(reinterpret_cast(file_handle), + _O_RDONLY | _O_BINARY | _O_NOINHERIT); } else { - return IOErrorFromWinError(last_error, "Failed to open file '", file_name.ToString(), - "'"); + return IOErrorFromWinError(last_error, "Failed to open local file '", + file_name.ToString(), "'"); } #else fd = open(file_name.ToNative().c_str(), O_RDONLY); @@ -905,10 +906,10 @@ Result FileOpenWritable(const PlatformFilename& file_name, bool write_only, DWORD last_error = GetLastError(); if (last_error == ERROR_SUCCESS || last_error == ERROR_ALREADY_EXISTS) { errno_actual = 0; - fd = _open_osfhandle((intptr_t)file_handle, oflag); + fd = _open_osfhandle(reinterpret_cast(file_handle), oflag); } else { - return IOErrorFromWinError(last_error, "Failed to open file '", file_name.ToString(), - "'"); + return IOErrorFromWinError(last_error, "Failed to open local file '", + file_name.ToString(), "'"); } #else int oflag = O_CREAT; diff --git a/cpp/src/arrow/util/io_util_test.cc b/cpp/src/arrow/util/io_util_test.cc index 09bf95c491028..8f2ed2d203f21 100644 --- a/cpp/src/arrow/util/io_util_test.cc +++ b/cpp/src/arrow/util/io_util_test.cc @@ -446,56 +446,6 @@ TEST(CreateDirTree, Basics) { ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("EF")); ASSERT_OK_AND_ASSIGN(created, CreateDirTree(fn)); ASSERT_TRUE(created); - -#ifndef __APPLE__ - bool deleted; -#ifdef _WIN32 - const std::string KEY_NAME = R"(SYSTEM\CurrentControlSet\Control\FileSystem)"; - const std::string VALUE_NAME = "LongPathsEnabled"; - DWORD value = 0; - DWORD size = sizeof(value); - LSTATUS status = RegGetValueA(HKEY_LOCAL_MACHINE, KEY_NAME.c_str(), VALUE_NAME.c_str(), - RRF_RT_REG_DWORD, NULL, &value, &size); - bool test_long_paths = (status == ERROR_SUCCESS && value == 1); - if (!test_long_paths) { - ARROW_LOG(WARNING) - << "Tests for accessing files with long path names have been disabled. " - << "To enable these tests, set the value of " << VALUE_NAME - << " in registry key \\HKEY_LOCAL_MACHINE\\" << KEY_NAME - << " to 1 on the test host."; - } -#else - bool test_long_paths = true; -#endif - - if (test_long_paths) { - const std::string LONG_BASE = "xxx-io-util-test-dir-long"; - PlatformFilename long_path, long_filename; - int fd = -1; - std::stringstream fs; - fs << LONG_BASE; - for (int i = 0; i < 64; ++i) { - fs << "/123456789ABCDEF"; - } - ASSERT_OK_AND_ASSIGN( - long_path, PlatformFilename::FromString(fs.str())); // long_path length > 1024 - ASSERT_OK_AND_ASSIGN(created, CreateDirTree(long_path)); - ASSERT_TRUE(created); - AssertExists(long_path); - ASSERT_OK_AND_ASSIGN(long_filename, - PlatformFilename::FromString(fs.str() + "/file.txt")); - ASSERT_OK_AND_ASSIGN(fd, FileOpenWritable(long_filename)); - ASSERT_OK(FileClose(fd)); - AssertExists(long_filename); - fd = -1; - ASSERT_OK_AND_ASSIGN(fd, FileOpenReadable(long_filename)); - ASSERT_OK(FileClose(fd)); - ASSERT_OK_AND_ASSIGN(deleted, DeleteDirContents(long_path)); - ASSERT_TRUE(deleted); - ASSERT_OK_AND_ASSIGN(deleted, DeleteDirTree(long_path)); - ASSERT_TRUE(deleted); - } -#endif } TEST(ListDir, Basics) { @@ -573,5 +523,55 @@ TEST(DeleteFile, Basics) { ASSERT_RAISES(IOError, DeleteFile(fn)); } +#ifndef __APPLE__ +TEST(FileUtils, LongPaths) { + // ARROW-8477: check using long file paths under Windows (> 260 characters). + bool created, deleted; +#ifdef _WIN32 + const char* kRegKeyName = R"(SYSTEM\CurrentControlSet\Control\FileSystem)"; + const char* kRegValueName = "LongPathsEnabled"; + DWORD value = 0; + DWORD size = sizeof(value); + LSTATUS status = RegGetValueA(HKEY_LOCAL_MACHINE, kRegKeyName, kRegValueName, + RRF_RT_REG_DWORD, NULL, &value, &size); + bool test_long_paths = (status == ERROR_SUCCESS && value == 1); + if (!test_long_paths) { + ARROW_LOG(WARNING) + << "Tests for accessing files with long path names have been disabled. " + << "To enable these tests, set the value of " << kRegValueName + << " in registry key \\HKEY_LOCAL_MACHINE\\" << kRegKeyName + << " to 1 on the test host."; + return; + } +#endif + + const std::string LONG_BASE = "xxx-io-util-test-dir-long"; + PlatformFilename long_path, long_filename; + int fd = -1; + std::stringstream fs; + fs << LONG_BASE; + for (int i = 0; i < 64; ++i) { + fs << "/123456789ABCDEF"; + } + ASSERT_OK_AND_ASSIGN( + long_path, PlatformFilename::FromString(fs.str())); // long_path length > 1024 + ASSERT_OK_AND_ASSIGN(created, CreateDirTree(long_path)); + ASSERT_TRUE(created); + AssertExists(long_path); + ASSERT_OK_AND_ASSIGN(long_filename, + PlatformFilename::FromString(fs.str() + "/file.txt")); + ASSERT_OK_AND_ASSIGN(fd, FileOpenWritable(long_filename)); + ASSERT_OK(FileClose(fd)); + AssertExists(long_filename); + fd = -1; + ASSERT_OK_AND_ASSIGN(fd, FileOpenReadable(long_filename)); + ASSERT_OK(FileClose(fd)); + ASSERT_OK_AND_ASSIGN(deleted, DeleteDirContents(long_path)); + ASSERT_TRUE(deleted); + ASSERT_OK_AND_ASSIGN(deleted, DeleteDirTree(long_path)); + ASSERT_TRUE(deleted); +} +#endif + } // namespace internal } // namespace arrow From 68d92777d8f605f3de0701fa6cbf1ac8f45d5f9e Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Mon, 20 Apr 2020 19:48:18 +0200 Subject: [PATCH 5/7] Ensure we delete the test directory tree at end of test --- ci/scripts/cpp_test.sh | 2 -- cpp/src/arrow/util/CMakeLists.txt | 2 ++ cpp/src/arrow/util/io_util_test.cc | 16 +++++++++++++--- cpp/src/arrow/util/io_util_test.manifest | 5 +++++ 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/ci/scripts/cpp_test.sh b/ci/scripts/cpp_test.sh index ee7b201c8ac69..21284eef0faef 100755 --- a/ci/scripts/cpp_test.sh +++ b/ci/scripts/cpp_test.sh @@ -46,8 +46,6 @@ esac pushd ${build_dir} -ctest -V -R arrow-utility-test - ctest -L unittest --output-on-failure -j${n_jobs} if [ "${ARROW_FUZZING}" == "ON" ]; then diff --git a/cpp/src/arrow/util/CMakeLists.txt b/cpp/src/arrow/util/CMakeLists.txt index 377921b38cf7d..ce08839d03e7c 100644 --- a/cpp/src/arrow/util/CMakeLists.txt +++ b/cpp/src/arrow/util/CMakeLists.txt @@ -27,6 +27,8 @@ arrow_install_all_headers("arrow/util") # if(WIN32) + # This manifest enables long file paths on Windows 10+ + # See https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file#enable-long-paths-in-windows-10-version-1607-and-later if(MSVC) set(IO_UTIL_TEST_SOURCES io_util_test.cc io_util_test.manifest) else() diff --git a/cpp/src/arrow/util/io_util_test.cc b/cpp/src/arrow/util/io_util_test.cc index 8f2ed2d203f21..2384f2d4cd8e8 100644 --- a/cpp/src/arrow/util/io_util_test.cc +++ b/cpp/src/arrow/util/io_util_test.cc @@ -383,6 +383,10 @@ TEST(DeleteDirContents, Basics) { ASSERT_TRUE(deleted); AssertExists(parent); + // Now actually delete the test directory + ASSERT_OK_AND_ASSIGN(deleted, DeleteDirTree(parent)); + ASSERT_TRUE(deleted); + // It's not an error to call DeleteDirContents on a nonexistent path. ASSERT_OK_AND_ASSIGN(deleted, DeleteDirContents(child1)); ASSERT_FALSE(deleted); @@ -545,14 +549,16 @@ TEST(FileUtils, LongPaths) { } #endif - const std::string LONG_BASE = "xxx-io-util-test-dir-long"; - PlatformFilename long_path, long_filename; + const std::string BASE = "xxx-io-util-test-dir-long"; + PlatformFilename base_path, long_path, long_filename; int fd = -1; std::stringstream fs; - fs << LONG_BASE; + fs << BASE; for (int i = 0; i < 64; ++i) { fs << "/123456789ABCDEF"; } + ASSERT_OK_AND_ASSIGN(base_path, + PlatformFilename::FromString(BASE)); // long_path length > 1024 ASSERT_OK_AND_ASSIGN( long_path, PlatformFilename::FromString(fs.str())); // long_path length > 1024 ASSERT_OK_AND_ASSIGN(created, CreateDirTree(long_path)); @@ -570,6 +576,10 @@ TEST(FileUtils, LongPaths) { ASSERT_TRUE(deleted); ASSERT_OK_AND_ASSIGN(deleted, DeleteDirTree(long_path)); ASSERT_TRUE(deleted); + + // Now delete the whole test directory tree + ASSERT_OK_AND_ASSIGN(deleted, DeleteDirTree(base_path)); + ASSERT_TRUE(deleted); } #endif diff --git a/cpp/src/arrow/util/io_util_test.manifest b/cpp/src/arrow/util/io_util_test.manifest index b9c691c116ceb..b8b848870f1a0 100644 --- a/cpp/src/arrow/util/io_util_test.manifest +++ b/cpp/src/arrow/util/io_util_test.manifest @@ -17,6 +17,11 @@ specific language governing permissions and limitations under the License. --> + + From 4bae2931db1fa84e600be2cba2059366559be18e Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Mon, 20 Apr 2020 20:06:29 +0200 Subject: [PATCH 6/7] Fix XML syntax --- cpp/src/arrow/util/io_util_test.manifest | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/util/io_util_test.manifest b/cpp/src/arrow/util/io_util_test.manifest index b8b848870f1a0..de5c0d8a875b1 100644 --- a/cpp/src/arrow/util/io_util_test.manifest +++ b/cpp/src/arrow/util/io_util_test.manifest @@ -19,8 +19,8 @@ --> From 88fc5c562904847866d5776fe8fe2a8d95e97e2c Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Mon, 20 Apr 2020 20:25:15 +0200 Subject: [PATCH 7/7] Last fixlet (hopefully) --- cpp/src/arrow/util/io_util_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/src/arrow/util/io_util_test.cc b/cpp/src/arrow/util/io_util_test.cc index 2384f2d4cd8e8..b432d3367bdb5 100644 --- a/cpp/src/arrow/util/io_util_test.cc +++ b/cpp/src/arrow/util/io_util_test.cc @@ -383,10 +383,6 @@ TEST(DeleteDirContents, Basics) { ASSERT_TRUE(deleted); AssertExists(parent); - // Now actually delete the test directory - ASSERT_OK_AND_ASSIGN(deleted, DeleteDirTree(parent)); - ASSERT_TRUE(deleted); - // It's not an error to call DeleteDirContents on a nonexistent path. ASSERT_OK_AND_ASSIGN(deleted, DeleteDirContents(child1)); ASSERT_FALSE(deleted); @@ -398,6 +394,10 @@ TEST(DeleteDirContents, Basics) { #else ASSERT_EQ(ErrnoFromStatus(status), ENOENT); #endif + + // Now actually delete the test directory + ASSERT_OK_AND_ASSIGN(deleted, DeleteDirTree(parent)); + ASSERT_TRUE(deleted); } TEST(TemporaryDir, Basics) {