diff --git a/client/client_test.go b/client/client_test.go index a11418b3..fd28cfd2 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -44,6 +44,7 @@ func TestClientIntegration(t *testing.T) { testDuplicateWhiteouts, testSchema1Image, testMountWithNoSource, + testInvalidExporter, }) } @@ -853,3 +854,58 @@ loop0: time.Sleep(500 * time.Millisecond) } } + +func testInvalidExporter(t *testing.T, sb integration.Sandbox) { + requiresLinux(t) + t.Parallel() + c, err := New(sb.Address()) + require.NoError(t, err) + defer c.Close() + + def, err := llb.Image("busybox:latest").Marshal() + require.NoError(t, err) + + destDir, err := ioutil.TempDir("", "buildkit") + require.NoError(t, err) + defer os.RemoveAll(destDir) + + target := "example.com/buildkit/testoci:latest" + attrs := map[string]string{ + "name": target, + } + for _, exp := range []string{ExporterOCI, ExporterDocker} { + err = c.Solve(context.TODO(), def, SolveOpt{ + Exporter: exp, + ExporterAttrs: attrs, + }, nil) + // output file writer is required + require.Error(t, err) + err = c.Solve(context.TODO(), def, SolveOpt{ + Exporter: exp, + ExporterAttrs: attrs, + ExporterOutputDir: destDir, + }, nil) + // output directory is not supported + require.Error(t, err) + } + + err = c.Solve(context.TODO(), def, SolveOpt{ + Exporter: ExporterLocal, + ExporterAttrs: attrs, + }, nil) + // output directory is required + require.Error(t, err) + + f, err := os.Create(filepath.Join(destDir, "a")) + require.NoError(t, err) + defer f.Close() + err = c.Solve(context.TODO(), def, SolveOpt{ + Exporter: ExporterLocal, + ExporterAttrs: attrs, + ExporterOutput: f, + }, nil) + // output file writer is not supported + require.Error(t, err) + + checkAllReleasable(t, c, sb, true) +} diff --git a/client/solve.go b/client/solve.go index 1d8d81b9..c48b3ed3 100644 --- a/client/solve.go +++ b/client/solve.go @@ -81,28 +81,26 @@ func (c *Client) Solve(ctx context.Context, def *llb.Definition, opt SolveOpt, s switch opt.Exporter { case ExporterLocal: if opt.ExporterOutput != nil { - logrus.Warnf("output file writer is ignored for local exporter") + return errors.New("output file writer is not supported by local exporter") } - // it is ok to have empty output dir (just ignored) - // FIXME(AkihiroSuda): maybe disallow empty output dir? (breaks integration tests) - if opt.ExporterOutputDir != "" { - s.Allow(filesync.NewFSSyncTargetDir(opt.ExporterOutputDir)) + if opt.ExporterOutputDir == "" { + return errors.New("output directory is required for local exporter") } + s.Allow(filesync.NewFSSyncTargetDir(opt.ExporterOutputDir)) case ExporterOCI, ExporterDocker: if opt.ExporterOutputDir != "" { - logrus.Warnf("output directory %s is ignored for %s exporter", opt.ExporterOutputDir, opt.Exporter) + return errors.Errorf("output directory %s is not supported by %s exporter", opt.ExporterOutputDir, opt.Exporter) } - // it is ok to have empty output file (just ignored) - // FIXME(AkihiroSuda): maybe disallow empty output file? (breaks integration tests) - if opt.ExporterOutput != nil { - s.Allow(filesync.NewFSSyncTarget(opt.ExporterOutput)) + if opt.ExporterOutput == nil { + return errors.Errorf("output file writer is required for %s exporter", opt.Exporter) } + s.Allow(filesync.NewFSSyncTarget(opt.ExporterOutput)) default: if opt.ExporterOutput != nil { - logrus.Warnf("output file writer is ignored for %s exporter", opt.Exporter) + return errors.Errorf("output file writer is not supported by %s exporter", opt.Exporter) } if opt.ExporterOutputDir != "" { - logrus.Warnf("output directory %s is ignored for %s exporter", opt.ExporterOutputDir, opt.Exporter) + return errors.Errorf("output directory %s is not supported by %s exporter", opt.ExporterOutputDir, opt.Exporter) } } diff --git a/cmd/buildctl/build.go b/cmd/buildctl/build.go index 5741019e..24116301 100644 --- a/cmd/buildctl/build.go +++ b/cmd/buildctl/build.go @@ -216,8 +216,9 @@ func attrMap(sl []string) (map[string]string, error) { func resolveExporterOutput(exporter, output string) (io.WriteCloser, string, error) { switch exporter { case client.ExporterLocal: - // it is ok to have empty output dir (just ignored) - // FIXME(AkihiroSuda): maybe disallow empty output dir? (breaks integration tests) + if output == "" { + return nil, "", errors.New("output directory is required for local exporter") + } return nil, output, nil case client.ExporterOCI, client.ExporterDocker: if output != "" { @@ -231,13 +232,14 @@ func resolveExporterOutput(exporter, output string) (io.WriteCloser, string, err w, err := os.Create(output) return w, "", err } + // if no output file is specified, use stdout if _, err := console.ConsoleFromFile(os.Stdout); err == nil { return nil, "", errors.Errorf("output file is required for %s exporter. refusing to write to console", exporter) } return os.Stdout, "", nil default: // e.g. client.ExporterImage if output != "" { - logrus.Warnf("output %s is ignored for %s exporter", output, exporter) + return nil, "", errors.Errorf("output %s is not supported by %s exporter", output, exporter) } return nil, "", nil }