pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.27k stars 626 forks source link

Support more Python Protobuf plugins #20383

Closed isra17 closed 8 months ago

isra17 commented 8 months ago

Is your feature request related to a problem? Please describe.

The current python protobuf codegen backends allow pants to generate python stubs using grpcio. However, there are other available python package such as grpclib or betterproto that provides protoc plugins.

Describe the solution you'd like

It could be nice to be able to define the python plugin used to generate the python stubs.

Maybe something like:

protobuf_sources(
    name="protos",
    grpc=True,
    python_protoc_plugin="grpclib"
)

or maybe even make it a list so many protoc plugins can be registered:

protobuf_sources(
    name="protos",
    grpc=True,
    python_protoc_plugins=["grpclib", "mypy-protobuf"]
)

I'm not very knowledgeable of pants internal, so I'm not sure if there would be a clean way to make this part extendable by new rules (using the enum system?).

Additional context I have a basic working POC here: https://github.com/pantsbuild/pants/compare/main...isra17:pants:protobuf-grpclib?expand=1

Is that something you would want to see contribution for? If so, what about my current approach? If I add the tests, would you take this?

isra17 commented 8 months ago

I actually published my plugin as a working standalone plugin: https://github.com/Flared/flare-pants-plugins/tree/main/pants-plugins/protobuf/flare_protobuf_plugin

benjyw commented 8 months ago

Interesting! Yeah, I'd happily accept a patch to support grpclib, in the same way we currently support mypy-protobuf (the other example of "protoc plugin embedded in a python distribution").

However, do you think it's necessary to have this settable per-target? Or would a global setting as we currently have for mypy-protobuf suffice?

isra17 commented 8 months ago

However, do you think it's necessary to have this settable per-target? Or would a global setting as we currently have for mypy-protobuf suffice?

I think conceptually makes sense to make it target specific since protoc is run for each individual target. On the other hand, I see the benefits of keeping the current approach of mypy-protobuf and it keep the configuration simpler as well. I will update my fork.

benjyw commented 8 months ago

It does make conceptual sense but there is the counterweight argument of keeping things simple where we can...

benjyw commented 8 months ago

Fixed in #20387 , thanks @isra17 !