Repository navigation
Add buf as an alternate Python protobuf code generator - #23343
Conversation
|
Thanks for the PR! Recently the maintainers have been discussing that new backends should first start out as published plugins in a user's repo, so that the community can mess around and flesh out the APIs and utility, before it gets merged into Pants directly and becomes more relied on. I was just commenting on another PR yesterday that I need to add docs to this effect. A couple simple examples of that are here: This would also let us extract bits and pieces at a time that we know are more API stable (e.g. one there are some lint users who are happy, then pull that in, etc). |
|
Thanks for the quick response @sureshjoshi! In this case, This PR adds on the Would it still make sense to start as a plug in in this case? |
8af19b7 to
fc558dd
Compare
Ahh, okay okay, I thought I'll just point out that this will take some time to review, and just at a glance, it feels like a lot of code for what it's doing - which is common for Claude driven PRs, FYI. So, thanks for the PR, and please have some patience on the review. |
|
Thanks @sureshjoshi! Quite a few of the changes are just mechanical -- moving files related to buf linting/formatting that were under a subdirectory that wouldn't make sense if buf also does code generation. I could split those into a separate PR but I didn't since there wouldn't be a justification for moving those files without adding the new functionality. Let me know if you have a preference. I can also just put them in a different commit within this PR if it makes review easier. |
|
This would be awesome to have. I'll check this out and give it a try on our codebase. |
Several standalone smaller, simpler PRs will always make for an easier review :) |
Sounds good. I'll make a couple more PRs and then rebase to simplify this one. Here's the first to bump the |
|
Having some trouble using this with the Python betterproto2 compiler. If my understanding of this plugin is correct, it generates an individual I think the contents of each |
|
Installing protoc is increasingly annoying, and I guess What is the status? I do think multiple smaller PRs, or at least breaking out the fully mechanical parts, would help get this merged sooner. |
|
I put the mechanical changes into #23382 and then will rebase this PR to simplify it after that lands. I'll also address @edelmanjm's comment when we get to that stage. |
## Summary Hoists `subsystem.py` and `skip_field.py` from `src/python/pants/backend/codegen/protobuf/lint/buf/` up to `src/python/pants/backend/codegen/protobuf/buf/` so non-lint consumers can depend on the `BufSubsystem` without reaching into the lint package. Pure file relocation — `git mv` of two files plus import-path updates in the three files that referenced them. No behavior change. This is split out from #23343 (which adds Buf as an alternate Python protobuf code generator) to make that PR easier to review by separating the mechanical move from the substantive new code. Once this lands, #23343 will be rebased onto it and reduce to just the codegen feature. ## Test plan - [ ] `./pants test src/python/pants/backend/codegen/protobuf/lint/buf::` still passes (lint/format rules import from new path). - [ ] CI passes.
|
@benjyw Mechanical changes are landed. @edelmanjm -- it looks like MergeDigests already handles the duplicate files with the same contents (https://github.com/pantsbuild/pants/blob/main/src/rust/fs/store/src/snapshot_ops.rs#L32-L33) so you probably have different file contents being generated. As for supporting MultipleSourcesField, that's probably a larger change outside the scope of this PR. That may require changing the behavior of the existing protoc generator and I didn't want to touch that. |
|
Just checking in to see if there's any movement on this; been enjoying using it so far, would love to see it in main. |
|
Thanks for the contribution. We've just branched for 2.33.x, so merging this pull request now will come out in 2.34.x, please move the release notes updates to docs/notes/2.34.x.md if that's appropriate. |
|
@mfairley Are you still interested in owning this? We'd love to see it get merged, but I wasn't sure if you were still actively maintaining this PR. |
@edelmanjm Yes, I'm still happy to own this but my understanding is that it's waiting for review from @sureshjoshi as the next step. |
|
@sureshjoshi What's required to get this reviewed? Happy to help in any way I can; would love to see this merged. |
|
I will review ASAP. Meanwhile if you merge main and push it should fix the CI issue. |
|
@mfairley Would you like to rebase, or would you prefer if I did it? |
@edelmanjm Feel free to rebase |
A `remote:` entry with a version but no `revision:` was replaced with Pants's registry default, so `buf.build/protocolbuffers/python:v33.0` silently became v34.1 and could produce code newer than the protobuf runtime in use.
The Python module mapper tried to work out which buf target owns each module generated from a BSR dep. Pants can't reliably know that: many targets can generate the same dep, which ones do depends on what they import, and the module names depend on the generator and the template's `out:`. The result was ambiguous or wrong ownership, so nothing useful was inferred. Code that imports a BSR dep's generated modules should depend on a package that provides them, and set `modules=` if Pants can't work out the names.
When a buf target had no `buf.gen.yaml`, the Python module just kind of guess. This was weird and kind of nonsensicle; if if there's no `buf.gen.yaml`, no registration should happen.
Both the `pyi` and `python` plugins mapped to `_pb2`
Also simplifies the implementation a bit so we're not just copying all the defaults
`adhoc_tool` and `shell_command` put their `runnable_dependencies` on PATH by writing a shim per target, named after the target, that runs it the way `pants run` would. That logic was a private helper; this makes it a rule, `ResolveRunnableDependenciesRequest` -> `ResolvedRunnableDependencies`, so other backends can put Pants-built tools on a process's PATH the same way.
`buf lint` failed whenever buf.yaml declared a local check plugin, since the sandbox held only buf, the protos, and the config. This adds `[buf].plugins`, a list of runnable targets. Each is put on buf's PATH, so buf can call them.
A `buf.gen.yaml` can name a `local:` plugin, either as a path or as a name that buf looks up on PATH. Pants had no way to build such a plugin and supply it. This adds `[buf].codegen_plugins`, a list of runnable targets, matching `[buf].plugins` for `buf lint`. Each is put on buf's PATH under its target name via the runnable-dependency shims.
`buf generate` runs every plugin in the template it is given, and each language's codegen sandbox carries only that language's toolchain. So languages whose plugins need different tooling cannot share one template, and a singular `[buf].gen_template` cannot express that. This new setup allows every language to specify its own template, allowing for per-language generation.
Before this, pants only recognized generated Python files ending in `_pb2`, `_pb2_grpc`, `_grpc` or `_connect`. That covers the standard Google compiler, but in order to support any compiler (e.g. protobuf-py), we need to accomondate more potential outputs.
For each plugin that generates Python, Pants needs two things: what its generated files are called, and which libraries they import. Before this commit, these were in two separate lists, with the assumption that Google's `protobuf` was always used. This refactors things so they're both in one dataclass, with one entry per plugin, scoped correctly to what each plugin needs. Users can describe their own plugins with `[python-protobuf].extra_buf_plugins`, which replaces `extra_buf_plugin_suffixes`.
Running `buf generate` is largely same for every language. This moves the vast majority of the work into shared code; each language only needs to pass its template option, plus any toolchains its plugins need.
protoc's download also contains the `.proto` files for Protobuf's well-known types. Pants put all of it in the sandbox, so buf compiled those too.
This is needed for buf support
Runs through the shared `run_buf_generate` rule
TypeScript can only be generated with buf, and Go, Java and Scala only with protoc. Each language validates its dependencies and generators, failing if it depends on files it can't generate.
Used to be hard coded to just buf.gen.yaml
Codegen tool selection (i.e. protoc vs. buf) is now exclusively on a per-language basis, and can't be changed between each target. This simplifies things, since we don't worry on a target-by-target basis how we're doing codegen. Right now, only Python supports both buf and protoc, but this will be expanded in the future.
|
Fixed, rebased, resolved conflicts! |
|
Final request: please edit Claude's extremely verbose PR description down to something succinct enough to be suitable for a commit description. Any detail you want to conserve (including the LLM disclosure) can go in a separate comment. Once that is done I will merge. |
|
Thanks @benjyw. I've trimmed the description down to something suitable for the commit message. For anyone who wants to try it, https://github.com/mfairley/example-buf is a working example: a Python ConnectRPC server with protovalidate, plus a TypeScript client generated from the same buf.lock. Related issues: partially addresses #20684 (Python and TypeScript, but not Go yet), and follows on from #13189 and #20383. LLM assistance: Code primarily written by Claude Code, but I iterated on the design with Claude. (Per the contribution guide.) |
|
Thanks @mfairley and @edelmanjm ! Sorry for the long road, but it's in now. I will be creating the 2.34.x release branch hopefully by this weekend. |
Adds
buf generateas an alternative toprotocfor generatingcode from Protobuf. buf is increasingly the standard Protobuf
toolchain, and it can fetch pinned plugins and dependencies from
the Buf Schema Registry rather than requiring them to be installed
locally. Plugins and output paths come from a
buf.gen.yamltemplate, rather than from Pants options.
Python opts in with
[python-protobuf].codegen_tool = "buf"(the default remains
protoc). TypeScript, via the newpants.backend.experimental.codegen.protobuf.typescriptbackend,always uses buf.
In more detail:
the plugins named in the template.
remote:BSR plugins must be pinned to a version and revision,so that codegen is reproducible. Pants fills in pins for common
plugins, and
[buf].extra_plugin_pinsadds others.local:codegen plugins(
[buf].codegen_plugins) and asbuf lintcheck plugins(
[buf].plugins).pants generate-lockfiles --resolve=bufgeneratesbuf.lock,which is required when
buf.yamldeclaresdeps:.checknow runs codegen, so generated code isvisible to
tsc.Partially addresses #20684.