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

cmd/go: TestScript/get_update_all changes mtimes in GOROOT/bin #37573

Closed
bcmills opened this issue Feb 28, 2020 · 15 comments
Closed

cmd/go: TestScript/get_update_all changes mtimes in GOROOT/bin #37573

bcmills opened this issue Feb 28, 2020 · 15 comments
Assignees
Labels
Milestone

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 28, 2020

2020-02-28T20:15:38-964fac3/linux-amd64-longtest

test_race_install_cgo was converted to a script test in CL 214426. The old test was not run in parallel, and asserted that go test -race -i runtime/race did not reinstall cmd/cgo.

I suspect that this flake was caused by some other test somehow overwriting cmd/cgo when it should not have.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Mar 2, 2020

@bcmills bcmills changed the title cmd/go: flake in TestScript/test_race_install_cgo cmd/go: TestScript/test_race_install_cgo is flaky Mar 2, 2020
@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Mar 8, 2020

Another failure on windows-amd64-longtest at a7f918c1 from today.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Mar 17, 2020

2020-03-16T22:31:39-2fbca94/windows-amd64-longtest
2020-03-15T00:17:52-dad94e7/windows-amd64-longtest
2020-03-13T19:06:00-be72e3c/linux-amd64-longtest
2020-03-11T19:59:39-f6a0d72/windows-amd64-longtest
2020-03-09T20:19:25-5fac45a/linux-amd64-longtest

I think this is a pairwise-incompatibility with some other test, but so far I haven't been able to track it down running each cmd/go test pairwise with test_race_install_cgo with a high -count.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 17, 2020

Change https://golang.org/cl/223745 mentions this issue: cmd/go: replace TestCgoDependsOnSyscall with a simpler script test

gopherbot pushed a commit that referenced this issue Mar 17, 2020
The existing test attempted to remove '_race' binaries from
GOROOT/pkg, which could not only fail if GOROOT is read-only, but also
interfere with other tests run in parallel.

Updates #30316
Updates #37573
Updates #17751

Change-Id: Id7e2286ab67f8333baf4d52244b7f4476aa93a46
Reviewed-on: https://go-review.googlesource.com/c/go/+/223745
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 17, 2020

Change https://golang.org/cl/223746 mentions this issue: cmd/go: add a "don't care about success" operator to script_test

@bcmills bcmills assigned bcmills and unassigned matloob Mar 17, 2020
@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Mar 17, 2020

I'm hoping that CL 223745 does the trick, but there may be other interactions lurking.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Mar 17, 2020

That CL didn't do it: https://build.golang.org/log/d9ad32ca35be46c7fac61bf609655f3e3100a803

At least the feedback was quick!

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 18, 2020

Change https://golang.org/cl/223755 mentions this issue: cmd/dist: run a checkNotStale on the builders before running tests

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Mar 18, 2020

FINALLY found it!

Doesn't seem to reproduce at all when running as a not-root user. That's why it was so [redacted] hard to track down.

For some reason, get_update_all adjusts file attributes (but not content!) on ~everything in GOROOT/pkg/tool, but only when running as root.

~/bisect/src$ sudo ../bin/go test cmd/go -run='TestScript/(get_update_all|test_race_install_cgo)'
go test proxy running at GOPROXY=http://127.0.0.1:41653/mod
--- FAIL: TestScript (0.02s)
    --- FAIL: TestScript/test_race_install_cgo (0.95s)
        script_test.go:202:
            # Tests Issue #10500 (0.938s)
            > [!race] skip
            > ! stale cmd/cgo
            > env GOBIN=$WORK/bin
            > go install mtime sametime
            > go tool -n cgo
            [stdout]
            /usr/local/google/home/bcmills/bisect/pkg/tool/linux_amd64/cgo
            > cp stdout cgopath.txt
            > exec $GOBIN/mtime cgopath.txt # get the mtime of the file whose name is in cgopath.txt
            [stdout]
            "2020-03-17T23:53:40.190152563-04:00"
            > cp stdout cgotime_before.txt
            > ? go test -race -i runtime/race
            > exec $GOBIN/mtime cgopath.txt # get the mtime of the file whose name is in cgopath.txt
            [stdout]
            "2020-03-17T23:54:37.402358823-04:00"
            > cp stdout cgotime_after.txt
            > exec $GOBIN/sametime cgotime_before.txt cgotime_after.txt
            [stderr]
            time in cgotime_before.txt (2020-03-17 23:53:40.190152563 -0400 EDT) is not the same as time in cgotime_after.txt (2020-03-17 23:54:37.402358823 -0400 EDT)[exit status 1]
            FAIL: testdata/script/test_race_install_cgo.txt:22: unexpected command failure

FAIL
FAIL    cmd/go  2.813s
FAIL
@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Mar 18, 2020

I think I've figured out what's going on.

Our own test from CL 12174 (2015) was rendered unparallelizable by our own intentional breakage of the non-intrusive behavior that test was relying on in CL 76590 (2017).

// Whether we're smart enough to avoid a complete rebuild
// depends on exactly what the staleness and rebuild algorithms
// are, as well as potentially the state of the Go build cache.
// We don't really want users to be able to infer (or worse start depending on)
// those details from whether the modification time changes during
// "go install", so do a best-effort update of the file times to make it
// look like we rewrote a.Target even if we did not. Updating the mtime
// may also help other mtime-based systems that depend on our
// previous mtime updates that happened more often.
// This is still not perfect - we ignore the error result, and if the file was
// unwritable for some reason then pretending to have written it is also
// confusing - but it's probably better than not doing the mtime update.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 18, 2020

Change https://golang.org/cl/223919 mentions this issue: cmd/go: make 'go get' and 'go install' avoid updating mtimes of binaries that are not stale

@bcmills bcmills added NeedsFix and removed Testing labels Mar 18, 2020
@bcmills bcmills changed the title cmd/go: TestScript/test_race_install_cgo is flaky cmd/go: 'go get ...' spuriously updates the mtime of cmd/cgo when it was not stale Mar 18, 2020
@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Mar 18, 2020

Retitled for the diagnosed root cause.

bcmills added a commit to bcmills/watchdir that referenced this issue Mar 18, 2020
gopherbot pushed a commit that referenced this issue Mar 18, 2020
Use that operator to make test_race_install_cgo agnostic to whether GOROOT/pkg is writable.

Updates #37573
Updates #30316

Change-Id: I018c63b3c369209345069f917bbb3a52179e2b58
Reviewed-on: https://go-review.googlesource.com/c/go/+/223746
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 18, 2020

Change https://golang.org/cl/223925 mentions this issue: cmd/go: rebuild cmd/cgo in test_race_install_cgo instead of asserting that it is not stale

gopherbot pushed a commit that referenced this issue Mar 18, 2020
Some of the darwin-amd64 builders are providing a stale environment.
Let's un-break them while we investigate.

Updates #37573
Updates #33598

Change-Id: I8b79778fe4d5aa916557c1ba89fa9c776d130b01
Reviewed-on: https://go-review.googlesource.com/c/go/+/223925
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@bcmills bcmills changed the title cmd/go: 'go get ...' spuriously updates the mtime of cmd/cgo when it was not stale cmd/go: TestScript/get_update_all changes mtimes in GOROOT/bin Mar 26, 2020
@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Mar 26, 2020

Retitled again. After discussion with @rsc, we decided that the mtime change is correct and should be kept, and instead we should prohibit the test version of the cmd/go binary from attempting to change metadata in GOROOT/bin.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 27, 2020

Change https://golang.org/cl/225897 mentions this issue: cmd/go/internal/work: disallow testgo binary from installing to GOROOT

@bcmills bcmills added the Testing label Mar 27, 2020
@gopherbot gopherbot closed this in d22086e Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.