From 61e8a24340e7fed6af4385844ef97a6af6796aba Mon Sep 17 00:00:00 2001 From: edgchen1 <18449977+edgchen1@users.noreply.github.com> Date: Wed, 18 Mar 2020 17:57:57 -0700 Subject: [PATCH] Address PR comments (#3255) * Added comment for ntfw_remove(). * Rewrite WindowsEnv::DeleteFolder(), some other clean up. --- onnxruntime/core/platform/env.h | 17 ++-- onnxruntime/core/platform/posix/env.cc | 13 +-- onnxruntime/core/platform/windows/env.cc | 83 ++++++++++--------- onnxruntime/test/framework/path_lib_test.cc | 2 - onnxruntime/test/platform/env_test.cc | 10 ++- onnxruntime/test/util/include/gtest_utils.h | 20 ----- .../test/framework/checkpointing_test.cc | 2 +- .../framework/distributed_run_context_test.cc | 2 +- .../graph/optimizer_graph_builder_test.cc | 2 +- .../test/model/data_loader_test.cc | 2 +- 10 files changed, 71 insertions(+), 82 deletions(-) delete mode 100644 onnxruntime/test/util/include/gtest_utils.h diff --git a/onnxruntime/core/platform/env.h b/onnxruntime/core/platform/env.h index 75b73dae86..8b4deb84b2 100644 --- a/onnxruntime/core/platform/env.h +++ b/onnxruntime/core/platform/env.h @@ -24,10 +24,10 @@ limitations under the License. #include #include "core/common/common.h" +#include "core/common/path_string.h" #include "core/framework/callback.h" #include "core/platform/env_time.h" #include "core/platform/telemetry.h" -#include "core/session/onnxruntime_c_api.h" // for ORTCHAR_T #ifndef _WIN32 #include @@ -81,7 +81,7 @@ class Env { * Gets the length of the specified file. */ virtual common::Status GetFileLength( - const ORTCHAR_T* file_path, size_t& length) const = 0; + const PathChar* file_path, size_t& length) const = 0; /** * Copies the content of the file into the provided buffer. @@ -91,7 +91,7 @@ class Env { * @param buffer The buffer in which to write. */ virtual common::Status ReadFileIntoBuffer( - const ORTCHAR_T* file_path, FileOffsetType offset, size_t length, + const PathChar* file_path, FileOffsetType offset, size_t length, gsl::span buffer) const = 0; using MappedMemoryPtr = std::unique_ptr; @@ -107,7 +107,7 @@ class Env { * unmaps the memory (unless release()'d) when destroyed. */ virtual common::Status MapFileIntoMemory( - const ORTCHAR_T* file_path, FileOffsetType offset, size_t length, + const PathChar* file_path, FileOffsetType offset, size_t length, MappedMemoryPtr& mapped_memory) const = 0; #ifdef _WIN32 @@ -115,9 +115,6 @@ class Env { virtual bool FolderExists(const std::wstring& path) const = 0; /// \brief Recursively creates the directory, if it doesn't exist. virtual common::Status CreateFolder(const std::wstring& path) const = 0; - // Recursively deletes the directory and its contents. - // Note: This function is not thread safe! - virtual common::Status DeleteFolder(const std::wstring& path) const = 0; //Mainly for use with protobuf library virtual common::Status FileOpenRd(const std::wstring& path, /*out*/ int& fd) const = 0; //Mainly for use with protobuf library @@ -129,7 +126,7 @@ class Env { virtual common::Status CreateFolder(const std::string& path) const = 0; // Recursively deletes the directory and its contents. // Note: This function is not thread safe! - virtual common::Status DeleteFolder(const std::string& path) const = 0; + virtual common::Status DeleteFolder(const PathString& path) const = 0; //Mainly for use with protobuf library virtual common::Status FileOpenRd(const std::string& path, /*out*/ int& fd) const = 0; //Mainly for use with protobuf library @@ -139,8 +136,8 @@ class Env { /** Gets the canonical form of a file path (symlinks resolved). */ virtual common::Status GetCanonicalPath( - const std::basic_string& path, - std::basic_string& canonical_path) const = 0; + const PathString& path, + PathString& canonical_path) const = 0; //This functions is always successful. It can't fail. virtual PIDType GetSelfPid() const = 0; diff --git a/onnxruntime/core/platform/posix/env.cc b/onnxruntime/core/platform/posix/env.cc index 6bd3f11f1f..276daebe6d 100644 --- a/onnxruntime/core/platform/posix/env.cc +++ b/onnxruntime/core/platform/posix/env.cc @@ -84,6 +84,7 @@ long int TempFailureRetry(TFunc retriable_operation, TFuncArgs&&... args) { return result; } +// nftw() callback to remove a file int nftw_remove( const char* fpath, const struct stat* /*sb*/, int /*typeflag*/, struct FTW* /*ftwbuf*/) { @@ -141,7 +142,7 @@ class PosixEnv : public Env { return getpid(); } - Status GetFileLength(const ORTCHAR_T* file_path, size_t& length) const override { + Status GetFileLength(const PathChar* file_path, size_t& length) const override { ScopedFileDescriptor file_descriptor{open(file_path, O_RDONLY)}; if (file_descriptor.Get() < 0) { return ReportSystemError("open", file_path); @@ -162,7 +163,7 @@ class PosixEnv : public Env { } Status ReadFileIntoBuffer( - const ORTCHAR_T* file_path, FileOffsetType offset, size_t length, + const PathChar* file_path, FileOffsetType offset, size_t length, gsl::span buffer) const override { ORT_RETURN_IF_NOT(file_path); ORT_RETURN_IF_NOT(offset >= 0); @@ -207,7 +208,7 @@ class PosixEnv : public Env { } Status MapFileIntoMemory( - const ORTCHAR_T* file_path, FileOffsetType offset, size_t length, + const PathChar* file_path, FileOffsetType offset, size_t length, MappedMemoryPtr& mapped_memory) const override { ORT_RETURN_IF_NOT(file_path); ORT_RETURN_IF_NOT(offset >= 0); @@ -285,7 +286,7 @@ class PosixEnv : public Env { return Status::OK(); } - common::Status DeleteFolder(const std::string& path) const override { + common::Status DeleteFolder(const PathString& path) const override { const auto result = nftw( path.c_str(), &nftw_remove, 32, FTW_DEPTH | FTW_PHYS); ORT_RETURN_IF_NOT( @@ -318,8 +319,8 @@ class PosixEnv : public Env { } common::Status GetCanonicalPath( - const std::basic_string& path, - std::basic_string& canonical_path) const override { + const PathString& path, + PathString& canonical_path) const override { MallocdStringPtr canonical_path_cstr{realpath(path.c_str(), nullptr)}; if (!canonical_path_cstr) { return ReportSystemError("realpath", path); diff --git a/onnxruntime/core/platform/windows/env.cc b/onnxruntime/core/platform/windows/env.cc index 8f6312259e..b2206d1df0 100644 --- a/onnxruntime/core/platform/windows/env.cc +++ b/onnxruntime/core/platform/windows/env.cc @@ -18,7 +18,6 @@ limitations under the License. #include #include -#include #include #include @@ -32,6 +31,8 @@ limitations under the License. #include "core/platform/scoped_resource.h" #include "core/platform/windows/telemetry.h" +#include "core/framework/path_lib.h" // for LoopDir() // TODO path_lib is moving to core/platform + namespace onnxruntime { namespace { @@ -89,7 +90,7 @@ class WindowsEnv : public Env { return GetCurrentProcessId(); } - Status GetFileLength(const ORTCHAR_T* file_path, size_t& length) const override { + Status GetFileLength(const PathChar* file_path, size_t& length) const override { ScopedFileHandle file_handle{CreateFileW( file_path, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL)}; LARGE_INTEGER filesize; @@ -105,7 +106,7 @@ class WindowsEnv : public Env { } Status ReadFileIntoBuffer( - const ORTCHAR_T* const file_path, const FileOffsetType offset, const size_t length, + const PathChar* const file_path, const FileOffsetType offset, const size_t length, const gsl::span buffer) const override { ORT_RETURN_IF_NOT(file_path); ORT_RETURN_IF_NOT(offset >= 0); @@ -153,7 +154,7 @@ class WindowsEnv : public Env { } Status MapFileIntoMemory( - const ORTCHAR_T*, FileOffsetType, size_t, + const PathChar*, FileOffsetType, size_t, MappedMemoryPtr&) const override { return ORT_MAKE_STATUS(ONNXRUNTIME, NOT_IMPLEMENTED, "MapFileIntoMemory is not implemented on Windows."); } @@ -198,44 +199,48 @@ class WindowsEnv : public Env { return Status::OK(); } - common::Status DeleteFolder(const std::wstring& path) const override { - // SHFileOperation() will also delete files, so check for directory first - ORT_RETURN_IF_NOT(FolderExists(path), "Directory does not exist: ", ToMBString(path)); + common::Status DeleteFolder(const PathString& path) const override { + Status final_status = Status::OK(); + LoopDir( + path, + [this, &path, &final_status]( + const PathString& child_basename, OrtFileType file_type) { + // ignore . and .. + if (child_basename == ORT_TSTR(".") || child_basename == ORT_TSTR("..")) { + return true; + } - const std::wstring path_ending_with_double_null = path + L'\0'; - SHFILEOPSTRUCTW sh_file_op{}; - sh_file_op.wFunc = FO_DELETE; - sh_file_op.pFrom = path_ending_with_double_null.c_str(); - sh_file_op.fFlags = FOF_NO_UI; + const PathString child_path = path + GetPathSep() + child_basename; - const auto result = ::SHFileOperationW(&sh_file_op); - ORT_RETURN_IF_NOT(result == 0, "SHFileOperation() failed with error: ", result); + if (file_type == OrtFileType::TYPE_DIR) { + const auto delete_dir_status = DeleteFolder(child_path); + if (!delete_dir_status.IsOK()) { + final_status = delete_dir_status; + } + } else { // not directory + if (!DeleteFileW(child_path.c_str())) { + const auto err = GetLastError(); + final_status = ORT_MAKE_STATUS( + ONNXRUNTIME, FAIL, + "DeleteFile() failed - path: ", ToMBString(child_path), + ", error code: ", err); + } + } - ORT_RETURN_IF_NOT( - !sh_file_op.fAnyOperationsAborted, - "SHFileOperation() indicated that an operation was aborted."); + return final_status.IsOK(); + }); - return Status::OK(); - } + ORT_RETURN_IF_ERROR(final_status); - common::Status DeleteFolder(const std::string& path) const override { - // SHFileOperation() will also delete files, so check for directory first - ORT_RETURN_IF_NOT(FolderExists(path), "Directory does not exist: ", path); + if (!RemoveDirectoryW(path.c_str())) { + const auto err = GetLastError(); + final_status = ORT_MAKE_STATUS( + ONNXRUNTIME, FAIL, + "RemoveDirectory() failed - path: ", ToMBString(path), + ", error code: ", err); + } - const std::string path_ending_with_double_null = path + '\0'; - SHFILEOPSTRUCTA sh_file_op{}; - sh_file_op.wFunc = FO_DELETE; - sh_file_op.pFrom = path_ending_with_double_null.c_str(); - sh_file_op.fFlags = FOF_NO_UI; - - const auto result = ::SHFileOperationA(&sh_file_op); - ORT_RETURN_IF_NOT(result == 0, "SHFileOperation() failed with error: ", result); - - ORT_RETURN_IF_NOT( - !sh_file_op.fAnyOperationsAborted, - "SHFileOperation() indicated that an operation was aborted."); - - return Status::OK(); + return final_status; } common::Status FileOpenRd(const std::wstring& path, /*out*/ int& fd) const override { @@ -279,8 +284,8 @@ class WindowsEnv : public Env { } common::Status GetCanonicalPath( - const std::basic_string& path, - std::basic_string& canonical_path) const override { + const PathString& path, + PathString& canonical_path) const override { // adapted from MSVC STL std::filesystem::canonical() implementation // https://github.com/microsoft/STL/blob/ed3cbf36416a385828e7a5987ca52cb42882d84b/stl/inc/filesystem#L2986 @@ -297,7 +302,7 @@ class WindowsEnv : public Env { file_handle.IsValid(), "CreateFile() failed: ", GetLastError()); constexpr DWORD initial_buffer_size = MAX_PATH; - std::vector result_buffer{}; + std::vector result_buffer{}; result_buffer.resize(initial_buffer_size); while (true) { diff --git a/onnxruntime/test/framework/path_lib_test.cc b/onnxruntime/test/framework/path_lib_test.cc index 0b8fd2a3c1..c95ad4629b 100644 --- a/onnxruntime/test/framework/path_lib_test.cc +++ b/onnxruntime/test/framework/path_lib_test.cc @@ -8,8 +8,6 @@ #include "core/platform/env.h" #include "test/framework/test_utils.h" #include "core/framework/path_lib.h" // TODO fix include order dependency, path_lib.h should be first -#include "test/util/include/gtest_utils.h" -#include "test/util/include/temp_dir.h" namespace onnxruntime { namespace test { diff --git a/onnxruntime/test/platform/env_test.cc b/onnxruntime/test/platform/env_test.cc index 886c56e1b3..e0bffed8c4 100644 --- a/onnxruntime/test/platform/env_test.cc +++ b/onnxruntime/test/platform/env_test.cc @@ -3,10 +3,12 @@ #include "core/platform/env.h" +#include + #include "gtest/gtest.h" #include "core/common/path_string.h" -#include "test/util/include/gtest_utils.h" +#include "test/util/include/asserts.h" namespace onnxruntime { namespace test { @@ -21,6 +23,12 @@ TEST(PlatformEnvTest, DirectoryCreationAndDeletion) { ASSERT_STATUS_OK(env.CreateFolder(sub_dir)); ASSERT_TRUE(env.FolderExists(sub_dir)); + // create a file in the subdirectory + { + std::ofstream outfile{sub_dir + ORT_TSTR("/file")}; + outfile << "hello!"; + } + ASSERT_STATUS_OK(env.DeleteFolder(root_dir)); ASSERT_FALSE(env.FolderExists(root_dir)); } diff --git a/onnxruntime/test/util/include/gtest_utils.h b/onnxruntime/test/util/include/gtest_utils.h deleted file mode 100644 index a517d36ad4..0000000000 --- a/onnxruntime/test/util/include/gtest_utils.h +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -#pragma once - -#include "gtest/gtest.h" - -#include "core/common/status.h" - -#define ASSERT_STATUS_OK(expr) \ - do { \ - const common::Status status = (expr); \ - ASSERT_TRUE(status.IsOK()) << status.ErrorMessage(); \ - } while (0) - -#define EXPECT_STATUS_OK(expr) \ - do { \ - const common::Status status = (expr); \ - EXPECT_TRUE(status.IsOK()) << status.ErrorMessage(); \ - } while (0) diff --git a/orttraining/orttraining/test/framework/checkpointing_test.cc b/orttraining/orttraining/test/framework/checkpointing_test.cc index 53a8074578..940d0ebd16 100644 --- a/orttraining/orttraining/test/framework/checkpointing_test.cc +++ b/orttraining/orttraining/test/framework/checkpointing_test.cc @@ -14,7 +14,7 @@ #include "core/framework/path_lib.h" #include "core/framework/tensor.h" #include "core/framework/tensorprotoutils.h" -#include "test/util/include/gtest_utils.h" +#include "test/util/include/asserts.h" #include "test/util/include/temp_dir.h" using onnxruntime::test::TemporaryDirectory; diff --git a/orttraining/orttraining/test/framework/distributed_run_context_test.cc b/orttraining/orttraining/test/framework/distributed_run_context_test.cc index f309f9262a..5fd7439cae 100644 --- a/orttraining/orttraining/test/framework/distributed_run_context_test.cc +++ b/orttraining/orttraining/test/framework/distributed_run_context_test.cc @@ -6,7 +6,7 @@ #include "core/common/common.h" #include "orttraining/core/framework/distributed_run_context.h" -#include "test/util/include/gtest_utils.h" +#include "test/util/include/asserts.h" namespace onnxruntime { namespace training { diff --git a/orttraining/orttraining/test/graph/optimizer_graph_builder_test.cc b/orttraining/orttraining/test/graph/optimizer_graph_builder_test.cc index d39c01ad1d..7b8611c98b 100644 --- a/orttraining/orttraining/test/graph/optimizer_graph_builder_test.cc +++ b/orttraining/orttraining/test/graph/optimizer_graph_builder_test.cc @@ -20,7 +20,7 @@ #include "orttraining/core/graph/adasum_optimizer_graph_builder.h" #include "orttraining/core/graph/zero_optimizer_graph_builder.h" #include "test/framework/test_utils.h" -#include "test/util/include/gtest_utils.h" +#include "test/util/include/asserts.h" #include "test/test_environment.h" using onnxruntime::test::CountOpsInGraph; diff --git a/orttraining/orttraining/test/model/data_loader_test.cc b/orttraining/orttraining/test/model/data_loader_test.cc index ed4fe9c4cd..fcdaa99818 100644 --- a/orttraining/orttraining/test/model/data_loader_test.cc +++ b/orttraining/orttraining/test/model/data_loader_test.cc @@ -9,7 +9,7 @@ #include "orttraining/models/runner/training_util.h" #include "core/framework/path_lib.h" // TODO fix include ordering dependency #include "orttraining/models/runner/data_loader.h" -#include "test/util/include/gtest_utils.h" +#include "test/util/include/asserts.h" #include "test/util/include/temp_dir.h" using onnxruntime::test::TemporaryDirectory;