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/buildlet: add context support #35707

Closed
bradfitz opened this issue Nov 20, 2019 · 5 comments
Closed

x/build/buildlet: add context support #35707

bradfitz opened this issue Nov 20, 2019 · 5 comments
Labels
Builders x/build issues (builders, bots, dashboards) NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@bradfitz
Copy link
Contributor

The build system in general predates the context package.

The coordinator and other pieces have started using contexts over time, but the biggest missing piece right now is the buildlet package.

This is a tracking bug to add context support to the buildlet client.
@gopherbot gopherbot added this to the Unreleased milestone Nov 20, 2019
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Nov 20, 2019
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/207908 mentions this issue: buildlet: add context argument to Client.Exec

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 20, 2019
gopherbot pushed a commit to golang/build that referenced this issue Nov 20, 2019
@bradfitz
buildlet: add context argument to Client.Exec
5a36865 
(And update all the callers in the tree.)

Updates golang/go#35707

Change-Id: I504ef73ea4ba7f8028f47a658c1cd519c7b5d986
Reviewed-on: https://go-review.googlesource.com/c/build/+/207908
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/208157 mentions this issue: buildlet: add context argument to most of the remaining Client methods

gopherbot pushed a commit to golang/build that referenced this issue Nov 20, 2019
@bradfitz
buildlet: add context argument to most of the remaining Client methods
d4e4f4e 
(And update all the callers in the tree.)

Updates golang/go#35707

Change-Id: I54769bbe374f31ae1dd07776b27818db91ce8c70
Reviewed-on: https://go-review.googlesource.com/c/build/+/208157
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@dmitshur
Copy link
Contributor

I suspect CL 207908 and CL 208157 made a lot of progress on this, and we're in a state where most buildlet methods that should have a context already have it. For the rest, we may add context when there's a need, or leave it as is otherwise. There's probably not much remaining value in keeping this issue open since it's less actionable now, so we can consider closing it as done.

CC @cagedmantis who's working with the buildlet API recently as part of #47521 and may have thoughts on this.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/407555 mentions this issue: buildlet: fix Exec to return ErrTimeout on timeout

gopherbot pushed a commit to golang/build that referenced this issue May 27, 2022
@dmitshur @gopherbot
buildlet: fix Exec to return ErrTimeout on timeout
33d38b8 
The coordinator relies on Exec reporting that the given timeout was
exceeded in order to mark a build as failed instead of retrying it.
A refactor resulted in Exec no longer doing that, despite what its
documentation promises, so fix that.

Also add a test since evidence shows that catching a regression can
be helpful.

For golang/go#42699.
Updates golang/go#35707.

Change-Id: Iacef90b83e7b81fad88a33baa6489d5157e3528f
Reviewed-on: https://go-review.googlesource.com/c/build/+/407555
Reviewed-by: Carlos Amedee <carlos@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@dmitshur
Copy link
Contributor

We can call this done. At least no use in tracking this via an open issue anymore.

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) 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
3 participants
@bradfitz @dmitshur @gopherbot