build: change build handler to evaluate instead of onresult #3224
+228
−378
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jsternberg
force-pushed
the
evaluate-handler
branch
from
9bbac00 to
0725723
Compare
jsternberg
force-pushed
the
evaluate-handler
branch
from
0725723 to
9cbb185
Compare
tonistiigi
requested changes
Jun 6, 2025
There was a problem hiding this comment.
(buildx) reload
[+] Building 0.0s (0/1)
panic: send on closed channel
goroutine 188 [running]:
github.com/docker/buildx/util/progress.(*Printer).Write(0x14000067580, 0x140007f5440)
github.com/docker/buildx/util/progress/printer.go:67 +0x2c
github.com/docker/buildx/util/progress.(*prefixed).Write(0x1d2a8112?, 0xedfd53156?)
github.com/docker/buildx/util/progress/multiwriter.go:32 +0x54
github.com/docker/buildx/util/progress.(*pw).Write(0x14000746d40, 0x140007f5380)
github.com/docker/buildx/util/progress/reset.go:66 +0xa0
github.com/docker/buildx/util/progress.NewChannel.func1()
github.com/docker/buildx/util/progress/writer.go:73 +0x50
created by github.com/docker/buildx/util/progress.NewChannel in goroutine 186
github.com/docker/buildx/util/progress/writer.go:54 +0xb0
9bbac00 to
e8f1e3c
Compare
jsternberg
force-pushed
the
evaluate-handler
branch
5 times, most recently
from
d9c4d23 to
a0d2017
Compare
|
Added an additional commit to refactor the pause/unpause functionality for the printer. It seems the new evaluation method is causing some conflicts because it's possible for statuses to be sent while the printer is paused. This change should make that more reliable. |
tonistiigi
reviewed
Jun 9, 2025
| @@ -504,24 +507,28 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opts map[ | |||
| results.Set(rKey, res) | |||
|
|
|||
| if children := childTargets[rKey]; len(children) > 0 { | |||
| if err := waitForChildren(ctx, res, results, children); err != nil { | |||
| if err := waitForChildren(ctx, bh, c, res, results, children); err != nil { | |||
There was a problem hiding this comment.
Why is this connected to waitForChildren. Wouldn't it make more sense to have errgroup with one part going to waitForChildren and other to eval callback. Or can't the hook just run before?
| return nil, err | ||
| } | ||
| } else if bh != nil && bh.Evaluate != nil { | ||
| if err := bh.Evaluate(ctx, c, res); err != nil { |
There was a problem hiding this comment.
In case of ErrRestart shouldn't we return canceled from the callback. Or maybe canceled wrapped with "build restarted by client".
monitor/monitor.go
Outdated
| OnResult: func(driverIndex int, gotRes *build.ResultHandle) { | ||
| m.mu.Lock() | ||
| defer m.mu.Unlock() | ||
| Evaluate: func(ctx context.Context, c gateway.Client, res *gateway.Result) error { |
There was a problem hiding this comment.
As this doesn't use any local variables in the closure, move this to actual private method. Or you can just add Evaluate to Monitor and pass it as handler.
This changes the progress printer's pause and unpause implementation to be reentrant to prevent race conditions and it also allows the status updates to be buffered when the display is paused. The previous implementation mixed the pause implementation with the finish implementation and could cause a send on closed channel panic because it could close the status channel before it had finished being used. Now, the status channel is not closed. When the display is enabled, the status channel will be forwarded to an internal channel that is used to display the updates. When the display is paused, the status channel will have the statuses buffered in memory to be sent when the progress display is resumed. The `Unpause` method has also been renamed to `Resume`. Signed-off-by: Jonathan A. Sternberg <[email protected]>
jsternberg
force-pushed
the
evaluate-handler
branch
3 times, most recently
from
fbce087 to
9cbee6a
Compare
This changes the build handler to customize the behavior of evaluate rather than onresult and also simplifies the `ResultHandle`. The `ResultHandle` is now only valid within the gateway callback and can be used to start containers from the handler. `Evaluate` now executes inside of the gateway callback rather than having a separate implementation that executes or re-invokes the build. This keeps the gateway callback session open until the debugger has returned. The `ErrReload` for monitor has now been moved into the `build` package and been renamed to `ErrRestart`. This is because it restarts the build so the name makes a bit more sense. The actual use of this functionality is still tied to the monitor reload. Signed-off-by: Jonathan A. Sternberg <[email protected]>
jsternberg
force-pushed
the
evaluate-handler
branch
from
9cbee6a to
069c20d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes the build handler to customize the behavior of evaluate
rather than onresult and also simplifies the
ResultHandle. TheResultHandleis now only valid within the gateway callback and can beused to start containers from the handler.
Evaluatenow executes inside of the gateway callback rather thanhaving a separate implementation that executes or re-invokes the build.
This keeps the gateway callback session open until the debugger has
returned.
The
ErrReloadfor monitor has now been moved into thebuildpackageand been renamed to
ErrRestart. This is because it restarts the buildso the name makes a bit more sense. The actual use of this functionality
is still tied to the monitor reload.