cmd/go: loading dependencies with go test -i does not correctly handle *.go import paths

[Helcaraxan]

What version of Go are you using (go version)?

$ go version
go version go1.13.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/Users/USERNAME/go/bin"
GOCACHE="/Users/USERNAME/Library/Caches/go-build"
GOENV="/Users/USERNAME/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/USERNAME>/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/USERNAME/go/src/github.com/resgateio/resgate/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/1y/y8rl2vbs3jzc3y9lsfms_5qm0000gn/T/go-build160952166=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Running go test -i to prefetch dependencies for tests.

What did you expect to see?

The command ending with a 0 exit-code.

What did you see instead?

An error message:

can't load package: named files must be .go files: bytes

[Helcaraxan]

Issue was detected on this repo when modifying its CI.

My preliminary analysis has given me the following:

1. The error is triggered here.
2. This called from the following stack:
   • go test -i handling
   • Package loading
   • Packages resolution

The particular issue lies in this piece of the PackagesAndErrors function. When this is reached via the above described stack, and if one of the dependencies has an import path ending with *.go (while not being a file - example here), this results in all dependencies wrongly being considered as individual Go files.

I see two issues here, that will likely need to be addressed separately:

1. The code I referenced above should I believe be more selective and do some careful parsing of all patterns strings passed to PackagesAndErrors(). As a result it should still return a potential multi-element slice of *Package objects. Any non *.go$ patterns should be parsed as would be done in the rest of the function. Any *.go$ patterns should be grouped per containing package and those groups then individually passed to the underlying GoFilesPackage() function.
2. Packages with import paths ending in .go would still likely result in an error even with the improved parsing above. We'll need to come up with a correct way of handling such an edge-case.

NB: the second code path through which this specific case could be hit is via go run.

cc @jirenius - as he raised this issue on Slack - https://gophers.slack.com/archives/C9BMAAFFB/p1570004894155200

cc @Skarlso @bcmills as it looks like you were the last to touch / review the code-path in question.

[Helcaraxan]

If people agree with my assessment above I'd be happy to deal with issue 1. and raise a PR. If there are suggestions on how we could locally deal with 2. I'd take those as well and raise another PR for that.

[Skarlso]

Yep, I remember this. It was so that errors of these kinds are surfaced properly in the Err section. This fixed some of the logic of the compare too because it was only ever checking the first file and never the second.

Is it possible that the test which triggers it is wrong?

[Helcaraxan]

I am not entirely sure what test covers this (nothing in the obvious pkg_test.go). Do you have a pointer?

My bad. It looks to be in test.go.

[Skarlso]

Wait, sorry, I don't understand the question. Also, read Cl as Ci not CL. :D I thought this problem occurd on a testing infrastructure.

[Helcaraxan]

No the CI was indeed for Continuous Integration (but on the project where the issue was detected, not go CI). And am looking through the test.go file right now to see if the test contains any issues and / or covers these edge-cases.

[Skarlso]

Okay, let me know if you find something.

[Helcaraxan]

It does not appear that the test you added was wrong @Skarlso. This issue involves an edge-case that does not yet appear to be covered by tests. I am writing new tests right now to get a reproduction after which I can start looking at writing the actual fix.

I was able to to trigger the error as well through go list. Simply supply mixed patterns:

$ go list ./server/... ./nats/nats.go
can't load package: named files must be .go files: ./server/...

$ go list ./server/...
github.com/resgateio/resgate/server
github.com/resgateio/resgate/server/codec
github.com/resgateio/resgate/server/mq
github.com/resgateio/resgate/server/rescache
github.com/resgateio/resgate/server/reserr
github.com/resgateio/resgate/server/rpc

$ go list ./nats/nats.go
command-line-arguments

[bcmills]

Is this a duplicate of #32483? (Did it work with Go 1.12.10, and is it fixed at head?)

[Helcaraxan]

Let me check with a master version of go.

[Helcaraxan]

Have verified with the latest master and can still reproduce the issue using the go list approach shown above.

The #32483 covers issue 2. that I identified above. I'll focus on creating a fix for the issue related to specifying mixed patterns to PackagesAndErrors().

[gopherbot]

Change https://golang.org/cl/198459 mentions this issue: cmd/go: fix package listing with mixed patterns

[Helcaraxan]

@andybons FWIW, this issue is not specific to modules but independent of whether modules are used or not.

[Helcaraxan]

@gopherbot please backport to 1.13, this was a regression

[gopherbot]

Backport issue(s) opened: #34694 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

[gopherbot]

Change https://golang.org/cl/198957 mentions this issue: cmd/go: fix listing of ambiguous paths