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

Separate py_grpc_library and py_proto_library. #19822

Merged
merged 20 commits into from
Aug 7, 2019

Conversation

gnossen
Copy link
Contributor

@gnossen gnossen commented Aug 1, 2019

By popular demand, we'll now be offering separate py_grpc_library and
py_proto_library targets sharing the same interface as within google3.
This change necessitated some modifications to how we pull in our own
Python-level dependencies and how we make those available to folks
pulling in our project via Bazel.

There is now a grpc_python_deps() Bazel workspace rule that pulls in the
appropriate dependencies, which should be called from the client
project's WORKSPACE file. A test has been added to the bazel/test/
directory to verify that this behavior works as intended.

It's worth noting that the protobuf repository's usage of Starlark
bind() caused a great deal of trouble in ensuring that we could also
pull in six. So did bazelbuild/bazel#1550 which necessitated
the change in our dependency resolution methodology. Fingers crossed
that bazelbuild/bazel#1943 will be brought back to life in the near future.

This change also required a change in the way generated proto code is
imported in the channelz and health-check modules, as well as in their
associated tests. We were importing them two different ways, each
relative. This resulted in two different module objects being imported
into the process, which were incompatible. I am not sure exactly what
precipitated this behavior, as this should have been possible before
this PR. As a workaround, I am simply trying two different absolute
imports and using the one that works. This should function both inside
and outside of Bazel environments.

By popular demand, we'll now be offering separate py_grpc_library and
py_proto_library targets sharing the same interface as within google3.
This change necessitated some modifications to how we pull in our own
Python-level dependencies and how we make those available to those
pulling in our project via Bazel.

There is now a grpc_python_deps() Bazel workspace rule that pulls in the
appropriate dependencies, which should be called from the client
project's WORKSPACE file. A test has been added to the bazel/test/
directory to verify that this behavior works as intended.

It's worth noting that the protobuf repository's usage of Starlark
bind() caused a great deal of trouble in ensuring that we could also
pull in six.

This change also required a change in the way generated proto code is
imported in the channelz and health-check modules, as well as in their
associated tests. We were importing them two different ways, each
relative. This resulted in two different module objects being imported
into the process, which were incompatible. I am not sure exactly what
caused this behavior to begin, as this should have been possible before
this PR. As a workaround, I am simply trying two different absolute
imports and using the one that works. This should function both inside
and outside of Bazel environments.
@gnossen gnossen added area/build kind/enhancement lang/Python release notes: yes Indicates if PR needs to be in release notes labels Aug 1, 2019
@gnossen
Copy link
Contributor Author

gnossen commented Aug 1, 2019

Cross-reference: #19255

Copy link
Contributor

@lidizheng lidizheng left a comment

Choose a reason for hiding this comment

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

This is a really complicate work to resolve recent Bazel breakages. Thank you for doing this.
Just a few nits about if we can make this solution even better.

bazel/grpc_python_deps.bzl Show resolved Hide resolved
examples/python/debug/get_stats.py Show resolved Hide resolved
bazel/test/python_test_repo/WORKSPACE Show resolved Hide resolved
bazel/test/python_test_repo/tools/bazel Show resolved Hide resolved
bazel/python_rules.bzl Show resolved Hide resolved
bazel/python_rules.bzl Show resolved Hide resolved
Copy link
Contributor

@lidizheng lidizheng left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for trying out my suggestions!

bazel/python_rules.bzl Outdated Show resolved Hide resolved
@gnossen gnossen merged commit 835786a into grpc:master Aug 7, 2019
@gnossen gnossen deleted the py_proto_library branch August 7, 2019 23:33
@lock lock bot locked as resolved and limited conversation to collaborators Nov 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants