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

Quiche changes to avoid gcc flags on Windows #8514

Merged
merged 3 commits into from
Oct 22, 2019
Merged

Quiche changes to avoid gcc flags on Windows #8514

merged 3 commits into from
Oct 22, 2019

Conversation

achasveachas
Copy link
Contributor

Description:
The -w flags 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

@achasveachas
Quiche changes to avoid gcc flags on Windows
37fd5fc 
Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io>
Signed-off-by: William Rowe <wrowe@pivotal.io>
danzh2010
danzh2010 reviewed Oct 8, 2019
View reviewed changes
bazel/external/quiche.BUILD Outdated
copts = quiche_copt,
copts = select({
"@envoy//bazel:windows_x86_64": [],
"//conditions:default": quiche_copt,
Copy link
Contributor

Choose a reason for hiding this comment

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

W/o quiche_copt, can these targets be built in Windows? Do we still treat those warnings as error in Windows?

Copy link
Contributor

Choose a reason for hiding this comment

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

Negative. They are invalid command line options, compilation will not proceed.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explanation!

@wrowe
Copy link
Contributor

wrowe commented Oct 8, 2019

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.

danzh2010
danzh2010 previously approved these changes Oct 8, 2019
View reviewed changes
lizan
lizan reviewed Oct 9, 2019
View reviewed changes
bazel/external/quiche.BUILD Outdated
@@ -108,7 +108,10 @@ envoy_cc_library(
name = "http2_constants_lib",
srcs = ["quiche/http2/http2_constants.cc"],
hdrs = ["quiche/http2/http2_constants.h"],
copts = quiche_copt,
copts = select({
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 refactor them to quiche_copt = select {...} so you don't have to replace copt everywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly, no. Select may not be nested.

Copy link
Contributor

@wrowe wrowe Oct 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

(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.)

Copy link
Member

Choose a reason for hiding this comment

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

you're not nesting if you just make select to L55 and drop everywhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, our earlier attempts were apparently overly complex, I believe the current commit is the simplest representation you were asking us for?

@wrowe
Copy link
Contributor

wrowe commented Oct 10, 2019

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.

@achasveachas @wrowe
Add platform logic to a few more BUILD targets
02cc715 
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>
danzh2010
danzh2010 previously approved these changes Oct 10, 2019
View reviewed changes
@stale
Copy link

stale bot commented Oct 17, 2019

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!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 17, 2019
@wrowe
Simplify quiche_copts logic, suggested by lizan
284164f 
Signed-off-by: William A Rowe Jr <wrowe@pivotal.io>
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 22, 2019
danzh2010
danzh2010 approved these changes Oct 22, 2019
View reviewed changes
Copy link
Contributor

@danzh2010 danzh2010 left a comment

Choose a reason for hiding this comment

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

This looks much cleaner! Thanks for optimizing it.

mattklein123
mattklein123 approved these changes Oct 22, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit 9ad274d into envoyproxy:master Oct 22, 2019
spenceral added a commit to spenceral/envoy that referenced this pull request Oct 23, 2019
@spenceral
Merge branch 'master' into downstream-protocol-error
e4eb45c 
* 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>
derekargueta pushed a commit to derekargueta/envoy that referenced this pull request Oct 24, 2019
@achasveachas @derekargueta
Quiche changes to avoid gcc flags on Windows (envoyproxy#8514)
5cde584 
Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io>
Signed-off-by: William Rowe <wrowe@pivotal.io>
@sunjayBhatia sunjayBhatia deleted the fix-quiche branch December 24, 2019 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wrowe wrowe wrowe left review comments

@lizan lizan lizan left review comments

@mattklein123 mattklein123 mattklein123 approved these changes

@danzh2010 danzh2010 danzh2010 approved these changes

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

Assignees

@danzh2010 danzh2010

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@achasveachas @wrowe @lizan @mattklein123 @danzh2010