Build with Visual C++ (#9190)

Co-authored-by: Mehrdad <noreply@github.com>
Co-authored-by: Simon Mo <xmo@berkeley.edu>

.bazelrc

@@ -1,7 +1,7 @@
 # Must be first. Enables build:windows, build:linux, build:macos, build:freebsd, build:openbsd
 build --enable_platform_specific_config
 ###############################################################################
-# On       Windows, provide: USE_CLANG_CL=1, CC=clang, BAZEL_LLVM, BAZEL_SH
+# On       Windows, provide: CC=clang, BAZEL_LLVM, BAZEL_SH
 # On all platforms, provide: PYTHON3_BIN_PATH=python
 ###############################################################################
 build --action_env=PATH
@@ -11,27 +11,29 @@ build --experimental_ui_deduplicate
 # This workaround is needed to prevent Bazel from compiling the same file twice (once PIC and once not).
 build:linux --force_pic
 build:macos --force_pic
+build:clang-cl --compiler=clang-cl
+build:msvc --compiler=msvc-cl
 # TODO(mehrdadn): Revert the "-\\.(asm|S)$" exclusion when this Bazel bug
 #                 for compiling assembly files is fixed on Windows:
 #                 https://github.com/bazelbuild/bazel/issues/8924
 # Warnings should be errors
-build --per_file_copt="-\\.(asm|S)$,-.*/arrow/util/logging\\.cc@-Werror"
+build:linux    --per_file_copt="-\\.(asm|S)$,-.*/arrow/util/logging\\.cc@-Werror"
+build:macos    --per_file_copt="-\\.(asm|S)$,-.*/arrow/util/logging\\.cc@-Werror"
+build:clang-cl --per_file_copt="-\\.(asm|S)$,-.*/arrow/util/logging\\.cc@-Werror"
+build:msvc     --per_file_copt="-\\.(asm|S)$,-.*/arrow/util/logging\\.cc@-WX"
 # Ignore warnings for protobuf generated files and external projects.
-build --per_file_copt="-\\.(asm|S)$,\\.pb\\.cc$@-w"
+build --per_file_copt="\\.pb\\.cc$@-w"
 build --per_file_copt="-\\.(asm|S)$,external/.*@-w"
-# Ignore this warning since it's impractical to fix in the relevant headers
-build --per_file_copt="-\\.(asm|S)$,.*/ray/raylet/reconstruction_policy_test\\.cc@-Wno-inconsistent-missing-override"
-# Ignore this warning since CPython and Cython have issue removing deprecated tp_print on MacOS
-build --per_file_copt="-\\.(asm|S)$,.*/_raylet\\.cpp@-Wno-deprecated-declarations"
-build --per_file_copt="-\\.(asm|S)$,.*/_streaming\\.cpp@-Wno-deprecated-declarations"
+# Enable some C warnings that really should be errors (e.g. implicit void* <-> int conversion)
 # Ignore minor warnings for host tools, which we generally can't control
-build --host_copt="-Wno-builtin-macro-redefined"
-build --host_copt="-Wno-inconsistent-missing-override"
-build --host_copt="-Wno-microsoft-unqualified-friend"
+build:clang-cl --host_copt="-Wno-inconsistent-missing-override"
+build:clang-cl --host_copt="-Wno-microsoft-unqualified-friend"
 # This workaround is needed due to https://github.com/bazelbuild/bazel/issues/4341
 build --per_file_copt="-\\.(asm|S)$,external/com_github_grpc_grpc/.*@-DGRPC_BAZEL_BUILD"
 # Don't generate warnings about kernel features we don't need https://github.com/ray-project/ray/issues/6832
 build:linux --per_file_copt="-\\.(asm|S)$,external/com_github_grpc_grpc/.*@-DGPR_MANYLINUX1"
+# Ignore wchar_t -> char conversion warning on MSVC
+build:msvc --per_file_copt="external/boost/libs/regex/src/wc_regex_traits\\.cpp@-wd4244"
 build --http_timeout_scaling=5.0
 build --verbose_failures
 build:iwyu --experimental_action_listener=//:iwyu_cpp
@@ -45,8 +47,8 @@ build:windows --incompatible_strict_action_env
 # For colored output (seems necessary on Windows)
 build:windows --color=yes
 # For compiler colored output (seems necessary on Windows)
-build:windows --per_file_copt="-\\.(asm|S)$@-fansi-escape-codes"
-build:windows --per_file_copt="-\\.(asm|S)$@-fcolor-diagnostics"
+build:clang-cl --per_file_copt="-\\.(asm|S)$@-fansi-escape-codes"
+build:clang-cl --per_file_copt="-\\.(asm|S)$@-fcolor-diagnostics"
 
 # Debug build flags. Uncomment in '-c dbg' builds to enable checks in the C++ standard library:
 #build:linux --cxxopt="-D_GLIBCXX_DEBUG=1"

.github/workflows/main.yml

@@ -20,9 +20,11 @@ jobs:
           - name: ubuntu
             os: ubuntu-16.04
             python-version: 3.6
-          - name: windows
+          - name: windows-msvc
             os: windows-2019
             python-version: 3.8
+            # Can be 'msvc' or 'clang-cl'
+            config: msvc
           - name: macos
             os: macos-10.15
             python-version: 3.6
@@ -36,6 +38,7 @@ jobs:
             os: ubuntu-latest
             jenkins-test: RUN_SGD_TESTS
     env:
+      BAZEL_CONFIG: ${{ matrix.config }}
       PYTHON: ${{ matrix.python-version }}
       TRAVIS_COMMIT: ${{ github.sha }}
       #TRAVIS_PULL_REQUEST is defined in the scripts to account for "false"

BUILD.bazel

@@ -9,6 +9,16 @@ load("@rules_proto_grpc//python:defs.bzl", "python_grpc_compile")
 load("@com_github_google_flatbuffers//:build_defs.bzl", "flatbuffer_cc_library")
 load("//bazel:ray.bzl", "COPTS", "PYX_COPTS", "PYX_SRCS")
 
+config_setting(
+    name = "windows_msvc-cl",
+    values = {"compiler": "msvc-cl"},
+)
+
+config_setting(
+    name = "windows_clang-cl",
+    values = {"compiler": "clang-cl"},
+)
+
 # === Begin of protobuf definitions ===
 
 proto_library(
@@ -813,7 +823,14 @@ cc_test(
 cc_test(
     name = "reconstruction_policy_test",
     srcs = ["src/ray/raylet/reconstruction_policy_test.cc"],
-    copts = COPTS,
+    copts = COPTS + select({
+        "//:windows_msvc-cl": [
+        ],
+        "//conditions:default": [
+            # Ignore this warning since it's impractical to fix in the relevant headers
+            "-Wno-inconsistent-missing-override",
+        ],
+    }),
     deps = [
         ":node_manager_fbs",
         ":object_manager",

bazel/ray.bzl

@@ -2,18 +2,30 @@ load("@com_github_google_flatbuffers//:build_defs.bzl", "flatbuffer_library_publ
 load("@com_github_checkstyle_java//checkstyle:checkstyle.bzl", "checkstyle_test")
 load("@bazel_common//tools/maven:pom_file.bzl", "pom_file")
 
+WINDOWS_COPTS = [
+    # TODO(mehrdadn): (How to) support dynamic linking?
+    "-DRAY_STATIC",
+]
+
 COPTS = ["-DRAY_USE_GLOG"] + select({
-    "@bazel_tools//src/conditions:windows": [
+    "//:windows_msvc-cl": [
+    ] + WINDOWS_COPTS,
+    "//:windows_clang-cl": [
         "-Wno-builtin-macro-redefined",  # To get rid of warnings caused by deterministic build macros (e.g. #define __DATE__ "redacted")
         "-Wno-microsoft-unqualified-friend",  # This shouldn't normally be enabled, but otherwise we get: google/protobuf/map_field.h: warning: unqualified friend declaration referring to type outside of the nearest enclosing namespace is a Microsoft extension; add a nested name specifier (for: friend class DynamicMessage)
-        # TODO(mehrdadn): (How to) support dynamic linking?
-        "-DRAY_STATIC",
-    ],
+    ] + WINDOWS_COPTS,
     "//conditions:default": [
     ],
 })
 
-PYX_COPTS = [] + select({
+PYX_COPTS = select({
+    "//:windows_msvc-cl": [
+    ],
+    "//conditions:default": [
+        # Ignore this warning since CPython and Cython have issue removing deprecated tp_print on MacOS
+        "-Wno-deprecated-declarations",
+    ],
+}) + select({
     "@bazel_tools//src/conditions:windows": [
         "/FI" + "src/shims/windows/python-nondebug.h",
     ],

bazel/ray_deps_setup.bzl

@@ -93,6 +93,7 @@ def ray_deps_setup():
         sha256 = "9a91274dfd131111227b39ffa3cf7b446fbbd7ee2e5a94c8e7d8ad334b4ff255",
         patches = [
             "//thirdparty/patches:hiredis-connect-rename.patch",
+            "//thirdparty/patches:hiredis-windows-msvc.patch",
             "//thirdparty/patches:hiredis-windows-sigpipe.patch",
             "//thirdparty/patches:hiredis-windows-sockets.patch",
             "//thirdparty/patches:hiredis-windows-strerror.patch",

ci/travis/ci.sh

@@ -447,9 +447,10 @@ init() {
 }
 
 build() {
+  # NOTE: Do not add build flags here. Use .bazelrc and --config instead.
+  bazel build -k "//:*"  # Full build first, since pip install will build only a subset of targets
+
   if ! need_wheels; then
-    # NOTE: Do not add build flags here. Use .bazelrc and --config instead.
-    bazel build -k "//:*"  # Full build first, since pip install will build only a subset of targets
     install_ray
     if [ "${LINT-}" = 1 ]; then
       # Try generating Sphinx documentation. To do this, we need to install Ray first.

ci/travis/install-bazel.sh

@@ -46,9 +46,6 @@ if [ "${OSTYPE}" = "msys" ]; then
   target="${MINGW_DIR-/usr}/bin/bazel.exe"
   mkdir -p "${target%/*}"
   curl -f -s -L -R -o "${target}" "https://github.com/bazelbuild/bazel/releases/download/${version}/bazel-${version}-${platform}-${achitecture}.exe"
-  tee /etc/profile.d/bazel.sh > /dev/null <<EOF; . /etc/profile.d/bazel.sh
-export USE_CLANG_CL=1  # Clang front-end for Visual C++
-EOF
 else
   target="./install.sh"
   curl -f -s -L -R -o "${target}" "https://github.com/bazelbuild/bazel/releases/download/${version}/bazel-${version}-installer-${platform}-${achitecture}.sh"

src/ray/core_worker/test/direct_task_transport_test.cc

@@ -169,7 +169,7 @@ class MockRayletClient : public WorkerLeaseInterface {
 };
 
 TEST(TestMemoryStore, TestPromoteToPlasma) {
-  bool num_plasma_puts = 0;
+  size_t num_plasma_puts = 0;
   auto mem = std::make_shared<CoreWorkerMemoryStore>(
       [&](const RayObject &obj, const ObjectID &obj_id) { num_plasma_puts += 1; });
   ObjectID obj1 = ObjectID::FromRandom();

src/ray/gcs/test/redis_gcs_client_test.cc

@@ -306,7 +306,7 @@ class TaskTableTestHelper {
                                        std::shared_ptr<gcs::RedisGcsClient> client) {
     // Add a table entry.
     const auto task_id = RandomTaskId();
-    const int num_modifications = 3;
+    uint64_t num_modifications = 3;
     const auto data = CreateTaskTableData(task_id, 0);
     RAY_CHECK_OK(client->raylet_task_table().Add(job_id, task_id, data, nullptr));
 
@@ -318,8 +318,9 @@ class TaskTableTestHelper {
 
     // The callback for a notification from the table. This should only be
     // received for keys that we requested notifications for.
-    auto notification_callback = [task_id](gcs::RedisGcsClient *client, const TaskID &id,
-                                           const TaskTableData &data) {
+    auto notification_callback = [task_id, num_modifications](gcs::RedisGcsClient *client,
+                                                              const TaskID &id,
+                                                              const TaskTableData &data) {
       ASSERT_EQ(id, task_id);
       // Check that we only get notifications for the first and last writes,
       // since notifications are canceled in between.
@@ -337,7 +338,8 @@ class TaskTableTestHelper {
 
     // The callback for a notification from the table. This should only be
     // received for keys that we requested notifications for.
-    auto subscribe_callback = [job_id, task_id](gcs::RedisGcsClient *client) {
+    auto subscribe_callback = [job_id, task_id,
+                               num_modifications](gcs::RedisGcsClient *client) {
       // Request notifications, then cancel immediately. We should receive a
       // notification for the current value at the key.
       RAY_CHECK_OK(client->raylet_task_table().RequestNotifications(
@@ -1330,7 +1332,7 @@ class HashTableTestHelper {
  public:
   static void TestHashTable(const JobID &job_id,
                             std::shared_ptr<gcs::RedisGcsClient> client) {
-    const int expected_count = 14;
+    uint64_t expected_count = 14;
     ClientID client_id = ClientID::FromRandom();
     // Prepare the first resource map: data_map1.
     DynamicResourceTable::DataMap data_map1;
@@ -1368,7 +1370,7 @@ class HashTableTestHelper {
       test->IncrementNumCallbacks();
     };
     auto notification_callback =
-        [data_map1, data_map2, compare_test](
+        [data_map1, data_map2, compare_test, expected_count](
             RedisGcsClient *client, const ClientID &id,
             const std::vector<ResourceChangeNotification> &result) {
           RAY_CHECK(result.size() == 1);
@@ -1464,8 +1466,9 @@ class HashTableTestHelper {
     // Step 4: Removing all elements will remove the home Hash table from GCS.
     RAY_CHECK_OK(client->resource_table().RemoveEntries(
         job_id, client_id, {"GPU", "CPU", "CUSTOM", "None-Existent"}, nullptr));
-    auto lookup_callback5 = [](RedisGcsClient *client, const ClientID &id,
-                               const DynamicResourceTable::DataMap &callback_data) {
+    auto lookup_callback5 = [expected_count](
+                                RedisGcsClient *client, const ClientID &id,
+                                const DynamicResourceTable::DataMap &callback_data) {
       ASSERT_EQ(callback_data.size(), 0);
       test->IncrementNumCallbacks();
       // It is not sure which of notification or lookup callback will come first.

src/ray/stats/metric_exporter.cc

@@ -52,18 +52,18 @@ void MetricExporter::ExportToPoints(
     }
   }
   hist_mean /= view_data.size();
-  MetricPoint mean_point{.metric_name = metric_name + ".mean",
-                         .timestamp = current_sys_time_ms(),
-                         .value = hist_mean,
-                         .tags = tags};
-  MetricPoint max_point{.metric_name = metric_name + ".max",
-                        .timestamp = current_sys_time_ms(),
-                        .value = hist_max,
-                        .tags = tags};
-  MetricPoint min_point{.metric_name = metric_name + ".min",
-                        .timestamp = current_sys_time_ms(),
-                        .value = hist_min,
-                        .tags = tags};
+  MetricPoint mean_point = {metric_name + ".mean",
+                            current_sys_time_ms(),
+                            hist_mean,
+                            tags};
+  MetricPoint max_point = {metric_name + ".max",
+                           current_sys_time_ms(),
+                           hist_max,
+                           tags};
+  MetricPoint min_point = {metric_name + ".min",
+                           current_sys_time_ms(),
+                           hist_min,
+                           tags};
   points.push_back(std::move(mean_point));
   points.push_back(std::move(max_point));
   points.push_back(std::move(min_point));

src/ray/stats/metric_exporter.h

@@ -63,10 +63,10 @@ class MetricExporter final : public opencensus::stats::StatsExporter::Handler {
         tags[keys[i]] = row.first[i];
       }
       // Current timestamp is used for point not view data time.
-      MetricPoint point{.metric_name = metric_name,
-                        .timestamp = current_sys_time_ms(),
-                        .value = static_cast<double>(row.second),
-                        .tags = tags};
+      MetricPoint point{metric_name,
+                        current_sys_time_ms(),
+                        static_cast<double>(row.second),
+                        tags};
       RAY_LOG(DEBUG) << "Metric name " << metric_name << ", value " << point.value;
       points.push_back(std::move(point));
       if (points.size() >= report_batch_size_) {

src/ray/util/logging.cc

@@ -26,7 +26,14 @@
 
 #ifdef RAY_USE_GLOG
 #include <sys/stat.h>
+#ifdef _MSC_VER
+#pragma warning(push)
+#pragma warning(disable : 4722)  // Ignore non-returning destructor warning in GLOG
+#endif
 #include "glog/logging.h"
+#ifdef _MSC_VER
+#pragma warning(pop)
+#endif
 #endif
 
 #include "ray/util/filesystem.h"

src/ray/util/util_test.cc

@@ -48,7 +48,7 @@ TEST(UtilTest, ParseCommandLineTest) {
     ASSERT_EQ(ParseCommandLine(R"(\" )", syn), ArgList({R"(")"}));
     ASSERT_EQ(ParseCommandLine(R"(" a")", syn), ArgList({R"( a)"}));
     ASSERT_EQ(ParseCommandLine(R"("\\")", syn), ArgList({R"(\)"}));
-    ASSERT_EQ(ParseCommandLine(R"("\"")", syn), ArgList({R"(")"}));
+    ASSERT_EQ(ParseCommandLine(/*R"("\"")"*/ "\"\\\"\"", syn), ArgList({R"(")"}));
     ASSERT_EQ(ParseCommandLine(R"(a" b c"d )", syn), ArgList({R"(a b cd)"}));
     ASSERT_EQ(ParseCommandLine(R"(\"a b)", syn), ArgList({R"("a)", R"(b)"}));
     ASSERT_EQ(ParseCommandLine(R"(| ! ^ # [)", syn), ArgList({"|", "!", "^", "#", "["}));

src/shims/windows/unistd.cc

@@ -35,7 +35,7 @@ pid_t getppid() {
   ULONG cb = sizeof(info);
   NTSTATUS status = NtQueryInformationProcess(GetCurrentProcess(), 0, &info, cb, &cb);
   if ((status >= 0 || status == STATUS_BUFFER_OVERFLOW) && cb >= sizeof(info)) {
-    ppid = reinterpret_cast<DWORD>(info.Reserved3);
+    ppid = static_cast<DWORD>(reinterpret_cast<uintptr_t>(info.Reserved3));
   }
   pid_t result = 0;
   if (ppid > 0) {

streaming/src/data_writer.cc

@@ -269,7 +269,7 @@ bool DataWriter::CollectFromRingBuffer(ProducerChannelInfo &channel_info,
   auto &q_id = channel_info.channel_id;
 
   std::list<StreamingMessagePtr> message_list;
-  uint64_t bundle_buffer_size = 0;
+  uint32_t bundle_buffer_size = 0;
   const uint32_t max_queue_item_size = channel_info.queue_size;
   while (message_list.size() < runtime_context_->GetConfig().GetRingBufferCapacity() &&
          !buffer_ptr->IsEmpty()) {

thirdparty/patches/hiredis-windows-msvc.patch

@@ -0,0 +1,24 @@
+diff --git sds.h sds.h
+--- sds.h
++++ sds.h
+@@ -43,1 +43,6 @@
+ 
++#if defined(_MSC_VER) && !defined(__clang__)
++#pragma pack(push, 1)
++#pragma push_macro("__attribute__")
++#define __attribute__(A)
++#endif
+@@ -74,1 +78,5 @@
++#if defined(_MSC_VER) && !defined(__clang__)
++#pragma pop_macro("__attribute__")
++#pragma pack(pop)
++#endif
+ 
+diff --git net.c net.c
+--- net.c
++++ net.c
+@@ -52,1 +52,4 @@
+ #include <stdlib.h>
++#if defined(_MSC_VER) && !defined(__clang__) && !defined(strdup)
++#define strdup _strdup
++#endif