From 0c1fd312264c4012e05f72e6f4544f8b41bcd9f5 Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Fri, 21 Apr 2023 11:13:56 +0100 Subject: [PATCH] build: refactor out common build command components We had some duplicated code between the basic runBuild and launchControllerAndRunBuild. This patch refactors out the common logic (since it's only really like to keep growing), and has runBuild call into either the controller or directly start the build depending on whether BUILDX_EXPERIMENTAL is set. Signed-off-by: Justin Chadwell --- commands/build.go | 294 ++++++++++++++++++++++------------------------ 1 file changed, 141 insertions(+), 153 deletions(-) diff --git a/commands/build.go b/commands/build.go index 08221288..d347ef48 100644 --- a/commands/build.go +++ b/commands/build.go @@ -88,7 +88,7 @@ type buildOptions struct { control.ControlOptions } -func (o *buildOptions) toControllerOptions() (controllerapi.BuildOptions, error) { +func (o *buildOptions) toControllerOptions() (*controllerapi.BuildOptions, error) { var err error opts := controllerapi.BuildOptions{ Allow: o.allow, @@ -130,43 +130,43 @@ func (o *buildOptions) toControllerOptions() (controllerapi.BuildOptions, error) } opts.Attests, err = buildflags.ParseAttests(inAttests) if err != nil { - return controllerapi.BuildOptions{}, err + return nil, err } opts.NamedContexts, err = buildflags.ParseContextNames(o.contexts) if err != nil { - return controllerapi.BuildOptions{}, err + return nil, err } opts.Exports, err = buildflags.ParseExports(o.outputs) if err != nil { - return controllerapi.BuildOptions{}, err + return nil, err } for _, e := range opts.Exports { if (e.Type == client.ExporterLocal || e.Type == client.ExporterTar) && o.imageIDFile != "" { - return controllerapi.BuildOptions{}, errors.Errorf("local and tar exporters are incompatible with image ID file") + return nil, errors.Errorf("local and tar exporters are incompatible with image ID file") } } opts.CacheFrom, err = buildflags.ParseCacheEntry(o.cacheFrom) if err != nil { - return controllerapi.BuildOptions{}, err + return nil, err } opts.CacheTo, err = buildflags.ParseCacheEntry(o.cacheTo) if err != nil { - return controllerapi.BuildOptions{}, err + return nil, err } opts.Secrets, err = buildflags.ParseSecretSpecs(o.secrets) if err != nil { - return controllerapi.BuildOptions{}, err + return nil, err } opts.SSH, err = buildflags.ParseSSHSpecs(o.ssh) if err != nil { - return controllerapi.BuildOptions{}, err + return nil, err } - return opts, nil + return &opts, nil } func (o *buildOptions) toProgress() (string, error) { @@ -185,9 +185,8 @@ func (o *buildOptions) toProgress() (string, error) { return o.progress, nil } -func runBuild(dockerCli command.Cli, in buildOptions) error { +func runBuild(dockerCli command.Cli, options buildOptions) (err error) { ctx := appcontext.Context() - ctx, end, err := tracing.TraceCurrentCommand(ctx, "build") if err != nil { return err @@ -196,38 +195,146 @@ func runBuild(dockerCli command.Cli, in buildOptions) error { end(err) }() - opts, err := in.toControllerOptions() - if err != nil { - return err - } - progress, err := in.toProgress() - if err != nil { - return err - } - // Avoid leaving a stale file if we eventually fail - if in.imageIDFile != "" { - if err := os.Remove(in.imageIDFile); err != nil && !os.IsNotExist(err) { + if options.imageIDFile != "" { + if err := os.Remove(options.imageIDFile); err != nil && !os.IsNotExist(err) { return errors.Wrap(err, "removing image ID file") } } - resp, _, err := cbuild.RunBuild(ctx, dockerCli, opts, os.Stdin, progress, nil, false) + + progressMode, err := options.toProgress() if err != nil { return err } - if in.quiet { + + var resp *client.SolveResponse + var retErr error + if isExperimental() { + resp, retErr = runControllerBuild(ctx, dockerCli, options, progressMode) + } else { + resp, retErr = runBasicBuild(ctx, dockerCli, options, progressMode) + } + if retErr != nil { + return retErr + } + + if options.quiet { fmt.Println(resp.ExporterResponse[exptypes.ExporterImageDigestKey]) } - if in.imageIDFile != "" { + if options.imageIDFile != "" { dgst := resp.ExporterResponse[exptypes.ExporterImageDigestKey] if v, ok := resp.ExporterResponse[exptypes.ExporterImageConfigDigestKey]; ok { dgst = v } - return os.WriteFile(in.imageIDFile, []byte(dgst), 0644) + return os.WriteFile(options.imageIDFile, []byte(dgst), 0644) } + return nil } +func runBasicBuild(ctx context.Context, dockerCli command.Cli, options buildOptions, progressMode string) (*client.SolveResponse, error) { + opts, err := options.toControllerOptions() + if err != nil { + return nil, err + } + + resp, _, err := cbuild.RunBuild(ctx, dockerCli, *opts, os.Stdin, progressMode, nil, false) + return resp, err +} + +func runControllerBuild(ctx context.Context, dockerCli command.Cli, options buildOptions, progressMode string) (*client.SolveResponse, error) { + if options.invoke != nil && (options.dockerfileName == "-" || options.contextPath == "-") { + // stdin must be usable for monitor + return nil, errors.Errorf("Dockerfile or context from stdin is not supported with invoke") + } + + c, err := controller.NewController(ctx, options.ControlOptions, dockerCli) + if err != nil { + return nil, err + } + defer func() { + if err := c.Close(); err != nil { + logrus.Warnf("failed to close server connection %v", err) + } + }() + + // Start build + opts, err := options.toControllerOptions() + if err != nil { + return nil, err + } + + // NOTE: buildx server has the current working directory different from the client + // so we need to resolve paths to abosolute ones in the client. + opts, err = resolvePaths(opts) + if err != nil { + return nil, err + } + + var ref string + var retErr error + var resp *client.SolveResponse + f := ioset.NewSingleForwarder() + f.SetReader(os.Stdin) + if !options.noBuild { + pr, pw := io.Pipe() + f.SetWriter(pw, func() io.WriteCloser { + pw.Close() // propagate EOF + logrus.Debug("propagating stdin close") + return nil + }) + + ref, resp, err = c.Build(ctx, *opts, pr, os.Stdout, os.Stderr, progressMode) + if err != nil { + var be *controllererrors.BuildError + if errors.As(err, &be) { + ref = be.Ref + retErr = err + // We can proceed to monitor + } else { + return nil, errors.Wrapf(err, "failed to build") + } + } + + if err := pw.Close(); err != nil { + logrus.Debug("failed to close stdin pipe writer") + } + if err := pr.Close(); err != nil { + logrus.Debug("failed to close stdin pipe reader") + } + } + + // post-build operations + if options.invoke != nil && options.invoke.needsMonitor(retErr) { + pr2, pw2 := io.Pipe() + f.SetWriter(pw2, func() io.WriteCloser { + pw2.Close() // propagate EOF + return nil + }) + con := console.Current() + if err := con.SetRaw(); err != nil { + if err := c.Disconnect(ctx, ref); err != nil { + logrus.Warnf("disconnect error: %v", err) + } + return nil, errors.Errorf("failed to configure terminal: %v", err) + } + err = monitor.RunMonitor(ctx, ref, opts, options.invoke.InvokeConfig, c, progressMode, pr2, os.Stdout, os.Stderr) + con.Reset() + if err := pw2.Close(); err != nil { + logrus.Debug("failed to close monitor stdin pipe reader") + } + if err != nil { + logrus.Warnf("failed to run monitor: %v", err) + } + } else { + if err := c.Disconnect(ctx, ref); err != nil { + logrus.Warnf("disconnect error: %v", err) + } + } + + return resp, retErr +} + func buildCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command { options := buildOptions{} cFlags := &commonFlags{} @@ -252,16 +359,14 @@ func buildCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command { } options.progress = cFlags.progress cmd.Flags().VisitAll(checkWarnedFlags) - if isExperimental() { - if invokeFlag != "" { - invokeConfig, err := parseInvokeConfig(invokeFlag) - if err != nil { - return err - } - options.invoke = &invokeConfig - options.noBuild = invokeFlag == "debug-shell" + + if invokeFlag != "" { + invoke, err := parseInvokeConfig(invokeFlag) + if err != nil { + return err } - return launchControllerAndRunBuild(dockerCli, options) + options.invoke = &invoke + options.noBuild = invokeFlag == "debug-shell" } return runBuild(dockerCli, options) }, @@ -494,123 +599,6 @@ func updateLastActivity(dockerCli command.Cli, ng *store.NodeGroup) error { return txn.UpdateLastActivity(ng) } -func launchControllerAndRunBuild(dockerCli command.Cli, options buildOptions) error { - ctx := context.TODO() - - if options.invoke != nil && (options.dockerfileName == "-" || options.contextPath == "-") { - // stdin must be usable for monitor - return errors.Errorf("Dockerfile or context from stdin is not supported with invoke") - } - - c, err := controller.NewController(ctx, options.ControlOptions, dockerCli) - if err != nil { - return err - } - defer func() { - if err := c.Close(); err != nil { - logrus.Warnf("failed to close server connection %v", err) - } - }() - - // Start build - opts, err := options.toControllerOptions() - if err != nil { - return err - } - progress, err := options.toProgress() - if err != nil { - return err - } - - // NOTE: buildx server has the current working directory different from the client - // so we need to resolve paths to abosolute ones in the client. - optsP, err := resolvePaths(&opts) - if err != nil { - return err - } - opts = *optsP - - var ref string - var retErr error - f := ioset.NewSingleForwarder() - f.SetReader(os.Stdin) - if !options.noBuild { - pr, pw := io.Pipe() - f.SetWriter(pw, func() io.WriteCloser { - pw.Close() // propagate EOF - logrus.Debug("propagating stdin close") - return nil - }) - - // Avoid leaving a stale file if we eventually fail - if options.imageIDFile != "" { - if err := os.Remove(options.imageIDFile); err != nil && !os.IsNotExist(err) { - return errors.Wrap(err, "removing image ID file") - } - } - - var resp *client.SolveResponse - ref, resp, err = c.Build(ctx, opts, pr, os.Stdout, os.Stderr, progress) - if err != nil { - var be *controllererrors.BuildError - if errors.As(err, &be) { - ref = be.Ref - retErr = err - // We can proceed to monitor - } else { - return errors.Wrapf(err, "failed to build") - } - } - if err := pw.Close(); err != nil { - logrus.Debug("failed to close stdin pipe writer") - } - if err := pr.Close(); err != nil { - logrus.Debug("failed to close stdin pipe reader") - } - - if options.quiet { - fmt.Println(resp.ExporterResponse[exptypes.ExporterImageDigestKey]) - } - if options.imageIDFile != "" { - dgst := resp.ExporterResponse[exptypes.ExporterImageDigestKey] - if v, ok := resp.ExporterResponse[exptypes.ExporterImageConfigDigestKey]; ok { - dgst = v - } - return os.WriteFile(options.imageIDFile, []byte(dgst), 0644) - } - - } - - // post-build operations - if options.invoke != nil && options.invoke.needsMonitor(retErr) { - pr2, pw2 := io.Pipe() - f.SetWriter(pw2, func() io.WriteCloser { - pw2.Close() // propagate EOF - return nil - }) - con := console.Current() - if err := con.SetRaw(); err != nil { - if err := c.Disconnect(ctx, ref); err != nil { - logrus.Warnf("disconnect error: %v", err) - } - return errors.Errorf("failed to configure terminal: %v", err) - } - err = monitor.RunMonitor(ctx, ref, &opts, options.invoke.InvokeConfig, c, progress, pr2, os.Stdout, os.Stderr) - con.Reset() - if err := pw2.Close(); err != nil { - logrus.Debug("failed to close monitor stdin pipe reader") - } - if err != nil { - logrus.Warnf("failed to run monitor: %v", err) - } - } else { - if err := c.Disconnect(ctx, ref); err != nil { - logrus.Warnf("disconnect error: %v", err) - } - } - return nil -} - type invokeConfig struct { controllerapi.InvokeConfig invokeFlag string