-
Notifications
You must be signed in to change notification settings - Fork 471
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.
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
Build: Support for printing outline/targets of the current build #1100
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,7 @@ import ( | |
gateway "github.com/moby/buildkit/frontend/gateway/client" | ||
"github.com/moby/buildkit/session" | ||
"github.com/moby/buildkit/session/upload/uploadprovider" | ||
"github.com/moby/buildkit/solver/errdefs" | ||
"github.com/moby/buildkit/solver/pb" | ||
"github.com/moby/buildkit/util/apicaps" | ||
"github.com/moby/buildkit/util/entitlements" | ||
|
@@ -57,6 +58,10 @@ var ( | |
errDockerfileConflict = errors.New("ambiguous Dockerfile source: both stdin and flag correspond to Dockerfiles") | ||
) | ||
|
||
const ( | ||
printFallbackImage = "docker/dockerfile-upstream:1.4-outline@sha256:ccd574ab34a8875c64bb6a8fb3cfae2e6d62d31b28b9f688075cc14c9b669a59" | ||
) | ||
|
||
type Options struct { | ||
Inputs Inputs | ||
|
||
|
@@ -81,7 +86,13 @@ type Options struct { | |
Ulimits *opts.UlimitOpt | ||
|
||
// Linked marks this target as exclusively linked (not requested by the user). | ||
Linked bool | ||
Linked bool | ||
PrintFunc *PrintFunc | ||
} | ||
|
||
type PrintFunc struct { | ||
Name string | ||
Format string | ||
} | ||
|
||
type Inputs struct { | ||
|
@@ -730,7 +741,7 @@ func BuildWithResultHandler(ctx context.Context, drivers []DriverInfo, opt map[s | |
} | ||
} | ||
|
||
if noMobyDriver != nil && !noDefaultLoad() { | ||
if noMobyDriver != nil && !noDefaultLoad() && noPrintFunc(opt) { | ||
var noOutputTargets []string | ||
for name, opt := range opt { | ||
if !opt.Linked && len(opt.Exports) == 0 { | ||
|
@@ -1039,22 +1050,69 @@ func BuildWithResultHandler(ctx context.Context, drivers []DriverInfo, opt map[s | |
defer func() { <-done }() | ||
|
||
cc := c | ||
var printRes map[string][]byte | ||
rr, err := c.Build(ctx, so, "buildx", func(ctx context.Context, c gateway.Client) (*gateway.Result, error) { | ||
res, err := c.Solve(ctx, req) | ||
if err != nil { | ||
return nil, err | ||
} | ||
results.Set(resultKey(dp.driverIndex, k), res) | ||
if resultHandleFunc != nil { | ||
resultHandleFunc(dp.driverIndex, &ResultContext{cc, res}) | ||
var isFallback bool | ||
var origErr error | ||
for { | ||
if opt.PrintFunc != nil { | ||
if _, ok := req.FrontendOpt["frontend.caps"]; !ok { | ||
req.FrontendOpt["frontend.caps"] = "moby.buildkit.frontend.subrequests+forward" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think it would be nice to have consts for the subrequest caps on the buildkit side for these caps similar to solver/pb/caps.go There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be defined in some package that defines keys for Dockerfile frontend compatibility interface. https://github.com/moby/buildkit/blob/master/frontend/dockerfile/builder/build.go#L43-L47 Unfortunately no such package exists atm |
||
} else { | ||
req.FrontendOpt["frontend.caps"] += ",moby.buildkit.frontend.subrequests+forward" | ||
} | ||
req.FrontendOpt["requestid"] = "frontend." + opt.PrintFunc.Name | ||
if isFallback { | ||
req.FrontendOpt["build-arg:BUILDKIT_SYNTAX"] = printFallbackImage | ||
} | ||
} | ||
res, err := c.Solve(ctx, req) | ||
if err != nil { | ||
if origErr != nil { | ||
return nil, err | ||
} | ||
var reqErr *errdefs.UnsupportedSubrequestError | ||
if !isFallback { | ||
if errors.As(err, &reqErr) { | ||
switch reqErr.Name { | ||
case "frontend.outline", "frontend.targets": | ||
isFallback = true | ||
origErr = err | ||
continue | ||
} | ||
return nil, err | ||
} | ||
// buildkit v0.8 vendored in Docker 20.10 does not support typed errors | ||
if strings.Contains(err.Error(), "unsupported request frontend.outline") || strings.Contains(err.Error(), "unsupported request frontend.targets") { | ||
isFallback = true | ||
origErr = err | ||
continue | ||
} | ||
} | ||
return nil, err | ||
} | ||
if opt.PrintFunc != nil { | ||
printRes = res.Metadata | ||
} | ||
results.Set(resultKey(dp.driverIndex, k), res) | ||
if resultHandleFunc != nil { | ||
resultHandleFunc(dp.driverIndex, &ResultContext{cc, res}) | ||
} | ||
return res, nil | ||
} | ||
return res, nil | ||
}, ch) | ||
if err != nil { | ||
return err | ||
} | ||
res[i] = rr | ||
|
||
if rr.ExporterResponse == nil { | ||
rr.ExporterResponse = map[string]string{} | ||
} | ||
for k, v := range printRes { | ||
rr.ExporterResponse[k] = string(v) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we store the values in here, are they going to be output to the metadata file as well? I think that makes sense. I wonder if we need to do something here to make sure we don't write to any known keys here, if the frontend's returned Metadata contains a duplicate key for whatever reason, we might potentially corrupt some data here. Maybe it's worth keeping this info separate with a composite struct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I wonder if we could do something similar to annotations, where we have the prefix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes.
How can it be duplicated if it is a map?
I don't think that would be right. The point of subrequests is to take a different code path and return results for it, so it is not "do build and also return extra keys that would need to be namespace". Oth, subrequest build does not mean that it needs to return json/text. A subrequest could for example do a build with a different configuration and return a different image. In that case, it does want to return the correct image keys so they get exported correctly, not namespaced special keys. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I meant duplicated values between Agreed though, thanks for the clarification on subrequests. |
||
} | ||
|
||
d := drivers[dp.driverIndex].Driver | ||
if d.IsMobyDriver() { | ||
for _, e := range so.Exports { | ||
|
@@ -1630,3 +1688,12 @@ func tryNodeIdentifier(configDir string) (out string) { | |
} | ||
return | ||
} | ||
|
||
func noPrintFunc(opt map[string]Options) bool { | ||
for _, v := range opt { | ||
if v.PrintFunc != nil { | ||
return false | ||
} | ||
} | ||
return true | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,7 @@ const defaultTargetName = "default" | |
type buildOptions struct { | ||
contextPath string | ||
dockerfileName string | ||
printFunc string | ||
|
||
allow []string | ||
buildArgs []string | ||
|
@@ -122,6 +123,11 @@ func runBuild(dockerCli command.Cli, in buildOptions) (err error) { | |
return err | ||
} | ||
|
||
printFunc, err := parsePrintFunc(in.printFunc) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
opts := build.Options{ | ||
Inputs: build.Inputs{ | ||
ContextPath: in.contextPath, | ||
|
@@ -141,6 +147,7 @@ func runBuild(dockerCli command.Cli, in buildOptions) (err error) { | |
Tags: in.tags, | ||
Target: in.target, | ||
Ulimits: in.ulimits, | ||
PrintFunc: printFunc, | ||
} | ||
|
||
platforms, err := platformutil.Parse(in.platforms) | ||
|
@@ -307,6 +314,14 @@ func buildTargets(ctx context.Context, dockerCli command.Cli, opts map[string]bu | |
|
||
printWarnings(os.Stderr, printer.Warnings(), progressMode) | ||
|
||
for k := range resp { | ||
if opts[k].PrintFunc != nil { | ||
if err := printResult(opts[k].PrintFunc, resp[k].ExporterResponse); err != nil { | ||
return "", nil, err | ||
} | ||
} | ||
} | ||
|
||
return resp[defaultTargetName].ExporterResponse["containerimage.digest"], res, err | ||
} | ||
|
||
|
@@ -463,6 +478,10 @@ func buildCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command { | |
|
||
flags.StringArrayVar(&options.platforms, "platform", platformsDefault, "Set target platform for build") | ||
|
||
if isExperimental() { | ||
flags.StringVar(&options.printFunc, "print", "", "Print result of information request (outline, targets)") | ||
tonistiigi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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") | ||
|
@@ -481,7 +500,7 @@ func buildCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command { | |
|
||
flags.Var(options.ulimits, "ulimit", "Ulimit options") | ||
|
||
if os.Getenv("BUILDX_EXPERIMENTAL") == "1" { | ||
if isExperimental() { | ||
flags.StringVar(&options.invoke, "invoke", "", "Invoke a command after the build. BUILDX_EXPERIMENTAL=1 is required.") | ||
} | ||
|
||
|
@@ -596,6 +615,34 @@ func parseContextNames(values []string) (map[string]build.NamedContext, error) { | |
return result, nil | ||
} | ||
|
||
func parsePrintFunc(str string) (*build.PrintFunc, error) { | ||
if str == "" { | ||
return nil, nil | ||
} | ||
csvReader := csv.NewReader(strings.NewReader(str)) | ||
fields, err := csvReader.Read() | ||
if err != nil { | ||
return nil, err | ||
} | ||
f := &build.PrintFunc{} | ||
for _, field := range fields { | ||
parts := strings.SplitN(field, "=", 2) | ||
if len(parts) == 2 { | ||
if parts[0] == "format" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels a bit confusing to me since we already have the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was made to match #1100 (comment) |
||
f.Format = parts[1] | ||
} else { | ||
return nil, errors.Errorf("invalid print field: %s", field) | ||
} | ||
} else { | ||
if f.Name != "" { | ||
return nil, errors.Errorf("invalid print value: %s", str) | ||
} | ||
f.Name = field | ||
} | ||
} | ||
return f, nil | ||
} | ||
|
||
func writeMetadataFile(filename string, dt interface{}) error { | ||
b, err := json.MarshalIndent(dt, "", " ") | ||
if err != nil { | ||
|
@@ -652,3 +699,11 @@ func (w *wrapped) Error() string { | |
func (w *wrapped) Unwrap() error { | ||
return w.err | ||
} | ||
|
||
func isExperimental() bool { | ||
if v, ok := os.LookupEnv("BUILDKIT_EXPERIMENTAL"); ok { | ||
vv, _ := strconv.ParseBool(v) | ||
return vv | ||
} | ||
return false | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,48 @@ | ||||||||||||||||||||||||||||||||||||
package commands | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||||||||||
"fmt" | ||||||||||||||||||||||||||||||||||||
"io" | ||||||||||||||||||||||||||||||||||||
"log" | ||||||||||||||||||||||||||||||||||||
"os" | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
"github.com/docker/buildx/build" | ||||||||||||||||||||||||||||||||||||
"github.com/docker/docker/api/types/versions" | ||||||||||||||||||||||||||||||||||||
"github.com/moby/buildkit/frontend/subrequests" | ||||||||||||||||||||||||||||||||||||
"github.com/moby/buildkit/frontend/subrequests/outline" | ||||||||||||||||||||||||||||||||||||
"github.com/moby/buildkit/frontend/subrequests/targets" | ||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
func printResult(f *build.PrintFunc, res map[string]string) error { | ||||||||||||||||||||||||||||||||||||
switch f.Name { | ||||||||||||||||||||||||||||||||||||
case "outline": | ||||||||||||||||||||||||||||||||||||
return printValue(outline.PrintOutline, outline.SubrequestsOutlineDefinition.Version, f.Format, res) | ||||||||||||||||||||||||||||||||||||
case "targets": | ||||||||||||||||||||||||||||||||||||
return printValue(targets.PrintTargets, targets.SubrequestsTargetsDefinition.Version, f.Format, res) | ||||||||||||||||||||||||||||||||||||
case "subrequests.describe": | ||||||||||||||||||||||||||||||||||||
return printValue(subrequests.PrintDescribe, subrequests.SubrequestsDescribeDefinition.Version, f.Format, res) | ||||||||||||||||||||||||||||||||||||
default: | ||||||||||||||||||||||||||||||||||||
if dt, ok := res["result.txt"]; ok { | ||||||||||||||||||||||||||||||||||||
fmt.Print(dt) | ||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||
log.Printf("%s %+v", f, res) | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
return nil | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
type printFunc func([]byte, io.Writer) error | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
func printValue(printer printFunc, version string, format string, res map[string]string) error { | ||||||||||||||||||||||||||||||||||||
if format == "json" { | ||||||||||||||||||||||||||||||||||||
fmt.Fprintln(os.Stdout, res["result.json"]) | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not hardcoded anywhere but defined in the subrequest definition that frontend shows. If handling a specific one, client should look what outputs it has from the description. This is a generic "best-effort" because we let the user specify any function name/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Could you use the Lines 607 to 623 in 98f9f80
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A source is the original source file that we don't control. It can be any kind of bytes. I think we shouldn't make assumptions about this. Even if it is a Dockerfile, showing some encoded string in here for the whole big file would not make it clearer and the user would need to do some decoding from a string to try to get the original form. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not encoded JSON so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah right that makes sense. |
||||||||||||||||||||||||||||||||||||
return nil | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
if res["version"] != "" && versions.LessThan(version, res["version"]) && res["result.txt"] != "" { | ||||||||||||||||||||||||||||||||||||
// structure is too new and we don't know how to print it | ||||||||||||||||||||||||||||||||||||
fmt.Fprint(os.Stdout, res["result.txt"]) | ||||||||||||||||||||||||||||||||||||
return nil | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
return printer([]byte(res["result.json"]), os.Stdout) | ||||||||||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should use a loop here to do the retries? As I understand it, it's so that we can have a second try with a fallback, but that might be easier to read in-serial, instead of with the loop/continue which is hard to reason about control flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand. Without the loop, a bunch of this code would need to be duplicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we could try and move the logic into a separate function/method, as a generic
makeRequest
or something function, then we could use a bool return to check if it was successful, and if not, we could make a fallback call to the same function, which would keep the logic needed inBuildWithResultHandler
to a minimum.