Skip to content

controller: followup refactoring on Build API #1671

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

Closed
ktock wants to merge 1 commit into from
Closed

controller: followup refactoring on Build API #1671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 15 additions & 12 deletions commands/bake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/docker/buildx/bake"
"github.com/docker/buildx/build"
"github.com/docker/buildx/builder"
controllerapi "github.com/docker/buildx/controller/pb"
"github.com/docker/buildx/util/buildflags"
"github.com/docker/buildx/util/confutil"
"github.com/docker/buildx/util/dockerutil"
Expand All @@ -29,7 +28,11 @@ type bakeOptions struct {
printOnly bool
sbom string
provenance string
controllerapi.CommonOptions
Copy link
Collaborator

@jedevc jedevc Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

@jedevc jedevc Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

Copy link
Collaborator

@jedevc jedevc Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


builder string
metadataFile string
exportPush bool
exportLoad bool
}

func runBake(dockerCli command.Cli, targets []string, in bakeOptions, cFlags commonFlags) (err error) {
Expand Down Expand Up @@ -64,12 +67,12 @@ func runBake(dockerCli command.Cli, targets []string, in bakeOptions, cFlags com
}

overrides := in.overrides
if in.ExportPush {
if in.ExportLoad {
if in.exportPush {
if in.exportLoad {
return errors.Errorf("push and load may not be set together at the moment")
}
overrides = append(overrides, "*.push=true")
} else if in.ExportLoad {
} else if in.exportLoad {
overrides = append(overrides, "*.output=type=docker")
}
if cFlags.noCache != nil {
Expand Down Expand Up @@ -97,7 +100,7 @@ func runBake(dockerCli command.Cli, targets []string, in bakeOptions, cFlags com
// instance only needed for reading remote bake files or building
if url != "" || !in.printOnly {
b, err := builder.New(dockerCli,
builder.WithName(in.Builder),
builder.WithName(in.builder),
builder.WithContextPathHash(contextPathHash),
)
if err != nil {
Expand Down Expand Up @@ -180,12 +183,12 @@ func runBake(dockerCli command.Cli, targets []string, in bakeOptions, cFlags com
return wrapBuildError(err, true)
}

if len(in.MetadataFile) > 0 {
if len(in.metadataFile) > 0 {
dt := make(map[string]interface{})
for t, r := range resp {
dt[t] = decodeExporterResponse(r.ExporterResponse)
}
if err := writeMetadataFile(in.MetadataFile, dt); err != nil {
if err := writeMetadataFile(in.metadataFile, dt); err != nil {
return err
}
}
Expand All @@ -209,8 +212,8 @@ func bakeCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command {
if !cmd.Flags().Lookup("pull").Changed {
cFlags.pull = nil
}
options.Builder = rootOpts.builder
options.MetadataFile = cFlags.metadataFile
options.builder = rootOpts.builder
options.metadataFile = cFlags.metadataFile
// Other common flags (noCache, pull and progress) are processed in runBake function.
return runBake(dockerCli, args, options, cFlags)
},
Expand All @@ -219,9 +222,9 @@ func bakeCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command {
flags := cmd.Flags()

flags.StringArrayVarP(&options.files, "file", "f", []string{}, "Build definition file")
flags.BoolVar(&options.ExportLoad, "load", false, `Shorthand for "--set=*.output=type=docker"`)
flags.BoolVar(&options.exportLoad, "load", false, `Shorthand for "--set=*.output=type=docker"`)
flags.BoolVar(&options.printOnly, "print", false, "Print the options without building")
flags.BoolVar(&options.ExportPush, "push", false, `Shorthand for "--set=*.output=type=registry"`)
flags.BoolVar(&options.exportPush, "push", false, `Shorthand for "--set=*.output=type=registry"`)
flags.StringVar(&options.sbom, "sbom", "", `Shorthand for "--set=*.attest=type=sbom"`)
flags.StringVar(&options.provenance, "provenance", "", `Shorthand for "--set=*.attest=type=provenance"`)
flags.StringArrayVar(&options.overrides, "set", nil, `Override target value (e.g., "targetpattern.key=value")`)
Expand Down
56 changes: 41 additions & 15 deletions commands/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"strings"

"github.com/containerd/console"
"github.com/docker/buildx/build"
"github.com/docker/buildx/controller"
cbuild "github.com/docker/buildx/controller/build"
"github.com/docker/buildx/controller/control"
Expand All @@ -35,6 +36,7 @@ import (
"github.com/moby/buildkit/exporter/containerimage/exptypes"
"github.com/moby/buildkit/util/appcontext"
"github.com/moby/buildkit/util/grpcerrors"
"github.com/moby/buildkit/util/progress/progressui"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -75,7 +77,13 @@ type buildOptions struct {
progress string
quiet bool

controllerapi.CommonOptions
builder string
metadataFile string
noCache bool
pull bool
exportPush bool
exportLoad bool

control.ControlOptions
}

Expand All @@ -97,7 +105,12 @@ func (o *buildOptions) toControllerOptions() (controllerapi.BuildOptions, error)
Tags: o.tags,
Target: o.target,
Ulimits: dockerUlimitToControllerUlimit(o.ulimits),
Opts: &o.CommonOptions,
Builder: o.builder,
MetadataFile: o.metadataFile,
NoCache: o.noCache,
Pull: o.pull,
ExportPush: o.exportPush,
ExportLoad: o.exportLoad,
}

inAttests := append([]string{}, o.attests...)
Expand Down Expand Up @@ -179,7 +192,7 @@ func runBuild(dockerCli command.Cli, in buildOptions) error {
if err != nil {
return err
}
progress, err := in.toProgress()
progressMode, err := in.toProgress()
if err != nil {
return err
}
Expand All @@ -190,7 +203,20 @@ func runBuild(dockerCli command.Cli, in buildOptions) error {
return errors.Wrap(err, "removing image ID file")
}
}
resp, _, err := cbuild.RunBuild(ctx, dockerCli, opts, os.Stdin, progress, nil)
resp, _, err := cbuild.RunBuild(ctx, dockerCli, opts, os.Stdin, cbuild.ProgressConfig{
Printer: func(ctx2 context.Context, ng *store.NodeGroup) (*progress.Printer, error) {
return progress.NewPrinter(ctx2, os.Stderr, os.Stderr, progressMode, progressui.WithDesc(
fmt.Sprintf("building with %q instance using %s driver", ng.Name, ng.Driver),
fmt.Sprintf("%s:%s", ng.Driver, ng.Name),
))
},
PrintWarningsFunc: func(warnings []client.VertexWarning) {
cbuild.PrintWarnings(os.Stderr, warnings, progressMode)
},
PrintResultFunc: func(f *build.PrintFunc, res map[string]string) error {
return cbuild.PrintResult(os.Stdout, f, res)
},
})
if err != nil {
return err
}
Expand Down Expand Up @@ -218,15 +244,15 @@ func buildCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command {
Args: cli.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
options.contextPath = args[0]
options.Builder = rootOpts.builder
options.MetadataFile = cFlags.metadataFile
options.NoCache = false
options.builder = rootOpts.builder
options.metadataFile = cFlags.metadataFile
options.noCache = false
if cFlags.noCache != nil {
options.NoCache = *cFlags.noCache
options.noCache = *cFlags.noCache
}
options.Pull = false
options.pull = false
if cFlags.pull != nil {
options.Pull = *cFlags.pull
options.pull = *cFlags.pull
}
options.progress = cFlags.progress
cmd.Flags().VisitAll(checkWarnedFlags)
Expand Down Expand Up @@ -267,7 +293,7 @@ func buildCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command {

flags.StringArrayVar(&options.labels, "label", []string{}, "Set metadata for an image")

flags.BoolVar(&options.ExportLoad, "load", false, `Shorthand for "--output=type=docker"`)
flags.BoolVar(&options.exportLoad, "load", false, `Shorthand for "--output=type=docker"`)

flags.StringVar(&options.networkMode, "network", "default", `Set the networking mode for the "RUN" instructions during build`)

Expand All @@ -281,7 +307,7 @@ func buildCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command {
flags.StringVar(&options.printFunc, "print", "", "Print result of information request (e.g., outline, targets) [experimental]")
}

flags.BoolVar(&options.ExportPush, "push", false, `Shorthand for "--output=type=registry"`)
flags.BoolVar(&options.exportPush, "push", false, `Shorthand for "--output=type=registry"`)

flags.BoolVarP(&options.quiet, "quiet", "q", false, "Suppress the build output and print image ID on success")

Expand Down Expand Up @@ -523,7 +549,7 @@ func launchControllerAndRunBuild(dockerCli command.Cli, options buildOptions) er
return err
}
opts = *optsP
ref, resp, err := c.Build(ctx, opts, pr, os.Stdout, os.Stderr, progress)
ref, resp, err := control.Build(ctx, c, opts, pr, os.Stdout, os.Stderr, progress)
if err != nil {
return errors.Wrapf(err, "failed to build") // TODO: allow invoke even on error
}
Expand Down Expand Up @@ -805,8 +831,8 @@ func resolvePaths(options *controllerapi.BuildOptions) (_ *controllerapi.BuildOp
}
options.SSH = ssh

if options.Opts != nil && options.Opts.MetadataFile != "" {
options.Opts.MetadataFile, err = filepath.Abs(options.Opts.MetadataFile)
if options.MetadataFile != "" {
options.MetadataFile, err = filepath.Abs(options.MetadataFile)
if err != nil {
return nil, err
}
Expand Down
8 changes: 2 additions & 6 deletions commands/build_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,14 +228,10 @@ func TestResolvePaths(t *testing.T) {
{
name: "metadatafile",
options: controllerapi.BuildOptions{
Opts: &controllerapi.CommonOptions{
MetadataFile: "test1",
},
MetadataFile: "test1",
},
want: controllerapi.BuildOptions{
Opts: &controllerapi.CommonOptions{
MetadataFile: filepath.Join(tmpwd, "test1"),
},
MetadataFile: filepath.Join(tmpwd, "test1"),
},
},
}
Expand Down
Loading