Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
http3: changing how http3 is built #15540
http3: changing how http3 is built #15540
Changes from all commits
2cabd77
8313e4f
76336ff
ac00575
0862919
6e61af1
dd80ec0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 your plan to follow up here and completely remove these as extensions? TODO? Also this will allow us to use the GSO writer for udp_proxy also and remove all of that config as well. TODO around that?
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.
hm, I didn't think having quic as a "required" extension made any less sense than tls as a required extension, but if you want to remove it entirely we can. Want me to do that here, or a follow up?
I wasn't planning on following up on the GSO bits, I was largely hoping to just make the QUIC hackery I'm dealing with less hacky. Would you or @danzh2010 be up for tackling GSO as a followup?
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.
TLS as an extension is "required" because of the openssl stuff. I think if we completely remove QUIC as an extension it will clean up a lot of the places I have commented on where we are referencing extension code in core code and vice versa. I'm happy to do some of this work as a side project if you don't have time but let's add the TODO either way?
Yes I can work on this as part of ^ if you don't want to do it now.
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.
cc @danzh2010 David said you had a workaround for this - lmk if this conflicts or you have a better plan? Honestly I was surprised this compiled, having assumed we needed to match the google log defines...
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.
cc @DavidSchinazi
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.
This works. I'll have a more complete fix which will bring back DEBUG (this time without breaking macOS) ASAP. Feel free to merge this.
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.
The other fix I mentioned is at #15616