Skip to content

Commit 4b4c263

Browse files
jedevcjedevc
committed
controller: refactor progress api
Refactor the progress printer creation to the caller-side of the controller api. Then, instead of passing around status channels, we can simply pass around the higher level interface progress.Writer, which allows us to correctly pull warnings out of the remote controller. Signed-off-by: Justin Chadwell <me@jedevc.com>
1 parent 48b733d commit 4b4c263

File tree

10 files changed

+249
-198
lines changed

10 files changed

+249
-198
lines changed

‎commands/build.go‎

Lines changed: 88 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package commands
22

33
import (
4+
"bytes"
45
"context"
56
"encoding/base64"
67
"encoding/csv"
@@ -15,6 +16,7 @@ import (
1516

1617
"github.com/containerd/console"
1718
"github.com/docker/buildx/build"
19+
"github.com/docker/buildx/builder"
1820
"github.com/docker/buildx/controller"
1921
cbuild "github.com/docker/buildx/controller/build"
2022
"github.com/docker/buildx/controller/control"
@@ -35,8 +37,11 @@ import (
3537
"github.com/docker/docker/pkg/ioutils"
3638
"github.com/moby/buildkit/client"
3739
"github.com/moby/buildkit/exporter/containerimage/exptypes"
40+
"github.com/moby/buildkit/solver/errdefs"
3841
"github.com/moby/buildkit/util/appcontext"
3942
"github.com/moby/buildkit/util/grpcerrors"
43+
"github.com/moby/buildkit/util/progress/progressui"
44+
"github.com/morikuni/aec"
4045
"github.com/pkg/errors"
4146
"github.com/sirupsen/logrus"
4247
"github.com/spf13/cobra"
@@ -200,7 +205,22 @@ func runBuild(dockerCli command.Cli, in buildOptions) error {
200205
if err != nil {
201206
return err
202207
}
203-
progress, err := in.toProgress()
208+
209+
progressMode, err := in.toProgress()
210+
if err != nil {
211+
return err
212+
}
213+
b, err := builder.New(dockerCli,
214+
builder.WithName(opts.Builder),
215+
builder.WithContextPathHash(opts.ContextPath),
216+
)
217+
if err != nil {
218+
return err
219+
}
220+
printer, err := progress.NewPrinter(context.TODO(), os.Stderr, os.Stderr, progressMode, progressui.WithDesc(
221+
fmt.Sprintf("building with %q instance using %s driver", b.Name, b.Driver),
222+
fmt.Sprintf("%s:%s", b.Driver, b.Name),
223+
))
204224
if err != nil {
205225
return err
206226
}
@@ -211,10 +231,14 @@ func runBuild(dockerCli command.Cli, in buildOptions) error {
211231
return errors.Wrap(err, "removing image ID file")
212232
}
213233
}
214-
resp, _, err := cbuild.RunBuild(ctx, dockerCli, opts, os.Stdin, progress, nil)
234+
resp, _, err := cbuild.RunBuild(ctx, dockerCli, opts, os.Stdin, printer)
235+
if err1 := printer.Wait(); err == nil {
236+
err = err1
237+
}
215238
if err != nil {
216239
return err
217240
}
241+
printWarnings(os.Stderr, printer.Warnings(), progressMode)
218242
if in.quiet {
219243
fmt.Println(resp.ExporterResponse[exptypes.ExporterImageDigestKey])
220244
}
@@ -517,7 +541,22 @@ func launchControllerAndRunBuild(dockerCli command.Cli, options buildOptions) er
517541
if err != nil {
518542
return err
519543
}
520-
progress, err := options.toProgress()
544+
545+
progressMode, err := options.toProgress()
546+
if err != nil {
547+
return err
548+
}
549+
b, err := builder.New(dockerCli,
550+
builder.WithName(opts.Builder),
551+
builder.WithContextPathHash(opts.ContextPath),
552+
)
553+
if err != nil {
554+
return err
555+
}
556+
printer, err := progress.NewPrinter(context.TODO(), os.Stderr, os.Stderr, progressMode, progressui.WithDesc(
557+
fmt.Sprintf("building with %q instance using %s driver", b.Name, b.Driver),
558+
fmt.Sprintf("%s:%s", b.Driver, b.Name),
559+
))
521560
if err != nil {
522561
return err
523562
}
@@ -550,7 +589,10 @@ func launchControllerAndRunBuild(dockerCli command.Cli, options buildOptions) er
550589
}
551590

552591
var resp *client.SolveResponse
553-
ref, resp, err = c.Build(ctx, opts, pr, os.Stdout, os.Stderr, progress)
592+
ref, resp, err = c.Build(ctx, opts, pr, printer)
593+
if err1 := printer.Wait(); err == nil {
594+
err = err1
595+
}
554596
if err != nil {
555597
var be *controllererrors.BuildError
556598
if errors.As(err, &be) {
@@ -561,6 +603,7 @@ func launchControllerAndRunBuild(dockerCli command.Cli, options buildOptions) er
561603
return errors.Wrapf(err, "failed to build")
562604
}
563605
}
606+
printWarnings(os.Stderr, printer.Warnings(), progressMode)
564607
if err := pw.Close(); err != nil {
565608
logrus.Debug("failed to close stdin pipe writer")
566609
}
@@ -595,7 +638,7 @@ func launchControllerAndRunBuild(dockerCli command.Cli, options buildOptions) er
595638
}
596639
return errors.Errorf("failed to configure terminal: %v", err)
597640
}
598-
err = monitor.RunMonitor(ctx, ref, &opts, options.invoke.InvokeConfig, c, progress, pr2, os.Stdout, os.Stderr)
641+
err = monitor.RunMonitor(ctx, ref, &opts, options.invoke.InvokeConfig, c, pr2, os.Stdout, os.Stderr, printer)
599642
con.Reset()
600643
if err := pw2.Close(); err != nil {
601644
logrus.Debug("failed to close monitor stdin pipe reader")
@@ -881,3 +924,43 @@ func resolvePaths(options *controllerapi.BuildOptions) (_ *controllerapi.BuildOp
881924

882925
return options, nil
883926
}
927+
928+
func printWarnings(w io.Writer, warnings []client.VertexWarning, mode string) {
929+
if len(warnings) == 0 || mode == progress.PrinterModeQuiet {
930+
return
931+
}
932+
fmt.Fprintf(w, "\n ")
933+
sb := &bytes.Buffer{}
934+
if len(warnings) == 1 {
935+
fmt.Fprintf(sb, "1 warning found")
936+
} else {
937+
fmt.Fprintf(sb, "%d warnings found", len(warnings))
938+
}
939+
if logrus.GetLevel() < logrus.DebugLevel {
940+
fmt.Fprintf(sb, " (use --debug to expand)")
941+
}
942+
fmt.Fprintf(sb, ":\n")
943+
fmt.Fprint(w, aec.Apply(sb.String(), aec.YellowF))
944+
945+
for _, warn := range warnings {
946+
fmt.Fprintf(w, " - %s\n", warn.Short)
947+
if logrus.GetLevel() < logrus.DebugLevel {
948+
continue
949+
}
950+
for _, d := range warn.Detail {
951+
fmt.Fprintf(w, "%s\n", d)
952+
}
953+
if warn.URL != "" {
954+
fmt.Fprintf(w, "More info: %s\n", warn.URL)
955+
}
956+
if warn.SourceInfo != nil && warn.Range != nil {
957+
src := errdefs.Source{
958+
Info: warn.SourceInfo,
959+
Ranges: warn.Range,
960+
}
961+
src.Print(w)
962+
}
963+
fmt.Fprintf(w, "\n")
964+
965+
}
966+
}

‎commands/debug-shell.go‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"github.com/docker/buildx/controller/control"
1111
controllerapi "github.com/docker/buildx/controller/pb"
1212
"github.com/docker/buildx/monitor"
13+
"github.com/docker/buildx/util/progress"
1314
"github.com/docker/cli/cli/command"
1415
"github.com/pkg/errors"
1516
"github.com/sirupsen/logrus"
@@ -18,7 +19,7 @@ import (
1819

1920
func debugShellCmd(dockerCli command.Cli) *cobra.Command {
2021
var options control.ControlOptions
21-
var progress string
22+
var progressMode string
2223

2324
cmd := &cobra.Command{
2425
Use: "debug-shell",
@@ -38,9 +39,15 @@ func debugShellCmd(dockerCli command.Cli) *cobra.Command {
3839
if err := con.SetRaw(); err != nil {
3940
return errors.Errorf("failed to configure terminal: %v", err)
4041
}
42+
43+
printer, err := progress.NewPrinter(context.TODO(), os.Stderr, os.Stderr, progressMode)
44+
if err != nil {
45+
return err
46+
}
47+
4148
err = monitor.RunMonitor(ctx, "", nil, controllerapi.InvokeConfig{
4249
Tty: true,
43-
}, c, progress, os.Stdin, os.Stdout, os.Stderr)
50+
}, c, os.Stdin, os.Stdout, os.Stderr, printer)
4451
con.Reset()
4552
return err
4653
},
@@ -51,7 +58,7 @@ func debugShellCmd(dockerCli command.Cli) *cobra.Command {
5158
flags.StringVar(&options.Root, "root", "", "Specify root directory of server to connect [experimental]")
5259
flags.BoolVar(&options.Detach, "detach", runtime.GOOS == "linux", "Detach buildx server (supported only on linux) [experimental]")
5360
flags.StringVar(&options.ServerConfig, "server-config", "", "Specify buildx server config file (used only when launching new server) [experimental]")
54-
flags.StringVar(&progress, "progress", "auto", `Set type of progress output ("auto", "plain", "tty"). Use plain to show container output`)
61+
flags.StringVar(&progressMode, "progress", "auto", `Set type of progress output ("auto", "plain", "tty"). Use plain to show container output`)
5562

5663
return cmd
5764
}

‎controller/build/build.go‎

Lines changed: 5 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,10 @@
11
package build
22

33
import (
4-
"bytes"
54
"context"
65
"encoding/base64"
76
"encoding/csv"
87
"encoding/json"
9-
"fmt"
108
"io"
119
"os"
1210
"path/filepath"
@@ -30,12 +28,8 @@ import (
3028
"github.com/docker/go-units"
3129
"github.com/moby/buildkit/client"
3230
"github.com/moby/buildkit/session/auth/authprovider"
33-
"github.com/moby/buildkit/solver/errdefs"
3431
"github.com/moby/buildkit/util/grpcerrors"
35-
"github.com/moby/buildkit/util/progress/progressui"
36-
"github.com/morikuni/aec"
3732
"github.com/pkg/errors"
38-
"github.com/sirupsen/logrus"
3933
"google.golang.org/grpc/codes"
4034
)
4135

@@ -46,7 +40,7 @@ const defaultTargetName = "default"
4640
// NOTE: When an error happens during the build and this function acquires the debuggable *build.ResultContext,
4741
// this function returns it in addition to the error (i.e. it does "return nil, res, err"). The caller can
4842
// inspect the result and debug the cause of that error.
49-
func RunBuild(ctx context.Context, dockerCli command.Cli, in controllerapi.BuildOptions, inStream io.Reader, progressMode string, statusChan chan *client.SolveStatus) (*client.SolveResponse, *build.ResultContext, error) {
43+
func RunBuild(ctx context.Context, dockerCli command.Cli, in controllerapi.BuildOptions, inStream io.Reader, progress progress.Writer) (*client.SolveResponse, *build.ResultContext, error) {
5044
if in.NoCache && len(in.NoCacheFilter) > 0 {
5145
return nil, nil, errors.Errorf("--no-cache and --no-cache-filter cannot currently be used together")
5246
}
@@ -164,6 +158,7 @@ func RunBuild(ctx context.Context, dockerCli command.Cli, in controllerapi.Build
164158
contextPathHash = in.ContextPath
165159
}
166160

161+
// TODO: this should not be loaded this side of the controller api
167162
b, err := builder.New(dockerCli,
168163
builder.WithName(in.Builder),
169164
builder.WithContextPathHash(contextPathHash),
@@ -179,7 +174,7 @@ func RunBuild(ctx context.Context, dockerCli command.Cli, in controllerapi.Build
179174
return nil, nil, err
180175
}
181176

182-
resp, res, err := buildTargets(ctx, dockerCli, b.NodeGroup, nodes, map[string]build.Options{defaultTargetName: opts}, progressMode, in.MetadataFile, statusChan)
177+
resp, res, err := buildTargets(ctx, dockerCli, b.NodeGroup, nodes, map[string]build.Options{defaultTargetName: opts}, progress, in.MetadataFile)
183178
err = wrapBuildError(err, false)
184179
if err != nil {
185180
// NOTE: buildTargets can return *build.ResultContext even on error.
@@ -193,32 +188,17 @@ func RunBuild(ctx context.Context, dockerCli command.Cli, in controllerapi.Build
193188
// NOTE: When an error happens during the build and this function acquires the debuggable *build.ResultContext,
194189
// this function returns it in addition to the error (i.e. it does "return nil, res, err"). The caller can
195190
// inspect the result and debug the cause of that error.
196-
func buildTargets(ctx context.Context, dockerCli command.Cli, ng *store.NodeGroup, nodes []builder.Node, opts map[string]build.Options, progressMode string, metadataFile string, statusChan chan *client.SolveStatus) (*client.SolveResponse, *build.ResultContext, error) {
197-
ctx2, cancel := context.WithCancel(context.TODO())
198-
defer cancel()
199-
200-
printer, err := progress.NewPrinter(ctx2, os.Stderr, os.Stderr, progressMode, progressui.WithDesc(
201-
fmt.Sprintf("building with %q instance using %s driver", ng.Name, ng.Driver),
202-
fmt.Sprintf("%s:%s", ng.Driver, ng.Name),
203-
))
204-
if err != nil {
205-
return nil, nil, err
206-
}
207-
191+
func buildTargets(ctx context.Context, dockerCli command.Cli, ng *store.NodeGroup, nodes []builder.Node, opts map[string]build.Options, progress progress.Writer, metadataFile string) (*client.SolveResponse, *build.ResultContext, error) {
208192
var res *build.ResultContext
209193
var mu sync.Mutex
210194
var idx int
211-
resp, err := build.BuildWithResultHandler(ctx, nodes, opts, dockerutil.NewClient(dockerCli), confutil.ConfigDir(dockerCli), progress.Tee(printer, statusChan), func(driverIndex int, gotRes *build.ResultContext) {
195+
resp, err := build.BuildWithResultHandler(ctx, nodes, opts, dockerutil.NewClient(dockerCli), confutil.ConfigDir(dockerCli), progress, func(driverIndex int, gotRes *build.ResultContext) {
212196
mu.Lock()
213197
defer mu.Unlock()
214198
if res == nil || driverIndex < idx {
215199
idx, res = driverIndex, gotRes
216200
}
217201
})
218-
err1 := printer.Wait()
219-
if err == nil {
220-
err = err1
221-
}
222202
if err != nil {
223203
return nil, res, err
224204
}
@@ -229,8 +209,6 @@ func buildTargets(ctx context.Context, dockerCli command.Cli, ng *store.NodeGrou
229209
}
230210
}
231211

232-
printWarnings(os.Stderr, printer.Warnings(), progressMode)
233-
234212
for k := range resp {
235213
if opts[k].PrintFunc != nil {
236214
if err := printResult(opts[k].PrintFunc, resp[k].ExporterResponse); err != nil {
@@ -242,46 +220,6 @@ func buildTargets(ctx context.Context, dockerCli command.Cli, ng *store.NodeGrou
242220
return resp[defaultTargetName], res, err
243221
}
244222

245-
func printWarnings(w io.Writer, warnings []client.VertexWarning, mode string) {
246-
if len(warnings) == 0 || mode == progress.PrinterModeQuiet {
247-
return
248-
}
249-
fmt.Fprintf(w, "\n ")
250-
sb := &bytes.Buffer{}
251-
if len(warnings) == 1 {
252-
fmt.Fprintf(sb, "1 warning found")
253-
} else {
254-
fmt.Fprintf(sb, "%d warnings found", len(warnings))
255-
}
256-
if logrus.GetLevel() < logrus.DebugLevel {
257-
fmt.Fprintf(sb, " (use --debug to expand)")
258-
}
259-
fmt.Fprintf(sb, ":\n")
260-
fmt.Fprint(w, aec.Apply(sb.String(), aec.YellowF))
261-
262-
for _, warn := range warnings {
263-
fmt.Fprintf(w, " - %s\n", warn.Short)
264-
if logrus.GetLevel() < logrus.DebugLevel {
265-
continue
266-
}
267-
for _, d := range warn.Detail {
268-
fmt.Fprintf(w, "%s\n", d)
269-
}
270-
if warn.URL != "" {
271-
fmt.Fprintf(w, "More info: %s\n", warn.URL)
272-
}
273-
if warn.SourceInfo != nil && warn.Range != nil {
274-
src := errdefs.Source{
275-
Info: warn.SourceInfo,
276-
Ranges: warn.Range,
277-
}
278-
src.Print(w)
279-
}
280-
fmt.Fprintf(w, "\n")
281-
282-
}
283-
}
284-
285223
func parsePrintFunc(str string) (*build.PrintFunc, error) {
286224
if str == "" {
287225
return nil, nil

‎controller/control/controller.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,13 @@ import (
44
"context"
55
"io"
66

7-
"github.com/containerd/console"
87
controllerapi "github.com/docker/buildx/controller/pb"
8+
"github.com/docker/buildx/util/progress"
99
"github.com/moby/buildkit/client"
1010
)
1111

1212
type BuildxController interface {
13-
Build(ctx context.Context, options controllerapi.BuildOptions, in io.ReadCloser, w io.Writer, out console.File, progressMode string) (ref string, resp *client.SolveResponse, err error)
13+
Build(ctx context.Context, options controllerapi.BuildOptions, in io.ReadCloser, progress progress.Writer) (ref string, resp *client.SolveResponse, err error)
1414
// Invoke starts an IO session into the specified process.
1515
// If pid doesn't matche to any running processes, it starts a new process with the specified config.
1616
// If there is no container running or InvokeConfig.Rollback is speicfied, the process will start in a newly created container.

‎controller/local/controller.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,14 @@ import (
55
"io"
66
"sync/atomic"
77

8-
"github.com/containerd/console"
98
"github.com/docker/buildx/build"
109
cbuild "github.com/docker/buildx/controller/build"
1110
"github.com/docker/buildx/controller/control"
1211
controllererrors "github.com/docker/buildx/controller/errdefs"
1312
controllerapi "github.com/docker/buildx/controller/pb"
1413
"github.com/docker/buildx/controller/processes"
1514
"github.com/docker/buildx/util/ioset"
15+
"github.com/docker/buildx/util/progress"
1616
"github.com/docker/cli/cli/command"
1717
"github.com/moby/buildkit/client"
1818
"github.com/pkg/errors"
@@ -42,13 +42,13 @@ type localController struct {
4242
buildOnGoing atomic.Bool
4343
}
4444

45-
func (b *localController) Build(ctx context.Context, options controllerapi.BuildOptions, in io.ReadCloser, w io.Writer, out console.File, progressMode string) (string, *client.SolveResponse, error) {
45+
func (b *localController) Build(ctx context.Context, options controllerapi.BuildOptions, in io.ReadCloser, progress progress.Writer) (string, *client.SolveResponse, error) {
4646
if !b.buildOnGoing.CompareAndSwap(false, true) {
4747
return "", nil, errors.New("build ongoing")
4848
}
4949
defer b.buildOnGoing.Store(false)
5050

51-
resp, res, buildErr := cbuild.RunBuild(ctx, b.dockerCli, options, in, progressMode, nil)
51+
resp, res, buildErr := cbuild.RunBuild(ctx, b.dockerCli, options, in, progress)
5252
// NOTE: RunBuild can return *build.ResultContext even on error.
5353
if res != nil {
5454
b.buildConfig = buildConfig{

0 commit comments

Comments
 (0)