[core] Use statusor for filesystem and resolve dependency

src/ray/common/BUILD

@@ -283,7 +283,12 @@ ray_cc_library(
     hdrs = ["status.h"],
     deps = [
         ":source_location",
-        "//src/ray/util",
+        "//src/ray/util:logging",
+        "//src/ray/util:macros",
+        "//src/ray/util:visibility",
+        "@boost//:system",
+        "@com_google_absl//absl/container:flat_hash_map",
+        "@com_google_absl//absl/strings",
     ],
 )
 

src/ray/common/buffer.h

@@ -21,6 +21,7 @@
 
 #include "ray/common/status.h"
 #include "ray/thirdparty/aligned_alloc.h"
+#include "ray/util/logging.h"
 
 #define BUFFER_ALIGNMENT 64
 

src/ray/common/status.h

@@ -32,20 +32,15 @@
 #include <iosfwd>
 #include <string>
 
+#include "absl/strings/str_cat.h"
 #include "ray/common/source_location.h"
 #include "ray/util/logging.h"
 #include "ray/util/macros.h"
 #include "ray/util/visibility.h"
 
-namespace boost {
-
-namespace system {
-
+namespace boost::system {
 class error_code;
-
-}  // namespace system
-
-}  // namespace boost
+}  // namespace boost::system
 
 // Return the given status if it is not OK.
 #define RAY_RETURN_NOT_OK(s)           \
@@ -328,6 +323,12 @@ class RAY_EXPORT Status {
 
   std::string message() const { return ok() ? "" : state_->msg; }
 
+  template <typename... T>
+  Status &operator<<(T &&...msg) {
+    absl::StrAppend(&state_->msg, std::forward<T>(msg)...);
+    return *this;
+  }
+
  private:
   struct State {
     StatusCode code;

src/ray/common/test/BUILD

@@ -160,6 +160,7 @@ ray_cc_test(
 ray_cc_library(
     name = "testing",
     hdrs = ["testing.h"],
+    deps = ["//src/ray/util:macros"],
     testonly = True,
 )
 

src/ray/util/BUILD

@@ -40,26 +40,30 @@ ray_cc_library(
 # TODO(hjiang): filesystem and logging has interdependency, we should split them into three targets: filesystem, logging, ray_check_macros.
 ray_cc_library(
     name = "logging",
-    hdrs = [
-        "filesystem.h",
-        "logging.h",
-    ],
-    srcs = [
-        "filesystem.cc",
-        "logging.cc",
-    ],
+    hdrs = ["logging.h"],
+    srcs = ["logging.cc"],
     deps = [
         ":event_label",
         ":macros",
+        ":string_utils",
         ":thread_utils",
         "@com_github_spdlog//:spdlog",
         "@com_google_absl//absl/debugging:failure_signal_handler",
         "@com_google_absl//absl/strings:str_format",
-        "@com_google_googletest//:gtest_main",
+        "@com_google_googletest//:gtest_prod",
         "@nlohmann_json",
     ],
 )
 
+ray_cc_library(
+    name = "filesystem",
+    hdrs = ["filesystem.h"],
+    srcs = ["filesystem.cc"],
+    deps = [
+        "//src/ray/common:status_or",
+    ],
+)
+
 ray_cc_library(
     name = "container_util",
     hdrs = ["container_util.h"],
@@ -82,6 +86,7 @@ ray_cc_library(
     ],
     deps = [
         ":cmd_line_utils",
+        ":filesystem",
         ":logging",
         ":macros",
         "@boost//:asio",

src/ray/util/filesystem.cc

@@ -17,8 +17,6 @@
 #include <cstdlib>
 #include <fstream>
 
-#include "ray/util/logging.h"
-
 #ifdef _WIN32
 #include <Windows.h>
 #endif
@@ -57,17 +55,20 @@ std::string GetUserTempDir() {
   while (!result.empty() && IsDirSep(result.back())) {
     result.pop_back();
   }
-  RAY_CHECK(!result.empty());
   return result;
 }
 
-std::string CompleteReadFile(const std::string &fname) {
+StatusOr<std::string> CompleteReadFile(const std::string &fname) {
   std::ifstream file(fname);
-  RAY_CHECK(file.good()) << "Fails to open file " << fname;
+  if (!file.good()) {
+    return Status::IOError("") << "Fails to open file " << fname;
+  }
 
   std::ostringstream buffer;
   buffer << file.rdbuf();
-  RAY_CHECK(file.good()) << "Fails to read from file " << fname;
+  if (!file.good()) {
+    return Status::IOError("") << "Fails to read from file " << fname;
+  }
 
   std::string content = buffer.str();
   file.close();

src/ray/util/filesystem.h

@@ -19,6 +19,8 @@
 #include <string>
 #include <utility>
 
+#include "ray/common/status_or.h"
+
 // Filesystem and path manipulation APIs.
 // (NTFS stream & attribute paths are not supported.)
 
@@ -42,29 +44,9 @@ static inline bool IsDirSep(char ch) {
   return result;
 }
 
-/// \return The result of joining multiple path components.
-template <class... Paths>
-std::string JoinPaths(std::string base, const Paths &...components) {
-  auto join = [](auto &joined_path, const auto &component) {
-    // if the components begin with "/" or "////", just get the path name.
-    if (!component.empty() &&
-        component.front() == std::filesystem::path::preferred_separator) {
-      joined_path = std::filesystem::path(joined_path)
-                        .append(std::filesystem::path(component).filename().string())
-                        .string();
-    } else {
-      joined_path = std::filesystem::path(joined_path).append(component).string();
-    }
-  };
-  (join(base, std::string_view(components)), ...);
-  return base;
-}
-
 // Read the whole content for the given [fname], and return as string.
 // If any error happens, error message will be logged and the program will exit
 // immediately.
-//
-// TODO(hjiang): Use StatusOr as return type in the followup PR.
-std::string CompleteReadFile(const std::string &fname);
+StatusOr<std::string> CompleteReadFile(const std::string &fname);
 
 }  // namespace ray

src/ray/util/logging.cc

@@ -41,7 +41,7 @@
 #include "absl/strings/str_format.h"
 #include "nlohmann/json.hpp"
 #include "ray/util/event_label.h"
-#include "ray/util/filesystem.h"
+#include "ray/util/string_utils.h"
 #include "ray/util/thread_utils.h"
 #include "spdlog/sinks/basic_file_sink.h"
 #include "spdlog/sinks/rotating_file_sink.h"
@@ -367,7 +367,7 @@ void RayLog::InitLogFormat() {
     app_name_without_path = "DefaultApp";
   } else {
     // Find the app name without the path.
-    std::string app_file_name = ray::GetFileName(app_name);
+    std::string app_file_name = std::filesystem::path(app_name).filename().string();
     if (!app_file_name.empty()) {
       app_name_without_path = app_file_name;
     }

src/ray/util/logging.h

@@ -61,6 +61,7 @@
 #include <vector>
 
 #include "ray/util/macros.h"
+#include "ray/util/string_utils.h"
 
 #if defined(_WIN32)
 #ifndef _WINDOWS_

src/ray/util/string_utils.h

@@ -14,6 +14,7 @@
 
 #pragma once
 
+#include <filesystem>
 #include <string>
 
 namespace ray {
@@ -27,4 +28,22 @@ std::string StringToHex(const std::string &str);
 /// \return The scanned prefix of the string, if any.
 std::string ScanToken(std::string::const_iterator &c_str, std::string format);
 
+/// \return The result of joining multiple path components.
+template <class... Paths>
+std::string JoinPaths(std::string base, const Paths &...components) {
+  auto join = [](auto &joined_path, const auto &component) {
+    // if the components begin with "/" or "////", just get the path name.
+    if (!component.empty() &&
+        component.front() == std::filesystem::path::preferred_separator) {
+      joined_path = std::filesystem::path(joined_path)
+                        .append(std::filesystem::path(component).filename().string())
+                        .string();
+    } else {
+      joined_path = std::filesystem::path(joined_path).append(component).string();
+    }
+  };
+  (join(base, std::string_view(components)), ...);
+  return base;
+}
+
 }  // namespace ray

src/ray/util/tests/BUILD

@@ -217,6 +217,7 @@ ray_cc_test(
     name = "pipe_logger_test",
     srcs = ["pipe_logger_test.cc"],
     deps = [
+        "//src/ray/common/test:testing",
         "//src/ray/util",
         "//src/ray/util:pipe_logger",
         "@com_google_googletest//:gtest_main",
@@ -230,6 +231,7 @@ ray_cc_test(
     name = "stream_redirection_utils_test",
     srcs = ["stream_redirection_utils_test.cc"],
     deps = [
+        "//src/ray/common/test:testing",
         "//src/ray/util",
         "//src/ray/util:stream_redirection_utils",
         "@com_google_googletest//:gtest_main",

src/ray/util/tests/pipe_logger_test.cc

@@ -23,6 +23,7 @@
 #include <string_view>
 
 #include "absl/cleanup/cleanup.h"
+#include "ray/common/test/testing.h"
 #include "ray/util/filesystem.h"
 #include "ray/util/util.h"
 
@@ -62,8 +63,9 @@ TEST_P(PipeLoggerTest, NoPipeWrite) {
 
   // Check log content after completion.
   const auto actual_content = CompleteReadFile(test_file_path);
+  RAY_ASSERT_OK(actual_content);
   const std::string expected_content = absl::StrFormat("%s%s", kLogLine1, kLogLine2);
-  EXPECT_EQ(actual_content, expected_content);
+  EXPECT_EQ(*actual_content, expected_content);
 }
 
 INSTANTIATE_TEST_SUITE_P(PipeLoggerTest, PipeLoggerTest, testing::Values(1024, 3));
@@ -114,8 +116,13 @@ TEST_P(PipeLoggerTest, PipeWrite) {
   stream_redirection_handle.Close();
 
   // Check log content after completion.
-  EXPECT_EQ(CompleteReadFile(log_file_path1), kLogLine2);
-  EXPECT_EQ(CompleteReadFile(log_file_path2), kLogLine1);
+  const auto actual_content1 = CompleteReadFile(log_file_path1);
+  RAY_ASSERT_OK(actual_content1);
+  EXPECT_EQ(*actual_content1, kLogLine2);
+
+  const auto actual_content2 = CompleteReadFile(log_file_path2);
+  RAY_ASSERT_OK(actual_content1);
+  EXPECT_EQ(*actual_content2, kLogLine1);
 }
 
 TEST(PipeLoggerTestWithTee, RedirectionWithTee) {
@@ -145,8 +152,9 @@ TEST(PipeLoggerTestWithTee, RedirectionWithTee) {
   EXPECT_EQ(stdout_content, absl::StrFormat("%s%s", kLogLine1, kLogLine2));
 
   // Check log content after completion.
-  EXPECT_EQ(CompleteReadFile(test_file_path),
-            absl::StrFormat("%s%s", kLogLine1, kLogLine2));
+  const auto actual_content = CompleteReadFile(test_file_path);
+  RAY_ASSERT_OK(actual_content);
+  EXPECT_EQ(*actual_content, absl::StrFormat("%s%s", kLogLine1, kLogLine2));
 }
 
 TEST(PipeLoggerTestWithTee, RotatedRedirectionWithTee) {
@@ -181,8 +189,13 @@ TEST(PipeLoggerTestWithTee, RotatedRedirectionWithTee) {
   EXPECT_EQ(stderr_content, absl::StrFormat("%s%s", kLogLine1, kLogLine2));
 
   // Check log content after completion.
-  EXPECT_EQ(CompleteReadFile(test_file_path), kLogLine2);
-  EXPECT_EQ(CompleteReadFile(log_file_path2), kLogLine1);
+  const auto actual_content1 = CompleteReadFile(log_file_path1);
+  RAY_ASSERT_OK(actual_content1);
+  EXPECT_EQ(*actual_content1, kLogLine2);
+
+  const auto actual_content2 = CompleteReadFile(log_file_path2);
+  RAY_ASSERT_OK(actual_content2);
+  EXPECT_EQ(*actual_content2, kLogLine1);
 }
 
 }  // namespace

src/ray/util/tests/stream_redirection_utils_test.cc

@@ -22,6 +22,7 @@
 #include <iostream>
 #include <thread>
 
+#include "ray/common/test/testing.h"
 #include "ray/util/filesystem.h"
 #include "ray/util/util.h"
 
@@ -56,10 +57,14 @@ TEST(LoggingUtilTest, RedirectStderr) {
 
   // Check log content after completion.
   const std::string log_file_path1 = test_file_path;
-  EXPECT_EQ(CompleteReadFile(test_file_path), kLogLine2);
+  const auto actual_content1 = CompleteReadFile(log_file_path1);
+  RAY_ASSERT_OK(actual_content1);
+  EXPECT_EQ(*actual_content1, kLogLine2);
 
   const std::string log_file_path2 = absl::StrFormat("%s.1", test_file_path);
-  EXPECT_EQ(CompleteReadFile(log_file_path2), kLogLine1);
+  const auto actual_content2 = CompleteReadFile(log_file_path2);
+  RAY_ASSERT_OK(actual_content2);
+  EXPECT_EQ(*actual_content2, kLogLine1);
 
   // Check tee-ed to stderr content.
   std::string stderr_content = testing::internal::GetCapturedStderr();