Support protobuf 3.22 or upper#2163
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2163 +/- ##
==========================================
- Coverage 87.62% 87.52% -0.10%
==========================================
Files 198 199 +1
Lines 5856 5981 +125
==========================================
+ Hits 5131 5234 +103
- Misses 725 747 +22
|
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
… gRPC Signed-off-by: owent <admin@owent.net>
904b15c to
f95fd57
Compare
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
ce406cb to
0dc7575
Compare
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
be9b3f9 to
6fa4e95
Compare
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
a45451a to
8956193
Compare
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
…erent cmake CONFIG Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
|
There seems some syntax error for modern protobuf without abseil. Is this expected, or modern protobuf does require abseil? |
| namespace | ||
| { | ||
|
|
||
| using Base64EscapeChars = const unsigned char[64]; |
There was a problem hiding this comment.
should these methods be moved to https://github.com/open-telemetry/opentelemetry-cpp/blob/main/api/include/opentelemetry/common/string_util.h ?
There was a problem hiding this comment.
There are a lot of codes here, do you think it's better to move it into sdk?
There was a problem hiding this comment.
Yeah good point, not required. We can move it later if need to share with other components. Thanks.
There was a problem hiding this comment.
base64 algorithm is moved into sdk now.
There was a problem hiding this comment.
Really sorry if my comment above was not clear. I did realize that this will bloat the sdk, and we should move it only when we need to share it with other components.
I just tested this CI job and find protobuf can not work when both external and internal abseil-cpp exists.Because they share the same header paths and namespace and will have conflicts with each other. @lalitb This PR is ready to be reviewed again and thanks, |
Thanks for your analysis @owent. This would be helpful. |
@owent - Seems you missed my comment here - #2163 (comment) . Actually, I didn't ask for moving base64 from otlp to sdk in my subsequent comments. But if you want, we can keep it in sdk for now, and have a separate PR to move back to otlp. Rest of the PR looks good. |
Thanks. I can move it back to otlp exporter later in another PR. |
Fixes #2095
Changes
Protobuf depends on abseil-cpp 20230125 from v22/v4.22.
It removed
stringpiece.h,google::protobuf::Base64Escapeand useabsl::Base64Escapefor alternative.We have encountered multiple problems related to the usage of CMake packages in the past (#705 #1359). As a solution, I have added a patch scriptcmake/patch-imported-config.cmakethat allows users to use prebuilt packages with different CONFIG settings when building otel-cpp.BTW: gRPC v1.55.0 also depend the new protobuf and abseil-cpp, so I also test the new gRPC version in the new CI job.
cmake_modern_protobuf_grpc_test) to test building with new protobuf and gRPC.Patch imported target in cmake CONFIG packages to enable users to use prebuilt packages with different CONFIG settings.--experimental_allow_proto3_optionalwhen running protoc when we use protobuf 3.16 or upper. (It's not experimental any more)For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes