-
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 28 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 |
---|---|---|
|
@@ -34,37 +34,44 @@ 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, since | ||
# debugging info detailing some 1600 test binaries would be wasteful. | ||
# 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"], | ||
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"], | ||
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"], | ||
# TODO: Replace with /Zc:preprocessor for cl.exe versions >= 16.5 | ||
repository + "//bazel:windows_x86_64": ["-experimental:preprocessor", "-Wv:19.4"], | ||
# Allow 'nodiscard' function results values to be discarded for test code only | ||
# TODO(envoyproxy/windows-dev): Replace with /Zc:preprocessor for cl.exe versions >= 16.5 | ||
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"], | ||
|
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]; | ||
|
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?