Make using varients of protobuf easier. #17
Conversation
Signed-off-by: John Plevyak <[email protected]>
|
This makes it possible to compile with full, lite or no protobuf support. |
Signed-off-by: John Plevyak <[email protected]>
| // NOLINT(namespace-envoy) | ||
|
|
||
| #pragma once | ||
| #define PROXY_WASM_PROTOBUF_FULL 1 |
There was a problem hiding this comment.
This is defined twice, once in copts and in once the header file.
There was a problem hiding this comment.
Yes, yes I did. This is unfortunately AFAICT it is required because bazel doesn't seem to work the way one would reasonably expect it to work. The copts doesn't seem to be contagious to targets which depend on a a header from the library which defines the copt. Consequently in order to get the #define to actually impact the header when it is included in another target, ,e.g. gcpc_call_cpp.cc in test/extensions/filters/http/wasm/test_data, on needs to add it to that target or conversely one could use a header which includes it such as here.
That said, my understanding of bazel is perhaps not what it should be and if you have a better solution I am all ears.
There was a problem hiding this comment.
Yeah, copts are not propagated "upwards", so having this defined in headers is fine... With that said, is there any reason to have it defined in copts as well?
There was a problem hiding this comment.
I don't want to add the copts in the wasm_cc_binary target because that is messy. This way I can conceal that mess in the proxy-wasm-cpp-sdk library proper.
There was a problem hiding this comment.
You don't need copts in either place, just having this header should be enough.
There was a problem hiding this comment.
Erm, I meant that you should remove define from copts in :proxy_wasm_intrinsics_lite and : proxy_wasm_intrinsics_full targets.
There was a problem hiding this comment.
Then I have to add it at the higher level in the wasm_cc_binary targets which is messy and inconvenient. I can't include a different header in the proxy_wasm_intrinsics.cc file without having a #define so I need two targets for it with different copts. If you have a way to get this to work I would like to here it but I tried several ways and this was the best way I could find to get bazel to do what I want. I suppose I could build wasm_cc_binary_full and wasm_cc_binary_lite, but that doesn't seem to be an improvement either.
There was a problem hiding this comment.
wasm_cc_binary_full and wasm_cc_binary_lite sound messy, let's not do that.
I still think that copts is not necessary, but it's also not a blocker, so feel free to merge this as-is.
| // NOLINT(namespace-envoy) | ||
|
|
||
| #pragma once | ||
| #define PROXY_WASM_PROTOBUF_LITE 1 |
| @@ -335,6 +335,7 @@ class RootContext : public ContextBase { | |||
| uint32_t timeout_milliseconds, HttpCallCallback callback); | |||
| // NB: the message is the response if status == OK and an error message | |||
| // otherwise. Returns false on setup error. | |||
| #ifdef PROXY_WASM_PROTOBUF | |||
| WasmResult grpcSimpleCall(StringView service, StringView service_name, StringView method_name, | |||
There was a problem hiding this comment.
What about other gRPC functions? i.e. open_stream, send_message, cancel, and close? Shouldn't we be disabling them as well?
There was a problem hiding this comment.
We could. I was hoping to talk to others who might be interested in using say flatbuffers with gRPC. They might have alternative solutions for the functions which depend directly on MessageLite.
There was a problem hiding this comment.
I'd err on the of side having good support for features that we support, and not having support for features that we don't officially support, i.e. I'm 100% for adding back other functions iff someone adds support for gRPC with flatbuffers, but since we don't have it yet, I think we should disable reminding gRPC functions when compiled without protobuf.
There was a problem hiding this comment.
There is a bug preventing me from fixing this: envoyproxy/envoy-wasm#508
There was a problem hiding this comment.
gRPC doesn't have to be a protobuf payload, any buffer (string/string_view) should work and Envoy has support of that. But we should turn off those method with protobuf in its signature.
There was a problem hiding this comment.
I turned the protobuf ones off, but I didn't provide the "string/string_vew" alternative. WDYT @PiotrSikora ? Should I add those.
There was a problem hiding this comment.
We should probably add those (always, not only when building without protobuf), but it's fine to do that in a follow-up PR.
|
Any updates? |
| @@ -335,6 +335,7 @@ class RootContext : public ContextBase { | |||
| uint32_t timeout_milliseconds, HttpCallCallback callback); | |||
| // NB: the message is the response if status == OK and an error message | |||
| // otherwise. Returns false on setup error. | |||
| #ifdef PROXY_WASM_PROTOBUF | |||
| WasmResult grpcSimpleCall(StringView service, StringView service_name, StringView method_name, | |||
There was a problem hiding this comment.
We should probably add those (always, not only when building without protobuf), but it's fine to do that in a follow-up PR.
| // NOLINT(namespace-envoy) | ||
|
|
||
| #pragma once | ||
| #define PROXY_WASM_PROTOBUF_FULL 1 |
There was a problem hiding this comment.
wasm_cc_binary_full and wasm_cc_binary_lite sound messy, let's not do that.
I still think that copts is not necessary, but it's also not a blocker, so feel free to merge this as-is.
Signed-off-by: John Plevyak [email protected]