Quiche changes to avoid gcc flags on Windows#8514
Quiche changes to avoid gcc flags on Windows#8514mattklein123 merged 3 commits intoenvoyproxy:masterfrom greenhouse-org:fix-quiche
Conversation
Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io> Signed-off-by: William Rowe <wrowe@pivotal.io>
bazel/external/quiche.BUILD
Outdated
| copts = quiche_copt, | ||
| copts = select({ | ||
| "@envoy//bazel:windows_x86_64": [], | ||
| "//conditions:default": quiche_copt, |
There was a problem hiding this comment.
W/o quiche_copt, can these targets be built in Windows? Do we still treat those warnings as error in Windows?
There was a problem hiding this comment.
Negative. They are invalid command line options, compilation will not proceed.
There was a problem hiding this comment.
Sorry, I meant, with this patch avoiding invalid command line options, we are able to compile, we aren't alerting on such warnings, although we aren't at the windows equiv of Wall Werror.
|
In PR 8280 we proposed to accept all invalid, unguarded gcc #pragma statements with the /Wd4068 toggle. This gets quiche building if the command line is not polluted, as proposed in this patch. |
bazel/external/quiche.BUILD
Outdated
| srcs = ["quiche/http2/http2_constants.cc"], | ||
| hdrs = ["quiche/http2/http2_constants.h"], | ||
| copts = quiche_copt, | ||
| copts = select({ |
There was a problem hiding this comment.
Can you refactor them to quiche_copt = select {...} so you don't have to replace copt everywhere?
There was a problem hiding this comment.
Sadly, no. Select may not be nested.
There was a problem hiding this comment.
(We had attempted to do so earlier, a macro in bazel/envoy_build_system might be useful here, but the definition could not be abstracted during our attempt.)
There was a problem hiding this comment.
you're not nesting if you just make select to L55 and drop everywhere else?
There was a problem hiding this comment.
Thanks, our earlier attempts were apparently overly complex, I believe the current commit is the simplest representation you were asking us for?
|
There are a few flags which occur in the test/ framework for quiche, we are attaching these to this ticket as a second commit to this PR. |
This avoids more gcc flags from being passed to MSVC compiler. Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io> Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
danzh2010
left a comment
There was a problem hiding this comment.
This looks much cleaner! Thanks for optimizing it.
* master: (54 commits) Update dependencies - Go, Bazel toos, xxHash, nanopb, rules_go, curl, protobuf, Jinja, yaml-cpp, re2 (envoyproxy#8728) test: increase coverage of listener_manager_impl.cc (envoyproxy#8737) test: modify some macros to reduce number of uncovered lines reported (envoyproxy#8725) build: add a script to setup clang (envoyproxy#8682) http: fix ssl_redirect on external (envoyproxy#8664) docs: update fedora build requirements (envoyproxy#8641) fix draining listener removal logs (envoyproxy#8733) dubbo: fix router doc (envoyproxy#8734) server: provide server health status when stats disabled (envoyproxy#8482) router: adding a knob to configure a cap on buffering for shadowing/retries (envoyproxy#8574) tcp proxy: add default 1 hour idle timeout (envoyproxy#8705) thrift: fix filter names in docs (envoyproxy#8726) Quiche changes to avoid gcc flags on Windows (envoyproxy#8514) test: increase test coverage in Router::HeaderParser (envoyproxy#8721) admin: add drain listeners endpoint (envoyproxy#8415) buffer filter: add content-length when ending stream with trailers (envoyproxy#8609) clarify draining option docs (envoyproxy#8712) build: ignore go-control-plane mirror git commit error code (envoyproxy#8703) api: remove API type database from checked in artifacts. (envoyproxy#8716) admin: correct help strings (envoyproxy#8710) ... Signed-off-by: Spencer Lewis <slewis@squareup.com>
Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io> Signed-off-by: William Rowe <wrowe@pivotal.io>
Description:
The
-wflags are not recognized by msvc.Signed-off-by: Yechiel Kalmenson ykalmenson@pivotal.io
Signed-off-by: William Rowe wrowe@pivotal.io
Risk Level: Low
Testing: Local on Windows
Docs Changes: None
Release Notes: None