Use statusor for filesystem and resolve dependency

Signed-off-by: dentiny <[email protected]>

src/ray/common/BUILD

@@ -283,7 +283,11 @@ ray_cc_library(
     hdrs = ["status.h"],
     deps = [
         ":source_location",
-        "//src/ray/util",
+        "//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,36 +32,14 @@
 #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
-
-// Return the given status if it is not OK.
-#define RAY_RETURN_NOT_OK(s)           \
-  do {                                 \
-    const ::ray::Status &_s = (s);     \
-    if (RAY_PREDICT_FALSE(!_s.ok())) { \
-      return _s;                       \
-    }                                  \
-  } while (0)
-
-// If the status is not OK, CHECK-fail immediately, appending the status to the
-// logged message. The message can be appended with <<.
-#define RAY_CHECK_OK(s)                          \
-  if (const ::ray::Status &_status_ = (s); true) \
-  RAY_CHECK_WITH_DISPLAY(_status_.ok(), #s)      \
-      << "Status not OK: " << _status_.ToString() << " "
+}  // namespace boost::system
 
 namespace ray {
 
@@ -328,6 +306,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/status_or.h

@@ -20,27 +20,8 @@
 #include <utility>
 
 #include "absl/base/attributes.h"
-#include "ray/common/macros.h"
 #include "ray/common/status.h"
 
-#define __RAY_ASSIGN_OR_RETURN_IMPL(var, expr, statusor_name) \
-  auto statusor_name = (expr);                                \
-  RAY_RETURN_NOT_OK(statusor_name.status());                  \
-  var = std::move(statusor_name).value()
-
-// Use `RAY_UNIQUE_VARIABLE` to add line number into variable name, since we could have
-// multiple macros used in one code block.
-#define RAY_ASSIGN_OR_RETURN(var, expr) \
-  __RAY_ASSIGN_OR_RETURN_IMPL(var, expr, RAY_UNIQUE_VARIABLE(statusor))
-
-#define __RAY_ASSIGN_OR_CHECK_IMPL(var, expr, statusor_name) \
-  auto statusor_name = (expr);                               \
-  RAY_CHECK_OK(statusor_name.status());                      \
-  var = std::move(statusor_name).value()
-
-#define RAY_ASSIGN_OR_CHECK(var, expr) \
-  __RAY_ASSIGN_OR_CHECK_IMPL(var, expr, RAY_UNIQUE_VARIABLE(statusor))
-
 namespace ray {
 
 template <typename T>

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",
+        ":filesystem",
         ":macros",
         ":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"],

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.)
 
@@ -63,8 +65,6 @@ std::string JoinPaths(std::string base, const Paths &...components) {
 // 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.h

@@ -233,6 +233,42 @@ enum class RayLogLevel {
                        ? ray::RayLogLevel::level                                         \
                        : ray::RayLogLevel::otherLevel)
 
+// Ray status macros.
+// Return the given status if it is not OK.
+#define RAY_RETURN_NOT_OK(s)           \
+  do {                                 \
+    const ::ray::Status &_s = (s);     \
+    if (RAY_PREDICT_FALSE(!_s.ok())) { \
+      return _s;                       \
+    }                                  \
+  } while (0)
+
+// If the status is not OK, CHECK-fail immediately, appending the status to the
+// logged message. The message can be appended with <<.
+#define RAY_CHECK_OK(s)                          \
+  if (const ::ray::Status &_status_ = (s); true) \
+  RAY_CHECK_WITH_DISPLAY(_status_.ok(), #s)      \
+      << "Status not OK: " << _status_.ToString() << " "
+
+// Ray StatusOr macros.
+#define __RAY_ASSIGN_OR_RETURN_IMPL(var, expr, statusor_name) \
+  auto statusor_name = (expr);                                \
+  RAY_RETURN_NOT_OK(statusor_name.status());                  \
+  var = std::move(statusor_name).value()
+
+// Use `RAY_UNIQUE_VARIABLE` to add line number into variable name, since we could have
+// multiple macros used in one code block.
+#define RAY_ASSIGN_OR_RETURN(var, expr) \
+  __RAY_ASSIGN_OR_RETURN_IMPL(var, expr, RAY_UNIQUE_VARIABLE(statusor))
+
+#define __RAY_ASSIGN_OR_CHECK_IMPL(var, expr, statusor_name) \
+  auto statusor_name = (expr);                               \
+  RAY_CHECK_OK(statusor_name.status());                      \
+  var = std::move(statusor_name).value()
+
+#define RAY_ASSIGN_OR_CHECK(var, expr) \
+  __RAY_ASSIGN_OR_CHECK_IMPL(var, expr, RAY_UNIQUE_VARIABLE(statusor))
+
 // To make the logging lib plugable with other logging libs and make
 // the implementation unawared by the user, RayLog is only a declaration
 // which hide the implementation into logging.cc file.

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"
 
@@ -61,7 +62,7 @@ TEST_P(PipeLoggerTest, NoPipeWrite) {
   stream_redirection_handle.Close();
 
   // Check log content after completion.
-  const auto actual_content = CompleteReadFile(test_file_path);
+  RAY_ASSIGN_OR_EXPECT(const auto actual_content, CompleteReadFile(test_file_path));
   const std::string expected_content = absl::StrFormat("%s%s", kLogLine1, kLogLine2);
   EXPECT_EQ(actual_content, expected_content);
 }
@@ -114,8 +115,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(test_file_path1);
+  RAY_ASSERT_OK(actual_content1);
+  EXPECT_EQ(*actual_content1, kLogLine2);
+
+  const auto actual_content2 = CompleteReadFile(test_file_path2);
+  RAY_ASSERT_OK(actual_content1);
+  EXPECT_EQ(*actual_content2, kLogLine1);
 }
 
 TEST(PipeLoggerTestWithTee, RedirectionWithTee) {
@@ -145,8 +151,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 +188,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(test_file_path1);
+  RAY_ASSERT_OK(actual_content1);
+  EXPECT_EQ(*actual_content1, kLogLine2);
+
+  const auto actual_content2 = CompleteReadFile(test_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();