controller: followup refactoring on Build API

[ktock]

This PR addresses the following:

• Do not pass progressMode to controller.Build() for controller: strongly type the controller api #1614 (comment)
• Extract nested CommonOptions on controller API for controller: strongly type the controller api #1614 (comment)

[jedevc]

Hm, I think this also needs fixing, it's weird how we pass both the progressMode and a statusChan down into this build function, ideally we'd just pass the statusChan in.

The implementation seems not quite right in cbuild.RunBuild as well - it uses the progress mode to open a printer on os.Stderr, which we probably don't want to do (it doesn't work the same at all across local/remote).

The other confusing part here is the warnings, which are directly printed out onto os.Stderr - this isn't great, since we lose those warnings on the remote controller - but those should already be transmitted as part of the solve status? We should collect those from outside the controller (so printWarnings should probably go in controller.Build).

[ktock]

Fixed to print progress/warnings on the client side. Now (controller/build).RunBuild is configurable about the method to print the progress/warnings.

$ BUILDX_EXPERIMENTAL=1 /tmp/out/buildx build /tmp/ctx3
INFO: connecting to buildx server
[+] Building 0.9s (6/6) FINISHED                  docker-container:testbuilder
 => [internal] load .dockerignore                                         0.0s
 => => transferring context: 2B                                           0.0s
 => [internal] load build definition from Dockerfile                      0.0s
 => => transferring dockerfile: 107B                                      0.0s
 => WARN: Empty continuation line found in: RUN echo hi > /hi     echo "  0.0s
 => [internal] load metadata for docker.io/library/busybox:latest         0.6s
 => [1/3] FROM docker.io/library/busybox@sha256:c118f538365369207c12e579  0.0s
 => => resolve docker.io/library/busybox@sha256:c118f538365369207c12e579  0.0s
 => CACHED [2/3] RUN echo hi > /hi     echo "hello"                       0.0s
 => CACHED [3/3] RUN echo aaaa > /a                                       0.0s

 1 warning found (use --debug to expand):
 - Empty continuation line found in: RUN echo hi > /hi     echo "hello"

[jedevc]

Thanks @ktock 🙏 sorry for the delays in reviews, things are a bit busy atm 🎉

[ktock]

@jedevc Thank you for your time for reviewing 🙏 Fixed the patch based on the comments.

[jedevc]

Would it be possible to split the changes to the common options out into a separate PR so we can merge those in asap? Those changes look good to me, and then we can unblock some other stuff.

I'm not 100% sure about the progress changes here, the progress config doesn't seem like it'll catch everything on the remote side.

I think what we want is a way of exposing a progress.Writer over the network, so that the client can pass a progress.Writer through controller.Build. Then we don't also have to pass a statusChan, since we can use progress.Tee if we need the individual solve status events. Then everything that gets written to that progress.Writer, either on the client, the local controller, or the remote controller will end up printed (even if it's contents written directly to the progress writer by e.g. the driver loader).

[jedevc]

Essentially, something like: master...jedevc:buildx:ktock-progress-printer (the remote side isn't properly implemented, and just creates a dummy printer, and then Tees out the channel to transmit over the connection).

We should create the progress printer outside of each controller call, and pass it into the interface. Then we can lift printWarnings out of the controller entirely 🎉 (should make things a bit neater).

Only thing I'm not sure about is how to handle the description: https://github.com/jedevc/buildx/blob/ktock-progress-printer/controller/build/build.go#L185-L193 - we need to have this info ready when we create the progress writer, so we need to load the drivers client before calling RunBuild (so maybe this is blocked on doing that?).

[jedevc]

We could also supplement progress.Printer with a new Println method or similar, that just prints out the content after progress has completed - we can then use this to handle the PrintFunc case.

[jedevc]

Closing this in preference of #1737 and #1755.