-
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
Creating a no exceptions build for Envoy Mobile. #29840
Conversation
1412fbd
to
ab7c117
Compare
5d7f22b
to
4ab63d8
Compare
63ffdc1
to
6021f9b
Compare
The initial passes of this were over 100 files of workarounds to build bazel build --config=remote --define=admin_html=disabled --define=admin_functionality=disabled --define envoy_exceptions=disabled --define envoy_mobile_listener=disabled --define=envoy_yaml=disabled //test/performance:test_binary_size --copt=-fno-unwind-tables --copt=-fno-exceptions Now we're in the low 70 files, but there's still work todo (see TODO). Landing now as E-M already panics on bad config, and we want to see what the binary size impact is for our E-M usage. Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
6021f9b
to
c31be28
Compare
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.
Wow, looks great. Mostly mechanical. Woo hoo
#ifdef ENVOY_DISABLE_EXCEPTIONS | ||
PANIC(error); | ||
#else | ||
throw SocketBindException(error, result.errno_); |
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 can't use throwExceptionOrPanic() because this exception type's constructor is special?
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.
yeah, I tried to normalize the constructor like I did elsewhere but it turns out we use the errno in tests to keep listening on random ports until we get one that works so it's non-trivial
Also tagging a !google !EM reviewer :-) |
/retest |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
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.
lgtm
The initial passes of this were over 100 files of workarounds to build
bazel build --config=remote --define=admin_html=disabled --define=admin_functionality=disabled --define envoy_exceptions=disabled --define envoy_mobile_listener=disabled --define=envoy_yaml=disabled //test/performance:test_binary_size --copt=-fno-unwind-tables --copt=-fno-exceptions
Now we're in the low 70 files, but there's still work todo (see TODO). Landing now as E-M already panics on bad config, and we want to see what the binary size impact is for our E-M usage.
I will continue working on envoyproxy/envoy-mobile#176 to do proper error handling with the end goal of removing this macro.
Risk Level: low
Testing: n/a
Docs Changes:
Release Notes:
envoyproxy/envoy-mobile#176