Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Helcaraxan opened this issue Oct 2, 2019 · 16 comments
Labels
FrozenDueToAge modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@Helcaraxan
Copy link
Contributor

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
Copy link
Contributor Author

Helcaraxan commented Oct 2, 2019

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:

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
Copy link
Contributor Author

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
Copy link
Contributor

Skarlso commented Oct 2, 2019

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
Copy link
Contributor Author

Helcaraxan commented Oct 2, 2019

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
Copy link
Contributor

Skarlso commented Oct 2, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor

Skarlso commented Oct 2, 2019

Okay, let me know if you find something.

@Helcaraxan
Copy link
Contributor Author

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
Copy link
Contributor

bcmills commented Oct 2, 2019

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

@Helcaraxan
Copy link
Contributor Author

Let me check with a master version of go.

@Helcaraxan
Copy link
Contributor Author

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().

@andybons andybons added modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 2, 2019
@andybons andybons added this to the Unplanned milestone Oct 2, 2019
@gopherbot
Copy link

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

@Helcaraxan
Copy link
Contributor Author

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

@Helcaraxan
Copy link
Contributor Author

@gopherbot please backport to 1.13, this was a regression

@gopherbot
Copy link

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
Copy link

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

gopherbot pushed a commit that referenced this issue Oct 7, 2019
Passing ambiguous patterns, ending in `.go`, to `go list` results in them
being interpreted as Go files despite potentially being package references.
This can then result in errors on other package references.

The parsing logic is modified to check for a locally present file
corresponding to any pattern ending in `.go`. If no such file is present
the pattern is considered to be a package reference.

We're also adding a variety of non-regression tests that fail with the
original parsing code but passes after applying the fix.

Updates #34653
Fixes #34694

Change-Id: I073871da0dfc5641a359643f95ac14608fdca09b
GitHub-Last-Rev: 5abc200
GitHub-Pull-Request: #34663
Reviewed-on: https://go-review.googlesource.com/c/go/+/198459
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
(cherry picked from commit 33683f1d64df0cef2c598a84b741abb5af8abe5e)
Reviewed-on: https://go-review.googlesource.com/c/go/+/198957
Reviewed-by: Jay Conrod <jayconrod@google.com>
@golang golang locked and limited conversation to collaborators Oct 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants