Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct windows build flags for clang-cl/msvc-cl under opt/fastbuild/dbg #13133

Merged
merged 10 commits into from
Oct 11, 2020
29 changes: 15 additions & 14 deletions .bazelrc
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ build --host_javabase=@bazel_tools//tools/jdk:remote_jdk11
build --javabase=@bazel_tools//tools/jdk:remote_jdk11
build --enable_platform_specific_config

# Enable position independent code, this option is not supported on Windows and default on on macOS.
# Enable position independent code (this is the default on macOS and Windows)
# (Workaround for https://github.com/bazelbuild/rules_foreign_cc/issues/421)
build:linux --copt=-fPIC
build:linux --cxxopt=-std=c++17
build:linux --conlyopt=-fexceptions
Expand Down Expand Up @@ -284,27 +285,27 @@ build:windows --define signal_trace=disabled
build:windows --define hot_restart=disabled
build:windows --define tcmalloc=disabled
build:windows --define manual_stamp=manual_stamp
build:windows --cxxopt="/std:c++17"

# Should not be required after upstream fix to bazel,
# and already a no-op to linux/macos builds
# see issue https://github.com/bazelbuild/rules_foreign_cc/issues/301
# TODO(wrowe,sunjayBhatia): Resolve bugs upstream in curl and rules_foreign_cc
# See issue https://github.com/bazelbuild/rules_foreign_cc/issues/301
build:windows --copt="-DCARES_STATICLIB"
build:windows --copt="-DNGHTTP2_STATICLIB"
build:windows --copt="-DCURL_STATICLIB"
build:windows --cxxopt="/std:c++17"

# Required to work around build defects on Windows MSVC cl
# Unguarded gcc pragmas in quiche are not recognized by MSVC
build:msvc-cl --copt="/wd4068"
wrowe marked this conversation as resolved.
Show resolved Hide resolved
# Allows 'nodiscard' function return values to be discarded
build:msvc-cl --copt="/wd4834"
# Allows inline functions to be undefined
build:msvc-cl --copt="/wd4506"
build:msvc-cl --copt="-D_SILENCE_EXPERIMENTAL_FILESYSTEM_DEPRECATION_WARNING"
# Override any clang preference if building msvc-cl
# Drop the determinism feature (-DDATE etc are a no-op in msvc-cl)
build:msvc-cl --action_env=USE_CLANG_CL=""
build:msvc-cl --define clang_cl=0
build:msvc-cl --features=-determinism
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is needed? it wasn't here before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three things going on.

Config msvc-cl is no longer strictly needed, those flags moved to bazel/envoy_*.bzl

When specifically requesting msvc-cl, we will disable clang-cl even if it the default in the dev's environment

We mark the build non-deterministic because the DATE and other timestamp overrides do absolutely nothing in msvc-cl, and that build cannot be deterministic. We are in a different, better situation with clang-cl, it is very noisy about overriding these intrinsic macros, but the reassignment takes place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok sgtm


# Windows build behaviors when using clang-cl
build:clang-cl --action_env=USE_CLANG_CL=1
build:clang-cl --define clang_cl=1

# Required to work around Windows clang-cl build defects
# Ignore conflicting definitions of _WIN32_WINNT
# Overriding __TIME__ etc is problematic (and is actually an invalid no-op)
# Override determinism flags (DATE etc) is valid on clang-cl compiler
build:clang-cl --copt="-Wno-macro-redefined"
build:clang-cl --copt="-Wno-builtin-macro-redefined"
build:clang-cl --action_env=USE_CLANG_CL=1
Expand Down
27 changes: 27 additions & 0 deletions bazel/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,33 @@ config_setting(
},
)

config_setting(
name = "clang_cl_opt_build",
values = {
"cpu": "x64_windows",
"define": "clang_cl=1",
"compilation_mode": "opt",
},
)

config_setting(
name = "clang_cl_dbg_build",
values = {
"cpu": "x64_windows",
"define": "clang_cl=1",
"compilation_mode": "dbg",
},
)

config_setting(
name = "clang_cl_fastbuild_build",
values = {
"cpu": "x64_windows",
"define": "clang_cl=1",
"compilation_mode": "fastbuild",
},
)

config_setting(
name = "opt_build",
values = {"compilation_mode": "opt"},
Expand Down
7 changes: 6 additions & 1 deletion bazel/envoy_binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,13 @@ def _envoy_linkopts():
"-pagezero_size 10000",
"-image_base 100000000",
],
"@envoy//bazel:clang_cl_opt_build": [
"-DEFAULTLIB:ws2_32.lib",
"-DEFAULTLIB:iphlpapi.lib",
"-DEBUG:FULL",
"-WX",
],
"@envoy//bazel:windows_x86_64": [
"-DEFAULTLIB:advapi32.lib",
"-DEFAULTLIB:ws2_32.lib",
"-DEFAULTLIB:iphlpapi.lib",
"-WX",
Expand Down
14 changes: 11 additions & 3 deletions bazel/envoy_internal.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,16 @@ def envoy_copts(repository, test = False):
"-DNOIME",
"-DNOCRYPT",
# Ignore unguarded gcc pragmas in quiche (unrecognized by MSVC)
# TODO(wrowe): Drop this change when fixed in bazel/external/quiche.genrule_cmd
# TODO(wrowe,sunjayBhatia): Drop this change when fixed in bazel/external/quiche.genrule_cmd
"-wd4068",
# this is to silence the incorrect MSVC compiler warning when trying to convert between
# std::optional data types while conversions between primitive types are producing no error
# Silence incorrect MSVC compiler warnings when converting between std::optional
# data types (while conversions between primitive types are producing no error)
"-wd4244",
# Allow inline functions to be undefined
"-wd4506",
# Allow 'nodiscard' function return values to be discarded
# TODO(wrowe,sunjayBhatia): Drop this option when all causes are fixed
"-wd4834",
]

return select({
Expand All @@ -51,6 +56,9 @@ def envoy_copts(repository, test = False):
repository + "//bazel:windows_opt_build": [],
repository + "//bazel:windows_fastbuild_build": [],
repository + "//bazel:windows_dbg_build": [],
repository + "//bazel:clang_cl_opt_build": [] if test else ["-Z7", "-fstandalone-debug"],
repository + "//bazel:clang_cl_fastbuild_build": ["-fno-standalone-debug"],
repository + "//bazel:clang_cl_dbg_build": ["-fstandalone-debug"],
}) + select({
repository + "//bazel:clang_build": ["-fno-limit-debug-info", "-Wgnu-conditional-omitted-operand", "-Wc++2a-extensions", "-Wrange-loop-analysis"],
repository + "//bazel:gcc_build": ["-Wno-maybe-uninitialized"],
Expand Down
6 changes: 4 additions & 2 deletions bazel/envoy_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ def _envoy_cc_test_infrastructure_library(
tags = [],
include_prefix = None,
copts = [],
alwayslink = 1,
**kargs):
# Add implicit tcmalloc external dependency(if available) in order to enable CPU and heap profiling in tests.
deps += tcmalloc_external_deps(repository)
Expand All @@ -44,7 +45,7 @@ def _envoy_cc_test_infrastructure_library(
],
tags = tags,
include_prefix = include_prefix,
alwayslink = 1,
alwayslink = alwayslink,
linkstatic = envoy_linkstatic(),
**kargs
)
Expand All @@ -58,7 +59,6 @@ def _envoy_test_linkopts():
"-image_base 100000000",
],
"@envoy//bazel:windows_x86_64": [
"-DEFAULTLIB:advapi32.lib",
"-DEFAULTLIB:ws2_32.lib",
"-DEFAULTLIB:iphlpapi.lib",
"-WX",
Expand Down Expand Up @@ -205,6 +205,7 @@ def envoy_cc_test_library(
tags = [],
include_prefix = None,
copts = [],
alwayslink = 1,
**kargs):
deps = deps + [
repository + "//test/test_common:printers_includes",
Expand All @@ -222,6 +223,7 @@ def envoy_cc_test_library(
include_prefix,
copts,
visibility = ["//visibility:public"],
alwayslink = alwayslink,
**kargs
)

Expand Down
6 changes: 0 additions & 6 deletions bazel/foreign_cc/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -138,11 +138,6 @@ envoy_cmake_external(
"ZLIB_LIBRARY": "$EXT_BUILD_DEPS/zlib",
"ZLIB_INCLUDE_DIR": "$EXT_BUILD_DEPS/zlib/include",
"CMAKE_CXX_COMPILER_FORCED": "on",
"CMAKE_C_FLAGS_BAZEL": "-fPIC",
# Note we use Bazel's flags (not _RELEASE/_DEBUG CMake flags), but this toggle
# also works around a bug in CMP0091 logic which re-injected a badly placed -M flag.
# See https://github.com/bazelbuild/rules_foreign_cc/issues/426
"CURL_STATIC_CRT": "on",
},
defines = ["CURL_STATICLIB"],
generate_crosstool_file = True,
Expand Down Expand Up @@ -219,7 +214,6 @@ envoy_cmake_external(
envoy_cmake_external(
name = "zlib",
cache_entries = {
"BUILD_SHARED_LIBS": "off",
"CMAKE_CXX_COMPILER_FORCED": "on",
"CMAKE_C_COMPILER_FORCED": "on",
"SKIP_BUILD_EXAMPLES": "on",
Expand Down
29 changes: 29 additions & 0 deletions bazel/foreign_cc/curl.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
diff --git a/CMakeLists.txt b/CMakeLists.txt
wrowe marked this conversation as resolved.
Show resolved Hide resolved
index ec1cfa782..0c5a72f00 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -42,0 +42,5 @@
+# revert CMake bug triggered by curl's defined max CMake policy version, see https://gitlab.kitware.com/cmake/cmake/-/issues/21288
+if(POLICY CMP0091)
+ cmake_policy(SET CMP0091 OLD)
+endif()
+
@@ -249,3 +254,6 @@
- set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
- set(CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE} /MT")
- set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} /MTd")
+ foreach(build_suffix "" _DEBUG _RELEASE _MINSIZEREL _RELWITHDEBINFO)
+ set(flags_var CMAKE_C_FLAGS${build_suffix})
+ if("${${flags_var}}" MATCHES "/MD")
+ string(REGEX REPLACE "/MD" "/MT" ${flags_var} "${${flags_var}}")
+ endif()
+ endforeach()
diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt
index 911c9096d..ba6af1bf1 100644
--- a/lib/CMakeLists.txt
+++ b/lib/CMakeLists.txt
@@ -91,4 +91,0 @@ add_library(
-if(MSVC AND NOT BUILD_SHARED_LIBS)
- set_target_properties(${LIB_NAME} PROPERTIES STATIC_LIBRARY_FLAGS ${CMAKE_EXE_LINKER_FLAGS})
-endif()
-
10 changes: 10 additions & 0 deletions bazel/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,16 @@ def _com_github_curl():
build_file_content = BUILD_ALL_CONTENT + """
cc_library(name = "curl", visibility = ["//visibility:public"], deps = ["@envoy//bazel/foreign_cc:curl"])
""",
# Patch curl 7.72.0 due to CMake's problematic implementation of policy `CMP0091`
# introduced in CMake 3.15 and then deprecated in CMake 3.18. Curl forcing the CMake
# ruleset to 3.16 breaks the Envoy windows fastbuild target.
# Also cure a fatal assumption creating a static library using LLVM `lld-link.exe`
# adding dynamic link flags, which breaks the Envoy clang-cl library archive step.
# Upstream patch submitted: https://github.com/curl/curl/pull/6050
# TODO(https://github.com/envoyproxy/envoy/issues/11816): This patch is obsoleted
# by elimination of the curl dependency.
patches = ["@envoy//bazel/foreign_cc:curl.patch"],
patch_args = ["-p1"],
**location
)
native.bind(
Expand Down
2 changes: 2 additions & 0 deletions test/exe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ envoy_cc_test(
name = "main_common_test",
srcs = ["main_common_test.cc"],
data = ["//test/config/integration:google_com_proxy_port_0"],
# TODO(envoyproxy/windows-dev): diagnose clang-cl fastbuild TIMEOUT flake
tags = ["flaky_on_windows"],
deps = [
"//source/common/api:api_lib",
"//source/exe:main_common_lib",
Expand Down
2 changes: 2 additions & 0 deletions test/extensions/filters/http/adaptive_concurrency/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ envoy_extension_cc_test(
"adaptive_concurrency_filter_integration_test.h",
],
extension_name = "envoy.filters.http.adaptive_concurrency",
# TODO(envoyproxy/windows-dev): diagnose clang-cl build test failure
tags = ["fails_on_windows"],
deps = [
"//source/extensions/filters/http/adaptive_concurrency:config",
"//source/extensions/filters/http/fault:config",
Expand Down
2 changes: 2 additions & 0 deletions test/extensions/filters/http/common/compressor/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,6 @@ envoy_cc_benchmark_binary(
envoy_benchmark_test(
name = "compressor_filter_speed_test_benchmark_test",
benchmark_binary = "compressor_filter_speed_test",
# TODO(envoyproxy/windows-dev): diagnose clang-cl build test failure
tags = ["fails_on_windows"],
)
2 changes: 2 additions & 0 deletions test/extensions/filters/http/grpc_json_transcoder/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ envoy_extension_cc_test(
"//test/proto:bookstore_proto_descriptor",
],
extension_name = "envoy.filters.http.grpc_json_transcoder",
# TODO(envoyproxy/windows-dev): diagnose clang-cl build test failure
tags = ["fails_on_windows"],
deps = [
"//source/common/grpc:codec_lib",
"//source/common/http:header_map_lib",
Expand Down
2 changes: 2 additions & 0 deletions test/extensions/filters/http/jwt_authn/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ envoy_extension_cc_test(
name = "group_verifier_test",
srcs = ["group_verifier_test.cc"],
extension_name = "envoy.filters.http.jwt_authn",
# TODO(envoyproxy/windows-dev): Diagnose msvc-cl fastbuild test failure
tags = ["fails_on_windows"],
deps = [
":mock_lib",
":test_common_lib",
Expand Down
2 changes: 2 additions & 0 deletions test/extensions/filters/http/lua/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ envoy_extension_cc_test(
name = "lua_filter_test",
srcs = ["lua_filter_test.cc"],
extension_name = "envoy.filters.http.lua",
# TODO(envoyproxy/windows-dev): diagnose clang-cl build test failure
tags = ["fails_on_windows"],
deps = [
"//source/common/stream_info:stream_info_lib",
"//source/extensions/filters/http/lua:lua_filter_lib",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ envoy_extension_cc_test(
"//test/config/integration/certs",
],
extension_name = "envoy.filters.network.sni_dynamic_forward_proxy",
# TODO(envoyproxy/windows-dev): diagnose clang-cl build test failure
tags = ["fails_on_windows"],
deps = [
"//source/extensions/clusters/dynamic_forward_proxy:cluster",
"//source/extensions/filters/listener/tls_inspector:config",
Expand Down
1 change: 1 addition & 0 deletions test/extensions/quic_listeners/quiche/integration/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ envoy_cc_test(
size = "medium",
srcs = ["quic_http_integration_test.cc"],
data = ["//test/config/integration/certs"],
# TODO(envoyproxy/windows-dev): diagnose msvc-cl build test failure
tags = [
"fails_on_windows",
"nofips",
Expand Down
2 changes: 2 additions & 0 deletions test/extensions/stats_sinks/hystrix/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ envoy_extension_cc_test(
name = "hystrix_test",
srcs = ["hystrix_test.cc"],
extension_name = "envoy.stat_sinks.hystrix",
# TODO(envoyproxy/windows-dev): Diagnose msvc-cl fastbuild test failure
tags = ["fails_on_windows"],
deps = [
"//source/common/json:json_loader_lib",
"//source/common/stats:stats_lib",
Expand Down
2 changes: 2 additions & 0 deletions test/server/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,8 @@ envoy_cc_test(
":server_test_data",
":static_validation_test_data",
],
# TODO(envoyproxy/windows-dev): diagnose clang-cl build test timeout
tags = ["fails_on_windows"],
deps = [
"//source/common/version:version_lib",
"//source/extensions/access_loggers/file:config",
Expand Down