Update imports to reference vendored packages

[lmars]

The dependencies have already been vendored using godep (see 5d8ca38) but they were not being referenced.

This is the result of running godep save -r ./....

[kouno]

I'm not sure I'm following. I looked at the documentation again and I thought it is okay as long as you run any go command prefixed with godep (like bundle exec would do for ruby but with more leniency).

On the other hand, it's true that you may have a missing package using the godep prefix because it would search your GOPATH... I still haven't figured out how we can automatically detect a missing dependency in the Godep folder...

All in all, I would prefer to not reference the godep folder at all.

WDYT?

[lmars]

godep has two strategies, vendoring or prefixing all commands with godep.

Using the godep prefix parses Godeps.json, go gets all the packages into TMPDIR, then alters GOPATH to point at those.

Vendoring downloads all the dependent packages into Godeps/_workspace and updates all the imports to point at the vendored packages, so then you don't need godep installed to use the library.

So it looks like the intention in this repository was to use the vendored strategy, so this PR fixes that.

If we want to use the godep prefix strategy, then we should remove the vendored packages, but I prefer the vendoring as it removes a dependency for people just wanting to run go test for example (like me 😄).

[kouno]

Using the godep prefix parses Godeps.json, go gets all the packages into TMPDIR, then alters GOPATH to point at those.

AFAIK godep doesn't download any package unless you use godep get. I verified this by deleting my Godeps/_workspace/ and $GOPATH/src/github.com/flynn/flynn/ and tried running godep go test ./rest/ which resulted in:

[17:09:49] kouno:ably-go git:(master*) $ godep go test ./rest
rest/auth.go:14:2: cannot find package "github.com/flynn/flynn/pkg/random" in any of:
        /usr/local/Cellar/go/1.4/libexec/src/github.com/flynn/flynn/pkg/random (from $GOROOT)
        /Users/kouno/.gocode/src/github.com/ably/ably-go/Godeps/_workspace/src/github.com/flynn/flynn/pkg/random (from $GOPATH)
        /Users/kouno/.gocode/src/github.com/flynn/flynn/pkg/random
godep: go exit status 1

The only thing that godep does is prefixing GOPATH to add godep path at the very front of your GOPATH.

[16:58:41] kouno:realtime git:(master*) $ godep go test .
Running Suite: Realtime Suite
=============================
Random Seed: 1425315524
Will run 1 of 1 specs

GOPATH: /Users/kouno/.gocode/src/github.com/ably/ably-go/Godeps/_workspace:/Users/kouno/.gocode

[...]

[16:58:51] kouno:realtime git:(master*) $ go test .
Running Suite: Realtime Suite
=============================
Random Seed: 1425315532
Will run 1 of 1 specs

GOPATH: /Users/kouno/.gocode

So IMO, as long as we run go commands with the godep prefix it will lookup our vendored files by default anyway. Which means we don't have to update any line of code in ably-go

Note that as I said in a previous comment, the only downside is that it will look up in GOPATH too, which means it won't tell you if you are missing a dependency in your vendored files...

[kouno]

I don't think you are supposed to do this. The godep prefix will make it so that the Godep folder is the first being looked into.

Also it would be quite a pain when running godep update my_package/path to have to update this line again. And we would have to make sure that any new import is actually going to the Godeps folder.

[lmars]

it would be quite a pain when running godep update my_package/path to have to update this line again. And we would have to make sure that any new import is actually going to the Godeps folder.

godep knows how to rewrite the paths and add code to the Godeps directory (I used godep to make this change).

[kouno]

Which command? 😱

[lmars]

godep save -r ./...

[lmars]

AFAIK godep doesn't download any package unless you use godep get.

Oh yes I am wrong, godep has changed to not use a tmp dir anymore (tools/godep@ddd7fbf).

as long as we run go commands with the godep prefix it will lookup our vendored files by default anyway ... the only downside is that it will look up in GOPATH too, which means it won't tell you if you are missing a dependency in your vendored files...

But why require the use of godep at all? Why not just reference the vendored imports directly (i.e. than change I have made here)? Then there is nothing to forget, and you wont miss dependencies.

[kouno]

But why require the use of godep at all? Why not just reference the vendored imports directly (i.e. than change I have made here)? Then there is nothing to forget, and you wont miss dependencies.

I would be okay with that if this would only affect our code but you are modifying vendor files which is a no-no in my book 😉. See my comment here: #3 (comment).

Also at this point I would say that it's the same as bundle exec. It's up to you to prefix it on your command line or configure your environment to always use it. Which is why we had scripts like https://github.com/gma/bundler-exec. I personally like to have a git ignored bin/go or bin/whatever if I want to have command overwritten in a specific folder...

[lmars]

@kouno I will leave it up to you whether to accept or reject this, I can see your concerns and I have no issue using the godep prefix as long as it is documented in the README that it is necessary.

[kouno]

Just saw your last comment... If godep supports that by default that's better and a bit different, even if I personally don't really like this philosophy.

I'll sleep on it and decide tomorrow 😄.

[kouno]

Ok so apparently the gods must have heard my painful cry over the internet because a new (or another?) discussion popped up in the golang google group (https://groups.google.com/forum/#!topic/golang-dev/nMWoEAG55v8).

Google is, as you stated, using this vendoring method (without godep apparently). But I really feel like this is an approach that is dirty in a few ways because of all the duplication it creates and because it modifies imports in the vendored files (even if that is done with a tool that makes it reproducible without too much hassle).

I'm going to leave this PR open as a reminder that it's an issue that needs to be fixed, and maybe when we get to go 1.5 we will have a consensus on the method the go team / community agreed on.

In the meantime, use the godep prefix. I'll update the README to reflect this choice.

[mattheworiordan]

@kouno it's interesting to read the discussion on Go 1.5 and it sounds like this is clearly an issue that does need to be standardised. Personally however, I think removing the need to run godep is the right way to go, and we should instead add an issue when Go 1.5 is released to follow whatever path is recommended then. I would prefer not to leave things in a state we are not happy with on the basis that we hope it will be fixed in the next release.

BTW. I agree the import path rewrite feels wrong, however what it does offer is robustness and ease of use for people using this library, which is arguably paramount for us.

[kouno]

@mattheworiordan I think you have a point there.

I'm merging @lmars changes for the sake of keeping the project stable for everyone. As a side note I added a pre-commit hook on my machine to avoid importing non-vendored packages.

#!/bin/bash

set -e

unset GIT_DIR

cd "$(godep path)/../.."
godep save -r ./...
godep_imports="$(git diff | grep 'Godeps/_workspace' | wc -l)"

if [ "$godep_imports" -gt 0 ]; then
  cat <<\EOF
ERROR: Some Go dependencies have been updated with `godep save -r ./...`
ERROR: Please add them to your commit.
EOF
  exit 1
fi

exit 0

I'll keep an eye on the go 1.5 discussion, and hopefully we can get this sorted in the (near?) future.