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: list command crashes on testdata packages under vendor #24854

Open
linzhp opened this Issue Apr 13, 2018 · 13 comments

Comments

Projects
None yet
5 participants
@linzhp
Contributor

linzhp commented Apr 13, 2018

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: /

linzhp added a commit to linzhp/go that referenced this issue Apr 14, 2018

@agnivade agnivade changed the title from "go list" command crash on testdata packages under vendor to cmd/go: list command crashes on testdata packages under vendor Apr 14, 2018

@gopherbot

This comment has been minimized.

gopherbot commented Apr 16, 2018

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

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Apr 17, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Apr 17, 2018

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

This comment has been minimized.

vitarb commented Apr 17, 2018

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

This comment has been minimized.

Contributor

ianlancetaylor commented Apr 17, 2018

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

@linzhp

This comment has been minimized.

Contributor

linzhp commented Apr 17, 2018

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

This comment has been minimized.

Contributor

ianlancetaylor commented Apr 17, 2018

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

This comment has been minimized.

Contributor

linzhp commented Apr 18, 2018

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

This comment has been minimized.

Contributor

ianlancetaylor commented Apr 18, 2018

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

This comment has been minimized.

Contributor

linzhp commented Apr 18, 2018

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

This comment has been minimized.

Contributor

linzhp commented Apr 18, 2018

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

This comment has been minimized.

Contributor

ianlancetaylor commented Apr 18, 2018

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

This comment has been minimized.

vitarb commented Apr 18, 2018

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

This comment has been minimized.

gopherbot commented Apr 19, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment