Rename examples folder so it's not counted as a source file

[rafecolton]

Otherwise, it breaks the build:

vendor/github.com/Sirupsen/logrus/examples/hook/hook.go:12: cannot use airbrake.NewHook(123, "xyz", "development") (type *airbrake.airbrakeHook) as type "github.com/rafecolton/docker-builder/vendor/github.com/Sirupsen/logrus".Hook in argument to log.Hooks.Add:
        *airbrake.airbrakeHook does not implement "github.com/rafecolton/docker-builder/vendor/github.com/Sirupsen/logrus".Hook (wrong type for Fire method)
                have Fire(*"github.com/sirupsen/logrus".Entry) error
                want Fire(*"github.com/rafecolton/docker-builder/vendor/github.com/Sirupsen/logrus".Entry) error

[dmathieu]

You're trying to fix the wrong issue.
Your vendor folder has a capital S at sirupsen. You need to rename it to be all lowercase.

Then, both packages will properly be detected as having the same signature and your build will be passing.
See #570 (comment)

[rafecolton]

@dmathieu the problem is actually that the airbrake library isn't vendored. So it's looking for the non-vendored logrus package (github.com/sirupsen/logrus) instead of the vendored version (github.com/rafecolton/docker-builder/vendor/github.com/Sirupsen/logrus).

If I add the airbrake library to my vendor/ tree, then I get the error you are referring to regarding case-insensitive import collision. This occurs because the airbrake hook library uses the lowercase import (see this line), while everything else in my vendor tree uses upper case.

# search for uses of capital 'S'
> ag -l --go --case-sensitive 'Sirupsen' vendor/ | wc -l
      34

# search for uses of lower case 's'
> ag -l --go --case-sensitive 'Sirupsen' vendor/ | wc -l
      0

(NOTE: Search uses ag)

To fix the import reference in my code base would be impractical, as every vendored library I use also uses the uppercase import. My options are as follows:

1. Vendor the airbrake library, and modify the import statement in it to use Sirupsen
2. The change proposed here

Given the options, I definitely choose option 2. I see no reason to vendor code that's only used for example purposes, and to modify it in the vendor/ tree in order to do so. Telling go build to exclude example code is much cleaner and simpler.

[dmathieu]

Rafe,

The change to lowercase the import statement and use sirupsen, as it should be used everywhere will have to happen anyway. Lower-case is the way every package should update to.

Go will fetch dependencies whether they are from vendor or not automatically, as in they have the same signature, as long as their path is the same. In your care, they aren't because of the capital S.

In your error:

have Fire(*"github.com/sirupsen/logrus".Entry) error

See the lowercase s?

want Fire(*"github.com/rafecolton/docker-builder/vendor/github.com/Sirupsen/logrus".Entry) error

And now the upper-case. This is where the difference is.

[rafecolton]

@dmathieu I agree that ideally, myself and all vendors I use would switch to the lowercase version.

In order for me to use the lower case sirupsen and for my project to still build, all of my vendored libraries must use the same version (upper or lower) as my code.

I updated my dependencies to ensure that I'm using the latest version in case all of them have already made the update.

Looking through my vendor library, the following packages (excluding ones I own) correctly use the lowercase version:

• github.com/Azure/go-ansiterm
• gopkg.in/gemnasium/logrus-airbrake-hook.v2
• github.com/sirupsen/logrus (this repo)

...and the following use the uppercase version:

• github.com/moby/moby/... (various packages, also referenced as github.com/docker/docker)

I found moby/moby#33998 for docker to make the update. Once that PR is merged, I can update my libraries and that will fix the case insensitive issue.

---

That all being said, this PR is still valid. The files in examples/ are not required per se, as they are examples and not part of the library. Changing the directory name to _examples/ will result in them not counting as "source," which is useful, as they are only examples.

[dmathieu]

I don't think we should rename this folder.
other projects use the same naming, including big ones, and we should follow similar conventions.

Furthermore, this does add the examples into your vendor folder. But those will never be loaded. So the only thing is uses is disk space. And if you need the few kilobytes used by those text files, you have other issues 😆

[rafecolton]

But those will never be loaded.

But they are loaded: https://golang.org/pkg/go/build/#Context.Import

[rafecolton]

In your Hugo example, there are no .go source files in the example directory

[dmathieu]

Do you have benchmarks showing performance difference between the two folders names?

[rafecolton]

When named without the underscores, the examples/basic and examples/hook directories show up as packages available for import. To verify, use the go list tool at the top level of either this project or a project with this one in its vendor/ tree. For example, at the top level of $GOPATH/src/github.com/sirupsen/logrus:

> cd $GOPATH/src/github.com/sirupsen/logrus
> go list ./...
github.com/sirupsen/logrus
github.com/sirupsen/logrus/examples/basic
github.com/sirupsen/logrus/examples/hook
github.com/sirupsen/logrus/hooks/syslog
github.com/sirupsen/logrus/hooks/test

Being available packages also means that any dependencies specified in those files (i.e. gopkg.in/gemnasium/logrus-airbrake-hook.v2) get pulled into the vendor/ tree by godep when running godep save ./....

[dmathieu]

Yes, but then again, is there any actual performance impact?

[rafecolton]

Yes, running go get ./... will take longer because it needs to get the dependencies of those example packages. Also proposing that the change be made on principle.

[dmathieu]

All right, I'm reopening this because of the go get, but I'd like other opinions.

Ping @seiffert @sirupsen

[sirupsen]

I agree with @dmathieu that if this solves @rafecolton's issue (hey Rafe, long time no see), it's a secondary effect—not a primary. I'm OK with renaming the examples/, because of the go get fetching dependencies (namely Airbrake). But perhaps we should just not import Airbrake in the examples? 🤷‍♂️

[rafecolton]

An alternate solution RE not importing Airbrake (hi Simon!) would be to convert the examples into testable examples. Then, the dependencies wouldn't get pulled in with go get unless the -t flag is passed.

[dmathieu]

That would seem like a very good solution to me. Being able to point everything at godoc is always nicer.

[sirupsen]

Agree, testable examples would be great.

[seiffert]

I'm 100% 👍 for making the examples testable, too.

One thing I'd like to ask is whether we actually need the extra example with a hook other than syslog (which is included in this repo)? There is a syslog example in the package's README which should be sufficient.

[rafecolton]

#616 replaces the example files with testable examples