Handle the standard M= and import_prefix= golang plugin flags

[bufdev]

Example: https://github.com/yarpc/yarpc-go/blob/dev/internal/protoplugin/runner.go#L50

Will be needed when request/response types are outside of the package of the service, or if generating the Twirp handlers to a different golang package.

Nice work by the way!

[spenczar]

Right, when we wrote the generator we skipped this part because it doesn't seem to be documented anywhere very well, so it seemed too hard to get right.

Could you write a bit more about what these things are used for, and what their precise meaning is?

[spenczar]

@peter-edge ping! i'm inclined to close this without further info :)

[jonathaningram]

@spenczar I just came across this when trying to organise my protos in a package or two :(

Hopefully this can help:

Say my proto files are something like:

// ./root-path/rpc/empty.proto
// Note in this case by "rpc" I don't mean Twirp's suggested folder structure, I just mean that the Empty message is related to general RPC calls.
syntax = "proto3";
package rpc;

// Empty is a message used when there is no value returned from a call.
message Empty{}

// ./root-path/my-domain/my-domain.proto
syntax = "proto3";
package mydomain;

import "rpc/empty.proto";

service MyDomainService {
  rpc Noop(rpc.Empty) returns (rpc.Empty);
}

And say I have a generate.sh file that looks something like:

#!/usr/bin/env bash

ROOT_PATH="something"

PKG_PREFIX="github.com/my-company/my-repo"

PKG_MAP=""
PKG_MAP="$PKG_MAP,Mrpc/empty.proto=$PKG_PREFIX/some-path/proto/rpc"

protoc --proto_path $PROTO_PATH --go_out=$PKG_MAP:$PROTO_PATH $PROTO_PATH/rpc/*.proto
protoc --proto_path $PROTO_PATH --twirp_out=$PROTO_PATH --go_out=$PKG_MAP:$PROTO_PATH $PROTO_PATH/my-domain/*.proto

As I understand it, the $PKG_MAP part allows me to tell the compiler/generator that every time in a proto file you come across import "rpc/empty.proto"; and you are generating the equivalent Go import path for that message, you should use the import path github.com/my-company/my-repo/some-path/proto/rpc. At the moment, without this, protoc-gen-twirp is just putting the import path down as import rpc "rpc" which is unfortunately not right as you know from this issue.

Also the description from the protoc-gen-go repo kind of says this simply: https://github.com/golang/protobuf/blob/master/protoc-gen-go/generator/generator.go#L561

ImportMap map[string]string // Mapping from .proto file name to import path

The command line args are read and saved into this map: https://github.com/golang/protobuf/blob/master/protoc-gen-go/generator/generator.go#L625

And (maybe?) quite simply, all that is needed is to lookup this map when generating an import: https://github.com/golang/protobuf/blob/master/protoc-gen-go/generator/generator.go#L1334

[jonathaningram]

@spenczar I went ahead and made a PR. Let me know what you think and how you want to proceed.

[spenczar]

@jonathaningram interesting! Can I propose using option go_package instead to solve this?

If you ./root-path/rpc/empty.proto had this line in it:

option go_package = "github.com/jonathaningram/project/rpc"`

then both protoc-gen-go and protoc-gen-twirp will generate correct import statements when the file is imported - no special handling required.

I think this is better because it puts the standard import path right in the proto file instead of requiring a funky invocation of protoc, and because it's immediately available across more tools.

[bufdev]

Sorry for the lack of response!

The best place to copy this off of is:

https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-grpc-gateway

It looks like you have a good start though.

[jonathaningram]

@spenczar let me check out your suggestion. I feel like I tried it and it didn't work, but will report back either way.

[jonathaningram]

@spenczar interesting. It appears to be working as you suggested 😎 (edit: so thanks!)

In that case, at this point I cannot really offer any reason I need this. Unless anyone else can offer a reason Twirp actually needs this I wonder if it's worth closing this and my PR? Can always reopen.

[spenczar]

Glad to hear it! I'll close this, yeah.

[sagikazarmark]

I do have a use case: when proto files are stored in a central repository and code is generated for each client. Using go_package option in that case wouldn't really work.

[jonathaningram]

Yeah it didn’t actually work for me. I haven’t managed to put together a full example. I will though. In short though, when running protoc within my repo you end up with a nested src/github.com/me/repo set of folders rather than it installing the generated files in the right spot. So yeah I’ll make a repro repo we can work from.

…

On Tue, 24 Apr 2018 at 9:21 pm, Márk Sági-Kazár ***@***.***> wrote: I do have a use case: when proto files are stored in a central repository and code is generated for each client. Using go_package option in that case wouldn't really work. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAmP6PqLqY74PipHsRPW52S1TzZDFnHiks5trwrTgaJpZM4RhOl4> .

[spenczar]

@jonathaningram, yes, you'll get nested folders if you say --twirp_out=. - the go packages will be interpreted relative to . in that case, so they will nest.

I use protoc invocations like this:

protoc --proto_path=$GOPATH/src --go_out=$GOPATH/src github.com/twitchtv/twirp/example

Then, files get unloaded into GOPATH at the right spot.

---

@sagikazarmark, your problem sounds much harder, yes. If there isn't a single canonical go package that maps to the protobuf file (because each client generates their own code) then you would definitely run into problems with go_package. That might be a sufficient reason to implement this. I'll take a closer look at your PR, @jonathaningram.

[lucianjon]

Is there a chance of adding this option? golang/protobuf#544

It handles our use case pretty well of building inside a docker image without mounting in ${GOPATH}/src

[sagikazarmark]

@lucianjon I suggest opening a separate issue for that with a description what it does and why it is useful.

[spenczar]

Fixed in #102. Thanks @jonathaningram!