-
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
mobile: adding a config knob for building without listeners #25443
Conversation
acda9fa
to
a71f48a
Compare
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@@ -575,6 +575,9 @@ stats_sinks: *stats_sinks | |||
reloadable_features: | |||
always_use_v6: *force_ipv6 | |||
skip_dns_lookup_for_proxied_requests: *skip_dns_lookup_for_proxied_requests | |||
#ifndef ENVOY_MOBILE_ENABLE_LISTENER |
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.
Does this work in a string literal like this?
@@ -4,11 +4,35 @@ | |||
#include "source/common/runtime/runtime_impl.h" | |||
|
|||
namespace Envoy { | |||
namespace { | |||
|
|||
bool hasApiBootstrap(const envoy::config::bootstrap::v3::Bootstrap& bootstrap) { |
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.
Should this be named useApiListener
?
mobile/library/cc/BUILD
Outdated
@@ -12,7 +12,7 @@ envoy_cc_library( | |||
hdrs = [ | |||
"engine_builder.h", | |||
], | |||
copts = select({ | |||
copts = envoy_mobile_copts("@envoy") + select({ |
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.
Why is this needed? We should already be getting these from envoy_cc_library
, no?
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.
doesn't need it. ended up scrapping this entire buld file and plumbing a different way to catch tests not using the config template. That said it'd still a draft so feel free to ignore until I've gotten to clean ups
OK, finally all cleaned up. Whoever gets it gets it :-) |
@@ -30,6 +30,7 @@ jobs: | |||
--action_env=LD_LIBRARY_PATH \ | |||
--test_env=ENVOY_IP_TEST_VERSIONS=v4only \ | |||
--test_output=all \ | |||
--define envoy_mobile_listener=disabled \ |
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.
Should we flip this in the compile_time_options cc_build CI job?
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.
we could as well, but eventually I want to make it the default so I largely wanted it somewhere I could easily regression test and I was also worried about it not playing well with other options.
|
||
# Compute the copts needed for Envoy Mobile libraries that don't use Envoy's main library wrappers. | ||
def envoy_mobile_copts(repository): | ||
return envoy_select_admin_functionality(["-DENVOY_ADMIN_FUNCTIONALITY"], repository) + \ | ||
envoy_select_enable_http3(["-DENVOY_ENABLE_QUIC"], repository) + \ | ||
envoy_select_envoy_mobile_listener(["-DENVOY_MOBILE_ENABLE_LISTENER"], repository) + \ |
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 is fine here, but it could be in the envoy_copts
macro since we don't need this defined for swift_library
, objc_library
or the JNI cc_library
s.
Risk Level: low
Testing: common CI, manual testing
Docs Changes: n/a
Release Notes: n/a
part of envoyproxy/envoy-mobile#2711