vendorcheck ./... not reporting missing package

[mholt]

vendorcheck ./... not reporting missing package

[bradleyfalzon]

Ah thanks @mholt, I see the issue.

It appears to be reproducible when the dependencies isn't installed at all. I.e. vendorcheck only alerts there's an issue when the program can compile.

[bradleyf@srv4 ~]$ docker run -it --rm golang:1.8 bash
root@54944206edc0:/go#
root@54944206edc0:/go#
root@54944206edc0:/go# mkdir src/vendtest && cd src/vendtest
root@54944206edc0:/go/src/vendtest# cat > main.go <<EOF
> package main
>
> import "github.com/pkg/errors"
>
> func main() {
>         println(errors.New("foo error"))
> }
> EOF
root@54944206edc0:/go/src/vendtest# go get github.com/FiloSottile/vendorcheck
root@54944206edc0:/go/src/vendtest# vendorcheck
root@54944206edc0:/go/src/vendtest# go get github.com/pkg/errors
root@54944206edc0:/go/src/vendtest# vendorcheck
[!] dependency not vendored: github.com/pkg/errors

You can see the first invocation of vendorcheck didn't detect the missing dependency, because it wasn't installed at all.

Could this have been the cause of your issue then?

[bradleyfalzon]

If this is the issue, then the simplest change would be to stop hiding compilation errors, and treat them as fatal:

diff --git a/main.go b/main.go
index 81ede3a..a3cc818 100644
--- a/main.go
+++ b/main.go
@@ -39,8 +39,8 @@ func main() {
 func missing(args []string, tests bool) {
        var conf loader.Config
        conf.ParserMode = parser.ImportsOnly
-       conf.AllowErrors = true
-       conf.TypeChecker.Error = func(error) {}
+       //conf.AllowErrors = true
+       conf.TypeChecker.DisableUnusedImportCheck = true
        for _, p := range args {
                if strings.Index(p, "/vendor/") != -1 {
                        // ignore vendored packages (if they are not imported by real ones)

This changes the behaviour into:

root@72c150e13374:/go/src/github.com/mholt/caddy# go install github.com/FileSottile/vendorcheck && vendorcheck ./...
/go/src/github.com/mholt/caddy/caddytls/config.go:12:2: could not import github.com/codahale/aesnicheck (cannot find package "github.com/codahale/aesnicheck" in any of:
        /go/src/github.com/mholt/caddy/vendor/github.com/codahale/aesnicheck (vendor tree)
        /usr/local/go/src/github.com/codahale/aesnicheck (from $GOROOT)
        /go/src/github.com/codahale/aesnicheck (from $GOPATH))
2017/07/23 04:28:29 couldn't load packages due to errors: github.com/mholt/caddy/caddytls

The DisableUnusedImportCheck is required because the parser is only loading the imports, and none of them are used, so the type checker reports those errors. We disable this check with the DisableUnusedImportCheck flag.

Once we add the dependency to out GOPATH, vendorcheck reports the violation correctly:

root@72c150e13374:/go/src/github.com/mholt/caddy# go get github.com/codahale/aesnicheck
root@72c150e13374:/go/src/github.com/mholt/caddy# go install github.com/FileSottile/vendorcheck && vendorcheck ./...
[!] dependency not vendored: github.com/codahale/aesnicheck

And when we vendor the dependency, we're no longer in violation:

root@72c150e13374:/go/src/github.com/mholt/caddy# mv $GOPATH/src/github.com/codahale vendor/github.com/
root@72c150e13374:/go/src/github.com/mholt/caddy# go install github.com/FileSottile/vendorcheck && vendorcheck ./...
root@72c150e13374:/go/src/github.com/mholt/caddy#

But this solution isn't ideal:

1. vendorcheck may then fail when there's other types of errors, such as unused variables
2. The cannot find package error is quite verbose, this was vendorcheck's responsibility, and it didn't handle that condition too well.

Other options could be to:

1. String match on that particular cannot find package error and show a more appropriate error
2. Document that vendorcheck needs a correct program (and show an error when it fails as discussed)

A final option could be using the AST from the parser instead of type checker to find all imports, and if a path for an import could not be found, show a more appropriate error. For example:

diff --git a/main.go b/main.go
index 81ede3a..fc89884 100644
--- a/main.go
+++ b/main.go
@@ -57,6 +57,40 @@ func missing(args []string, tests bool) {
                log.Fatal(err)
        }

+       importToFS := make(map[string]string) // map import path to fs path
+       for _, pi := range prog.AllPackages {
+               if len(pi.Files) == 0 {
+                       continue // virtual stdlib package
+               }
+               // Strip vendor prefix, import paths could be
+               // github.com/pkg/errors            <- not vendored
+               // foo/vendor/github.com/pkg/errors <- vendored
+               ipath := pi.String()
+               if indx := strings.LastIndex(pi.String(), "vendor/"); indx != -1 {
+                       ipath = pi.String()[indx+len("vendor/"):]
+               }
+               afile := prog.Fset.File(pi.Files[0].Pos()).Name()
+               importToFS[ipath] = filepath.Dir(afile)
+       }
+
+       for _, pi := range prog.Imported {
+               for _, file := range pi.Files {
+                       for _, imprt := range file.Imports {
+                               imprtPath := strings.Trim(imprt.Path.Value, `"`)
+                               fsPath, ok := importToFS[imprtPath]
+                               if !ok {
+                                       fmt.Printf("could not determine path for import %v\n", imprtPath)
+                               } else {
+                                       if !strings.Contains(fsPath, "/vendor/") {
+                                               fmt.Printf("dependency %v is not vendored (found at %v)\n", imprtPath, fsPath)
+                                       } else {
+                                               fmt.Printf("dependency %v *is* vendored (found at %v)\n", imprtPath, fsPath)
+                                       }
+                               }
+                       }
+               }
+       }
+
        exitCode := 0
        for _, pi := range prog.AllPackages {
                if len(pi.Files) == 0 {

Changes the behaviour to:

root@72c150e13374:/go/src/vendtest# vendorcheck ./...
could not determine path for import github.com/pkg/errors
root@72c150e13374:/go/src/vendtest# go get ./...
root@72c150e13374:/go/src/vendtest# vendorcheck ./...
dependency github.com/pkg/errors is not vendored (found at /go/src/github.com/pkg/errors)
[!] dependency not vendored: github.com/pkg/errors
root@72c150e13374:/go/src/vendtest# mkdir -p vendor/github.com/pkg && mv $GOPATH/src/github.com/pkg/errors vendor/github.com/pkg/
root@72c150e13374:/go/src/vendtest# vendorcheck ./...
dependency github.com/pkg/errors *is* vendored (found at /go/src/vendtest/vendor/github.com/pkg/errors)
root@72c150e13374:/go/src/vendtest#

The proposed patch still has at least two edge cases (stdlib imports and sub package imports would be incorrectly marked as a violation), but that's the general idea. It could replace then replace the existing for _, pi := range prog.AllPackages loop in missing function.

[bradleyfalzon]

This looks like it could just be due to vendorcheck doesn't, by default, check for dependencies in tests, and can be enabled with the -t option.

$ vendorcheck --help
Usage: vendorcheck [-t] package [package ...]
  -t    also check dependencies of test files
  -u    print unused vendored packages instead

[mholt]

That's what I thought at first too, but the file the error is reported from (caddytls/config.go) is not a test file.

[mholt]

@bradleyfalzon Wow, yes, I do believe that is accurate: the Go tool reports that the dependency was not found in my GOROOT or GOPATH at all. Nice work digging into this. My preference leans toward the AST since it doesn't rely on the program being correct, BUT I do understand the complexities of that. I'll let @FiloSottile give his feedback.

[bradleyfalzon]

Thanks @mholt, I'm of two minds, I think hiding the error and documenting that vendorcheck requires a correct program is probably enough. The scenario where not all dependencies are there is probably rare, and if it fails to compile its clear something wasn't vendored.

Also, the above AST method probably has some edge cases I haven't thought of.

[mholt]

For a near-term solution, I'd definitely be OK going with requiring a correct program. Much simpler and less error-prone than trying to hunt down the problems of traversing the AST...