[core] Fix windows build for pipe logger (#49780)

This PR does two things: - Finish a TODO for using compat macro to represent windows `HANDLE` type and unix file descriptor - Fix compilation on windows --------- Signed-off-by: dentiny <[email protected]>
ray-project · Jan 14, 2025 · 34c3f1e · 34c3f1e
1 parent 74bc097
commit 34c3f1e
Show file tree

Hide file tree

Showing 16 changed files with 72 additions and 43 deletions.
diff --git a/BUILD.bazel b/BUILD.bazel
@@ -361,7 +361,6 @@ ray_cc_library(
         "src/ray/object_manager/common.h",
         "src/ray/object_manager/plasma/client.h",
         "src/ray/object_manager/plasma/common.h",
-        "src/ray/object_manager/plasma/compat.h",
         "src/ray/object_manager/plasma/connection.h",
         "src/ray/object_manager/plasma/malloc.h",
         "src/ray/object_manager/plasma/plasma.h",
@@ -386,6 +385,7 @@ ray_cc_library(
         ":ray_common",
         "//src/ray/protobuf:common_cc_proto",
         "//src/ray/util",
+        "//src/ray/util:compat",
         "@msgpack",
     ],
 )

diff --git a/src/ray/common/BUILD b/src/ray/common/BUILD
@@ -1,5 +1,10 @@
 load("//bazel:ray.bzl", "ray_cc_library", "ray_cc_test")
 
+ray_cc_library(
+    name = "compat",
+    hdrs = ["compat.h"],
+)
+
 ray_cc_library(
     name = "constants",
     hdrs = ["constants.h"],

diff --git a/src/ray/object_manager/plasma/allocator.h b/src/ray/object_manager/plasma/allocator.h
@@ -18,7 +18,7 @@
 
 #include "absl/types/optional.h"
 #include "ray/object_manager/plasma/common.h"
-#include "ray/object_manager/plasma/compat.h"
+#include "ray/util/compat.h"
 
 namespace plasma {
 

diff --git a/src/ray/object_manager/plasma/common.h b/src/ray/object_manager/plasma/common.h
@@ -26,9 +26,9 @@
 
 #include "ray/common/id.h"
 #include "ray/object_manager/common.h"
-#include "ray/object_manager/plasma/compat.h"
 #include "ray/object_manager/plasma/plasma.h"
 #include "ray/object_manager/plasma/plasma_generated.h"
+#include "ray/util/compat.h"
 #include "ray/util/macros.h"
 
 namespace plasma {

diff --git a/src/ray/object_manager/plasma/connection.h b/src/ray/object_manager/plasma/connection.h
@@ -4,7 +4,7 @@
 #include "ray/common/client_connection.h"
 #include "ray/common/id.h"
 #include "ray/common/status.h"
-#include "ray/object_manager/plasma/compat.h"
+#include "ray/util/compat.h"
 
 namespace plasma {
 

diff --git a/src/ray/object_manager/plasma/malloc.h b/src/ray/object_manager/plasma/malloc.h
@@ -21,7 +21,7 @@
 #include <stddef.h>
 
 #include "absl/container/flat_hash_map.h"
-#include "ray/object_manager/plasma/compat.h"
+#include "ray/util/compat.h"
 
 namespace plasma {
 

diff --git a/src/ray/object_manager/plasma/plasma.h b/src/ray/object_manager/plasma/plasma.h
@@ -26,7 +26,7 @@
 #include <unordered_set>
 #include <vector>
 
-#include "ray/object_manager/plasma/compat.h"
+#include "ray/util/compat.h"
 
 namespace plasma {
 

diff --git a/src/ray/object_manager/plasma/shared_memory.h b/src/ray/object_manager/plasma/shared_memory.h
@@ -4,7 +4,7 @@
 #include <cstdint>
 #include <utility>
 
-#include "ray/object_manager/plasma/compat.h"
+#include "ray/util/compat.h"
 #include "ray/util/macros.h"
 
 namespace plasma {

diff --git a/src/ray/util/BUILD b/src/ray/util/BUILD
@@ -66,13 +66,19 @@ ray_cc_library(
     ],
 )
 
+ray_cc_library(
+    name = "compat",
+    hdrs = ["compat.h"],
+)
+
 ray_cc_library(
     name = "pipe_logger",
     hdrs = ["pipe_logger.h"],
     srcs = ["pipe_logger.cc"],
     deps = [
-        "@com_github_spdlog//:spdlog",
+        ":compat",
         ":util",
+        "@com_github_spdlog//:spdlog",
         "@com_google_absl//absl/strings",
     ],
 )
diff --git a/src/ray/object_manager/plasma/compat.h → src/ray/util/compat.h b/src/ray/object_manager/plasma/compat.h → src/ray/util/compat.h
@@ -1,3 +1,17 @@
+// Copyright 2017 The Ray Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//  http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 // Licensed to the Apache Software Foundation (ASF) under one
 // or more contributor license agreements.  See the NOTICE file
 // distributed with this work for additional information

diff --git a/src/ray/util/function_traits.h b/src/ray/util/function_traits.h
@@ -49,24 +49,26 @@ struct function_traits;
 
 // Specialization for function pointers
 template <typename R, typename... Args>
-struct function_traits<R (*)(Args...)> : internal::function_traits_helper<R(Args...)> {};
+struct function_traits<R (*)(Args...)>
+    : ::ray::internal::function_traits_helper<R(Args...)> {};
 
 template <typename R, typename... Args>
-struct function_traits<R(Args...)> : internal::function_traits_helper<R(Args...)> {};
+struct function_traits<R(Args...)> : ::ray::internal::function_traits_helper<R(Args...)> {
+};
 
 // Specialization for member function pointers
 template <typename C, typename R, typename... Args>
-struct function_traits<R (C::*)(Args...)> : internal::function_traits_helper<R(Args...)> {
-};
+struct function_traits<R (C::*)(Args...)>
+    : ::ray::internal::function_traits_helper<R(Args...)> {};
 
 // Specialization for const member function pointers
 template <typename C, typename R, typename... Args>
 struct function_traits<R (C::*)(Args...) const>
-    : internal::function_traits_helper<R(Args...)> {};
+    : ::ray::internal::function_traits_helper<R(Args...)> {};
 
 template <typename C, typename R, typename... Args>
 struct function_traits<R (C::*)(Args...) &&>
-    : internal::function_traits_helper<R(Args...)> {};
+    : ::ray::internal::function_traits_helper<R(Args...)> {};
 
 // Specialization for callable objects (e.g., std::function, lambdas and functors)
 template <typename T>
@@ -83,5 +85,5 @@ struct function_traits<T &&> : function_traits<T> {};
 // Because you can't manipulate argument packs easily, we need to do rebind to instead
 // manipulate the result type.
 template <typename NewR, typename T>
-using rebind_result_t = typename internal::rebind_result<NewR, T>::type;
+using rebind_result_t = typename ::ray::internal::rebind_result<NewR, T>::type;
 }  // namespace ray
diff --git a/src/ray/util/pipe_logger.cc b/src/ray/util/pipe_logger.cc
@@ -82,11 +82,9 @@ size_t Read(HANDLE read_handle, char *data, size_t len) {
   return bytes_read;
 }
 void CompleteWriteEOFIndicator(HANDLE write_handle) {
-  size_t bytes_written = 0;
-  BOOL result = WriteFile(
+  DWORD bytes_written = 0;
+  WriteFile(
       write_handle, kEofIndicator.c_str(), kEofIndicator.size(), &bytes_written, nullptr);
-  RAY_CHECK_EQ(bytes_written, static_cast<ssize_t>(kEofIndicator.size()));
-  RAY_CHECK(result);
 }
 #endif
 
@@ -243,7 +241,7 @@ RotationFileHandle CreatePipeAndStreamOutput(const std::string &fname,
 
 #if defined(__APPLE__) || defined(__linux__)
   RotationFileHandle stream_token{write_fd, std::move(termination_caller)};
-#else  // __linux__
+#elif defined(_WIN32)
   RotationFileHandle stream_token{write_handle, std::move(termination_caller)};
 #endif
 

diff --git a/src/ray/util/pipe_logger.h b/src/ray/util/pipe_logger.h
@@ -20,6 +20,8 @@
 #include <limits>
 #include <string>
 
+#include "ray/util/compat.h"
+
 namespace ray {
 
 // Environmenr variable, which indicates the pipe size of read.
@@ -36,12 +38,11 @@ struct LogRotationOption {
   size_t rotation_max_file_count = 1;
 };
 
-// TODO(hjiang): Use `MEMFD_TYPE_NON_UNIQUE` after we split `plasma/compat.h` into a
-// separate target, otherwise we're introducing too many unnecessary dependencies.
 class RotationFileHandle {
  public:
-  RotationFileHandle(int write_fd, std::function<void()> termination_caller)
-      : write_fd_(write_fd), termination_caller_(std::move(termination_caller)) {}
+  RotationFileHandle(MEMFD_TYPE_NON_UNIQUE write_handle,
+                     std::function<void()> termination_caller)
+      : write_handle_(write_handle), termination_caller_(std::move(termination_caller)) {}
   RotationFileHandle(const RotationFileHandle &) = delete;
   RotationFileHandle &operator=(const RotationFileHandle &) = delete;
 
@@ -51,58 +52,57 @@ class RotationFileHandle {
   // copiable to avoid manual `dup`.
 #if defined(__APPLE__) || defined(__linux__)
   RotationFileHandle(RotationFileHandle &&rhs) {
-    write_fd_ = rhs.write_fd_;
-    rhs.write_fd_ = -1;
+    write_handle_ = rhs.write_handle_;
+    rhs.write_handle_ = -1;
     termination_caller_ = std::move(rhs.termination_caller_);
   }
   RotationFileHandle &operator=(RotationFileHandle &&rhs) {
     if (this == &rhs) {
       return *this;
     }
-    write_fd_ = rhs.write_fd_;
-    rhs.write_fd_ = -1;
+    write_handle_ = rhs.write_handle_;
+    rhs.write_handle_ = -1;
     termination_caller_ = std::move(rhs.termination_caller_);
     return *this;
   }
   ~RotationFileHandle() {
     // Only invoke termination functor when handler at a valid state.
-    if (write_fd_ != -1) {
+    if (write_handle_ != -1) {
       termination_caller_();
     }
   }
 
-  int GetWriteHandle() const { return write_fd_; }
+  int GetWriteHandle() const { return write_handle_; }
 
- private:
-  int write_fd_;
 #elif defined(_WIN32)
   RotationFileHandle(RotationFileHandle &&rhs) {
-    write_fd_ = rhs.write_fd_;
-    rhs.write_fd_ = nullptr;
+    write_handle_ = rhs.write_handle_;
+    rhs.write_handle_ = nullptr;
     termination_caller_ = std::move(rhs.termination_caller_);
   }
   RotationFileHandle &operator=(RotationFileHandle &&rhs) {
     if (this == &rhs) {
       return *this;
     }
-    write_fd_ = rhs.write_fd_;
-    rhs.write_fd_ = nullptr;
+    write_handle_ = rhs.write_handle_;
+    rhs.write_handle_ = nullptr;
     termination_caller_ = std::move(rhs.termination_caller_);
     return *this;
   }
   ~RotationFileHandle() {
     // Only invoke termination functor when handler at a valid state.
-    if (write_fd_ != nullptr) {
+    if (write_handle_ != nullptr) {
       termination_caller_();
     }
   }
 
   HANDLE GetWriteHandle() const { return write_handle_; }
 
- private:
-  HANDLE write_handle_;
 #endif
 
+ private:
+  MEMFD_TYPE_NON_UNIQUE write_handle_;
+
   // Termination hook, used to flush and call completion.
   std::function<void()> termination_caller_;
 };

diff --git a/src/ray/util/tests/BUILD b/src/ray/util/tests/BUILD
@@ -227,9 +227,9 @@ ray_cc_test(
     name = "pipe_logger_test",
     srcs = ["pipe_logger_test.cc"],
     deps = [
-        "//src/ray/util:pipe_logger",
         ":unix_test_utils",
         "//src/ray/util",
+        "//src/ray/util:pipe_logger",
         "@com_google_googletest//:gtest_main",
     ],
     size = "small",

diff --git a/src/ray/util/tests/unix_test_utils.cc b/src/ray/util/tests/unix_test_utils.cc
@@ -18,6 +18,8 @@
 
 #include "ray/util/util.h"
 
+#if defined(__APPLE__) || defined(__linux__)
+
 namespace ray {
 
 std::string CompleteReadFile(const std::string &fname) {
@@ -40,3 +42,5 @@ std::string CompleteReadFile(const std::string &fname) {
 }
 
 }  // namespace ray
+
+#endif
diff --git a/src/ray/util/tests/unix_test_utils.h b/src/ray/util/tests/unix_test_utils.h
@@ -16,9 +16,7 @@
 
 #pragma once
 
-#if !defined(__APPLE__) && !defined(__linux__)
-#error "This header file can only be used in unix"
-#endif
+#if defined(__APPLE__) || defined(__linux__)
 
 #include <string>
 
@@ -29,3 +27,5 @@ namespace ray {
 std::string CompleteReadFile(const std::string &fname);
 
 }  // namespace ray
+
+#endif