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

x/build: consider opting-in to longtest builders by default for cmd/go changes, if/when they're under 15 min #42661

Open
dmitshur opened this issue Nov 17, 2020 · 10 comments
Open

x/build: consider opting-in to longtest builders by default for cmd/go changes, if/when they're under 15 min #42661

dmitshur opened this issue Nov 17, 2020 · 10 comments
Labels
Builders x/build issues (builders, bots, dashboards) GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Release Team
Milestone
Backlog

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Nov 17, 2020 •
edited

(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 go command? 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:

  • If this is something we want to do, would you prefer that "cmd/go" changes are determined by the commit subject having the "cmd/go" path present (which is up to the author), or by looking whether any files in src/cmd/go directory are modified?

Also CC @golang/release.
@dmitshur dmitshur added Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go labels Nov 17, 2020
@dmitshur dmitshur added this to the Backlog milestone Nov 17, 2020
@bcmills
Copy link
Contributor

bcmills commented Nov 17, 2020

(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.)

I think it was previously an implicit subtext of #37439. Thanks for filing it as a proper issue!

@bcmills
Copy link
Contributor

bcmills commented Nov 17, 2020 •
edited

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 x/tools 'bot triggers automatically).

I think we should use the same mechanism that we currently use to trigger x/tools, whatever that is. (I can't find it at the moment...)

@bcmills bcmills mentioned this issue Nov 18, 2020
Closed
@gopherbot
Copy link

Change https://golang.org/cl/279513 mentions this issue: dashboard: pick 4 TryBot helpers for -longtest SlowBots

gopherbot pushed a commit to golang/build that referenced this issue Dec 23, 2020
@dmitshur
dashboard: pick 4 TryBot helpers for -longtest SlowBots
7d700e0 
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>
@matloob
Copy link
Contributor

matloob commented Jan 7, 2021

This would be great! It would save me from accidentally submitting bugs in many cases...

@bcmills bcmills mentioned this issue Jan 15, 2021
Open
@toothrot toothrot added this to Planned in Go Release Team Mar 16, 2021
@gopherbot
Copy link

Change https://golang.org/cl/334889 mentions this issue: cmd/go: update error messages in tests to match CL 332573

gopherbot pushed a commit that referenced this issue Jul 15, 2021
cmd/go: update error messages in tests to match CL 332573
69728ea 
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>
@bcmills bcmills mentioned this issue Mar 9, 2022
Open
@bcmills
Copy link
Contributor

bcmills commented May 3, 2022

We should also run longtest builders by default any time there are changes in src/vendor or src/cmd/vendor. We've had too many build breaks lately due to stray edits.

@dmitshur dmitshur mentioned this issue Aug 10, 2022
Open
@mknyszek
Copy link
Contributor

mknyszek commented Dec 4, 2023 •
edited

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.

@dmitshur
Copy link
Contributor Author

dmitshur commented Dec 4, 2023 •
edited

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:

  • src/{,cmd/}go.{mod,sum}
  • src/{,cmd/}vendor/*
  • src/*_bundle.go

I defer to @bcmills for cmd/go details. Thanks @mknyszek!

@gopherbot
Copy link

Change https://go.dev/cl/547596 mentions this issue: main.star: run longtest builders in presubmit if vendor is modified

@mknyszek
Copy link
Contributor

mknyszek commented Dec 5, 2023

I sent out a CL for what you listed @dmitshur.

@bcmills That CL only seems to cover vendoring. Are there any other Go command related file paths that should enable the longtest builder? Thanks.

gopherbot pushed a commit to golang/build that referenced this issue Dec 6, 2023
@mknyszek
main.star: run longtest builders in presubmit if vendor is modified
7344bd8 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Release
Status: In Progress
Go Release Team
Planned
Milestone
Backlog
Development

No branches or pull requests
5 participants
@mknyszek @dmitshur @bcmills @gopherbot @matloob