http3: further cleanup and extension removal#15752
Conversation
Fixes #12829 Signed-off-by: Matt Klein <mklein@lyft.com>
|
Opening as a draft to check CI. I need to add more test coverage of the QUIC disabled case to make sure we do sufficient configuration error checking and can't hit any of the not reached guards. |
|
@alyssawilk @danzh2010 I think this should be ready for review. |
alyssawilk
left a comment
There was a problem hiding this comment.
So. Much. Cleaner.
Honestly I would tend to stick with some of the factories over proliferation of #defines but that's style and I'm good either way.
I can take a look at stubs instead, which I may have to do if we want to fully compile everything out even for tests. I will take another look. |
Signed-off-by: Matt Klein <mklein@lyft.com>
|
@PiotrSikora the last commit should fix the fips build. cc @keith thanks for working through that with me. @alyssawilk this whole situation is honestly a total mess and I think we clean it up with a new feature in bazel 4.0 which supports targets being "compatible with" flags. I would suggest we land this, upgrade to 4.0, and then we can clean up further with actually fully blocking the targets if disabled?cc @lizan |
Signed-off-by: Matt Klein <mklein@lyft.com>
Thanks! I can confirm it fixes the issue. |
alyssawilk
left a comment
There was a problem hiding this comment.
Just when we thought it couldn't get any harier :-P
question - do you think we should have the compile options build do one pass with
!quic
and another with
!fips
to futureproof against that sort of regression? I'm inlined to think it's worth it but we can always try with this as-is and revisit if there are future breakages
| }, | ||
| ) | ||
|
|
||
| selects.config_setting_group( |
There was a problem hiding this comment.
Hey @lizan can you do a pass of just the bazel magic?
There was a problem hiding this comment.
As long as this works it's definitely fine, I'm impressed it does though, we should separately change the apple config to use this instead, looking at the skylib impl it's very hard to parse, but I believe it's doing that same hack https://github.com/bazelbuild/bazel-skylib/blob/f80bc733d4b9f83d427ce3442be2e07427b2cc8d/lib/selects.bzl#L145-L186
There was a problem hiding this comment.
Believe it or not, I found this trick by googling, where I found the bazel issue you opened about the apple hack, where the person commented it could be done with this thing, but it wasn't merged yet. Now it's merged. :)
There was a problem hiding this comment.
Amazing, I forgot this, here's the update for the other ones that can benefit from this #15797
Signed-off-by: Matt Klein <mklein@lyft.com>
|
The change to HttpIntegrationTest::makeClientConnectionWithOptions() in test/integration/http_integration.cc breaks compilation on certain embedders with the following error message: This is part of presubmit and thus blocking submission of any CL for our project. Please fix. (Unfortunately I do not have my Envoy checkout any more, otherwise I would fix it myself.) |
|
I'll take a look |
Fixes #12829
Risk Level: Low
Testing: Existing/new tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A