jaegertracing / jaeger

CNCF Jaeger, a Distributed Tracing Platform
https://www.jaegertracing.io/
Apache License 2.0
20.61k stars 2.45k forks source link

dep upgrade broke `proto-install` #1258

Closed yurishkuro closed 5 years ago

yurishkuro commented 5 years ago

In order to guarantee repeatable protobuf code gen, we have a targer proto-install in the Makefile, which installs gogoproto plugins locally. Unfortunately, this target is not tested in the CI, and it has been broken by the dep upgrade.

Compare

$ git checkout 7b18bd9
$ rm -rf vendor
& glide install
$ make proto-install
go get -u github.com/golang/glog
go install \
        ./vendor/github.com/golang/protobuf/protoc-gen-go \
        ./vendor/github.com/gogo/protobuf/protoc-gen-gogo \
        ./vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway \
        ./vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
# ./vendor/github.com/mwitkow/go-proto-validators/protoc-gen-govalidators \
        # ./vendor/github.com/rakyll/statik

vs. master (or anything since 5f6af99):

$ gco master
Previous HEAD position was 7b18bd9 Add locking around partitionIDToState map accesses (#1239)
Switched to branch 'master'
Your branch is up to date with 'upstream/master'.
$ \rm -rf vendor
$ dep ensure && make proto-install
go get -u github.com/golang/glog
go install \
        ./vendor/github.com/golang/protobuf/protoc-gen-go \
        ./vendor/github.com/gogo/protobuf/protoc-gen-gogo \
        ./vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway \
        ./vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway/descriptor/grpc_api_configuration.go:9:2: cannot find package "github.com/ghodss/yaml" in any of:
    /Users/yurishkuro/golang/src/github.com/jaegertracing/jaeger/vendor/github.com/ghodss/yaml (vendor tree)
    /usr/local/opt/go/libexec/src/github.com/ghodss/yaml (from $GOROOT)
    /Users/yurishkuro/golang/src/github.com/ghodss/yaml (from $GOPATH)
make: *** [proto-install] Error 1

cc @isaachier @vprithvi

isaachier commented 5 years ago

I noticed this and was working on a fix the other day. The issue is dep prunes unused dependencies. The best idea I had was a file named vendor.go and import the packages like so:

import (
    _ "..."
)

Presumably would circumvent the pruning.

yurishkuro commented 5 years ago

Interestingly, the old glide.lock did not contain any references to github.com/ghodss/yaml either. So it might not be the pruning, but somehow we're picking up newer version of protoc-gen-grpc-gateway.

Can't we instruct dep not to prune deps for some modules?

isaachier commented 5 years ago

Figured out the canonical way to do this is to use the required list. Posting PR in a moment. See more here: https://golang.github.io/dep/docs/Gopkg.toml.html#required.

yurishkuro commented 5 years ago

Thanks for the ref. We should consider using virtualgo as well, it auto-installs dep-required packages .