-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Windows: enable tests and envoy-static.exe pdb file #13688
Changes from 6 commits
c26ed9c
d74faff
96b8c29
a04cd1d
d73e918
cdb8b44
869b718
5451544
747e250
e4048af
db0120b
eaea783
bc3d3a4
5c030e8
d4884e3
27d54a7
7468926
1a7523d
788a39e
b49a982
d794063
840f6fd
3ffbeba
d575f73
446f5f5
f26c01a
294dd22
dc68e63
5a058de
779a753
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,7 +60,7 @@ def _envoy_linkopts(): | |
"-pagezero_size 10000", | ||
"-image_base 100000000", | ||
], | ||
"@envoy//bazel:clang_cl_opt_build": [ | ||
"@envoy//bazel:windows_opt_build": [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,7 +54,7 @@ def envoy_copts(repository, test = False): | |
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"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought that this comment...
did exactly that? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.)" There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,13 @@ BAZEL_BUILD_OPTIONS=( | |
"${BAZEL_BUILD_EXTRA_OPTIONS[@]}" | ||
"${BAZEL_EXTRA_TEST_OPTIONS[@]}") | ||
|
||
skip_test_tags="--test_tag_filters=-skip_on_windows,-fails_on_windows,-flaky_on_windows" | ||
fail_test_tags="--test_tag_filters=fails_on_windows,flaky_on_windows" | ||
if [[ "${BAZEL_BUILD_OPTIONS[*]}" =~ "clang-cl" ]]; then | ||
skip_test_tags="${skip_test_tags},-skip_on_windows_clang,-fails_on_windows_clang" | ||
fail_test_tags="${fail_test_tags},fails_on_windows_clang" | ||
fi | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This extra logic will be discarded after all compile and runtime failures unique to clang-cl have been resolved. |
||
# Also setup some space for building Envoy standalone. | ||
ENVOY_BUILD_DIR="${BUILD_DIR}"/envoy | ||
mkdir -p "${ENVOY_BUILD_DIR}" | ||
|
@@ -73,10 +80,13 @@ cp -f bazel-bin/source/exe/envoy-static.exe "${ENVOY_DELIVERY_DIR}/envoy.exe" | |
tar czf "${ENVOY_BUILD_DIR}"/envoy_binary.tar.gz -C "${ENVOY_DELIVERY_DIR}" envoy.exe | ||
|
||
# Test invocations of known-working tests on Windows | ||
bazel "${BAZEL_STARTUP_OPTIONS[@]}" test "${BAZEL_BUILD_OPTIONS[@]}" //test/... --test_tag_filters=-skip_on_windows,-fails_on_windows,-flaky_on_windows --build_tests_only | ||
bazel "${BAZEL_STARTUP_OPTIONS[@]}" test "${BAZEL_BUILD_OPTIONS[@]}" //test/... "${skip_test_tags}" --build_tests_only | ||
|
||
# Build tests that are known-flaky or known-failing to ensure no compilation regressions | ||
bazel "${BAZEL_STARTUP_OPTIONS[@]}" build "${BAZEL_BUILD_OPTIONS[@]}" //test/... --test_tag_filters=-skip_on_windows,fails_on_windows,flaky_on_windows --build_tests_only | ||
bazel "${BAZEL_STARTUP_OPTIONS[@]}" build "${BAZEL_BUILD_OPTIONS[@]}" //test/... "${fail_test_tags}" --build_tests_only | ||
|
||
# Demonstrate broken linkage of all tests known to fail to build with clang-cl | ||
bazel "${BAZEL_STARTUP_OPTIONS[@]}" build "${BAZEL_BUILD_OPTIONS[@]}" //test/... "--test_tag_filters=skip_on_windows_clang" --build_tests_only --keep-going | ||
|
||
# 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" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -480,7 +480,7 @@ TEST_F(AsyncDataSourceTest, BaseIntervalGreaterThanMaxInterval) { | |
TestUtility::loadFromYamlAndValidate(yaml, config); | ||
EXPECT_TRUE(config.has_remote()); | ||
|
||
EXPECT_THROW_WITH_MESSAGE(std::make_unique<Config::DataSource::RemoteAsyncDataProvider>( | ||
EXPECT_THROW_WITH_MESSAGE((void)std::make_unique<Config::DataSource::RemoteAsyncDataProvider>( | ||
cm_, init_manager_, config.remote(), dispatcher_, random_, true, | ||
[&](const std::string&) {}), | ||
EnvoyException, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a wide open question, @htuch - is there someone of the google / googletest participants who could identify why this emits an error under clang-cl, but on no other architectures (such as gcc, linux clang, and not even msvc.) We could add the explicit flag -Wno-discard-results for clang-cl, but we hadn't done this for other architectures, and I'm wondering if that would be wise. I've fixed a couple of discarded non-expectation results, but in this EXPECT_[NO]THROW googletest pattern, it does get a little wordy. Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely would rather not have this void cast. I'll follow up on this offline, but one option is to build the cast explicitly into the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LMK if this is still an issue on Clang 11. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still an issue with -Wall -Werror; however, it makes sense to keep this potential defect detection in source/... --- but it doesn't seem interesting to track discarded results in test/... sources. Moved the warning suppression to the appropriate tests-only copt logic for clang-cl builds, and left a handful of corrections for source/... --- including documentation of why the results are discarded. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,8 @@ envoy_cc_test( | |
name = "codec_impl_test", | ||
srcs = ["codec_impl_test.cc"], | ||
shard_count = 5, | ||
# TODO(envoyproxy/windows-dev): Resolve missing pure virtual destructor bug | ||
tags = ["skip_on_windows_clang"], | ||
deps = CODEC_TEST_DEPS, | ||
) | ||
|
||
|
@@ -52,6 +54,8 @@ envoy_cc_test( | |
"--runtime-feature-disable-for-tests=envoy.reloadable_features.new_codec_behavior", | ||
], | ||
shard_count = 5, | ||
# TODO(envoyproxy/windows-dev): Resolve missing pure virtual destructor bug | ||
tags = ["skip_on_windows_clang"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what I'm looking for some possible assistance from google folks, @htuch - we are having serious problems deciphering why clang-cl is failing in a manner unique to building opt (/O2) on Windows using clang-cl, and in no other environments or compilers. This particular link failure reads as follows;
Any chance you can find some support for us researching what might be happening here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does appear to be resolved in clang 11.0.0 release. |
||
deps = CODEC_TEST_DEPS, | ||
) | ||
|
||
|
@@ -128,6 +132,8 @@ envoy_cc_test( | |
"response_header_corpus/simple_example_huffman", | ||
"response_header_corpus/simple_example_plain", | ||
], | ||
# TODO(envoyproxy/windows-dev): Resolve missing pure virtual destructor bug | ||
tags = ["skip_on_windows_clang"], | ||
deps = [ | ||
":frame_replay_lib", | ||
"//test/common/http/http2:codec_impl_test_util", | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?