client: disallow empty output for exportation

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

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)
+}

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

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
 	}