Allow *.orion.pb.go files to be generated into a different package from the generated protobufs

[benjaminheng]

Currently the generated *.orion.pb.go file needs to be placed as a sibling to the generated protobufs, because the orion file references an unexported variable:

Orion/protoc-gen-orion/orion.go

Line 262 in ec021a7

serviceDescVar := "_" + servName + "_serviceDesc"

In later versions of protoc-gen-go-grpc, this variable is now exported. #171 is a related issue to allow protoc-gen-orion to use the exported variable.

The variable being exported now also means that it should be possible to place the *.orion.pb.go file in a separate package. In Carousell we're considering having a Go module for all generated protobufs, and another module for all generated orion *.orion.pb.go files.

Ideally the module for generated protobufs will only contain two dependencies:

github.com/golang/protobuf
google.golang.org/grpc

Having the orion files as a sibling will require github.com/carousell/Orion to be added as another dependency. The orion dependency is too heavy for us to include. Not every service that consumes our generated protobufs will need Orion.

Thus ideally the orion file will live in a separate package. Example file structure:

gen/
  go/
    serviceA/serviceA.pb.go
    serviceB/serviceB.pb.go
    go.mod
  orion/
    serviceA/serviceB.orion.pb.go
    go.mod

I have some ideas of how to extend protoc-gen-orion to support this. I'll add more comments to the issue later.

[cs-lexliu]

Before we use the latest version of protoc-gen-go to get the exported service desc.
We have to modify the go_plugin_install to support higher protoc-gen-go version.
The current protoc-gen-go we used is v1.3.1, and this is getting from github.com/golang/protobuf/protoc-gen-go.
The higher version (> v1.5.2) of protoc-gen-go has been change to google.golang.org/protobuf/cmd/protoc-gen-go.

We also have to introduce google.golang.org/grpc/cmd/protoc-gen-go-grpc into our protogen tool. Because it's separated from protoc-gen-go after v1.20.0.

[cs-lexliu]

After protogen supports higher version.
I will pick [email protected] and [email protected] as the base version for our protogen v2 proposal (https://github.com/carousell/shared-proto/pull/2736). This is the minimum version to support the exported service desc.

To call the exported service desc from *.orion.pb.go, we have to make protoc-gen-orion support to retrieve go_package from protobufs. The go_package provides the package import path of generated *.pb.go files. protoc-gen-orion can import the package with go_package to get the exported service desc.

// applebomb/v1/applebomb.proto
syntax = "proto3";

package applebomb.v1;

option go_package = "github.com/carousell/shared-proto/gen/go/applebomb/v1;applebombv1";

import "applepie/v2/applepie.proto";
import "appletree/v1/appletree.proto";

service AppleBombService {
  rpc Bomb(appletree.v1.Tree) returns (applepie.v2.Pie);
}

// gen/go/applebomb/v1/applebomb.pb.go
package applebombv1
...
...
var AppleBombService_ServiceDesc = grpc.ServiceDesc{...}

// gen/orion/applebomb/v1/applebomb.orion.pb.go
import applebombv1 "github.com/carousell/shared-proto/gen/go/applebomb/v1"
...
...
func RegisterAppleBombServiceOrionServer(sf interface{}, orionServer orion.Server) error {
  err := orionServer.RegisterService(&applebombv1.AppleBombService_ServiceDesc, sf)
  ...
}

[benjaminheng]

Alright, the approach of having protoc-gen-orion read go_package is what I was intending to do as well.

I'm thinking about how to make the changes backwards compatible too.

1. We'll need to tell protoc-gen-orion to use the exported the grpc.ServiceDesc variable instead of the unexported one.
2. We'll also need to continue supporting the existing use-case of having the *.orion.pb.go file as a sibling of the *.pb.go files.

My proposal is:

1. Add a flag exported-service-desc=true. If true, then the exported variable name is used. Default false.
2. Add a flag standalone-mode=true. If true, then go_package is required and exported-service-desc is overridden to true. The package name from go_package is prepended to the exported variable (e.g. listing_proto.ListingService_ServiceDesc). Default false.
3. Add a deprecation warning when exported-service-desc is left as false. In 6 months (or whatever timeframe) we'll set the default to true. We'll create a ticket for tracking when to change the default.

With these two flags, our protoc command can be either:

protoc --orion_out=. --orion_opt="exported-service-desc=true" --orion_opt="standalone-mode=true" source.proto
protoc --orion_out="exported-service-desc=true,standalone-mode=true:." source.proto

What do you think?

[cs-lexliu]

The flag designed in your proposal is good.

[cs-lexliu]

How about having a v2 protoc-gen-orion.
It can prevent the breaking changes from migrating protoc-gen-go library to google.golang.org/protobuf/cmd/protoc-gen-go. Support exported-service-desc can be the default value in v2. So we reduce the effort on implementing the exported-service-desc flag.
Adding a new version protoc-gen-orion also help us speed up the v1 deprecation.

[cs-lexliu]

@benjaminheng Please check the PR. To make the changes smaller, the PR doesn't contain the versioning upgrading plan. We can have more discussion on the plan later.