Skip to content

Conversation

@sergiitk
Copy link
Contributor

@sergiitk sergiitk commented Nov 11, 2023

py_proto_library provided by @com_google_protobuf//:protobuf.bzl has been deprecated for a while now:

This is provided for backwards compatibility only. Bazel 5.3 will
introduce support for py_proto_library, which should be used instead.
https://github.com/protocolbuffers/protobuf/blob/32af7d211b85f71920acdfa9b796db008f8c2a45/protobuf.bzl#L642-L644

However, native py_proto_library has never been provided, see bazelbuild/bazel#3935. Instead @rules_python//python:proto.bzl is recommended. I attempted switching to this library, but it's not compatible with @com_google_googleapis's py_proto_library targets, see #69. I found a hacky workaround by using cc_proto_library to generate python targets, but downstream integration into Envoy failed (envoyproxy/envoy#30159).

This PR migrates py_proto_library implementation to to @com_github_grpc_grpc. This implementation is used by @com_google_googleapis's, and, more importantly, uses bazel aspects. Which decouples cncf/xds and Envoy's dependencies from concrete upstream py_proto_library implementations.

This resulted in a significant code improvement of bazel/api_build_system.bzl:

  • No more custom @com_google_googleapis dependency mapping needed via py_proto_library rules override.
  • No more hardcoded dependencies _xds_py_proto_library - proto dependency tree is determined from proto_library definitions via Basel aspects.
  • No more EXTERNAL_PROTO_PY_BAZEL_DEP_MAP dependency map needed - for the same reason.

Similar work in Envoy: envoyproxy/envoy#30834.

@sergiitk sergiitk force-pushed the grpc-py_proto_library branch from 83c031e to 8751dcc Compare November 13, 2023 19:51
@sergiitk sergiitk marked this pull request as ready for review November 13, 2023 20:10
@sergiitk
Copy link
Contributor Author

@adisuissa @kyessenov this is ready for review.
@htuch - FYI this addresses a bunch of TODO's assigned to you.

@htuch
Copy link
Contributor

htuch commented Nov 14, 2023

Overall I like this approach a lot. @phlax @moderation are there any concerns here about taking the dependency from https://github.com/grpc/grpc? It's already a build dependency, and these are Bazel rules, so we should still be able to compile Envoy without linking Google gRPC, but just wanted to check.

@phlax
Copy link
Member

phlax commented Nov 14, 2023

overall looks fine, and appears address a number of issues - not sure iiuc re grpc dep, but from what i do seems ok

Copy link

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Overall LGTM, thanks for fixing this!
Will it be possible to have the same change in https://github.com/envoyproxy/envoy/tree/main/api/bazel ?

@sergiitk
Copy link
Contributor Author

@adisuissa yes - I have a PR in progress: envoyproxy/envoy#30834.
Was out yesterday, planning to wrap it up today.

@moderation
Copy link

LGTM

@sergiitk
Copy link
Contributor Author

sergiitk commented Nov 15, 2023

Looks like to make it work nicely in Envoy, we'd have to merge this first. Otherwise, bazel complains about missing _genproto targets (generated by @com_google_protobuf//:protobuf.bzl's py_proto_library), because this cncf/xds used rules_override for googleapis before this PR.

ERROR: /private/var/tmp/_bazel_sergiitk/e60ee3f6402a793527d720007beb00cb/external/com_github_cncf_xds/xds/annotations/v3/BUILD:5:18: no such target '@com_google_googleapis//google/api:httpbody_py_proto_genproto': target 'httpbody_py_proto_genproto' not declared in package 'google/api' defined by /private/var/tmp/_bazel_sergiitk/e60ee3f6402a793527d720007beb00cb/external/com_google_googleapis/google/api/BUILD.bazel (Tip: use query "@com_google_googleapis//google/api:*" to see all the targets in that package) and referenced by '@com_github_cncf_xds//xds/annotations/v3:pkg_py_proto_genproto'

It might be possible to solve this in Envoy without merging this, but it'd be a throw-away work. Let's get this one in first.

Copy link

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!
Seems that the dependency shepherds are ok with the protoc dependency, so I'm going to approve this PR.
LGTM!

@htuch htuch merged commit 3a472e5 into cncf:main Nov 16, 2023
@sergiitk sergiitk deleted the grpc-py_proto_library branch November 16, 2023 18:57
@sergiitk sergiitk restored the grpc-py_proto_library branch November 16, 2023 18:57
@sergiitk sergiitk deleted the grpc-py_proto_library branch November 20, 2023 19:02
@alexeagle
Copy link

@sergiitk thanks for this example - but per https://groups.google.com/g/grpc-io/c/44OcoVSmeQk?pli=1 it seems like this is using something deprecated. I'd appreciate any pointers to an "officially recommended" way to generate service stubs with Bazel, on behalf of a lot of the community looking for it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants