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

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

Open
darkfeline opened this issue Jan 12, 2019 · 12 comments
Open

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

darkfeline opened this issue Jan 12, 2019 · 12 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@darkfeline
Copy link
Contributor

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 bcmills added the Testing An issue that has been verified to require only test changes, not just a test failure. label Jan 30, 2019
@bcmills bcmills added this to the Go1.13 milestone Jan 30, 2019
@bcmills
Copy link
Contributor

bcmills commented Jan 30, 2019

CC @robpike @rsc for path/filepath

@bcmills
Copy link
Contributor

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?

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jan 30, 2019
@darkfeline
Copy link
Contributor Author

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

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 bcmills added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jan 30, 2019
@bcmills
Copy link
Contributor

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

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

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

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

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

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.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@ALTree
Copy link
Member

ALTree commented Feb 10, 2022

Looks like there's now another std lib test that depends on GOROOT/test existing. Failure if it's deleted:

--- FAIL: TestImportTypeparamTests (0.00s)
    gcimporter_test.go:168: open /workdir/go/test/typeparam: no such file or directory
FAIL
FAIL	go/internal/gcimporter	0.482s
FAIL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

6 participants