Wrong import generation for external package

[Ganitzsh]

Hi there!

Thanks for bringing this framework to us 👍

I've been playing around with it for a bit today, and it looks like the generation of the imports has an issue. Either that or I'm just missing something so I hope you can help me out!

I'm generating the .twirp.go file using this command line:

protoc -I . --twirp_out=./out --go_out=./out service_def.proto dep1.proto dep2.proto...

In the generated twirp service file I have the following import:

import mypackagename "."

But this cannot work when I want to use the generated source from an external package, and of course I get the following error: local import "." in non-local package on my external package (implemetation) when I import a message type in my source code.

The upper part of my service is as follows:

syntax = "proto3";

package some.thing;
option go_package = "mypackagename";

[spenczar]

This looks like it could certainly be a bug in our handling of dot-imports within a package, they can be tricky.

Can you post a reproducing case with the complete text of the proto files?

[polds]

Sorry for hijacking but I just ran into I think the same issue while trying to import "google/protobuf/descriptor.proto";

Reproducible:

syntax = "proto3";
import "google/protobuf/descriptor.proto";
package twitch.twirp.example;
option go_package = "example";

extend google.protobuf.MessageOptions {
    string some_key = 1000;
}

message Hat {
  option (some_key) = "a hat!"
  int32 size = 1;
  string color = 2;
  string name = 3;
}

message Size {
  int32 inches = 1;
}

service Haberdasher {
  rpc MakeHat(Size) returns (Hat);
}

Yields the following Go import:

import proto "github.com/golang/protobuf/proto"
import fmt "fmt"
import math "math"
import google_protobuf "google/protobuf"

Which obviously yields the error: cannot find package "google/protobuf"

I verified that importing something like google/protobuf/timestamp.proto does work correctly, it's just the descriptor.proto that doesn't.

[spenczar]

@polds Can you make a separate issue? I am almost completely certain that that has a different root cause. The Go protobuf compiler has some fiddly behavior around the "Well Known Types", which I think you are running into.

[Ganitzsh]

@spenczar Okay so apparently this dot import is showing only when importing local definitions.

I tried the following:

The architecture is the following:

├── Makefile
└── testp
    ├── ext.proto
    └── foo.proto

Makefile (very basic, shouldn't be a source of trouble but it's just to be accurate):

all: gen-proto

gen-proto:
    protoc -I ./testp --twirp_out=./testp --go_out=./testp foo.proto ext.proto

foo.proto

syntax = "proto3";

package com.example.service;

option go_package = "fooservice";

import "google/protobuf/empty.proto";

import "ext.proto";

service Foo {
  rpc Bar(BarReq) returns (google.protobuf.Empty);
  rpc Ext(ExtReq) returns (ExtResp);
}

message BarReq {
  string value = 1;
}

ext.proto

syntax = "proto3";

package com.example.service;

option go_package = "fooservice";

message ExtReq {
  string value = 1;
}

message ExtResp {
  string value = 1;
}

Also I'm using the google.protobuf.Empty package and the output is the following: import google_protobuf "github.com/golang/protobuf/ptypes/empty" which works (if it can help @polds)

[rhysh]

Hi @Ganitzsh ,

The protobuf compiler doesn't enforce the same constraints that the Go compiler does—but I've found it works best when I use it in a similar style.

I had to modify your protoc invocation a little. I first ran protoc -I ./testp --twirp_out=./testp --go_out=./testp foo.proto ext.proto and got foo.proto: No such file or directory. I prefixed both .proto files with "./testp/" .. please let me know if that's not correct, since it's an important difference.

I ended up running this command:

protoc -I ./testp --twirp_out=./testp --go_out=./testp ./testp/foo.proto ./testp/ext.proto

Which gave me a foo.twirp.go including these lines:

import google_protobuf "github.com/golang/protobuf/ptypes/empty"
import fooservice "."

The import [...] "." line looks like the problem you describe.

---

A key point is to make sure you always use a single name every time you refer to a protobuf file. In your example, the file defining ExtReq is sometimes called ext.proto (in the foo.proto, in the import "ext.proto"; line) and sometimes called ./testp/ext.proto (in the protoc invocation, if I guessed correctly on how to modify the Makefile).

When I changed the import line to

import "testp/ext.proto";

and invoked the compiler as

protoc -I . --twirp_out=. --go_out=. ./testp/foo.proto ./testp/ext.proto

I got a foo.twirp.go with these lines:

import google_protobuf "github.com/golang/protobuf/ptypes/empty"
import fooservice "testp"

I think that solves the problem you described, after changing "testp" to a usable Go import path.

I refer to my .proto files by the part of their path name that comes after ${GOPATH}/src .. so if I were importing the Twirp example proto file, I'd say

import "github.com/twitchtv/twirp/example/service.proto";

[Ganitzsh]

Hey @rhysh,

Okay, now I understand better. It's a bit confusing tho, the -I flag is not really intuitive.

Is there any way to avoid this import [...] . in this case at all? I find it convenient to keep a "local" scope when it comes to generating the final files.

[rhysh]

I'd neglected to run go install on the generated files. When I move the directory to $GOPATH/src/testp and run go install testp, I get

can't load package: import cycle not allowed
package testp
        imports testp
import cycle not allowed
package testp
        imports testp

The problematic line is import fooservice "testp". The install succeeds after removing the line; the symbols weren't referenced anywhere. The generated interface correctly refers to the structs in the local package:

type Foo interface {
    Bar(context.Context, *BarReq) (*google_protobuf.Empty, error)

    Ext(context.Context, *ExtReq) (*ExtResp, error)
}

It looks like there's a bug in (*protoc-gen-twirp.twirp).generateImports. It adds imports for every message's package, so long as they come from a different file. It needs to instead check if they're from a different package.