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/maintner/maintnerd/maintapi: TRY= comments (for SlowBots) without a preceding blank line are silently ignored #38747

Closed
laboger opened this issue Apr 29, 2020 · 3 comments
Closed

x/build/maintner/maintnerd/maintapi: TRY= comments (for SlowBots) without a preceding blank line are silently ignored #38747

laboger opened this issue Apr 29, 2020 · 3 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@laboger
Copy link
Contributor

laboger commented Apr 29, 2020

When using trybots with ppc64/ppc64le I found that the initial trybot settings for a CL are not reset if a later TRY line is specified in the same CL. An example is in
https://go-review.googlesource.com/c/go/+/229761

On the first run I specified all 4 variations that use ppc64 code generations:
TRY=ppc64,ppc64le,linux-ppc64le-osupower9,aix-ppc64

After a later change I only wanted to retest one for ppc64 big endian, and one for little endian, but all 4 were rerun.

Also, it would be nice to have term like 'allppc64' to run all 4 variations of the ppc64 trybots, or at least have something simpler to specify power9 for linux/ppc64le.
@gopherbot gopherbot added this to the Unreleased milestone Apr 29, 2020
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Apr 29, 2020
@andybons andybons added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 29, 2020
@dmitshur dmitshur changed the title x/build: trybot settings not updated if changed on a later test run in same CL x/build/cmd/coordinator: SlowBot settings not updated if changed on a later test run in same CL Apr 29, 2020
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Apr 29, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Apr 29, 2020 •
edited

After a later change I only wanted to retest one for ppc64 big endian, and one for little endian, but all 4 were rerun.

I've looked into what happened in that CL. Your comment fell into a pitfall that wasn't documented at https://golang.org/wiki/SlowBots#pitfalls.

As currently implemented, the TRY= line must be a separate paragraph from the rest of the comment text. A comment like this:

Some text here.
TRY=foo

Is currently ignored because of this line. It wouldn't be ignored if written as:

Some text here.

TRY=foo

I've documented the pitfall in commit d2a88387 as an immediate step to make things better, but the next step should be to improve TRY= parsing so that such comments aren't silently ignored. I don't think it was the intention of the original parsing code to ignore them, it's just an edge case that didn't come up until now.

We can use this as the tracking issue.

Also, it would be nice to have term like 'allppc64' to run all 4 variations of the ppc64 trybots, or at least have something simpler to specify power9 for linux/ppc64le.

This is a small change to implement, it just requires updating the slowBotAliases map. Mind filing a separate issue for this, to make tracking work easier?

@dmitshur dmitshur changed the title x/build/cmd/coordinator: SlowBot settings not updated if changed on a later test run in same CL x/build/maintner/maintnerd/maintapi: TRY= comments (for SlowBots) without a preceding blank line are silently ignored Apr 29, 2020
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 29, 2020
@dmitshur dmitshur self-assigned this May 13, 2020
@gopherbot
Copy link

Change https://golang.org/cl/241323 mentions this issue: maintner/maintnerd/maintapi: look for TRY= in inline comments

@dmitshur
Copy link
Contributor

dmitshur commented Jul 8, 2020

Removed the fixed pitfall from the SlowBots wiki page in commit 2b70f5ce.

@golang golang locked and limited conversation to collaborators Jul 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@andybons @dmitshur @gopherbot @laboger