Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go_package option as go import path #129

Merged
merged 4 commits into from
Apr 18, 2016

Conversation

kazegusuri
Copy link
Contributor

go_package option can be specified as go import path like:

option go_package = "github.com/kazegusuri/test/v1";

Examples: Suppose I'm at github.com/kazegusuri/test dir.

$ tree 
.
├── v1
│   ├── message.pb.gw.go
│   └── message.proto
└── v2
    ├── message.pb.gw.go
    └── message.proto
$ cat v1/message.proto
syntax = "proto2";
package kazegusuri.test.v1;
option go_package = "github.com/kazegusuri/test/v1";

import "google/api/annotations.proto";

message StringMessage {
    required string msg = 1;
}

...
$ cat v2/message.proto
syntax = "proto2";
package kazegusuri.test.v2;
option go_package = "github.com/kazegusuri/test/v2";

import "google/api/annotations.proto";
import "v1/message.proto";

service EchoService {
    rpc Echo(v1.StringMessage) returns (v1.StringMessage) {
        ....
    }
}

v2/message.pb.gw.go generated from thease proto like following:

// Code generated by protoc-gen-grpc-gateway
// source: v2/message.proto
// DO NOT EDIT!

/*
Package v2 is a reverse proxy.

It translates gRPC into RESTful JSON APIs.
*/
package v2  // changed here 

import (
        "encoding/json"
        "io"
        "net/http"

        "github.com/gengo/grpc-gateway/runtime"
        "github.com/gengo/grpc-gateway/utilities"
        "github.com/golang/protobuf/proto"
        "github.com/kazegusuri/test/v1" // changed here
        "golang.org/x/net/context"
        "google.golang.org/grpc"
        "google.golang.org/grpc/codes"
        "google.golang.org/grpc/grpclog"
)
...

package v2 is from go_package option but its basename and import path is added from go_package option.

@achew22
Copy link
Collaborator

achew22 commented Apr 2, 2016

Kazegusuri,

Thanks so much for contributing to the project! It's contributions like this that make open source great.

Just a few comments to get the ball rolling:

  • I don't know if you saw that Travis CI runs automatically on pull requests. If you did you'll see that part of the build is to regenerate the .pb files on every patch. To get travis to bless your build you should update all your godeps , make clean and try building all the examples again then check in the results with your patch.
  • You should also make a new example that demonstrates this new code in action. Put it in the examples directory and compile it (just like in step 1) to make sure that future changes to grpc-gateway won't break your code.

Thanks again

@kazegusuri
Copy link
Contributor Author

I don't know if you saw that Travis CI runs automatically on pull requests. If you did you'll see that part of the build is to regenerate the .pb files on every patch. To get travis to bless your build you should update all your godeps , make clean and try building all the examples again then check in the results with your patch.

I have regenerated all pb files from proto, but the go:tips build failed while go:1.5 build succeeded. I have no idea why a part of build fails. I use go 1.6 locally and the tests succeed off cause.

You should also make a new example that demonstrates this new code in action. Put it in the examples directory and compile it (just like in step 1) to make sure that future changes to grpc-gateway won't break your code.

Just move a proto message to a new proto in other directory to test the changes instead of introducing new examples.

FYI, if go_package option is specified as go import path, generated files from the proto are stored in relative path as import path from current directory. This makes build script complicated, but the behavior is same to protoc-gen-go.

@yugui
Copy link
Member

yugui commented Apr 18, 2016

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants