Update protoc plugin to support Proto3 optional#884
Merged
stanley-cheung merged 4 commits intogrpc:masterfrom Jul 8, 2020
Merged
Update protoc plugin to support Proto3 optional#884stanley-cheung merged 4 commits intogrpc:masterfrom
stanley-cheung merged 4 commits intogrpc:masterfrom
Conversation
Yannic
reviewed
Jul 7, 2020
| rules_closure_toolchains() | ||
|
|
||
|
|
||
| load("@com_google_protobuf//:protobuf_deps.bzl", "protobuf_deps") |
Contributor
There was a problem hiding this comment.
Not sure if this and the updated bazel_skylib is necessary.
Also, I think this file needs formatting (CI passes though, maybe we're not checking that WORKSPACE is formatted?).
Collaborator
Author
There was a problem hiding this comment.
Let me double check. Thanks for checking in - was going to copy you for review.
I need to add the bazel_skylib because I got an error with something like common_settings.bzl not found (I lost that error message already).
Collaborator
Author
There was a problem hiding this comment.
And yes we probably only run buildifier on BUILD.bazel files but not WORKSPACE file. I will double check that too.
Collaborator
Author
There was a problem hiding this comment.
- Removed this
protobuf_depssection. Confirmed there's no need for that. - The error I got if I don't include this
bazel_skylibis
ERROR: /usr/local/google/home/stanleycheung/grpc-web/javascript/net/grpc/web/BUILD.bazel:4:1: error loading
package '@com_google_protobuf//': Unable to load file '@bazel_skylib//rules:common_settings.bzl': file doesn't exist
and referenced by '//javascript/net/grpc/web:protoc-gen-grpc-web'
ERROR: Analysis of target '//javascript/net/grpc/web:protoc-gen-grpc-web' failed; build aborted: error loading package
'@com_google_protobuf//': Unable to load file '@bazel_skylib//rules:common_settings.bzl': file doesn't exist
- Ran
buildifieragainst theWORKSPACEfile as well now.
@Yannic, PTAL, thanks!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proto3 introduced the
optionalsyntax to support field presence tracking in release 3.12.0. We need to add support in ourprotoc-gen-grpc-webprotoc plugin.See this doc for more details. Basically we need to add a function
GetSupportedFeatures()to our grpc protoc plugins to mark our compliance.See similar work being done in the main grpc repo: grpc/grpc#22998
This requires adding some bazel stuff to pin
@com_google_protobufto a higher version (i.e. at least3.12.0or above).