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.

Sign up for GitHub

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

Jump to bottom

Add Aspects to Bazel py_proto_library and py_grpc_library Rules #27275

Merged
merged 6 commits into from
Oct 21, 2021
Merged

Add Aspects to Bazel py_proto_library and py_grpc_library Rules #27275

merged 6 commits into from
Oct 21, 2021

Conversation

mbeards
Copy link
Contributor

@mbeards mbeards commented Sep 7, 2021 •
edited

py_proto_library is reworked to propagate dependencies to consumers as described in gRFC L86.

@gnossen gnossen mentioned this pull request Sep 21, 2021
Closed
@mbeards mbeards changed the title Py proto library aspect Implement aspect-based Python Protobuf Bazel logic Oct 21, 2021
@mbeards
Move most py_proto_library macro contents into rule.
cddf2da 
The _generate_pb2_src rule is modified to produce a valid PyInfo
provider that can be depended on directly by py_.* rules.

Keeping an intermediate macro in place increases complexity without
adding any real value.
@mbeards
Move most py_grpc_library macro contents into rule.
fca06bd 
The _generate_pb2_grpc_src rule is modified to produce a valid PyInfo
provider that can be depended on directly by py_.* rules.

Keeping an intermediate macro in place increase complexity without
adding any real value.
@mbeards mbeards marked this pull request as ready for review October 21, 2021 14:17
gnossen
gnossen approved these changes Oct 21, 2021
View reviewed changes
Copy link
Contributor

@gnossen gnossen left a comment

Choose a reason for hiding this comment

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

Basically just nits. I'm excited to see this landing. Thanks for the hard work!

bazel/test/python_test_repo/import_from_proto_library_package.py Outdated Show resolved Hide resolved
bazel/test/python_test_repo/BUILD Show resolved Hide resolved
bazel/test/python_test_repo/BUILD Outdated Show resolved Hide resolved
bazel/test/python_test_repo/BUILD Outdated Show resolved Hide resolved
bazel/test/python_test_repo/in_subpackage/BUILD Outdated Show resolved Hide resolved
src/python/grpcio_channelz/grpc_channelz/v1/BUILD.bazel Show resolved Hide resolved
bazel/python_rules.bzl Outdated Show resolved Hide resolved
bazel/python_rules.bzl Outdated Show resolved Hide resolved
bazel/python_rules.bzl Outdated Show resolved Hide resolved
@gnossen
Copy link
Contributor

gnossen commented Oct 21, 2021

You'll want to run this script to format your code.

@mbeards mbeards force-pushed the py_proto_library_aspect branch 3 times, most recently from e5d45d9 to 9677401 Compare October 21, 2021 19:25
@mbeards
Remove support for additional plugins.
1f6ea5e 
It's easy enough for a plugin author to build their own rule-set, and it
adds extra unused (and untested) complexity to gRPC's rules.
@mbeards mbeards force-pushed the py_proto_library_aspect branch 2 times, most recently from 622f888 to 9dea2d1 Compare October 21, 2021 19:39
@mbeards
Copy link
Contributor Author

mbeards commented Oct 21, 2021

You'll want to run this script to format your code.

Were there any formatting issues? The script didn't make any changes.

@gnossen
Copy link
Contributor

gnossen commented Oct 21, 2021

The previous run has some changes, but it looks like they might be obsolete now:

2021-10-21 17:22:33,174 ./bazel/test/python_test_repo/in_subpackage/BUILD:
--- ./bazel/test/python_test_repo/in_subpackage/BUILD	2021-10-21 17:22:26.439944245 +0000
+++ /tmp/buildifier-tmp-915974610	2021-10-21 17:22:33.144476524 +0000
@@ -16,8 +16,10 @@

 load("@rules_proto//proto:defs.bzl", "proto_library")

-package(default_testonly = 1,
-        default_visibility=["//visibility:public"],)
+package(
+    default_testonly = 1,
+    default_visibility = ["//visibility:public"],
+)

 proto_library(
     name = "subpackage_proto",
exit status 1
==========BUILDIFIER CHECK FAILED==========
Please try using the following script to fix automatically:

    tools/distrib/buildifier_format_code.sh

@gnossen gnossen changed the title Implement aspect-based Python Protobuf Bazel logic Add Aspects to Bazel py_proto_library and py_grpc_library Rules Oct 21, 2021
@gnossen
Copy link
Contributor

gnossen commented Oct 21, 2021

Known failures:

@gnossen
Copy link
Contributor

gnossen commented Oct 21, 2021

Going ahead and merging since the final pending test (Artifact Build MacOS) should be unaffected by this PR.

@gnossen gnossen merged commit e970b8f into grpc:master Oct 21, 2021
ctiller added a commit that referenced this pull request Oct 21, 2021
@ctiller
Revert "Add Aspects to Bazel py_proto_library and py_grpc_library Rul…
b97ccd7 
…es (#27275)"

This reverts commit e970b8f.
@ctiller ctiller mentioned this pull request Oct 21, 2021
Merged
ctiller added a commit that referenced this pull request Oct 22, 2021
@ctiller
Revert "Add Aspects to Bazel py_proto_library and py_grpc_library Rul…
763e5b4 
…es (#27275)" (#27805)

This reverts commit e970b8f.
gnossen added a commit to gnossen/grpc that referenced this pull request Oct 22, 2021
@gnossen
Revert "Revert "Add Aspects to Bazel py_proto_library and py_grpc_lib…
97eea36 
…rary Rules (grpc#27275)" (grpc#27805)"

This reverts commit 763e5b4.
@gnossen gnossen mentioned this pull request Oct 22, 2021
Merged
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Oct 22, 2021
gnossen added a commit that referenced this pull request Oct 22, 2021
@gnossen
Reland #27275 (#27806)
eab375e 
* Revert "Revert "Add Aspects to Bazel py_proto_library and py_grpc_library Rules (#27275)" (#27805)"

This reverts commit 763e5b4.

* Remove unused symbol
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnossen gnossen gnossen approved these changes

@ctiller ctiller Awaiting requested review from ctiller

@jtattermusch jtattermusch Awaiting requested review from jtattermusch

@nicolasnoble nicolasnoble Awaiting requested review from nicolasnoble

@veblush veblush Awaiting requested review from veblush veblush is a code owner

Assignees

@gnossen gnossen

Labels
disposition/requires reporter action imported Specifies if the PR has been imported to the internal repository infra/Bazel lang/Python release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mbeards @gnossen @grpc-kokoro