api: organize go proto libraries

[kyessenov]

api_go_proto_library is used in an ad-hoc fashion. It's not part of the api_proto_library macro, and is often neglected when protos are shuffled.

This issue is a proposal to organize the protos as follows:

• there should be only one go_proto_library per directory in xDS. This is aligned with normal Golang practices of Go package == proto package (e.g. https://github.com/googleapis/go-genproto/tree/master/googleapis/api/annotations);
• there should be a source proto_library per-directory with all protos as sources;
• all other go_proto_library should be removed.

This seems like a middle-ground due to various Bazel issues with golang:

• you cannot have two proto libraries as inputs to a Go proto library;
• you cannot have two Go proto libraries for the same proto.

This is also aligned with go-control-plane structure. Having one package per proto is onerous for Go developers.

Ref envoyproxy/go-control-plane#213

[mattklein123]

@htuch can you help oversee design review on this? Per offline discussion IMO we should not have any reference to Go at all in the xDS protos and this should be handled either internal to the bazel machinery or somehow by an external tool.

[kyessenov]

1. and 2) could be handled with a bazel macro api_proto_package or something like that.

[htuch]

First, super excited to see this getting some attention, thanks @kyessenov!

I think we need a design proposal that completely hides language specifics, and also avoids manually curating a dependency tree in parallel for Go. @kyessenov could you mock up some examples based on some simple package in the existing tree, showing what this would look like from a BUILD author perspective?

[kyessenov]

Here is the food for thoughts: https://github.com/envoyproxy/envoy/compare/master...kyessenov:new_go_library?expand=1

The basic idea is that we actually need a proto package rule per directory. Once we have that, we can create an associated go_proto_library.

[htuch]

Is this going to completely get rid of having to maintain a parallel dependency tree? Does the package level proto declaration need to take in the union of all proto dependencies? If we do this, I feel we might as well just do it for C++/Python etc. The downside is that we lose fine grained targets, which is a nice feature of Bazel for build performance and build graph analysis.

I'm trying to understand "Having one package per proto is onerous for Go developers." above as well. Is this just some aesthetic thing, could Go developers suck it up in the interest of not causing build complexity?

Also, related to this, I just opened cncf/udpa#1. We're moving the UDPA protos (just ORCA) outside of envoyproxy/envoy. The build system there will not have any Go support initially, as it's just too messy IMHO. We will retrofit whatever we figure out in this issue there.

[kyessenov]

Is this going to completely get rid of having to maintain a parallel dependency tree? Does the package level proto declaration need to take in the union of all proto dependencies? If we do this, I feel we might as well just do it for C++/Python etc. The downside is that we lose fine grained targets, which is a nice feature of Bazel for build performance and build graph analysis.

I would expect package level proto library needs to declare its package level proto library dependencies. A nice side effect of this is that proto packages do not become accidentally mired in dependency loops. Without some package-level grouping of protos that is understood by bazel, proto packages don't really mean much from the build stand point.
I don't think performance is an issue with Golang at all, the compiler is extremely fast.

By the way, I'm not arguing to replace existing non-Go rules. Multiple proto libraries can co-exist together for the same protos (e.g. https://github.com/googleapis/googleapis/blob/master/google/api/BUILD.bazel#L352) That would cause too much churn IMHO.

I'm trying to understand "Having one package per proto is onerous for Go developers." above as well. Is this just some aesthetic thing, could Go developers suck it up in the interest of not causing build complexity?

It is aesthetic, but as we have seen with gogo-vs-goproto saga, if we don't meet developers in the middle, they will just fork Go stubs down the line. Protos are already extremely verbous in golang due to some early design mistakes I suppose. Case in point -- the recent additions to googleapis all follow proto package convention (e.g. https://github.com/googleapis/go-genproto/tree/master/googleapis/iam/v1).

[htuch]

Stepping back, my real wish list here is:

1. Avoid multiple dependency graphs being maintained for a given proto. One source of proto dependency truth across all languages.
2. Make the changes an author needs to do to a BUILD feel be something that can be structurally enforced. I.e. either the macros or something like check_format makes it so that you can't make a mistake, no matter how hard you try :)

I don't think having a separate package level declaration in each file is going to solve (1), with some tooling maybe we can get (2) though.

Is there any way to auto-generate the package level declaration from the existing api_proto_library targets? E.g. have a BUILD.generated and two stage compile, or something like this?

[kyessenov]

I looked around for some bazel trick to aggregate rules in BUILD files, and it looks complicated, if possible at all. Generally, it's hard to make bazel to pre-process BUILD files. It treats them as canonical source of truth. Doing something outside of bazel before bazel would complicate the build system further IMHO.

I'm not really sure how to proceed here if (1) is required. Package-level proto libraries and individual proto libraries are in fact duplicate dependency trees. Individual proto libraries are incompatible with go-control-plane structure.

[htuch]

The other alternative is we do overhaul the existing C++/Python rules and make them package wide. That would be more disruptive I think than we're up for I'm guessing.

I think I'd be reluctantly willing to concede on (1) if we had robust (2). @mattklein123 WDYT?

[kyessenov]

(2) can be enforced by checking every API BUILD file for a call to the package level proto library. That should be an easy linter.

[htuch]

The linter should also verify that we have consistency in dependencies. Some of this will be picked up at build time, but due to transitive includes it might not be as robust as what we could get by verifying directly this consistency.

[kyessenov]

Transitive build dependencies between packages that form a loop should be detected by bazel I think. Let me know if I'm missing some case where it's not.

My main motivation is to converge go-control-plane and envoy's Bazel so that code written against one is portable to the other (in and out of google monorepo). Ideally, the generated code can be dumped automatically, just like with data-plane-api. The first step is to align the directory structure. The second step is to converge gogoproto/goproto, which should be much easier after the first step.

[mattklein123]

I'm willing to concede (1) also as long as we have (2), agreed.