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

x/tools: increased timeouts/flakes on windows TryBots #53493

Closed
findleyr opened this issue Jun 21, 2022 · 7 comments
Closed

x/tools: increased timeouts/flakes on windows TryBots #53493

findleyr opened this issue Jun 21, 2022 · 7 comments
Labels
FrozenDueToAge OS-Windows release-blocker Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Go1.19

Comments

@findleyr
Copy link
Contributor

https://build.golang.org/?repo=golang.org%2fx%2ftools shows six flakes on Windows since https://go.dev/cl/412779 was submitted. Given our low flake rate, I suspect a real regression. (thanks again @bcmills for leading efforts to reduce flakes).

CC @bcmills @matloob
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jun 21, 2022
@gopherbot gopherbot added this to the Unreleased milestone Jun 21, 2022
@bcmills
Copy link
Contributor

bcmills commented Jun 21, 2022

Looks like the failure mode is timeouts. We could either fall back to not indexing GOROOT at all, or maybe a special case to allow devel Go versions to be cached by default on the builders.

@findleyr
Copy link
Contributor Author

@bcmills sorry for the naive question, but isn't that CL about avoiding unnecessary work? How would it have caused more work, relative to its parent?

@bcmills
Copy link
Contributor

bcmills commented Jun 22, 2022

It also fixed a bug in the condition that triggered indexing in GOROOT at all.

@findleyr
Copy link
Contributor Author

Aha, thanks. Assuming the index is written to GOPATH, I suppose the problem is therefore that each gopls regression test has a fresh GOPATH and now incurs the cost of indexing GOROOT.

Perhaps we could add a shared GOPATH that is pre-seeded with an indexed GOROOT? Would that be possible?

I'd prefer not to disable go command behavior for our regression tests, as a big part of what the regression tests exercise is interaction with the go command.

@bcmills
Copy link
Contributor

bcmills commented Jun 22, 2022

Perhaps we could add a shared GOPATH that is pre-seeded with an indexed GOROOT? Would that be possible?

I suspect that the bottleneck at the moment is just verifying that GOROOT/src itself hasn't been invalidated since it was last indexed. We key off of runtime.Version() for that: we assume that a go built at a release or pre-release version (like go1.19 or go1.19beta1) is unmodified, whereas a go built at a development version (like devel devel go1.19-be0b2a393a Wed Jun 22 02:40:04 2022 +0000) may have local changes to the standard library on top of the named commit.

The trouble is, to detect local changes we need to walk the entire GOROOT/src directory tree to check modification times. On Linux that is fast, but on Windows it is apparently not. (At least, that's my working theory.)

I did add a GODEBUG=goindexsalt=foo environment variable to indicate that the source tree is unmodified since the last change to goindexsalt, so one option might be to set that explicitly in the x/tools tests.

Another option might be to also treat a devel version as stable if GO_BUILDER_NAME is non-empty. That would have the advantage of speeding up tests on the builders, but the disadvantage of being another way in which the builders differ from running go test locally.

@bcmills bcmills modified the milestones: Unreleased, Go1.19 Jun 22, 2022
@bcmills
Copy link
Contributor

bcmills commented Jun 22, 2022

(Marking as release-blocker for Go 1.19 because this is due to changes slated for release in cmd/go in the release.)

@gopherbot
Copy link

Change https://go.dev/cl/413634 mentions this issue: cmd/go: avoid indexing modules in GOROOT

@bcmills bcmills self-assigned this Jun 22, 2022
@rsc rsc unassigned bcmills Jun 22, 2022
@bcmills bcmills mentioned this issue Jun 27, 2022
Closed
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@jproberts
cmd/go: avoid indexing modules in GOROOT
76a1ccd 
Scanning GOROOT modules for changes appears to be causing Windows
builders to time out in x/tools tests. We may try a per-package index
instead, but for now just skip GOROOT modules (as we do for main
modules).

Fixes golang#53493.

Change-Id: Ic5bb90b4ce173a24fc6564e85fcde96e1f9b975f
Reviewed-on: https://go-review.googlesource.com/c/go/+/413634
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
@golang golang locked and limited conversation to collaborators Jul 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge OS-Windows release-blocker Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Go1.19
Development

No branches or pull requests
3 participants
@bcmills @gopherbot @findleyr