From 29acf272b7664cc4479fa7172305a4be7fe4551f Mon Sep 17 00:00:00 2001 From: mehrdadn Date: Thu, 2 Jul 2020 09:34:24 -0700 Subject: [PATCH] Build with Visual C++ (#9190) Co-authored-by: Mehrdad Co-authored-by: Simon Mo --- .bazelrc | 28 ++++++++++--------- .github/workflows/main.yml | 5 +++- BUILD.bazel | 19 ++++++++++++- bazel/ray.bzl | 22 +++++++++++---- bazel/ray_deps_setup.bzl | 1 + ci/travis/ci.sh | 5 ++-- ci/travis/install-bazel.sh | 3 -- .../test/direct_task_transport_test.cc | 2 +- src/ray/gcs/test/redis_gcs_client_test.cc | 19 +++++++------ src/ray/stats/metric_exporter.cc | 24 ++++++++-------- src/ray/stats/metric_exporter.h | 8 +++--- src/ray/util/logging.cc | 7 +++++ src/ray/util/util_test.cc | 2 +- src/shims/windows/unistd.cc | 2 +- streaming/src/data_writer.cc | 2 +- thirdparty/patches/hiredis-windows-msvc.patch | 24 ++++++++++++++++ 16 files changed, 120 insertions(+), 53 deletions(-) create mode 100644 thirdparty/patches/hiredis-windows-msvc.patch diff --git a/.bazelrc b/.bazelrc index 9c317ac77..39a15f9b6 100644 --- a/.bazelrc +++ b/.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" diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 7a059d1c2..5ca063f21 100644 --- a/.github/workflows/main.yml +++ b/.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" diff --git a/BUILD.bazel b/BUILD.bazel index a7e139279..3918c2af5 100644 --- a/BUILD.bazel +++ b/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", diff --git a/bazel/ray.bzl b/bazel/ray.bzl index eb67c348d..613a87b81 100644 --- a/bazel/ray.bzl +++ b/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", ], diff --git a/bazel/ray_deps_setup.bzl b/bazel/ray_deps_setup.bzl index 9a04a0f3e..2f2ae25b3 100644 --- a/bazel/ray_deps_setup.bzl +++ b/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", diff --git a/ci/travis/ci.sh b/ci/travis/ci.sh index 8a0b5ff0f..81397746a 100755 --- a/ci/travis/ci.sh +++ b/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. diff --git a/ci/travis/install-bazel.sh b/ci/travis/install-bazel.sh index ccd3dab04..bcdec5bf3 100755 --- a/ci/travis/install-bazel.sh +++ b/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 <( [&](const RayObject &obj, const ObjectID &obj_id) { num_plasma_puts += 1; }); ObjectID obj1 = ObjectID::FromRandom(); diff --git a/src/ray/gcs/test/redis_gcs_client_test.cc b/src/ray/gcs/test/redis_gcs_client_test.cc index 4b62a33a7..9191f622a 100644 --- a/src/ray/gcs/test/redis_gcs_client_test.cc +++ b/src/ray/gcs/test/redis_gcs_client_test.cc @@ -306,7 +306,7 @@ class TaskTableTestHelper { std::shared_ptr 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 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 &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. diff --git a/src/ray/stats/metric_exporter.cc b/src/ray/stats/metric_exporter.cc index 9f42edf1e..4a714e7d7 100644 --- a/src/ray/stats/metric_exporter.cc +++ b/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)); diff --git a/src/ray/stats/metric_exporter.h b/src/ray/stats/metric_exporter.h index 25b462139..c3fbbeacd 100644 --- a/src/ray/stats/metric_exporter.h +++ b/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(row.second), - .tags = tags}; + MetricPoint point{metric_name, + current_sys_time_ms(), + static_cast(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_) { diff --git a/src/ray/util/logging.cc b/src/ray/util/logging.cc index 7796a4079..3c82a7380 100644 --- a/src/ray/util/logging.cc +++ b/src/ray/util/logging.cc @@ -26,7 +26,14 @@ #ifdef RAY_USE_GLOG #include +#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" diff --git a/src/ray/util/util_test.cc b/src/ray/util/util_test.cc index 631cb8a8c..19bcb5776 100644 --- a/src/ray/util/util_test.cc +++ b/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({"|", "!", "^", "#", "["})); diff --git a/src/shims/windows/unistd.cc b/src/shims/windows/unistd.cc index a862384fb..2bcdcdde5 100644 --- a/src/shims/windows/unistd.cc +++ b/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(info.Reserved3); + ppid = static_cast(reinterpret_cast(info.Reserved3)); } pid_t result = 0; if (ppid > 0) { diff --git a/streaming/src/data_writer.cc b/streaming/src/data_writer.cc index af840c19a..6992b23e7 100644 --- a/streaming/src/data_writer.cc +++ b/streaming/src/data_writer.cc @@ -269,7 +269,7 @@ bool DataWriter::CollectFromRingBuffer(ProducerChannelInfo &channel_info, auto &q_id = channel_info.channel_id; std::list 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()) { diff --git a/thirdparty/patches/hiredis-windows-msvc.patch b/thirdparty/patches/hiredis-windows-msvc.patch new file mode 100644 index 000000000..33e0ea064 --- /dev/null +++ b/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 ++#if defined(_MSC_VER) && !defined(__clang__) && !defined(strdup) ++#define strdup _strdup ++#endif