This change is 

I haven't done the Bazel side of this yet, but wanted to see how the CIs feel about it for now.

Looks fine so far. I'm concerned about the Bazel side of this -- rules_go doesn't expose protoc as a configurable attribute. Bare minimum we'll probably have to fork rules_go to at least allow us to call buf protoc instead of protoc, but if we're lucky it'll be smooth sailing with that one change. Are the command-line interfaces the same (except for the include-path stuff you mentioned)?

The CLI is almost the same, though as seen here, -I differs slightly in that a) it does not seem to support a multi-element, :-separated path so instead it is just passed multiple times for each path element and b) paths cannot be within other paths.

From the docs it sounds like it is intended to otherwise be drop-in with just a couple exceptions that don't really affect us:

All flags and features of protoc are supported and compatible, with the following exceptions:

--encode
--decode
--decode_raw
--descriptor_set_in
The remaining flags can all be added if there is sufficient demand.

Additionally, we have added the flag:

--by_dir
Using this flag will cause buf protoc to run your protoc plugins in parallel on a per-directory basis, resulting in major further performance improvements.

@rickystewart It looks like bazel is actually already using its own protoc, not the one in c-deps/protobuf, so it doesn't seem to care except what we do to c-deps/protoc. The only bazel change I needed to make was configuring rewrite of the prometheus proto import, to match the changes I made to the include path on the Makefile side. So while we probably do want to fork and update go_rules at some point, to use buf instead of protoc, for performance reasons, fortunately it looks that can wait for a separate PR, rather than adding to this one. Also, buf's docs also mention bazel and some features, like symlink support, were added for bazel compatibility, so it sounds like other projects are using/trying to use buf and bazel, so I have some hope it may happen upstream (or at the very least, we could contribute it).

The by_dir flag looks like it could also be a nice speedup, but again I think I'll wait for a follow-up and just keep this PR to the minimum diff to remove protoc usage.

I also didn't remove the protoc submodule in this PR yet -- I have the commit that does so, but I was kinda thinking to wait until this lands, see if there's any fallout first?

--descriptor_set_in

We actually need this one. So step 1 would be going over to the buf folks and requesting that this flag be supported as well.

It looks like bazel is actually already using its own protoc, not the one in c-deps/protobuf, so it doesn't seem to care except what we do to c-deps/protoc.

It is using "its own protoc", but the protoc it's using is deliberately pinned to the version that we have in c-deps/protobuf, so that seems like a distinction without a difference to me.

So while we probably do want to fork and update go_rules at some point, to use buf instead of protoc, for performance reasons, fortunately it looks that can wait for a separate PR, rather than adding to this one.

AFAIC this is a regression in the status of the Bazel migration -- I don't want the Bazel build and the make build to diverge significantly in terms of behavior, for what are hopefully obvious reasons.

@rickystewart I don't think we ever use --descriptor_set. So when we add support to rules_go to call buf protoc instead of protoc, we can also just change it to only pass --descriptor_set_in if len(descriptors) > 0 and elide it otherwise, without waiting for buf to add any support for it. Eh, I found where rules_go passes it.

This would apparently require some dev-inf investment. We have a team meeting tomorrow, I'll bring it up there and we'll settle on some next steps.

Yeah, it looks like bazel does pass input descriptor sets for proto->proto dependencies. In the error, buf suggests just passing the original sources protos instead and I think that makes sense, but it'd be a more invasive change in go_rules and proto_rules. We might want to see what the @bufdev folks say, since based on a few mentions around the docs and a couple fixed issues around symlink support, it sounds like some folks have already worked on integrating buf into bazel builds and might have figured this out already.

As long as they're producing the same output, I'm a little sad that we'll say make builds have to keep using protoc, just because that's what bazel is using. If the build tool produces the same output, do we care how it gets there? Bazel isn't using host go toolchain but make-driven build is.

Thanks for bringing this to our attention! A few things from the comments:

buf is slightly stricter than protoc about its include path, specifically it does not allow an entry in the include path that has a prefix another entry already on the include path.

This is actually a bug on our part - this should not be true with buf protoc specifically, which is meant to be a drop-in replacement for protoc, so we'll look into this and fix it for buf protoc. This is a requirement for the rest of our code, and is something you should respect anyways (see https://docs.buf.build/build-configuration#root-requirements for a discussion of why), but should not be the case here.

Corollary: see #56863 (comment) for a discussion of how I tried to get buf to work with cockroach across the board, not just for protoc.

The CLI is almost the same, though as seen here, -I differs slightly in that a) it does not seem to support a multi-element, :-separated path

Let us test this out locally, we were not aware this was an option for protoc. If it is, we'll add support for it in buf protoc.

Every comment on bazel

We want to have better support for bazel as a first-class citizen, we're happy to find 30 minutes to talk on video if your team is interested, would be a good use case.

Also note we got metrics.proto moved to io/prometheus/client/metrics.proto in prometheus/client_model#46

See bufbuild/buf#350 and bufbuild/buf#351 for the first two issues.

A couple updates on the Bazel side.

• I thought that Bazel hardcoded the use of protoc for protobufs, but it turns out that there's a command-line flag for this: --proto_compiler (a build target label; default: "@com_google_protobuf//:protoc"). We can set this to a shim sh_binary that doesn't do anything besides call into buf protoc "$@", so this actually works perfectly fine.
• @dt went out of his way to hack rules_go to support using buf: https://github.com/cockroachdb/rules_go/compare/inuse...cockroachdb:config-protoc?expand=1. This isn't production-ready quite yet but it does demonstrate that buf works for this purpose which is encouraging.

@bufdev What's the status of --descriptor_set_in? Would supporting that flag be prohibitively difficult for y'all? Some context is below, but the TLDR is that the decision to adopt buf would become much easier for us if it buf fully supported descriptor sets.

Context on the above: code generation for protobufs in Bazel is split into a two-step process, which you can see e.g. in this BUILD file. The first step (serverpb_proto in that BUILD file) creates a descriptor set using --descriptor_set_out. The second step converts those descriptor sets to the actual generated code using --descriptor_set_in (serverpb_go_proto in that BUILD file). This has performance advantages over processing and re-processing the same .proto sources many times during the build, especially for multi-language builds, like ours which compiles to JS as well as Golang. @dt's patch attempts to work around that by just pulling in the entire transitive set of input files in each proto_library, but that makes code generation much more expensive/repetitive/wasteful for the reasons I just mentioned. Retaining descriptor set support would allow us to just slot buf in as a pure replacement for protoc without any downsides for performance.

A couple updates on the Bazel side.

* I thought that Bazel hardcoded the use of `protoc` for protobufs, but it turns out that there's a command-line flag for this: `--proto_compiler (a build target label; default: "@com_google_protobuf//:protoc")`. We can set this to a shim `sh_binary` that doesn't do anything besides call into `buf protoc "$@"`, so this actually works perfectly fine.

We could also theoretically create a buf-protoc binary that we release alongside buf, but given that we're trying to move people from protoc to buf build, buf generate, etc anyways, we'd prefer not to if you're OK with that :-)

* @dt went out of his way to hack `rules_go` to support using `buf`: https://github.com/cockroachdb/rules_go/compare/inuse...cockroachdb:config-protoc?expand=1. This isn't production-ready quite yet but it does demonstrate that `buf` works for this purpose which is encouraging.

@bufdev What's the status of --descriptor_set_in? Would supporting that flag be prohibitively difficult for y'all? Some context is below, but the TLDR is that the decision to adopt buf would become much easier for us if it buf fully supported descriptor sets.

I started investigating this today, and in short, it is possible and we should do it, but I would expect this to not be ready until as late as end of next week. There's a possibility I can completely hack it in tomorrow, but there's a lot of moving pieces.

If you can give us that time though, we can do it. We actually like that you're hitting all of this for us, as we want to provide better support for Bazel, and you're finding the holes (not that we like that you're struggling though!).

Context on the above: code generation for protobufs in Bazel is split into a two-step process, which you can see e.g. in this BUILD file. The first step (serverpb_proto in that BUILD file) creates a descriptor set using --descriptor_set_out. The second step converts those descriptor sets to the actual generated code using --descriptor_set_in (serverpb_go_proto in that BUILD file). This has performance advantages over processing and re-processing the same .proto sources many times during the build, especially for multi-language builds, like ours which compiles to JS as well as Golang. @dt's patch attempts to work around that by just pulling in the entire transitive set of input files in each proto_library, but that makes code generation much more expensive/repetitive/wasteful for the reasons I just mentioned. Retaining descriptor set support would allow us to just slot buf in as a pure replacement for protoc without any downsides for performance.

Yep, we'll get it done.

Also see bufbuild/buf#353, bufbuild/buf#357

We could also theoretically create a buf-protoc binary that we release alongside buf, but given that we're trying to move people from protoc to buf build, buf generate, etc anyways, we'd prefer not to if you're OK with that :-)

Nah, it's not necessary.

I started investigating this today, and in short, it is possible and we should do it, but I would expect this to not be ready until as late as end of next week. There's a possibility I can completely hack it in tomorrow, but there's a lot of moving pieces.

No reason to go that hard I think, there's no real urgency to this AFAIK. Feel free to take your time and do it right :) When you have a release/SHA/whatever that supports --descriptor_set_in (plus the other small things we've discussed today), just ping me again and I can drive this the rest of the way from our end.

OK we'll let you know - we'll get it done, there's some weeds to whack to get there though!

@rickystewart given that a) it looks like we could add buf support to rules_go, even without any upstream changes and b) upstream is working on changes that'll make it an even simpler swap as a drop-in soon (thanks so much!), how do we feel about proceeding here, with the Makefile-side of things?

I think my initial inclination would be towards merging this as is and then swapping bazel's protoc_compiler when the upstream buf changes land? That would mean that the bazel and make builds are temporarily using different proto compilers, but I think that'd be fine, right? Both because we don't actually use any bazel-produced artifacts yet but also because even if we did, buf produces identical outputs to protoc (byte-identical in some cases), and we already have other similarly minor or inconsequential differences between the two builds (e.g. make uses host's go toolchain or cmake, vendor, how protoc is invoked with --decriptor_sets vs --proto_path, etc).

#67091 for the follow-up work on the Bazel side.

aha, that was the magic line I needed to remove! Done.

ah, looks like you have a vendor conflict to deal with, sorry :p

TFTRs!

bors r+

Build succeeded:

• GitHub CI (Cockroach)

Wow, David! Nice work.