Build: one proto per proto_library

[ggirelli]

Bazel docs recommend:

One proto_library rule per .proto file.

I also encountered the following error when trying to include protovalidate in some internal repos:

Error in fail: attribute srcs: Can only compile a single .proto file at a time.

---

This is a very small change but it would be amazing to get it into the next release 🙏🏻

[CLAassistant]

All committers have signed the CLA.

[srikrsna-buf]

Hey @ggirelli, the proto_library rules are generated using gazelle. The default is to generate one rule per directory. Could you give us more details about the error and if possible the rule/tool that is generating the error?

[ggirelli]

Hi @srikrsna-buf thanks for the prompt reply! I see, gazelle does indeed bundle all .proto files in a single proto_library rule by default, but I still believe it would be better to have one rule per .proto file, as recommended by bazel's docs.

I am not super familiar with gazelle but I found discussions and PRs related to enabling a "per-file" mode (directive # gazelle:proto mode with file mode) instead of the default grouping:

• Add per-file mode for proto_library generation bazel-contrib/bazel-gazelle#1033
• New proto mode: one proto_library per proto bazel-contrib/bazel-gazelle#138

I guess we can close this PR as the solution would be to enable this mode instead of manually changing the build files, right?

As per your question, the rule throwing the error is from an internal tool that is enforcing the bazel's docs recommendation.

[ggirelli]

@srikrsna-buf I have updated the PR by reverting the previous changes and adding the # gazelle:proto file directive, is that okay?

[srikrsna-buf]

Hey @ggirelli You'll also need to run gazelle for it to update the targets.

[ggirelli]

@srikrsna-buf sounds good. I ended up changing it to set the directive only in that directory because otherwise it breaks most build files due to an inconsistent naming of the proto_library rules (i.e., a proto_library for file foo.proto is most often not called foo_proto as expected) 😐 I hope that's okay...

[srikrsna-buf]

You will need to add the expression_proto here

[ggirelli]

I am not so sure 🤔 :expression_proto is already in the dependencies of :validate_proto so it should work correctly as is, no?

[srikrsna-buf]

That won't generate for dependencies we will need a second cc_proto_library rule for generating the corresponding cc files for expression_proto. That should be a dependency for this library.

Output of bazel build //proto/protovalidate/buf/validate:validate_proto_cc:

Target //proto/protovalidate/buf/validate:validate_proto_cc up-to-date:
  bazel-bin/proto/protovalidate/buf/validate/_virtual_imports/validate_proto/buf/validate/validate.pb.h
  bazel-bin/proto/protovalidate/buf/validate/_virtual_imports/validate_proto/buf/validate/validate.pb.cc
  bazel-bin/proto/protovalidate/buf/validate/libvalidate_proto.a

Notice how it only generates validate files.

[ggirelli]

cc_proto_library supports only a singular proto_library dependency, so I cannot add :expression_proto directly there.

But, I can add a cc_proto_library for expression.proto.

[srikrsna-buf]

Sorry I meant the new cc_proto_library should be the dependency.

[ggirelli]

But cc_proto_library supports only a single proto_library dependency, with no other dependencies. If I try to pass an extra cc_proto_library as a dependency, the build will fail.

Honestly, I feel like the extra cc_proto_library is not helping.

With the previous build file (without the expression_proto_cc rule), while it is true that bazel build lists only the direct targets, I do see the dependency proto c code being generated:

$ bazel build //proto/protovalidate/buf/validate:validate_proto_cc
INFO: Streaming build results to: ###
INFO: Analyzed target //proto/protovalidate/buf/validate:validate_proto_cc (1 packages loaded, 5 targets configured).
INFO: Found 1 target...
Target //proto/protovalidate/buf/validate:validate_proto_cc up-to-date:
  bazel-bin/proto/protovalidate/buf/validate/_virtual_imports/validate_proto/buf/validate/validate.pb.h
  bazel-bin/proto/protovalidate/buf/validate/_virtual_imports/validate_proto/buf/validate/validate.pb.cc
  bazel-bin/proto/protovalidate/buf/validate/libvalidate_proto.a
  bazel-bin/proto/protovalidate/buf/validate/libvalidate_proto.so
INFO: Elapsed time: 5.506s, Critical Path: 4.76s
INFO: 27 processes: 19 internal, 8 linux-sandbox.
INFO: Streaming build results to: ###
INFO: Build completed successfully, 27 total actions

$ ls bazel-bin/proto/protovalidate/buf/validate/_virtual_imports/
expression_proto  validate_proto

$ ls bazel-bin/proto/protovalidate/buf/validate/_virtual_imports/expression_proto/buf/validate/expression.p*
bazel-bin/proto/protovalidate/buf/validate/_virtual_imports/expression_proto/buf/validate/expression.pb.cc
bazel-bin/proto/protovalidate/buf/validate/_virtual_imports/expression_proto/buf/validate/expression.pb.h
bazel-bin/proto/protovalidate/buf/validate/_virtual_imports/expression_proto/buf/validate/expression.proto

$ ls bazel-bin/proto/protovalidate/buf/validate/_virtual_imports/validate_proto/buf/validate/validate.p*
bazel-bin/proto/protovalidate/buf/validate/_virtual_imports/validate_proto/buf/validate/validate.pb.cc
bazel-bin/proto/protovalidate/buf/validate/_virtual_imports/validate_proto/buf/validate/validate.pb.h
bazel-bin/proto/protovalidate/buf/validate/_virtual_imports/validate_proto/buf/validate/validate.proto

[rodaine]

Thanks, @ggirelli!