Address PR comments (#3255)

* Added comment for ntfw_remove().

* Rewrite WindowsEnv::DeleteFolder(), some other clean up.

onnxruntime/core/platform/env.h

@@ -24,10 +24,10 @@ limitations under the License.
 #include <gsl/gsl>
 
 #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 <sys/types.h>
@@ -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<char> buffer) const = 0;
 
   using MappedMemoryPtr = std::unique_ptr<char[], OrtCallbackInvoker>;
@@ -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<ORTCHAR_T>& path,
-      std::basic_string<ORTCHAR_T>& 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;

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<char> 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<ORTCHAR_T>& path,
-      std::basic_string<ORTCHAR_T>& 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);

onnxruntime/core/platform/windows/env.cc

@@ -18,7 +18,6 @@ limitations under the License.
 
 #include <Shlwapi.h>
 #include <Windows.h>
-#include <shellapi.h>
 
 #include <fstream>
 #include <string>
@@ -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<char> 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<PathChar>() + 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<ORTCHAR_T>& path,
-      std::basic_string<ORTCHAR_T>& 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<ORTCHAR_T> result_buffer{};
+    std::vector<PathChar> result_buffer{};
     result_buffer.resize(initial_buffer_size);
 
     while (true) {

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 {

onnxruntime/test/platform/env_test.cc

@@ -3,10 +3,12 @@
 
 #include "core/platform/env.h"
 
+#include <fstream>
+
 #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));
 }

onnxruntime/test/util/include/gtest_utils.h

@@ -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)

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;

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 {

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;

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;