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.

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

Update grpc protoc plugin to be compliant of proto3 field presence #22998

Merged

Conversation

stanley-cheung
Copy link
Contributor

@stanley-cheung stanley-cheung commented May 20, 2020

Fixes #23030

Proto3 introduced the optional syntax to support field presence tracking in release 3.12.0. We need to add support in our grpc protoc plugin.

See this doc for more details. Basically we need to add a function GetSupportedFeatures() to our grpc protoc plugins to mark our compliance.

This PR also has to update the third_party/protobuf link to the v3.12.2 commit.

Internal bug: b/156243646

A couple of recent issues to keep an eye on in terms of interactions with this PR:

Current blockers:

@stanley-cheung
Copy link
Contributor Author

stanley-cheung commented May 20, 2020

There are some Objective-C failures, that may be due to a new change in Protobuf in 3.12 protocolbuffers/protobuf#7173

The error is

ERROR: /Volumes/BuildData/tmpfs/src/github/grpc/workspace_objc_macos_opt_native/src/objective-c/BUILD:240:1: C++ compilation of rule '//src/objective-c:proto_objc_rpc_internal_testing' failed (Exit 1) wrapped_clang failed: error executing command external/local_config_cc/wrapped_clang -arch x86_64 '-D_FORTIFY_SOURCE=1' -fstack-protector -fcolor-diagnostics -Wall -Wthread-safety -Wself-assign -fno-omit-frame-pointer -O0 -DDEBUG ... (remaining 71 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
In file included from src/objective-c/ProtoRPC/ProtoRPCLegacy.m:26:
In file included from external/com_google_protobuf/objectivec/GPBProtocolBuffers.h:44:
external/com_google_protobuf/objectivec/GPBWellKnownTypes.h:44:10: fatal error: 'GPBAny.pbobjc.h' file not found
 #import "GPBAny.pbobjc.h"
         ^~~~~~~~~~~~~~~~~
1 error generated.
Target //src/objective-c/examples:Sample failed to build

https://source.cloud.google.com/results/invocations/59fa0d35-8ca1-48b1-8ea0-d6230b7cc42a/targets/grpc%2Fcore%2Fpull_request%2Fmacos%2Fgrpc_basictests_objc_examples/log

Logged an issue to protobuf: protocolbuffers/protobuf#7532 to see if it's an issue with their :protobuf_objc target in the 3.12.x branch.

[Edit:] Fixed by protocolbuffers/protobuf#7538

@stanley-cheung
Copy link
Contributor Author

stanley-cheung commented May 20, 2020

There seems to be an issue with Python27 + Windows build after the upgrade to Protobuf 3.12 too

third_party\protobuf\src/google/protobuf/port_def.inc:457:54: error: expected primary-expression before ')' token

   PROTOBUF_EXPORT_TEMPLATE_##which##_##style(export, )

...

FAILED: build_artifact.python_windows_x64_Python27

Seems to be caused by this PR: protocolbuffers/protobuf#7344, and originally protocolbuffers/protobuf#6535

Log: https://source.cloud.google.com/results/invocations/b95cf1f2-191b-4fdb-8554-413524992d91/targets/grpc%2Fcore%2Fpull_request%2Fwindows%2Fgrpc_build_artifacts/log

Logged a separate issue #23011 to track this issue. Current idea is that mingw is being used to compile for Python 2.7, instead of msvc for Python 3. We are trying to see if there's a way to update mingw to fix this particular issue.

[Edit:] Fixed by protocolbuffers/protobuf#7539

@jtattermusch
Copy link
Contributor

jtattermusch commented May 20, 2020

@srini100 note that Stanley is already updating protobuf submodule as part of this PR.

@stanley-cheung
Copy link
Contributor Author

This is looking better. Both the Objective-C and the Python issues have found a fix. We just need protobuf team to merge in my fixes and tag a new release. Will keep monitoring that.

@stanley-cheung stanley-cheung force-pushed the proto3-presence-protoc-plugin branch 2 times, most recently from 0dc7df9 to b8fd8c7 Compare May 21, 2020 21:26
@stanley-cheung
Copy link
Contributor Author

All the issues are resolved as of this point, after protobuf team merged in my two fixes. All the tests are passing except for this known flake #23027.

I will wait for the protobuf team to cut a new release 3.12.2, which will possibly happen after the long weekend.

@stanley-cheung stanley-cheung force-pushed the proto3-presence-protoc-plugin branch 2 times, most recently from 5ca45ff to a2cba00 Compare May 27, 2020 17:35
Copy link
Contributor

@srini100 srini100 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM.

@stanley-cheung stanley-cheung merged commit 4b2cec8 into grpc:master May 28, 2020
@stanley-cheung stanley-cheung deleted the proto3-presence-protoc-plugin branch May 28, 2020 17:35
@karthikravis
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for proto3 field presence
4 participants