x/build: consider opting-in to longtest builders by default for cmd/go changes, if/when they're under 15 min #42661
Comments
dmitshur
added
Builders
|
If the longtest builders can be made to run at roughly the same latency as other TryBots, I think we would prefer to have them trigger by default (in much the same way that the I think we should use the same mechanism that we currently use to trigger |
|
Change https://golang.org/cl/279513 mentions this issue: |
Based on the data we have so far, 4 helpers seems like a good number to pick. It provides a balance between resource use and diminishing returns. We may need to adjust it as future work for issue 42661, but a first step will be to reduce overhead in accessing historical build timing data. Fixes golang/go#37439. Updates golang/go#42661. Change-Id: I1bfddf85f2dba8b42dc7b8f8749bbf45b614e93d Reviewed-on: https://go-review.googlesource.com/c/build/+/279513 Trust: Dmitri Shuralyov <dmitshur@golang.org> Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Carlos Amedee <carlos@golang.org> Reviewed-by: Alexander Rakoczy <alex@golang.org>
|
This would be great! It would save me from accidentally submitting bugs in many cases... |
|
Change https://golang.org/cl/334889 mentions this issue: |
I neglected to run the 'longtest' builder, and the tests that cover the error message changed in CL 332573 apparently do not run in short mode. Updates #36460 Updates #42661 Change-Id: I53500ddaca8ac9f0dfaab538923b3c9a4f71665e Reviewed-on: https://go-review.googlesource.com/c/go/+/334889 Trust: Bryan C. Mills <bcmills@google.com> Run-TryBot: Bryan C. Mills <bcmills@google.com> Reviewed-by: Jay Conrod <jayconrod@google.com> TryBot-Result: Go Bot <gobot@golang.org>
|
We should also run |
|
Which of these cases is still desirable? This has become pretty easy to do with LUCI and I already sent out a CL to run linux-amd64-staticlockranking when a CL modifies runtime files so the context is all fresh. |
|
I agree with the non-cmd/go-related suggestion in #42661 (comment). That is, I suggest opting-in to linux/amd64 longtest when any of these files are modified in the main go repo:
|
|
Change https://go.dev/cl/547596 mentions this issue: |
This change enables longtest builders in presubmit if files related to vendoring are modified. Having these enabled would've caught several issues in vendored repositories and in the main Go repository in the past. While we're here, let's also ignore location filters for security presubmit. Previously we applied them and this filtered down where the staticlockranking builder would run, but if we apply them now then we'll limit where longtest builders are run. However, we always want longtest builders to run for security presubmit. Ignoring location filters entirely is a reasonable decision here: we want to be as safe as possible with security presubmit, so we want to test as many configurations as possible. The extra builder runs are unlikely to cost much and will help to ensure stability when landing security CLs upstream. For golang/go#42661. Change-Id: Ic35216722ddca4c26cccbd96da045db5c100defb Reviewed-on: https://go-review.googlesource.com/c/build/+/547596 Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> TryBot-Bypass: Michael Knyszek <mknyszek@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
|
Change https://go.dev/cl/594615 mentions this issue: |
We have a recent data point showing the regexp added in CL 547596 isn't
working as intended: no longtest builder was started in CL 593684 even
though it modified vendored files.
I suspect the problem was that {path1,path2} is pseudo-syntax we use in
Go issue titles and such, but it's not a valid regexp pattern. Use one
with a capturing group like (path1|path2) instead.
Also generalize the pattern such that it works even if there is a new
module, something other than cmd or std. Such changes are rare enough
that we'd want a longtest builder to always run.
Do the same for ssacheck while here.
For golang/go#42661.
Change-Id: I6d52193bbbd46e4e4a990356a01261f63338e5f7
Reviewed-on: https://go-review.googlesource.com/c/build/+/594615
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
|
Change https://go.dev/cl/594616 mentions this issue: |
The good news is that CL 594615 works on paths like src/cmd/go.mod; see CL 594635. The bad news, I realize now, is that I fumbled the top-level edge case. Fix it. For golang/go#42661. Change-Id: Ie993d5789b3185e822a254ee8953c25ed9055ebe Reviewed-on: https://go-review.googlesource.com/c/build/+/594616 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>
(I have a feeling @bcmills may have already filed an issue for this, but I couldn't find it. Maybe it was a comment instead.)
With the progress made on #37439 (comment) recently, it seems we can speed up longtest SlowBots to run in under 15 minutes or so. If that can be done, what is your preference on enabling longtest builders by default for changes that touch the
gocommand? CC @bcmills, @jayconrod, @matloob.In either case, we need to wait for #37439 to be completed first, and give it a little time to see if that on its own is sufficient. But I wanted to create a tracking issue to start gathering feedback eagerly.
I'm also looking for feedback on:
Also CC @golang/release.
The text was updated successfully, but these errors were encountered: