vendor: only vendor on emitted binaries

[heyitsanthony]

Fixes #4913

[gyuho]

Do we have script or command to vendor this way?

[heyitsanthony]

@gyuho what do you mean? it's still godep...

[gyuho]

@heyitsanthony godep save now vendors into app directory?

[heyitsanthony]

@gyuho yeah, to vendor it's cd app && godep save ./.... Just tried it and seems to work OK

[xiang90]

add a Readme.md for app dir?

[xiang90]

we need to update the build script too? this approach looks good to me.

[heyitsanthony]

all fixed ptal /cc @xiang90

[xiang90]

LGTM.

[peterbourgon]

Not LGTM. This does not fix #4913. See https://github.com/zellyn/wtf2.

Please, please join the Gophers slack, channel #vendor to discuss the problem before trying another solution. Unfortunately the vendoring situation in Go at the moment is pretty fucked (term of art) and a lot of things that look reasonable on their face are subtly broken. There's a lot of experience in the channel that can save you a lot of work and mistakes :)

[kardianos]

It looks like this moves the vendor folder from /etcd/vendor/ to /etcd/app/vendor/ @peterbourgon is correct, this won't work; it just isn't how the vendor folder works.

What this will achieve is that when building, your client packages (libs) will see a version from $GOPATH while your cmds will simultaneously also include the version from the vendor folder.

@heyitsanthony I would strongly encourage you to run govendor list -v in the etcd root after and before this change. It will show you what is importing what and show you the duplicates (you don't need to "switch" to govendor to use the list function, we both use the vendor folder).

[heyitsanthony]

@peterbourgon if https://github.com/zellyn/wtf2/cmd is vendoring the context package, why isn't it also vendoring the lib package as well?

[heyitsanthony]

@kardianos I don't see any problems with the output. What should I be looking for? I've been using -a -v build flags to confirm builds are pulling in the correct dependencies.

I reran the builds again to be certain.

vendoring turns up no duplicates with unvendored packages, but uses 157 vendored packages:

GO_BUILD_FLAGS="-a -v" ./build   >vendor-build.txt 2>&1
# this works too if you only interested in one binary: go build -a -v -o out ./app/etcdmain >vendor-build.txt 2>&1
`sort vendor-build.txt | uniq | sed 's|github.com/coreos/etcd/app/vendor/||g' | sort | uniq -c | grep -v ' 1 '`

I tried it with unvendored etcdctl as well-- no vendoring happening:

go build -a -v -o out ./etcdctl >novendor.txt 2>&1
grep vendor novendor.txt

I then tried an external repo that uses the etcd client3 library and the context package without vendoring. I ran go build -a -v 2>&1 | grep etcd got:

github.com/coreos/etcd/storage/storagepb
github.com/coreos/etcd/pkg/tlsutil
github.com/coreos/etcd/etcdserver/api/v3rpc/rpctypes
github.com/coreos/etcd/etcdserver/etcdserverpb
github.com/coreos/etcd/clientv3
github.com/coreos/etcd/clientv3/concurrency

What's missing?

[kardianos]

@heyitsanthony
In the etcd repo root:

$ govendor list -v golang.org/x/net/context
 e  golang.org/x/net/context ::/app/vendor/golang.org/x/net/context    
    ├──  l  github.com/coreos/etcd/app/vendor/github.com/coreos/etcd/client
    ├──  l  github.com/coreos/etcd/app/vendor/github.com/coreos/etcd/compactor
    ├──  l  github.com/coreos/etcd/app/vendor/github.com/coreos/etcd/discovery
    ├──  l  github.com/coreos/etcd/app/vendor/github.com/coreos/etcd/etcdserver
    ├──  l  github.com/coreos/etcd/app/vendor/github.com/coreos/etcd/etcdserver/api/v2http
    ├──  l  github.com/coreos/etcd/app/vendor/github.com/coreos/etcd/etcdserver/api/v3rpc
    ├──  l  github.com/coreos/etcd/app/vendor/github.com/coreos/etcd/etcdserver/auth
    ├──  l  github.com/coreos/etcd/app/vendor/github.com/coreos/etcd/etcdserver/etcdserverpb
    ├──  l  github.com/coreos/etcd/app/vendor/github.com/coreos/etcd/pkg/schedule
    ├──  l  github.com/coreos/etcd/app/vendor/github.com/coreos/etcd/raft
    ├──  l  github.com/coreos/etcd/app/vendor/github.com/coreos/etcd/rafthttp
    ├──  l  github.com/coreos/etcd/app/vendor/github.com/coreos/etcd/storage
    ├──  e  github.com/coreos/etcd/app/vendor/golang.org/x/net/trace
    ├──  e  github.com/coreos/etcd/app/vendor/google.golang.org/grpc
    ├──  e  github.com/coreos/etcd/app/vendor/google.golang.org/grpc/credentials
    ├──  e  github.com/coreos/etcd/app/vendor/google.golang.org/grpc/metadata
    ├──  e  github.com/coreos/etcd/app/vendor/google.golang.org/grpc/peer
    └──  e  github.com/coreos/etcd/app/vendor/google.golang.org/grpc/transport
 e  golang.org/x/net/context    
    ├──  l  github.com/coreos/etcd/client
    ├──  l  github.com/coreos/etcd/clientv3
    ├──  l  github.com/coreos/etcd/clientv3/concurrency
    ├──  l  github.com/coreos/etcd/clientv3/integration
    ├──  l  github.com/coreos/etcd/clientv3/mirror
    ├──  l  github.com/coreos/etcd/compactor
    ├── pl  github.com/coreos/etcd/contrib/raftexample
    ├──  l  github.com/coreos/etcd/contrib/recipes
    ├──  l  github.com/coreos/etcd/discovery
    ├──  l  github.com/coreos/etcd/etcdctl/ctlv2/command
    ├──  l  github.com/coreos/etcd/etcdctl/ctlv3/command
    ├──  l  github.com/coreos/etcd/etcdserver
    ├──  l  github.com/coreos/etcd/etcdserver/api/v2http
    ├──  l  github.com/coreos/etcd/etcdserver/api/v3rpc
    ├──  l  github.com/coreos/etcd/etcdserver/auth
    ├──  l  github.com/coreos/etcd/etcdserver/etcdserverpb
    ├──  l  github.com/coreos/etcd/integration
    ├──  l  github.com/coreos/etcd/pkg/schedule
    ├──  l  github.com/coreos/etcd/raft
    ├──  l  github.com/coreos/etcd/raft/rafttest
    ├──  l  github.com/coreos/etcd/rafthttp
    ├──  l  github.com/coreos/etcd/storage
    ├──  l  github.com/coreos/etcd/tools/benchmark/cmd
    ├── pl  github.com/coreos/etcd/tools/functional-tester/etcd-tester
    └──  e  golang.org/x/net/context

Please note that two different context packages will be included in the end binary. Same is true for all the other packages in the /app/vendor folder. At the very least, I don't think that's what you were intending.

[heyitsanthony]

@kardianos can this be confirmed with an end binary?

I built an unvendored binary and ran strings over it. There was nothing that looked vendored:

go build -a -v -o test ./etcdctl 2>&1 | grep context
strings out | grep context | sort | uniq -c | sort -n

I got back what looks like good evidence for no duplicates:

      1 %s appears in an ambiguous URL context
      1 *[8]context.canceler
      1 *[8]template.context
      1 *context.CancelFunc
      1 *context.Context
      1 *context.cancelCtx
      1 *context.canceler
      1 *context.emptyCtx
      1 *context.timerCtx
      1 *context.valueCtx
      1 *map.bucket[context.canceler]bool
...

[kardianos]

What does go env say that built that?

[heyitsanthony]

@kardianos

go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/anthony/go/"
GORACE=""
GOROOT="/usr/lib/go"
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GO15VENDOREXPERIMENT="1"
CC="x86_64-pc-linux-gnu-gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0"
CXX="x86_64-pc-linux-gnu-g++"
CGO_ENABLED="1"

[kardianos]

@heyitsanthony Oh! Yes, now I see what you did. Wow. Yeah, I had considered that approach, but did seriously consider using symbolic links to link to your own repo roo in the vendor folder.

The key that I was missing before was the use of symlinks both in the app dir (rename to "cmd"?) and that app/vendor/github.com/coreos/etcd -> $repo_root . A description of the approach used might be a good addition in the commit message, namely what symlinks you are creating and why. That would have saved me a bunch of time :).

@peterbourgon While I'm not sure this approach is a good idea with the use of symlinks, it does actually solve the stated issue. I'll back out of this issue conversation. Interesting approach.

[peterbourgon]

I don't see what @heyitsanthony did. Can you explain? If these commits add symlinks to the repo, I can't figure out how to get GitHub to show them. Regardless, it's a bad idea to have symlinks anywhere in your $GOPATH, as the go tool goes out of its way to ignore them.

[kardianos]

@peterbourgon
Here: https://github.com/heyitsanthony/etcd/tree/revendor/app
and here: https://github.com/heyitsanthony/etcd/tree/revendor/app/vendor/github.com/coreos

... You are correct, symlinks are discouraged. Yes, go build ./... would build the same package twice.
But it does build.

[heyitsanthony]

reworked the windows builds, @gyuho confirmed it works. merging on greenlight

[xiang90]

@peterbourgon @heyitsanthony @kardianos

Sorry for the late reply. I think the current solution works for external projects that uses etcd subpkgs as dependencies. Correct me if I am wrong.

As there is probably no prefect solution right now. The tradeoff we want is to keep other people happy and potentially do more work at etcd developer side.

The symlinks only exists inside cmd dir and might causes issues for people who tries to build etcd, but it works. We really do not want to split bin and libs into different repos and vendor the libs again for the moment.

[ingvagabund]

This does not help automatic scripts to discover directories with vendored/bundled dependencies. It is always better to assume the vendor directory is in your root directory. Once the directory can be located everywhere, how can you distinguish between /vendor/someexoticprovidername/smth/package and /cmd/somecommand/vendor/someexoticprovidername/smth/package. Can't you just symlink vendor directory from within cmd directory?

[heyitsanthony]

@ingvagabund Putting vendor/ in the root directory will cause anything using etcd as a library to use etcd's vendoring inside the etcd package (cf. #4913). This causes especially bad breakage with respect to context.Context; the etcd client will use its vendored Context whereas all the code outside of etcd will use its own Context, leading to type conflicts when crossing package boundaries. There's no idiomatic go solution to handle this and it's still a matter of debate.

There's only one vendor directory in the entire repo, what's left to distinguish?

[ingvagabund]

I see. Thanks for the #4913 link. Need to read through the discussion to get better picture.