mobile: adding a config knob for building without listeners #25443
Conversation
alyssawilk
requested review from
jpsim,
Augustyniak,
RyanTheOptimist and
abeyad
as code owners
alyssawilk
force-pushed
the
listener
branch
3 times, most recently
from
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.
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.
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.
Why is this needed? We should already be getting these from envoy_cc_library, no?
There was a problem hiding this comment.
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.
Should we flip this in the compile_time_options cc_build CI job?
There was a problem hiding this comment.
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.
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_librarys.
Risk Level: low
Testing: common CI, manual testing
Docs Changes: n/a
Release Notes: n/a
part of envoyproxy/envoy-mobile#2711