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

Jump to bottom

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

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

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

linzhp opened this issue Apr 13, 2018 · 13 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@linzhp
Copy link
Contributor

linzhp commented Apr 13, 2018 •
edited

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
@linzhp
Fixed golang#24854
0b9af57
@agnivade agnivade changed the title "go list" command crash on testdata packages under vendor cmd/go: list command crashes on testdata packages under vendor Apr 14, 2018
@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 14, 2018
@linzhp linzhp mentioned this issue Apr 15, 2018
Closed
@gopherbot
Copy link

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

ianlancetaylor commented Apr 17, 2018 •
edited

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

vitarb commented Apr 17, 2018 •
edited

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

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

@linzhp
Copy link
Contributor Author

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

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

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

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

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

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

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

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?

@linzhp linzhp mentioned this issue Apr 19, 2018
Closed
@gopherbot
Copy link

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

@dominikh dominikh mentioned this issue Aug 13, 2019
Closed
@stamblerre stamblerre mentioned this issue Mar 7, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
5 participants
@linzhp @vitarb @agnivade @ianlancetaylor @gopherbot