cmd/go: list command crashes on testdata packages under vendor

[linzhp]

If the Go files in a testdata package import another package found in vendor directory, running go list command in the testdata package will result in an error:

unexpected directory layout:
    import path: p
    root: /Users/zplin/gocode/src
    dir: /Users/zplin/gocode/src/go_examples/vendor/p
    expand root: /Users/zplin/gocode/src
    expand dir: /Users/zplin/gocode/src/go_examples/vendor/p
    separator: /

However, if the testdata package doesn't import any package from vendor, the go list command works fine.

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

go version go1.10.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64" GOHOSTARCH="amd64" GOHOSTOS="darwin" GOOS="darwin"

What did you do?

git clone git@github.com:linzhp/go_examples.git
cd go_examples/testdata
go list

What did you expect to see?

_/Users/zplin/gocode/src/buck_go_examples/testdata

What did you see instead?

unexpected directory layout:
    import path: p
    root: /Users/zplin/gocode/src
    dir: /Users/zplin/gocode/src/go_examples/vendor/p
    expand root: /Users/zplin/gocode/src
    expand dir: /Users/zplin/gocode/src/go_examples/vendor/p
    separator: /

[gopherbot]

Change https://golang.org/cl/107301 mentions this issue: cmd/go/internal/load: Stop loading imports when the package root is empty

[ianlancetaylor]

As I just asked on the CL, why do we expect go list in a testdata directory to work? A testdata directory can hold any random code. Yes, in some cases, go list will work, but I don't see why we should go to any extra effort to make it work if it fails.

[vitarb]

I think predictable and consistent behavior is important here.

As an example consider a package that declares code under testdata that imports something from the package itself. Go list/build would work there just fine, however if package will be used as a vendor dependency we'll start seeing failures. We've seen a lot of cases when various third party packages depend on code from testdata, this would also succeed in case if packages are available in the gopath but would fail if they are located in the vendor folder.

This causes issues when we are trying to build code and list packages filtering files based on build constraints.

It would be a bit unpractical for us to attempt changing all third party packages that impose this pattern so I would prefer landing a fix in the go compiler itself.

Do you have any concerns regarding implementation that we can address?

[ianlancetaylor]

My concern is that people with invalid imports in their testdata directory will get meaningless error messages from go list.

[linzhp]

There is a lot of tests in go_test.go that still relies on code in testdata to work. The go/build package doesn't ignore all testdata package either. In fact, it only ignores testdata package in local imports and vendor directory. In the latter case, testdata is only ignored when it is in the middle of the path. This looks inconsistent, as I don't see the reason why testdata matters only in these two cases. When testdata gets ignored, rather than explicitly returning an error, go/build returns an incomplete package information without any error, leaving its caller to guess how to deal with such incompleteness. In such case, unfortunately, it only errors out two hops down the import graph when the testdata imports a vendor package which in turn imports something else, making the error message sound meaningless.

Like @VitalyArbuzov said, it's unpredictable and inconsistent. That allows most package developers to write and use code in testdata without problems, which only occur in some non-obvious cases.

[ianlancetaylor]

I don't know if I'm convinced, but can you test removing isTestdata entirely? It doesn't make sense to me to remove the test in one place but not the other.

[linzhp]

I tried that, but it failed many tests. The current fix is the minimal change I found to pass all tests, including the one added for this issue.

[ianlancetaylor]

Thanks for testing. I think that is a very strong sign that we should not make this change. It means that packages in GOPATH that happen to use testdata directories that are like the packages in GOROOT will start to fail with this change.

[linzhp]

A lot of the tests run code in testdata directory. After removing isTestdata, some tests failed with message like this:

non-standard import "run/internal" in standard package "cmd/go/testdata/src/run"

Because "cmd/go/testdata/src/run" resides in GOROOT, without checking it is in testdata, it will be treated as a Go standard library and be disallowed from importing non-standard library run/internal in GOPATH.

So ignoring testdata in GOROOT still makes sense. That's why I kept isTestdata for GOROOT.

[linzhp]

A possible fix to the failure non-standard import "run/internal" in standard package "cmd/go/testdata/src/run" is to stop searching GOROOT for local imports, which sounds reasonable. Not sure if it will fail other tests though.

[ianlancetaylor]

I understand that GOROOT and GOPATH are different, but it still seems to me that we need to treat testdata directories the same everywhere.

As mentiond on the CL, perhaps the bug is that go/build is paying too much attention to testdata packages, somehow.

[vitarb]

I'm trying to understand if you suggest being more permissive or restrictive in this case. Do you think go toolchain shouldn't ignore testdata at all or that it shouldn't allow compiling source code inside of testdata and/or adding dependencies on it from other packages?

[gopherbot]

Change https://golang.org/cl/108036 mentions this issue: cmd/go/internal/load: ignore vendor imports from testdata