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.

Sign up for GitHub

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

misc/cgo/stdio: test persistently broken on linux-arm-arm5spacemonkey builder #32407

Closed
bcmills opened this issue Jun 3, 2019 · 10 comments
Closed

misc/cgo/stdio: test persistently broken on linux-arm-arm5spacemonkey builder #32407

bcmills opened this issue Jun 3, 2019 · 10 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

bcmills commented Jun 3, 2019

(See previously #29177 and #28041.)

The current failure on the arm5spacemonkey builder is (usually) a permission denied error in misc/cgo/stdio. I'm not sure whether it's a bug (or unrealistic expectation) in the test, or a misconfiguration in the builder.

##### ../misc/cgo/stdio
go test misc/cgo/stdio.test: open /home/builder/stage0scratch/workdir/go/misc/cgo/stdio/test.test: permission denied
2019/06/02 22:52:55 Failed: exit status 1

CC @ianlancetaylor @dmitshur
@bcmills bcmills added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 3, 2019
@bcmills bcmills added this to the Go1.13 milestone Jun 3, 2019
@bcmills
Copy link
Contributor Author

bcmills commented Jun 3, 2019 •
edited

I'm not sure whether it's a bug (or unrealistic expectation) in the test, or a misconfiguration in the builder.

Perhaps it's neither: it may instead be a bug in cmd/dist.registerHostTest, which (possibly erroneously?) assumes that it can write executables in $GOROOT/misc/cgo/....

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@bcmills bcmills mentioned this issue Jul 12, 2019
Closed
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@bcmills bcmills mentioned this issue Oct 21, 2019
Closed
@esnolte
Copy link

esnolte commented Oct 21, 2019

Is there anything I can help with? The /home/builder/stage0scratch/workdir/go/misc/cgo/stdio directory is owned by builder but doesn't have write permissions on the directory.

builder@go-builder-1:~$ ls -al /home/builder/stage0scratch/workdir/go/misc/cgo/stdio
total 20
dr-xr-xr-x 3 builder builder 4096 Oct 21 12:29 .
dr-xr-xr-x 20 builder builder 4096 Oct 21 12:29 ..
-r--r--r-- 1 builder builder 1977 Oct 21 11:41 overlaydir_test.go
-r--r--r-- 1 builder builder 1504 Oct 21 11:41 stdio_test.go
dr-xr-xr-x 3 builder builder 4096 Oct 21 12:29 testdata

The missing write permission causes a permission denied error from the shell, e.g.
builder@go-builder-1:~$ touch /home/builder/stage0scratch/workdir/go/misc/cgo/stdio/foo
touch: cannot touch ‘/home/builder/stage0scratch/workdir/go/misc/cgo/stdio/foo’: Permission denied

@bcmills
Copy link
Contributor Author

bcmills commented Oct 21, 2019

@esnolte, I suspect that the fix belongs here:

go/src/cmd/dist/test.go

Lines 1009 to 1017 in a38a917

func (t *tester) runHostTest(dir, pkg string) error {
defer os.Remove(filepath.Join(goroot, dir, "test.test"))
cmd := t.dirCmd(dir, t.goTest(), "-c", "-o", "test.test", pkg)
cmd.Env = append(os.Environ(), "GOARCH="+gohostarch, "GOOS="+gohostos)
if err := cmd.Run(); err != nil {
return err
}
return t.dirCmd(dir, "./test.test", "-test.short").Run()
}

Currently runHostTest writes the test executable to the same directory as the package under test, then runs it. I don't know why that is split into two separate phases.

That behavior seems to have been present from the start, in CL 21102. Perhaps @eliasnaur or @ianlancetaylor remembers why it was done that way — perhaps it was something do to with caching?

@bcmills
Copy link
Contributor Author

bcmills commented Oct 21, 2019

Ah, that's the difference: the GOARCH and GOOS values used when compiling the test binary need to match the host arch and OS, whereas the GOARCH and GOOS values used when running the test should match the target platform.

So perhaps we just need to pick a more appropriate location to write the resulting binary.

@bcmills
Copy link
Contributor Author

bcmills commented Oct 21, 2019

From #33058 (comment), this seems to be an intentional consequence of #30316:

go/src/cmd/dist/test.go

Lines 194 to 197 in a38a917

// On a few builders, make GOROOT unwritable to catch tests writing to it.
if strings.HasPrefix(os.Getenv("GO_BUILDER_NAME"), "linux-") {
t.makeGOROOTUnwritable()
}

@bcmills
Copy link
Contributor Author

bcmills commented Nov 11, 2019

This turns out to be easy to reproduce locally by setting GO_BUILDER_NAME=linux-foo and running go tool dist test cstdio_test.

So the real question is: why doesn't it reproduce on the other linux- builders‽

@bcmills bcmills mentioned this issue Nov 11, 2019
Open
@gopherbot
Copy link

Change https://golang.org/cl/206458 mentions this issue: misc: ensure that test overlay directories are writable

@gopherbot
Copy link

Change https://golang.org/cl/206459 mentions this issue: cmd/dist: write binaries to to GOTMPDIR instead of GOROOT in runHostTest

@gopherbot
Copy link

Change https://golang.org/cl/206477 mentions this issue: cmd/go: in 'go build -o', allow the destination file to exist if it is empty

gopherbot pushed a commit that referenced this issue Nov 11, 2019
misc: ensure that test overlay directories are writable
6375fe4 
Otherwise, the test cannot create new files in the directory.

Updates #32407
Updates #30316

Change-Id: Ief0df94a202be92f57d458d4ab4e4daa9ec189b1
Reviewed-on: https://go-review.googlesource.com/c/go/+/206458
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit that referenced this issue Nov 11, 2019
cmd/go: in 'go build -o', allow the destination file to exist if it i…
c9a4b01 
…s empty

This allows the target of 'go build' to be a filename constructed
using ioutil.TempFile or similar, without racily deleting the file
before rebuilding it.

Updates #32407
Updates #28387

Change-Id: I4c5072830a02b93f0c4186b50bffa9de00257afe
Reviewed-on: https://go-review.googlesource.com/c/go/+/206477
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit that referenced this issue Nov 11, 2019
cmd/dist: write binaries to to GOTMPDIR instead of GOROOT in runHostTest
b1159ba 
Updates #32407
Updates #28387

Change-Id: I2ab933896940787b67ab5464c8213670e6e108c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/206459
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@bcmills
Copy link
Contributor Author

bcmills commented Nov 12, 2019

misc/cgo/stdio is no longer failing on this builder. On to the next failure! 😤

@bcmills bcmills closed this as completed Nov 12, 2019
@bcmills bcmills mentioned this issue Nov 12, 2019
Closed
@golang golang locked and limited conversation to collaborators Nov 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@rsc @andybons @bcmills @gopherbot @esnolte