Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make using varients of protobuf easier. #17

Merged
merged 2 commits into from
May 18, 2020
Merged

Make using varients of protobuf easier. #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a062854
Make using varients of protobuf easier.
jplevyak May 4, 2020
db6f3de
Make guards consistent.
jplevyak May 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 17 additions & 17 deletions BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ cc_library(
],
)

# Targets for building base library
cc_library(
name = "proxy_wasm_intrinsics",
srcs = [
Expand All @@ -29,10 +28,6 @@ cc_library(
"proxy_wasm_intrinsics.h",
],
visibility = ["//visibility:public"],
deps = [
":proxy_wasm_intrinsics_cc_proto",
"@com_google_protobuf//:protobuf",
],
)

cc_proto_library(
Expand All @@ -51,27 +46,32 @@ proto_library(
],
)

# Targets for build lite library
# include lite protobuf support
cc_library(
name = "proxy_wasm_intrinsics_lite",
srcs = [
"proxy_wasm_intrinsics.cc",
],
hdrs = [
"proxy_wasm_api.h",
"proxy_wasm_common.h",
"proxy_wasm_enums.h",
"proxy_wasm_externs.h",
"proxy_wasm_intrinsics.h",
],
copts = ["-DPROXY_WASM_PROTOBUF_LITE"],
hdrs = ["proxy_wasm_intrinsics_lite.h"],
copts = ["-DPROXY_WASM_PROTOBUF_LITE=1"],
visibility = ["//visibility:public"],
deps = [
":proxy_wasm_intrinsics",
":proxy_wasm_intrinsics_lite_cc_proto",
"@com_google_protobuf//:protobuf",
],
)

# include full protobuf support
cc_library(
name = "proxy_wasm_intrinsics_full",
hdrs = ["proxy_wasm_intrinsics_full.h"],
copts = ["-DPROXY_WASM_PROTOBUF_FULL=1"],
visibility = ["//visibility:public"],
deps = [
":proxy_wasm_intrinsics",
":proxy_wasm_intrinsics_cc_proto",
"@com_google_protobuf//:protobuf",
],
)

cc_proto_library(
name = "proxy_wasm_intrinsics_lite_cc_proto",
deps = [":proxy_wasm_intrinsics_lite_proto"],
Expand Down
8 changes: 8 additions & 0 deletions proxy_wasm_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about other gRPC functions? i.e. open_stream, send_message, cancel, and close? Shouldn't we be disabling them as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a bug preventing me from fixing this: envoyproxy/envoy-wasm#508

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I turned the protobuf ones off, but I didn't provide the "string/string_vew" alternative. WDYT @PiotrSikora ? Should I add those.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add those (always, not only when building without protobuf), but it's fine to do that in a follow-up PR.

const HeaderStringPairs &initial_metadata,
const google::protobuf::MessageLite &request,
Expand All @@ -361,6 +362,7 @@ class RootContext : public ContextBase {
const google::protobuf::MessageLite &request,
uint32_t timeout_milliseconds,
std::unique_ptr<GrpcCallHandlerBase> handler);
#endif
// Returns false on setup error.
WasmResult grpcStreamHandler(StringView service, StringView service_name, StringView method_name,
const HeaderStringPairs &initial_metadata,
Expand Down Expand Up @@ -1183,6 +1185,7 @@ inline Histogram<Tags...> *Histogram<Tags...>::New(StringView name,
std::vector<MetricTag>({toMetricTag(descriptors)...}));
}

#ifdef PROXY_WASM_PROTOBUF
inline WasmResult grpcCall(StringView service, StringView service_name, StringView method_name,
const HeaderStringPairs &initial_metadata,
const google::protobuf::MessageLite &request,
Expand All @@ -1197,6 +1200,7 @@ inline WasmResult grpcCall(StringView service, StringView service_name, StringVi
serialized_request.data(), serialized_request.size(), timeout_milliseconds,
token_ptr);
}
#endif

inline WasmResult grpcStream(StringView service, StringView service_name, StringView method_name,
const HeaderStringPairs &initial_metadata, uint32_t *token_ptr) {
Expand Down Expand Up @@ -1238,6 +1242,7 @@ inline void RootContext::onHttpCallResponse(uint32_t token, uint32_t headers, si
}
}

#ifdef PROXY_WASM_PROTOBUF
inline WasmResult RootContext::grpcSimpleCall(StringView service, StringView service_name,
StringView method_name,
const HeaderStringPairs &initial_metadata,
Expand All @@ -1252,6 +1257,7 @@ inline WasmResult RootContext::grpcSimpleCall(StringView service, StringView ser
}
return result;
}
#endif

inline void GrpcCallHandlerBase::cancel() {
grpcCancel(token_);
Expand Down Expand Up @@ -1371,6 +1377,7 @@ inline void RootContext::onGrpcClose(uint32_t token, GrpcStatus status) {
}
}

#ifdef PROXY_WASM_PROTOBUF
inline WasmResult RootContext::grpcCallHandler(StringView service, StringView service_name,
StringView method_name,
const HeaderStringPairs &initial_metadata,
Expand All @@ -1387,6 +1394,7 @@ inline WasmResult RootContext::grpcCallHandler(StringView service, StringView se
}
return result;
}
#endif

inline WasmResult RootContext::grpcStreamHandler(StringView service, StringView service_name,
StringView method_name,
Expand Down
7 changes: 5 additions & 2 deletions proxy_wasm_intrinsics.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,12 @@ template <typename T> using Optional = std::optional<T>;
#include "proxy_wasm_common.h"
#include "proxy_wasm_enums.h"
#include "proxy_wasm_externs.h"
#ifndef PROXY_WASM_PROTOBUF_LITE
#ifdef PROXY_WASM_PROTOBUF_FULL
#define PROXY_WASM_PROTOBUF 1
#include "proxy_wasm_intrinsics.pb.h"
#else
#endif
#ifdef PROXY_WASM_PROTOBUF_LITE
#define PROXY_WASM_PROTOBUF 1
#include "proxy_wasm_intrinsics_lite.pb.h"
#endif
#include "proxy_wasm_api.h"
25 changes: 25 additions & 0 deletions proxy_wasm_intrinsics_full.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright 2016-2019 Envoy Project Authors
* Copyright 2020 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/*
* API Available to WASM modules.
*/
// NOLINT(namespace-envoy)

#pragma once
#define PROXY_WASM_PROTOBUF_FULL 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is defined twice, once in copts and in once the header file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need copts in either place, just having this header should be enough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Erm, I meant that you should remove define from copts in :proxy_wasm_intrinsics_lite and : proxy_wasm_intrinsics_full targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#include "proxy_wasm_intrinsics.h"
25 changes: 25 additions & 0 deletions proxy_wasm_intrinsics_lite.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright 2016-2019 Envoy Project Authors
* Copyright 2020 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/*
* API Available to WASM modules.
*/
// NOLINT(namespace-envoy)

#pragma once
#define PROXY_WASM_PROTOBUF_LITE 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same here)

#include "proxy_wasm_intrinsics.h"