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.

Sign up for GitHub

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

Jump to bottom

Windows: enable tests and envoy-static.exe pdb file #13688

Merged
merged 30 commits into from
Dec 1, 2020
Merged

Windows: enable tests and envoy-static.exe pdb file #13688

Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
c26ed9c
Workaround problematic googletests with clang-cl compilation
wrowe Oct 9, 2020
d74faff
Activate the clang-cl pipeline with working opt build
wrowe Sep 16, 2020
96b8c29
Tag tests broken due to missing dtor in -c opt --config=clang-cl
sunjayBhatia Oct 21, 2020
a04cd1d
Windows: re-disable tests identifed in #13133
wrowe Oct 28, 2020
d73e918
Address test sequencing problems which cause windows flakes
wrowe Oct 29, 2020
cdb8b44
Demonstrate clang-cl build failures to share with maintainers
wrowe Oct 30, 2020
869b718
Fix bazel syntax
wrowe Oct 30, 2020
5451544
Merge remote-tracking branch 'origin/master' into add-clang-cl-build
wrowe Nov 3, 2020
747e250
Merge remote-tracking branch 'origin/master' into add-clang-cl-build
wrowe Nov 10, 2020
e4048af
Drop corrections of EXPECT_ arguments for result values ignored
wrowe Nov 10, 2020
db0120b
Merge remote-tracking branch 'origin/master' into add-clang-cl-build
wrowe Nov 11, 2020
eaea783
The clang-cl 11.0.0 release resolves this issue
wrowe Nov 11, 2020
bc3d3a4
Unmark tests now passing on clang-cl 11.0.0
wrowe Nov 11, 2020
5c030e8
Merge remote-tracking branch 'origin/master' into add-clang-cl-build
wrowe Nov 11, 2020
d4884e3
Merge remote-tracking branch 'origin/master' into add-clang-cl-build
wrowe Nov 17, 2020
27d54a7
Anticipate release of bazel 4.0.0
wrowe Nov 17, 2020
7468926
Merge remote-tracking branch 'origin/master' into add-clang-cl-build
wrowe Nov 19, 2020
1a7523d
Merge remote-tracking branch 'origin/master' into add-clang-cl-build
sunjayBhatia Nov 20, 2020
788a39e
Reidentify passing and failing/flaky tests for msvc-cl and clang-cl
sunjayBhatia Nov 20, 2020
b49a982
Defer clang-cl pipeline, fix format
wrowe Nov 20, 2020
d794063
Add ability to read text file in test helper
wrowe Nov 20, 2020
840f6fd
Fix copy paste typo
sunjayBhatia Nov 20, 2020
3ffbeba
Enable additional tests
wrowe Nov 20, 2020
d575f73
Reintroduce flaky tag with detail based on CI observation
wrowe Nov 21, 2020
446f5f5
Merge remote-tracking branch 'origin/master' into add-clang-cl-build
wrowe Nov 21, 2020
f26c01a
Re-flake a few tests observed in CI
wrowe Nov 21, 2020
294dd22
Quick doc fixes prompted by htuch
wrowe Nov 24, 2020
dc68e63
Merge remote-tracking branch 'origin/master' into add-clang-cl-build
wrowe Nov 24, 2020
5a058de
Merge remote-tracking branch 'origin/master' into add-clang-cl-build
wrowe Nov 25, 2020
779a753
Merge remote-tracking branch 'origin/master' into add-clang-cl-build
sunjayBhatia Nov 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .bazelrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,12 @@ build:clang-cl --define clang_cl=1
# 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"
# Workaround problematic missing override declarations of mocks
# TODO: resolve this class of problematic mocks, e.g.
Copy link
Member

Choose a reason for hiding this comment

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

Is the plan to do this in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unclear whether 11.0.0 is going to fix this for us, whether we can fix this in a broad stroke, or whether every mock declaration affected must be updated and was missed in the last googletest update. This is why it was left unassigned.

Copy link
Member

Choose a reason for hiding this comment

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

Can you file a tracking issue?

# ./test/mocks/http/stream.h(16,21): error: 'addCallbacks'
# overrides a member function but is not marked 'override'
# MOCK_METHOD(void, addCallbacks, (StreamCallbacks & callbacks));
build:clang-cl --copt="-Wno-inconsistent-missing-override"
build:clang-cl --action_env=USE_CLANG_CL=1

# Defaults to 'auto' - Off for windows, so override to linux behavior
Expand Down
8 changes: 8 additions & 0 deletions bazel/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,14 @@ config_setting(
},
)

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

config_setting(
name = "clang_cl_opt_build",
values = {
Expand Down
2 changes: 1 addition & 1 deletion bazel/envoy_binary.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def _envoy_linkopts():
"-pagezero_size 10000",
"-image_base 100000000",
],
"@envoy//bazel:clang_cl_opt_build": [
"@envoy//bazel:windows_opt_build": [
Copy link
Contributor

Choose a reason for hiding this comment

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

This expands creation of a .pdb debuginfo symbol file to the opt build of msvc + clang, now that our rbe gcp pool has a larger working set of memory to collect that during the link phase.

"-DEFAULTLIB:ws2_32.lib",
"-DEFAULTLIB:iphlpapi.lib",
"-DEBUG:FULL",
Expand Down
22 changes: 14 additions & 8 deletions bazel/envoy_internal.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,37 +34,43 @@ def envoy_copts(repository, test = False):
"-DNOIME",
"-DNOCRYPT",
# Ignore unguarded gcc pragmas in quiche (unrecognized by MSVC)
# TODO(wrowe,sunjayBhatia): Drop this change when fixed in bazel/external/quiche.genrule_cmd
"-wd4068",
# 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({
repository + "//bazel:windows_x86_64": msvc_options,
"//conditions:default": posix_options,
}) + select({
# Bazel adds an implicit -DNDEBUG for opt.
# Simplify the amount of symbolic debug info for test binaries,
# targets listed in order from generic to increasing specificity.
# Bazel adds an implicit -DNDEBUG for opt targets.
repository + "//bazel:opt_build": [] if test else ["-ggdb3", "-gsplit-dwarf"],
repository + "//bazel:fastbuild_build": [],
repository + "//bazel:dbg_build": ["-ggdb3", "-gsplit-dwarf"],
repository + "//bazel:windows_opt_build": [],
repository + "//bazel:windows_opt_build": [] if test else ["-Z7"],
Copy link
Contributor

Choose a reason for hiding this comment

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

This collects debuginfo symbol info to the .obj files during the opt build of msvc + clang, only for non-test sources. For test sources, do not collect debuginfo for the hundreds of test executables. (We would build fastbuild or dbg individually to troubleshoot a crashing test.)

Copy link
Member

Choose a reason for hiding this comment

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

Can you add this to the .bzl comments so this intuition is kept for posterity?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that this comment...

Simplify the amount of symbolic debug info for test binaries,
targets listed in order from generic to increasing specificity.

did exactly that?

Copy link
Member

Choose a reason for hiding this comment

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

You might be technically correct here, the best kind of correct. I think this is not really as helpful as something like "do not collect debuginfo for the hundreds of test executables. (We would build fastbuild or dbg individually to troubleshoot a crashing test.)"

Copy link
Contributor

Choose a reason for hiding this comment

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

@htuch have a look at the new iteration and let me know if that's helpful.

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"],
# Toggle expected features and warnings by compiler
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"],
# Allow 'nodiscard' function results values to be discarded for test code only
# TODO: Replace with /Zc:preprocessor for cl.exe versions >= 16.5
repository + "//bazel:windows_x86_64": ["-experimental:preprocessor", "-Wv:19.4"],
repository + "//bazel:windows_x86_64": ["-wd4834", "-experimental:preprocessor", "-Wv:19.4"] if test else ["-experimental:preprocessor", "-Wv:19.4"],
repository + "//bazel:clang_cl_build": ["-Wno-unused-result"] if test else [],
"//conditions:default": [],
}) + select({
repository + "//bazel:no_debug_info": ["-g0"],
Expand Down
6 changes: 3 additions & 3 deletions bazel/repository_locations.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,11 +537,11 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_desc = "Bazel rules for the C++ language",
project_url = "https://github.com/bazelbuild/rules_cc",
# TODO(lizan): pin to a point releases when there's a released version.
version = "818289e5613731ae410efb54218a4077fb9dbb03",
sha256 = "9d48151ea71b3e225adfb6867e6d2c7d0dce46cbdc8710d9a9a628574dfd40a0",
version = "b1c40e1de81913a3c40e5948f78719c28152486d",
sha256 = "71d037168733f26d2a9648ad066ee8da4a34a13f51d24843a42efa6b65c2420f",
strip_prefix = "rules_cc-{version}",
urls = ["https://github.com/bazelbuild/rules_cc/archive/{version}.tar.gz"],
release_date = "2020-05-13",
release_date = "2020-11-11",
use_category = ["build"],
),
rules_foreign_cc = dict(
Expand Down
6 changes: 3 additions & 3 deletions ci/windows_ci_steps.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,6 @@ bazel "${BAZEL_STARTUP_OPTIONS[@]}" test "${BAZEL_BUILD_OPTIONS[@]}" //test/...
bazel "${BAZEL_STARTUP_OPTIONS[@]}" build "${BAZEL_BUILD_OPTIONS[@]}" //test/... --test_tag_filters=-skip_on_windows,fails_on_windows,flaky_on_windows --build_tests_only

# Summarize tests bypasssed to monitor the progress of porting to Windows
echo "Tests bypassed as skip_on_windows: $(bazel query 'kind(".*test rule", attr("tags", "skip_on_windows", //test/...))' 2>/dev/null | sort | wc -l) known unbuildable or inapplicable tests"
echo "Tests bypassed as fails_on_windows: $(bazel query 'kind(".*test rule", attr("tags", "fails_on_windows", //test/...))' 2>/dev/null | sort | wc -l) known incompatible tests"
echo "Tests bypassed as flaky_on_windows: $(bazel query 'kind(".*test rule", attr("tags", "flaky_on_windows", //test/...))' 2>/dev/null | sort | wc -l) known unstable tests"
echo "Tests bypassed as skip_on_windows: $(bazel "${BAZEL_STARTUP_OPTIONS[@]}" query 'kind(".*test rule", attr("tags", "skip_on_windows", //test/...))' 2>/dev/null | sort | wc -l) known unbuildable or inapplicable tests"
echo "Tests bypassed as fails_on_windows: $(bazel "${BAZEL_STARTUP_OPTIONS[@]}" query 'kind(".*test rule", attr("tags", "fails_on_windows", //test/...))' 2>/dev/null | sort | wc -l) known incompatible tests"
echo "Tests bypassed as flaky_on_windows: $(bazel "${BAZEL_STARTUP_OPTIONS[@]}" query 'kind(".*test rule", attr("tags", "flaky_on_windows", //test/...))' 2>/dev/null | sort | wc -l) known unstable tests"
4 changes: 2 additions & 2 deletions source/extensions/clusters/redis/redis_cluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,8 @@ void RedisCluster::RedisDiscoverySession::startResolveRedis() {
if (parent_.hosts_.empty()) {
const int rand_idx = parent_.random_.random() % discovery_address_list_.size();
auto it = discovery_address_list_.begin();
std::next(it, rand_idx);
host = Upstream::HostSharedPtr{new RedisHost(parent_.info(), "", *it, parent_, true)};
host = Upstream::HostSharedPtr{
new RedisHost(parent_.info(), "", *std::next(it, rand_idx), parent_, true)};
Copy link
Contributor

Choose a reason for hiding this comment

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

@htuch - this is the only non-build, non-test change in this entire patchset. Would you like me to break it out for you as an independently reviewed PR, or are we ok to merge and iterate at this point? It's the remaining example of a discarded no-op result in the //source/... since I re-merged.

Copy link
Member

Choose a reason for hiding this comment

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

No, I was not asking for this. I was pointing out that the test changes alone could have been upstreamed independently. Anyway, NBD.

} else {
const int rand_idx = parent_.random_.random() % parent_.hosts_.size();
host = parent_.hosts_[rand_idx];
Expand Down
6 changes: 2 additions & 4 deletions test/common/network/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,10 +286,8 @@ envoy_cc_test(
envoy_cc_test(
name = "udp_listener_impl_batch_writer_test",
srcs = ["udp_listener_impl_batch_writer_test.cc"],
tags = [
# Skipping as quiche quic_gso_batch_writer.h does not exist on Windows
"skip_on_windows",
],
# Skipping as quiche quic_gso_batch_writer.h does not exist on Windows
tags = ["skip_on_windows"],
deps = [
":udp_listener_impl_test_base_lib",
"//source/common/event:dispatcher_lib",
Expand Down
1 change: 0 additions & 1 deletion test/common/network/cidr_range_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ TEST(TruncateIpAddressAndLength, Various) {
{{"ffff::ffff", 128}, {"ffff::ffff", 128}},
{{"ffff::ffff", 999}, {"ffff::ffff", 128}},
};
test_cases.size();
htuch marked this conversation as resolved.
Show resolved Hide resolved
for (const auto& kv : test_cases) {
InstanceConstSharedPtr inPtr = Utility::parseInternetAddress(kv.first.first);
EXPECT_NE(inPtr, nullptr) << kv.first.first;
Expand Down
4 changes: 2 additions & 2 deletions test/common/network/socket_option_factory_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ class SocketOptionFactoryTest : public testing::Test {
TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls_{[this]() {
// Before injecting OsSysCallsImpl, make sure validateIpv{4,6}Supported is called so the static
// bool is initialized without requiring to mock ::socket and ::close. :( :(
std::make_unique<Address::Ipv4Instance>("1.2.3.4", 5678);
std::make_unique<Address::Ipv6Instance>("::1:2:3:4", 5678);
(void)std::make_unique<Address::Ipv4Instance>("1.2.3.4", 5678);
(void)std::make_unique<Address::Ipv6Instance>("::1:2:3:4", 5678);
return &os_sys_calls_mock_;
}()};

Expand Down
4 changes: 2 additions & 2 deletions test/common/network/socket_option_test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ class SocketOptionTest : public testing::Test {
TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls_{[this]() {
// Before injecting OsSysCallsImpl, make sure validateIpv{4,6}Supported is called so the static
// bool is initialized without requiring to mock ::socket and ::close.
std::make_unique<Address::Ipv4Instance>("1.2.3.4", 5678);
std::make_unique<Address::Ipv6Instance>("::1:2:3:4", 5678);
(void)std::make_unique<Address::Ipv4Instance>("1.2.3.4", 5678);
(void)std::make_unique<Address::Ipv6Instance>("::1:2:3:4", 5678);
htuch marked this conversation as resolved.
Show resolved Hide resolved
return &os_sys_calls_;
}()};

Expand Down
2 changes: 2 additions & 0 deletions test/extensions/filters/http/dynamic_forward_proxy/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ envoy_extension_cc_test(
"//test/config/integration/certs",
],
extension_name = "envoy.filters.http.dynamic_forward_proxy",
# TODO(envoyproxy/windows-dev): Diagnose flake in msvc-cl pipeline, observed timeout in
# IpVersions/ProxyFilterIntegrationTest.ReloadClusterAndAttachToCache/IPv6
tags = ["flaky_on_windows"],
deps = [
"//source/extensions/clusters/dynamic_forward_proxy:cluster",
Expand Down
2 changes: 0 additions & 2 deletions test/extensions/filters/http/ratelimit/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,6 @@ envoy_extension_cc_test(
name = "ratelimit_integration_test",
srcs = ["ratelimit_integration_test.cc"],
extension_name = "envoy.filters.http.ratelimit",
# TODO(envoyproxy/windows-dev): Apparently resolved by Level Events PR #13787
tags = ["flaky_on_windows"],
deps = [
"//source/common/buffer:zero_copy_input_stream_lib",
"//source/common/grpc:codec_lib",
Expand Down
3 changes: 3 additions & 0 deletions test/extensions/filters/http/tap/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ envoy_extension_cc_test(
name = "tap_filter_integration_test",
srcs = ["tap_filter_integration_test.cc"],
extension_name = "envoy.filters.http.tap",
# TODO(envoyproxy/windows-dev): Diagnose flake, observed to hang at;
# IpVersions/TapIntegrationTest.AdminLdsReload/IPv6
tags = ["flaky_on_windows"],
deps = [
"//source/extensions/filters/http/tap:config",
"//test/extensions/common/tap:common",
Expand Down
5 changes: 1 addition & 4 deletions test/extensions/quic_listeners/quiche/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,7 @@ envoy_cc_test(
envoy_cc_test(
name = "active_quic_listener_test",
srcs = ["active_quic_listener_test.cc"],
tags = [
"fails_on_windows",
"nofips",
],
tags = ["nofips"],
deps = [
":quic_test_utils_for_envoy_lib",
":test_utils_lib",
Expand Down
2 changes: 0 additions & 2 deletions test/extensions/stats_sinks/metrics_service/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,6 @@ envoy_extension_cc_test(
name = "metrics_service_integration_test",
srcs = ["metrics_service_integration_test.cc"],
extension_name = "envoy.stat_sinks.metrics_service",
# TODO(envoyproxy/windows-dev): Apparently resolved by Level Events PR #13787
tags = ["flaky_on_windows"],
deps = [
"//source/common/buffer:zero_copy_input_stream_lib",
"//source/common/grpc:codec_lib",
Expand Down
Loading