Since `dev` merge, code no longer compiles, lots of strange changes

[dotwaffle]

Over the weekend, it would appear that much of the generated code no longer compiles, almost all of which is related to the packages golang.org/x/net/context (I'm using Go 1.10.3) or google.golang.org/grpc not being used:

./cluster.pb.go:18:8: imported and not used: "golang.org/x/net/context" as golang_org_x_net_context
./cluster.pb.go:19:8: imported and not used: "google.golang.org/grpc" as google_golang_org_grpc

If I run goimports -w on the generated files it does fix these errors, but there appear to be a large number of strange changes. for instance:

• All the fileDescriptor names moved from (e.g.) fileDescriptorCluster to fileDescriptor_cluster_c98421a8c26a96a1 (making diffs a lot less readable)
• There are a number of strange package import aliases, such as import strings "strings" and import reflect "reflect" etc -- do these really need to be aliased?
• Some of the import aliases have been renamed to strange unwieldly strings, e.g. import sortkeys "github.com/gogo/protobuf/sortkeys" is now import github_com_gogo_protobuf_sortkeys "github.com/gogo/protobuf/sortkeys" -- is that deliberate?
• Using gogofaster or gogoslick there are now a lot of XXX_ prefixed exported variables in structs, such as XXX_sizecache and XXX_NoUnkeyedLiteral, and the re-appearance of methods such as XXX_Unmarshal, XXX_Marshal, XXX_Merge, XXX_Size, XXX_DiscardUnknown etc.

I can't pinpoint where these issues began within the dev merge, but something is definitely substantially different compared to Friday. Should I be following some particular branch for a stable version?

[awalterschulze]

Yes this was a HUGE change.
These are all changes started by golang/protobuf and we merged with LOADS of effort, to keep the two repositories as compatible as possible.

See https://github.com/golang/protobuf/releases/tag/v1.1.0
Gogoprotobuf has made similar release to stay in sync with golang/protobuf
https://github.com/gogo/protobuf/releases/tag/v1.1.0
and a release before that, that you can pin to until you are ready for these changes https://github.com/gogo/protobuf/releases/tag/v1.0.0

Could you please provide a small example that reproduces your grpc problems?
Thanks for reporting and providing a work around.

Do the fileDescriptor renames bother you in the following diffs as well?

The import aliases could probably be fixed, but it is always hard, for some reason.

The XXX fields are unfortunate, but something that golang/protobuf deemed necessary. I don't think we ever had XXX_Marshal methods, so I don't know how they are reappearing?

Again thanks for reporting.

[dotwaffle]

Thanks Walter. It looks like my tests still work, so the functionality difference is minimal if not zero. The renaming of things such as fileDescriptor can be lived with, it's just that if the proto files change, every single Descriptor and EnumDescriptor method will change due to the new name!

The main issue is the addition of the additional XXX_ methods which clutter up the godoc output:

@@ -338,14 +571,38 @@ func (m *GetAllRouteClustersRequest) GetAssociationsToExpand() string {
 }

 type GetAllRouteClustersResponse struct {
-       RouteCluster []*RouteCluster `protobuf:"bytes,1,rep,name=route_cluster,json=routeCluster" json:"route_cluster,omitempty"`
+       RouteCluster         []*RouteCluster `protobuf:"bytes,1,rep,name=route_cluster,json=routeCluster" json:"route_cluster,omitempty"`
+       XXX_NoUnkeyedLiteral struct{}        `json:"-"`
+       XXX_sizecache        int32           `json:"-"`
 }

 func (m *GetAllRouteClustersResponse) Reset()      { *m = GetAllRouteClustersResponse{} }
 func (*GetAllRouteClustersResponse) ProtoMessage() {}
 func (*GetAllRouteClustersResponse) Descriptor() ([]byte, []int) {
-       return fileDescriptorCluster, []int{9}
+       return fileDescriptor_cluster_c98421a8c26a96a1, []int{9}
 }
+func (m *GetAllRouteClustersResponse) XXX_Unmarshal(b []byte) error {
+       return m.Unmarshal(b)
+}
+func (m *GetAllRouteClustersResponse) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) {
+       b = b[:cap(b)]
+       n, err := m.MarshalTo(b)
+       if err != nil {
+               return nil, err
+       }
+       return b[:n], nil
+}
+func (dst *GetAllRouteClustersResponse) XXX_Merge(src proto.Message) {
+       xxx_messageInfo_GetAllRouteClustersResponse.Merge(dst, src)
+}
+func (m *GetAllRouteClustersResponse) XXX_Size() int {
+       return m.Size()
+}
+func (m *GetAllRouteClustersResponse) XXX_DiscardUnknown() {
+       xxx_messageInfo_GetAllRouteClustersResponse.DiscardUnknown(m)
+}
+
+var xxx_messageInfo_GetAllRouteClustersResponse proto.InternalMessageInfo

I can live with these as the interface seems to be consistent, it's just noise in the documentation. I've added a goimports -w ./ line into my generation script for the time being to mitigate the compile errors.

Thanks again for looking into this!

[awalterschulze]

I would love to get rid of your goimports -w ./ as part of your build script if you could give us a small example that reproduces your problem?

I am also not a fan of the XXX methods, these come from golang/protobuf. I would advise to complain to them, because it is a bit outside of my power to get rid of these. Or maybe I made the wrong decision to stay close to the golang/protobuf implementation in the first place :(

[dotwaffle]

proto.zip
Use the above with the following protoc invocation:

% protoc \
        -I=. \
        -I="$GOPATH/src/github.com/gogo/protobuf/protobuf" \
        -I="$GOPATH/src" \
        --gogoslick_out=\
Mgogoproto/gogo.proto=github.com/gogo/protobuf/gogoproto,\
Mgoogle/protobuf/any.proto=github.com/gogo/protobuf/types,\
Mgoogle/protobuf/empty.proto=github.com/gogo/protobuf/types,\
Mgoogle/protobuf/duration.proto=github.com/gogo/protobuf/types,\
Mgoogle/protobuf/field_mask.proto=github.com/gogo/protobuf/types,\
Mgoogle/protobuf/struct.proto=github.com/gogo/protobuf/types,\
Mgoogle/protobuf/timestamp.proto=github.com/gogo/protobuf/types,\
Mgoogle/protobuf/wrappers.proto=github.com/gogo/protobuf/types,\
import_path=test_stuff,\
plugins=grpc:\
. \
proto/audit.proto

That will generate the audit.pb.go file that has the bad imports -- I've not included our entire set of proto files, but this is a subset (obfuscated slightly) that shows the problem.

[white-bear]

I have same issue. My generated files now have broken imports

import golang_org_x_net_context "golang.org/x/net/context"
import google_golang_org_grpc "google.golang.org/grpc"

They are not used in file - code contains short aliases like grpc and context:

var _ context.Context
var _ grpc.ClientConn

[bt]

I am also having the same issue; took me a while to realise this was related to the new version that was pushed since 2 days ago.

[mattkanwisher]

Running into same issue with the GPC and context imports

[bt]

Does anyone know how to pull the previous version with go get?

[taras-zak]

@bt maybe this helps:

git clone https://github.com/gogo/protobuf $GOPATH/src/github.com/gogo/protobuf
cd $GOPATH/src/github.com/gogo/protobuf && git checkout v1.0.0 && go install ./...

[bt]

@taras-zak Ah didn't know I could do it that way, thank you!

[awalterschulze]

@jmarais is working on a fix #429