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 protoc plugin to support Proto3 optional #884

Merged
merged 4 commits into from
Jul 8, 2020

Conversation

stanley-cheung
Copy link
Collaborator

Proto3 introduced the optional syntax to support field presence tracking in release 3.12.0. We need to add support in our protoc-gen-grpc-web 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.

See similar work being done in the main grpc repo: grpc/grpc#22998

This requires adding some bazel stuff to pin @com_google_protobuf to a higher version (i.e. at least 3.12.0 or above).

WORKSPACE Outdated
load("@io_bazel_rules_closure//closure:repositories.bzl", "rules_closure_dependencies", "rules_closure_toolchains")

rules_closure_dependencies()

rules_closure_toolchains()


load("@com_google_protobuf//:protobuf_deps.bzl", "protobuf_deps")
Copy link
Contributor

Choose a reason for hiding this comment

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

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?).

Copy link
Collaborator Author

@stanley-cheung stanley-cheung Jul 7, 2020

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And yes we probably only run buildifier on BUILD.bazel files but not WORKSPACE file. I will double check that too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • Removed this protobuf_deps section. Confirmed there's no need for that.
  • The error I got if I don't include this bazel_skylib is
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 buildifier against the WORKSPACE file as well now.

@Yannic, PTAL, thanks!

Copy link
Contributor

@Yannic Yannic left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

2 participants