diff --git a/src/ray/common/BUILD b/src/ray/common/BUILD index f5e6b9d8b9cd..25b7d8c0f3b8 100644 --- a/src/ray/common/BUILD +++ b/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", ], ) diff --git a/src/ray/common/buffer.h b/src/ray/common/buffer.h index 0ce0829d0af7..a4607e60ce29 100644 --- a/src/ray/common/buffer.h +++ b/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 diff --git a/src/ray/common/status.h b/src/ray/common/status.h index e8206ba90aaf..c7bd759352b3 100644 --- a/src/ray/common/status.h +++ b/src/ray/common/status.h @@ -32,20 +32,15 @@ #include #include +#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 + Status &operator<<(T &&...msg) { + absl::StrAppend(&state_->msg, std::forward(msg)...); + return *this; + } + private: struct State { StatusCode code; diff --git a/src/ray/common/test/BUILD b/src/ray/common/test/BUILD index 75f15f741c6b..08bf2a269523 100644 --- a/src/ray/common/test/BUILD +++ b/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, ) diff --git a/src/ray/util/BUILD b/src/ray/util/BUILD index 2afc20d3fa84..6d871ce4e846 100644 --- a/src/ray/util/BUILD +++ b/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", diff --git a/src/ray/util/filesystem.cc b/src/ray/util/filesystem.cc index 9d4e2fd163d9..d8e4f3f135d3 100644 --- a/src/ray/util/filesystem.cc +++ b/src/ray/util/filesystem.cc @@ -17,8 +17,6 @@ #include #include -#include "ray/util/logging.h" - #ifdef _WIN32 #include #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 ReadEntireFile(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(); diff --git a/src/ray/util/filesystem.h b/src/ray/util/filesystem.h index beed7387b102..bc1b8a0430b0 100644 --- a/src/ray/util/filesystem.h +++ b/src/ray/util/filesystem.h @@ -19,6 +19,8 @@ #include #include +#include "ray/common/status_or.h" + // Filesystem and path manipulation APIs. // (NTFS stream & attribute paths are not supported.) @@ -42,29 +44,8 @@ static inline bool IsDirSep(char ch) { return result; } -/// \return The result of joining multiple path components. -template -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); +// Return IO error status if open and read operation fail. +StatusOr ReadEntireFile(const std::string &fname); } // namespace ray diff --git a/src/ray/util/logging.cc b/src/ray/util/logging.cc index 96835c5912a5..f2460b5e371f 100644 --- a/src/ray/util/logging.cc +++ b/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; } diff --git a/src/ray/util/logging.h b/src/ray/util/logging.h index 8b6401f99be2..5da393f4304e 100644 --- a/src/ray/util/logging.h +++ b/src/ray/util/logging.h @@ -61,6 +61,7 @@ #include #include "ray/util/macros.h" +#include "ray/util/string_utils.h" #if defined(_WIN32) #ifndef _WINDOWS_ diff --git a/src/ray/util/string_utils.h b/src/ray/util/string_utils.h index e66b117397b8..6d8d2121b10a 100644 --- a/src/ray/util/string_utils.h +++ b/src/ray/util/string_utils.h @@ -14,6 +14,7 @@ #pragma once +#include #include 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 +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 diff --git a/src/ray/util/tests/BUILD b/src/ray/util/tests/BUILD index 7159a0f61a69..7e8608e90e19 100644 --- a/src/ray/util/tests/BUILD +++ b/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", diff --git a/src/ray/util/tests/pipe_logger_test.cc b/src/ray/util/tests/pipe_logger_test.cc index 3badef49eeb4..61d4f4345ae5 100644 --- a/src/ray/util/tests/pipe_logger_test.cc +++ b/src/ray/util/tests/pipe_logger_test.cc @@ -23,6 +23,7 @@ #include #include "absl/cleanup/cleanup.h" +#include "ray/common/test/testing.h" #include "ray/util/filesystem.h" #include "ray/util/util.h" @@ -61,9 +62,10 @@ TEST_P(PipeLoggerTest, NoPipeWrite) { stream_redirection_handle.Close(); // Check log content after completion. - const auto actual_content = CompleteReadFile(test_file_path); + const auto actual_content = ReadEntireFile(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)); @@ -112,8 +114,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 = ReadEntireFile(log_file_path1); + RAY_ASSERT_OK(actual_content1); + EXPECT_EQ(*actual_content1, kLogLine2); + + const auto actual_content2 = ReadEntireFile(log_file_path2); + RAY_ASSERT_OK(actual_content1); + EXPECT_EQ(*actual_content2, kLogLine1); } TEST(PipeLoggerTestWithTee, RedirectionWithTee) { @@ -143,8 +150,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 = ReadEntireFile(test_file_path); + RAY_ASSERT_OK(actual_content); + EXPECT_EQ(*actual_content, absl::StrFormat("%s%s", kLogLine1, kLogLine2)); } TEST(PipeLoggerTestWithTee, RotatedRedirectionWithTee) { @@ -179,8 +187,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 = ReadEntireFile(log_file_path1); + RAY_ASSERT_OK(actual_content1); + EXPECT_EQ(*actual_content1, kLogLine2); + + const auto actual_content2 = ReadEntireFile(log_file_path2); + RAY_ASSERT_OK(actual_content2); + EXPECT_EQ(*actual_content2, kLogLine1); } // Testing senario: log to stdout and file; check whether these two sinks generate diff --git a/src/ray/util/tests/stream_redirection_utils_test.cc b/src/ray/util/tests/stream_redirection_utils_test.cc index f6995e68fde9..77ba330e1636 100644 --- a/src/ray/util/tests/stream_redirection_utils_test.cc +++ b/src/ray/util/tests/stream_redirection_utils_test.cc @@ -22,6 +22,7 @@ #include #include +#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 = ReadEntireFile(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 = ReadEntireFile(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();