From 2d55e3c2eebb3a2a78e757254bf8cb6d89a14602 Mon Sep 17 00:00:00 2001 From: dentiny Date: Thu, 23 Jan 2025 04:58:25 +0000 Subject: [PATCH 1/4] Use statusor for filesystem and resolve dependency Signed-off-by: dentiny --- src/ray/common/BUILD | 7 ++++- src/ray/common/buffer.h | 1 + src/ray/common/status.h | 17 ++++++------ src/ray/common/test/BUILD | 1 + src/ray/util/BUILD | 23 +++++++++------- src/ray/util/filesystem.cc | 13 +++++----- src/ray/util/filesystem.h | 24 +++-------------- src/ray/util/logging.cc | 4 +-- src/ray/util/logging.h | 1 + src/ray/util/string_utils.h | 19 ++++++++++++++ src/ray/util/tests/BUILD | 2 ++ src/ray/util/tests/pipe_logger_test.cc | 26 ++++++++++++++----- .../tests/stream_redirection_utils_test.cc | 9 +++++-- 13 files changed, 91 insertions(+), 56 deletions(-) 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..b96d6b0c35a1 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 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(); diff --git a/src/ray/util/filesystem.h b/src/ray/util/filesystem.h index beed7387b102..a75bcec8b673 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,9 @@ 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); +StatusOr CompleteReadFile(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 cfa3f149d19d..96114bab3321 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,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 diff --git a/src/ray/util/tests/stream_redirection_utils_test.cc b/src/ray/util/tests/stream_redirection_utils_test.cc index f6995e68fde9..cdeb418be11d 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 = 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(); From 5015cee8a3d6a367253e93019560cb22878da895 Mon Sep 17 00:00:00 2001 From: dentiny Date: Thu, 23 Jan 2025 16:33:31 +0000 Subject: [PATCH 2/4] fix build Signed-off-by: dentiny --- src/ray/util/tests/pipe_logger_test.cc | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/ray/util/tests/pipe_logger_test.cc b/src/ray/util/tests/pipe_logger_test.cc index 96114bab3321..5f20f62eb4e8 100644 --- a/src/ray/util/tests/pipe_logger_test.cc +++ b/src/ray/util/tests/pipe_logger_test.cc @@ -62,9 +62,10 @@ TEST_P(PipeLoggerTest, NoPipeWrite) { stream_redirection_handle.Close(); // Check log content after completion. - RAY_ASSIGN_OR_EXPECT(const auto actual_content, CompleteReadFile(test_file_path)); + 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)); @@ -115,11 +116,11 @@ TEST_P(PipeLoggerTest, PipeWrite) { stream_redirection_handle.Close(); // Check log content after completion. - const auto actual_content1 = CompleteReadFile(test_file_path1); + const auto actual_content1 = CompleteReadFile(log_file_path1); RAY_ASSERT_OK(actual_content1); EXPECT_EQ(*actual_content1, kLogLine2); - const auto actual_content2 = CompleteReadFile(test_file_path2); + const auto actual_content2 = CompleteReadFile(log_file_path2); RAY_ASSERT_OK(actual_content1); EXPECT_EQ(*actual_content2, kLogLine1); } @@ -188,11 +189,11 @@ TEST(PipeLoggerTestWithTee, RotatedRedirectionWithTee) { EXPECT_EQ(stderr_content, absl::StrFormat("%s%s", kLogLine1, kLogLine2)); // Check log content after completion. - const auto actual_content1 = CompleteReadFile(test_file_path1); + const auto actual_content1 = CompleteReadFile(log_file_path1); RAY_ASSERT_OK(actual_content1); EXPECT_EQ(*actual_content1, kLogLine2); - const auto actual_content2 = CompleteReadFile(test_file_path2); + const auto actual_content2 = CompleteReadFile(log_file_path2); RAY_ASSERT_OK(actual_content2); EXPECT_EQ(*actual_content2, kLogLine1); } From 2ef5af02f0367e503771dd8aee149927465e8ca8 Mon Sep 17 00:00:00 2001 From: dentiny Date: Thu, 23 Jan 2025 21:45:17 +0000 Subject: [PATCH 3/4] fix comment Signed-off-by: dentiny --- src/ray/util/filesystem.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ray/util/filesystem.h b/src/ray/util/filesystem.h index a75bcec8b673..d6f9d575d0a8 100644 --- a/src/ray/util/filesystem.h +++ b/src/ray/util/filesystem.h @@ -45,8 +45,7 @@ static inline bool IsDirSep(char ch) { } // 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. +// Return IO error status if open and read operation fail. StatusOr CompleteReadFile(const std::string &fname); } // namespace ray From 8e987ecaaa78865966a68542da8bfdb40e0318d2 Mon Sep 17 00:00:00 2001 From: dentiny Date: Fri, 24 Jan 2025 01:33:44 +0000 Subject: [PATCH 4/4] rename function Signed-off-by: dentiny --- src/ray/util/filesystem.cc | 2 +- src/ray/util/filesystem.h | 2 +- src/ray/util/tests/pipe_logger_test.cc | 12 ++++++------ src/ray/util/tests/stream_redirection_utils_test.cc | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/ray/util/filesystem.cc b/src/ray/util/filesystem.cc index b96d6b0c35a1..d8e4f3f135d3 100644 --- a/src/ray/util/filesystem.cc +++ b/src/ray/util/filesystem.cc @@ -58,7 +58,7 @@ std::string GetUserTempDir() { return result; } -StatusOr CompleteReadFile(const std::string &fname) { +StatusOr ReadEntireFile(const std::string &fname) { std::ifstream file(fname); if (!file.good()) { return Status::IOError("") << "Fails to open file " << fname; diff --git a/src/ray/util/filesystem.h b/src/ray/util/filesystem.h index d6f9d575d0a8..bc1b8a0430b0 100644 --- a/src/ray/util/filesystem.h +++ b/src/ray/util/filesystem.h @@ -46,6 +46,6 @@ static inline bool IsDirSep(char ch) { // Read the whole content for the given [fname], and return as string. // Return IO error status if open and read operation fail. -StatusOr CompleteReadFile(const std::string &fname); +StatusOr ReadEntireFile(const std::string &fname); } // namespace ray diff --git a/src/ray/util/tests/pipe_logger_test.cc b/src/ray/util/tests/pipe_logger_test.cc index 5f20f62eb4e8..83a89b815ca0 100644 --- a/src/ray/util/tests/pipe_logger_test.cc +++ b/src/ray/util/tests/pipe_logger_test.cc @@ -62,7 +62,7 @@ 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); @@ -116,11 +116,11 @@ TEST_P(PipeLoggerTest, PipeWrite) { stream_redirection_handle.Close(); // Check log content after completion. - const auto actual_content1 = CompleteReadFile(log_file_path1); + const auto actual_content1 = ReadEntireFile(log_file_path1); RAY_ASSERT_OK(actual_content1); EXPECT_EQ(*actual_content1, kLogLine2); - const auto actual_content2 = CompleteReadFile(log_file_path2); + const auto actual_content2 = ReadEntireFile(log_file_path2); RAY_ASSERT_OK(actual_content1); EXPECT_EQ(*actual_content2, kLogLine1); } @@ -152,7 +152,7 @@ TEST(PipeLoggerTestWithTee, RedirectionWithTee) { EXPECT_EQ(stdout_content, absl::StrFormat("%s%s", kLogLine1, kLogLine2)); // 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); EXPECT_EQ(*actual_content, absl::StrFormat("%s%s", kLogLine1, kLogLine2)); } @@ -189,11 +189,11 @@ TEST(PipeLoggerTestWithTee, RotatedRedirectionWithTee) { EXPECT_EQ(stderr_content, absl::StrFormat("%s%s", kLogLine1, kLogLine2)); // Check log content after completion. - const auto actual_content1 = CompleteReadFile(log_file_path1); + const auto actual_content1 = ReadEntireFile(log_file_path1); RAY_ASSERT_OK(actual_content1); EXPECT_EQ(*actual_content1, kLogLine2); - const auto actual_content2 = CompleteReadFile(log_file_path2); + const auto actual_content2 = ReadEntireFile(log_file_path2); RAY_ASSERT_OK(actual_content2); EXPECT_EQ(*actual_content2, kLogLine1); } diff --git a/src/ray/util/tests/stream_redirection_utils_test.cc b/src/ray/util/tests/stream_redirection_utils_test.cc index cdeb418be11d..77ba330e1636 100644 --- a/src/ray/util/tests/stream_redirection_utils_test.cc +++ b/src/ray/util/tests/stream_redirection_utils_test.cc @@ -57,12 +57,12 @@ TEST(LoggingUtilTest, RedirectStderr) { // Check log content after completion. const std::string log_file_path1 = test_file_path; - const auto actual_content1 = CompleteReadFile(log_file_path1); + 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); - const auto actual_content2 = CompleteReadFile(log_file_path2); + const auto actual_content2 = ReadEntireFile(log_file_path2); RAY_ASSERT_OK(actual_content2); EXPECT_EQ(*actual_content2, kLogLine1);