-
-
Notifications
You must be signed in to change notification settings - Fork 616
feat(gazelle): Gazelle plugin generates py_proto_library #3057
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
feat(gazelle): Gazelle plugin generates py_proto_library #3057
Conversation
…of scaffolding & tests
… the singular input proto
…opy is deprecated
…lign with the other directives
…ic to implement this
gazelle/python/testdata/directive_python_generate_proto/test.yaml
Outdated
Show resolved
Hide resolved
...lle/python/testdata/directive_python_generate_proto/test7_removes_when_unnecessary/BUILD.out
Show resolved
Hide resolved
gazelle/python/testdata/directive_python_generate_proto/test1_default_with_proto/foo.proto
Outdated
Show resolved
Hide resolved
While writing a test covering the bzlmod integration I ran into a small blocker, so I PRed a fix for that separately: #3060 |
Yep! Should've made this clear earlier. You can reproduce the errors I ran into by checking out 086c6aa (the last commit before I undid the upgrade & removed the tests), and undoing the upgrade:
which reads to me like Gazelle doesn't know about bzlmod, and is just looking for the WORKSPACE. |
@dougthor42 one change I just made -- I double-checked how the golang & java extensions work, and they both generate e.g. I think that makes sense? And it'll make it just a tiny bit easier to resolve imports down the line. |
The convention is to name the `py_proto_library` rule `foo_py_pb2`, when it is wrapping `proto_library` rule `foo_proto`
IIRC this comes from the fact that `protoc` makes `foo_pb2.py` files and they're imported in Python with `import foo_pb2`.
|
Ah, dang it. I knew that came from somewhere. It is... an unfortunate naming convention, I'll venture? I can put it back, but it's weird because a) it doesn't match other languages, and b) it doesn't even fit the actual naming of the generated file, which is like Let me know if you'd prefer I revert it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes I see the error and repro'd it locally. Turns out you still need a dummy (empty) WORKSPACE file in the test dir. You can re-add those test cases if you'd like, as long as you're not bumping versions. With the empty WORKSPACE file, things pass for me locally so hopefully they pass CI too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it, thank you so much! 🎉
@dougthor42 Thanks for the review -- really excited to get this in! I just noticed, as part of doing #3081, that I'm technically not following the convention you linked. I'm generating |
…y_proto_library` naming (#3093) Closes #3081. This adds support in the Gazelle plugin for controlling how the generated `py_proto_library` rules are named; support for these was originally added in #3057. We do this via a new Gazelle directive, `python_proto_naming_convention`, which is similar to `python_library_naming_convention` and the like, except it interpolates `$proto_name$`, which is the `proto_library` rule minus any trailing `_proto`. We default to `$proto_name$_py_pb2`. For instance, for a `proto_library` named `foo_proto`, the default value would generate `foo_py_pb2`, aligning with [the convention stated in the Bazel docs.](https://bazel.build/reference/be/protocol-buffer#py_proto_library)
…ation and usage (#3132) Part of #3082 2nd of probably 5 PRs. + Migrate installation and usage info from `gazelle/README.md` to `gazelle/docs/installation_and_usage.md` + Slight rewording and reformatting of the `Example` section. + Reorganized and modernized the `MODULE.bazel` and `BUILD.bazel` examples: + less details on rules_python as that's available in other docs + default to gazelle multilang support now that #3057 is merged. + Mechanical updates: + Wrap at ~80 chars + Use MyST directives and roles.
Fixes #2994.
Please go over this with a fine-toothed comb! This is my first contribution to
rules_python
/ the gazelle plugin, and while I've worked in Gazelle before, I'm pretty unfamiliar with the Python plugin's architecture.This adds support in the Gazelle plugin for generating
py_proto_library
rules automatically, if there are anyproto_library
rules detected in a given package. We do this via a new Gazelle directive,python_generate_proto
, which defaults totrue
, and controls whether these rules are generated.See the tests in
testdata/directive_python_generate_proto
for examples.By default, we source the
py_proto_library
rule from the@protobuf
repository. I think this the intended long-term home of the rule? Users are expected to usegazelle:map_kind
to change this if need be.I haven't done anything here to support resolution of imports of
py_proto_library
. I think this is worth landing first, to save folks from having to maintain these by hand. But this should lay the foundation for resolving that in #1703.