Allow generation of Twirp in a different Golang Package

bufdev commented 4 years ago

I'll keep this pretty short for now (I also know we discussed this very briefly @spenczar) but just to get the conversation going since this is on my near-term radar:

It would be nice to be able to have twitchtv/twirp generated code in a different package than the golang/protobuf generated code. This in theory would allow you to have separation of your Protobuf definitions and the specific RPC protocol you use. For example, I'd love to have:
foo/barpb - Protobuf messages for package foo.bar
foo/twitchbarpb - Twirp client/servers for package foo.bar
foo/grpcbarpb - gRPC client/servers for package foo.bar - I'm not sure if this is will be allowed with the new protoc-gen-go-grpc via https://github.com/protocolbuffers/protobuf-go/blob/master/compiler/protogen/protogen.go but we can leave this out of it
foo/magicbarpb - My custom magical RPC implementation (not in reality but as an example)

What this would require is protoc-gen-twirp recognizing when it was generating code in a different package than the package of the golang/protobuf generated code, and then generating imports, ie:

// all shorthand, not even importing context
import barpb "github.com/alice/bob/foo/barpb"

type EchoService interface {
  Echo(context.Context, *barpb.EchoRequest) (*barpb.EchoResponse, error)
}

This might be able to be implemented with some combination of the Mkey=value (which would be annoying), import_path, and import_prefix options, but honestly it would be so much easier if there was just a definition_path=github.com/alice/bob/foo/barpb option or something similar to just denote this easily. There are Golang Protobuf plugin libraries that handle this kind of behavior, so there's a lot of work to pattern off of.

Thoughts?

spenczar commented 4 years ago

As far as I know, this should be working today. This test case does it: https://github.com/twitchtv/twirp/blob/master/internal/twirptest/importer/importer.proto#L10

Do you have a case that's not working?

bufdev commented 4 years ago

This test case isn’t it - this is where the message definitions for the request/response types are in a different package - what I’m looking for is where you have messages in the same package, but you want twirp to generate to a different golang package than the golang package of the message definitions.

titpetric commented 4 years ago

@bufdev message definitions are generated with protoc go_out, not twirp. I'd move those definitions into a subfolder designating the package, and protoc generate them separately (will be it's own package). You can also force protoc to generate the message output into different packages on it's own, but twitch will of course not pick them up, as this relationship doesn't come from proto files.

For example, you can do:

package message;

import "rpc/user/user.proto";

And then use user.User or whatever in your local messages, or RPC definitions. With the import rule you have the explicit relationship defined, and the package source for the user scoped types too. The correct way would be to split up messages and import them, twirp respects this.

bufdev commented 4 years ago

This is not the issue - yes, of course you could put your service definitions in another package. However, I would like twirp to our services definitions in the same package into a different golang package. Please read my original description in this issue.

titpetric commented 4 years ago

I got it, I just explained that's not how it works because the codegen needs to know these relationships, and import is the way the codegen knows about it. A folder == package.

The main underlying question is how would you hint the protoc compiler, or the proto file schema, that the messages are part of a different package? Does some other protoc language target already support what you're trying to achieve, and what does the schema/command look like in that case?

If you provide an example of a service+message.proto files that would live in the same folder, that would be best. I imagine you want to use the go_package option inside your messages.proto?

bufdev commented 4 years ago

As explained above, this will probably require a new parameter option:

This might be able to be implemented with some combination of the Mkey=value (which would be annoying), import_path, and import_prefix options, but honestly it would be so much easier if there was just a definition_path=github.com/alice/bob/foo/barpb option or something similar to just denote this easily. There are Golang Protobuf plugin libraries that handle this kind of behavior, so there's a lot of work to pattern off of.

titpetric commented 4 years ago

Do you have any example of something like this available with a different codegen target? If anything, I believe it would be best to rely on established proto options like go_package (which should theoretically satisfy some of your codegen requirements). The best bet would be to see how you would do it with gRPC codegens, and then figure out a compatible option. Feature wise, I'm not sure twirp should go beyond that for your particular use case, as the implementation of this change is a feature request, and not expected functionality (esp. re: imports which provide you with what you need, alas with a change in your workflow).

On Thu, Dec 5, 2019 at 4:10 PM bufdev notifications@github.com wrote:

As explained above, this will probably require a new parameter option:

This might be able to be implemented with some combination of the Mkey=value (which would be annoying), import_path, and import_prefix options, but honestly it would be so much easier if there was just a definition_path=github.com/alice/bob/foo/barpb option or something similar to just denote this easily. There are Golang Protobuf plugin libraries that handle this kind of behavior, so there's a lot of work to pattern off of.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/twitchtv/twirp/issues/195?email_source=notifications&email_token=AABY7EAJF55RQCTIJOPFWDTQXEKW5A5CNFSM4JOK3Y42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGBA2NQ#issuecomment-562171190, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABY7EH56MCARC7PQ7UFSLDQXEKW5ANCNFSM4JOK3Y4Q .

bufdev commented 4 years ago

go_package does not satisfy this. This is a new feature request, by design.

grpc-go does not do this as grpc-go generates definitions to the same file, which is inherently in the same package. This may change with golang/protobuf v2 but does not seem to.

spenczar commented 4 years ago

what I’m looking for is where you have messages in the same package, but you want twirp to generate to a different golang package than the golang package of the message definitions.

I understand now. What's a reason you'd want this? Adding options adds cost, since it's less obvious how a project will be structured, and since we need to maintain it forever.

bufdev commented 4 years ago

The general thesis here is that Golang Protobuf plugin development is slightly broken across the golang/protobuf dependent ecosystem. This is in no way a Twirp-specific issue - Twirp is just following best practices here - but there's no reason Twirp can't lead the charge a bit.

The problem is that basically every major golang/protobuf-dependent plugin (ie a plugin that generates code that uses the golang/protobuf-generated structs for each message) generates code to the same Golang package that golang/protobuf uses. For example:

plugins=grpc generates the Client/Server interface in the same package (actually the same file, which is even worse haha).
protoc-gen-twirp generates Client/Server interfaces with slightly different names than the grpc-go interfaces in the same package
protoc-gen-validate generates .Validate() methods on each struct in the same package
protoc-gen-grpc-gateway generates server types to the same package

There's a world where we can use our Protobuf definitions to generate a lot more than we normally do today, but (staying golang-specific for now) as the number of golang/protobuf-dependent plugins you use approaches infinity (or more realistically, 10-20), you need to both:

Guarantee no overlapping naming for various systems - for example, Twirp does *ProtobufClient which isn't optimal. Doing this without knowing all the plugins that a user might want is basically impossible without really bad naming.
Have any user of golang/protobuf-dependent structs depend on every single piece of generated code. So for example I might have a Twirp daemon and a grpc-go daemon that both expose my API, but it's not possible to make "thinner" binaries where the Twirp daemon doesn't depend on grpc-go, and vice versa (this is what I'm discussing in the description a bit).

In reality, there should be some way to specify that my plugin depends on the golang/protobuf struct types, and to tell me where these struct types are.

Taking it further, it would be nice for this to be a bit more generic, so for example for any golang-based plugin foo, I can declare the general form of the golang package that foo generates to, and then dependent plugins can use this (So if I wrote protoc-gen-twirp-magic-extension, I could know both the location of protoc-gen-go and protoc-gen-twirp packages).

Does that make sense?

bufdev commented 4 years ago

spenczar commented 4 years ago

Okay, your argument about interoperability (like with protoc-gen-twirp-magic-extension) rules out using command-line options, since protoc-gen-twirp-magic-extension would need to know the arguments that had been passed into protoc-gen-twirp at some point in the past. It points way stronger toward using a FileOption.

This isn't the first time that FileOptions have come up. My concern with them is distributing the .proto definitions of the options, and helping users import them clearly. Is there a project that does that well that we can learn from? I guess gogo/protobuf does a pretty good job, maybe?

bufdev commented 4 years ago

I don't think this needs a FileOption, and actually I'd be against them - I don't think Protobuf definitions should have any language-specific options, to be honest, and I rather the push be in the opposite direction - Protobuf files define Protobuf, and generators define how the Protobuf files are converted to stubs exclusively.

I don't think protoc-gen-twirp-magic-extension needing to know the arguments passed to protoc-gen-twirp is an issue - that's up to the generator to properly structure arguments to each plugin As an example, protoc-gen-go can only be called on a per-package basis, So if you have packages a, b, c, you need three protoc invocations. If you use any of the Mfile=package, import_prefix, etc options, and say a imports b, you obviously need to structure these similarly across the invocations for a and b since a needs to know how to import b in the Golang-generated code. That's the same as protoc-gen-twirp-magic-extension needing to know about arguments toprotoc-gen-twirp (and protoc-gen-go).

Using FileOptions would lead to something like using a long-form go_package as above, and this ends up being highly restrictive, making it next to impossible to distribute Golang packages outside of a single pre-defined import path without modiying the go_package value, either ahead of time by literally re-writing the file, or doing it dynamically (which defeats the point). This problem is especially bad with monorepos, ironically.

bufdev commented 4 years ago

(A note for later) if we do decide to go with FileOptions (which again I'm going on record as being highly against), I wouldn't follow gogo/protobuf directly, I'd instead recommend having a single file proto/twitchtv/twirp/v5/twirp.proto with package twitchtv.twirp.v5, and users use -I path/to/twirp/proto.

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 60 days with no activity. Remove stale label / comment or this will be closed in 5 days

twitchtv / twirp

Allow generation of Twirp in a different Golang Package #195