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

path/filepath: TestBug3486 fails if GOROOT/test is removed #29713

Open
darkfeline opened this Issue Jan 12, 2019 · 11 comments

Comments

Projects
None yet
3 participants
@darkfeline
Copy link

darkfeline commented Jan 12, 2019

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

$ go version
go version go1.11.4 linux/amd64

Does this issue reproduce with the latest release?

Yes, 1.11.4 is the latest (not counting betas?).

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ionasal/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ionasal/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build457575040=/tmp/go-build -gno-record-gcc-switches"

What did you do?

mkdir -p /tmp/tmp3
cd /tmp/tmp3
go mod init example.com/test
cat >main.go <<EOF
package foo

import (
	_ "path/filepath"
)
EOF
go test all

What did you expect to see?

All tests pass

What did you see instead?

Test failures:

--- FAIL: TestBug3486 (0.00s)
    path_test.go:1268: lstat /usr/lib/go/test: no such file or directory
FAIL
FAIL	path/filepath	0.013s

(I found a bug for the same test, but the failure mode is different: #5863)

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Jan 30, 2019

CC @robpike @rsc for path/filepath

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Jan 30, 2019

How did you install your copy of the go toolchain?

The test is looking for the test subdirectory of runtime.GOROOT():

root, err := filepath.EvalSymlinks(runtime.GOROOT() + "/test")

The test passes when run with the go1.11.5 binary obtained via go get golang.org/dl/go1.11.5:

scratch$ go mod init golang.org/issue/scratch
go: creating new go.mod: module golang.org/issue/scratch

scratch$ go1.11.5 test -v -run=TestBug3486 path/filepath
=== RUN   TestBug3486
--- PASS: TestBug3486 (0.00s)
PASS
ok      path/filepath   0.023s

And the directory is clearly present in the official distribution tarball, at least for linux/amd64:

scratch$ curl -O https://dl.google.com/go/go1.11.5.src.tar.gz
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 20.1M  100 20.1M    0     0  55.6M      0 --:--:-- --:--:-- --:--:-- 55.4M

scratch$ tar -zxf go1.11.5.src.tar.gz

scratch$ ls -dF go/test
go/test/

So as far as I can tell, the most likely explanation is some sort of packaging issue. Perhaps your distro maintainer pruned files out of GOROOT without realizing that the test suite requires them?

@darkfeline

This comment has been minimized.

Copy link
Author

darkfeline commented Jan 30, 2019

I'm using the official go package for Arch Linux. It does seem to omit the test dir.

I can file a bug with the distro, but is the test dir required as part of a Go install? I have had no issues with it missing (other than this test failure). It also seems wrong that a stdlib test requires the toolchain/runtime test dir to exist. And furthermore end users almost certainly would have no need for toolchain/runtime tests for a Go install?

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Jan 30, 2019

is the test dir required as part of a Go install?

There is currently nothing in the documentation that says it can be removed, so I'd say that yes, it is required.

I would argue that anything that is safe to prune out for individual distros should also be pruned out of the standard release tarball (see #27151), and conversely, anything that is not pruned out of the standard release should not be pruned out in individual distros either.

If you think we should prune out the test directory (and make the tests pass without it), that's probably something to mention on #27151.

@bcmills bcmills changed the title path/filepath: TestBug3486 fails with lstat /usr/lib/go/test: no such file or directory path/filepath: TestBug3486 fails if GOROOT/test is removed Jan 30, 2019

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Jan 30, 2019

Marking as NeedsDecision: we need to determine whether ordinary tests in the standard library should depend on the existence of GOROOT/test.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Jan 30, 2019

I think it's perfectly fine for standard library tests to require the existence of GOROOT/test, just as on some systems they require the existence of GOROOT/lib.

In general, if you don't have a complete distribution, you shouldn't expect the tests to pass.

This is not to say that Arch Linux is doing the wrong thing by omitting the test directory from their default installation of Go. It's only to say that once they've made that decision, there is no reason for them to expect the standard library tests to continue to pass.

@darkfeline

This comment has been minimized.

Copy link
Author

darkfeline commented Jan 31, 2019

I believe this is the only stdlib test that requires GOROOT/test though (since I only ran into 2-3 failures, the others of which were unrelated to GOROOT/test). From what I can tell GOROOT/test is for toolchain/runtime tests. For a single stdlib test to add a dependency on all of GOROOT/test for what I suspect is just a test that needs to stat an arbitrary dir (will look at the test later) seem rather excessive.

Edit: Looking at

func TestBug3486(t *testing.T) { // https://golang.org/issue/3486

I think it could be easily rewritten to not rely on GOROOT/test. I'd be happy to write a patch if a Go dev agrees that it makes sense.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Jan 31, 2019

It is excessive and I don't mind changing it for the 1.13 release. But the general principle still holds: if the distro doesn't include the complete Go release, then you should not expect the standard library tests to pass. That just isn't an invariant we have any reason to maintain, nor are we going to test that it works.

@darkfeline

This comment has been minimized.

Copy link
Author

darkfeline commented Jan 31, 2019

I understand and that's fine.

However if you'll entertain a little philosophical discussion, I don't see when it should ever be the case that stdlib tests rely on any files outside of the src/ dir. If any stdlib test needs any additional files, shouldn't they be included as testdata like tests for regular Go packages would? Again, I understand if that invariant is not formally maintained, but would there ever be a reason to violate that invariant?

That just isn't an invariant we have any reason to maintain

Assuming there are no such reasons to violate this invariant (which of course could be a mistaken assumption on my part), one potential reason to maintain this invariant is that if the "lite" Go distribution idea in #27151 gains traction, a "lite" Go distribution that omits the test/ directory would still be able to run stdlib tests as part of a normal go test all workflow. If the only thing preventing this is spending a little effort to be conscientious about putting stdlib test files in testdata, it seems like a worthy investment (again, hypothetically).

I have already filed a downstream bug, so I'm not pushing this idea with the intent of changing Go policy for the sake of fixing an Arch Linux bug. I'd just like to clarify my own thoughts about this.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Feb 1, 2019

This kind of thing is only maintainable if it is tested. So if we decide that we care about a "lite" Go distribution, then our first step would be to arrange for our automated testers to remove the appropriate directories and then run all the tests. Unless and until we take that step, this simply isn't worth worrying about.

@darkfeline

This comment has been minimized.

Copy link
Author

darkfeline commented Feb 4, 2019

I took a little bit of time to see if I could fix this, and ironically, the reason this test is using the GOROOT/test subdir is to work around another stdlib test bug #28387 that is also causing go test all to fail.

This test was originally written for #3486 and only check GOROOT/lib and GOROOT/src. It was changed to use GOROOT/test in 5dc8c4d because the os tests write files into GOROOT/src causing a race. But it is exactly this behavior also causing os tests to fail in #28387.

So I will postpone fixing this bug until #28387 is resolved.

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