progress: add new BuildLog option

This commit adds a new `BuildLog` option to the `OSBuildOptions`
that can be used to generate a streamed buildlog (e.g. to a file
or a websocket).

This will be used in `ibcli` with a new `--with-buildlog` option.

bib/pkg/progress/export_test.go

@@ -10,6 +10,14 @@ type (
 	VerboseProgressBar  = verboseProgressBar
 )
 
+func MockOsStdout(w io.Writer) (restore func()) {
+	saved := osStdout
+	osStdout = w
+	return func() {
+		osStdout = saved
+	}
+}
+
 func MockOsStderr(w io.Writer) (restore func()) {
 	saved := osStderr
 	osStderr = w

bib/pkg/progress/progress.go

@@ -18,6 +18,7 @@ import (
 )
 
 var (
+	osStdout io.Writer = os.Stdout
 	osStderr io.Writer = os.Stderr
 
 	// This is only needed because pb.Pool require a real terminal.
@@ -315,37 +316,63 @@ type OSBuildOptions struct {
 	StoreDir  string
 	OutputDir string
 	ExtraEnv  []string
-}
 
-// XXX: merge variant back into images/pkg/osbuild/osbuild-exec.go
-func RunOSBuild(pb ProgressBar, manifest []byte, exports []string, opts *OSBuildOptions) error {
-	if opts == nil {
-		opts = &OSBuildOptions{}
-	}
+	// BuildLog writes the osbuild output to the given writer
+	BuildLog io.Writer
+}
 
+func writersForOSBuild(pb ProgressBar, opts *OSBuildOptions, internalBuildLog io.Writer) (io.Writer, io.Writer) {
 	// To keep maximum compatibility keep the old behavior to run osbuild
 	// directly and show all messages unless we have a "real" progress bar.
 	//
 	// This should ensure that e.g. "podman bootc" keeps working as it
 	// is currently expecting the raw osbuild output. Once we double
 	// checked with them we can remove the runOSBuildNoProgress() and
 	// just run with the new runOSBuildWithProgress() helper.
+	var osbuildStdout, osbuildStderr io.Writer
 	switch pb.(type) {
 	case *terminalProgressBar, *debugProgressBar:
-		return runOSBuildWithProgress(pb, manifest, exports, opts)
+		osbuildStdout = io.Discard
+		osbuildStderr = io.Discard
 	default:
-		return runOSBuildNoProgress(pb, manifest, exports, opts)
+		osbuildStdout = osStdout
+		osbuildStderr = osStderr
+		// No external build log requested and we won't need an
+		// internal one because all output goes directly to
+		// stdout/stderr. This is for maximum compatibility with
+		// the existing bootc-image-builder in "verbose" mode
+		// where stdout, stderr come directly from osbuild.
+		if opts.BuildLog == nil {
+			return osbuildStdout, osbuildStderr
+		}
 	}
-}
 
-func runOSBuildNoProgress(pb ProgressBar, manifest []byte, exports []string, opts *OSBuildOptions) error {
-	_, err := osbuild.RunOSBuild(manifest, opts.StoreDir, opts.OutputDir, exports, nil, opts.ExtraEnv, false, os.Stderr)
-	return err
+	// There is a slight wrinkle here: when requesting a buildlog
+	// we can no longer write to separate stdout/stderr streams
+	// without being racy and give potential out-of-order output
+	// (which is very bad and confusing in a log). The reason is
+	// that if cmd.Std{out,err} are different "go" will start two
+	// go-routine to monitor/copy those are racy when both stdout,stderr
+	// output happens close together (TestRunOSBuildWithBuildlog demos
+	// that). We cannot have our cake and eat it so here we need to
+	// combine osbuilds stderr into our stdout.
+	if opts.BuildLog == nil {
+		opts.BuildLog = io.Discard
+	}
+	mw := io.MultiWriter(osbuildStdout, internalBuildLog, opts.BuildLog)
+	return mw, mw
 }
 
-var osbuildCmd = "osbuild"
+var osbuildCmd string
+
+// XXX: merge variant back into images/pkg/osbuild/osbuild-exec.go
+func RunOSBuild(pb ProgressBar, manifest []byte, exports []string, opts *OSBuildOptions) error {
+	if opts == nil {
+		opts = &OSBuildOptions{}
+	}
+	var internalBuildLog bytes.Buffer
+	osbuildStdout, osbuildStderr := writersForOSBuild(pb, opts, &internalBuildLog)
 
-func runOSBuildWithProgress(pb ProgressBar, manifest []byte, exports []string, opts *OSBuildOptions) error {
 	rp, wp, err := os.Pipe()
 	if err != nil {
 		return fmt.Errorf("cannot create pipe for osbuild: %w", err)
@@ -365,11 +392,10 @@ func runOSBuildWithProgress(pb ProgressBar, manifest []byte, exports []string, o
 		cmd.Args = append(cmd.Args, "--export", export)
 	}
 
-	var stdio bytes.Buffer
 	cmd.Env = append(os.Environ(), opts.ExtraEnv...)
 	cmd.Stdin = bytes.NewBuffer(manifest)
-	cmd.Stdout = &stdio
-	cmd.Stderr = &stdio
+	cmd.Stdout = osbuildStdout
+	cmd.Stderr = osbuildStderr
 	cmd.ExtraFiles = []*os.File{wp}
 
 	osbuildStatus := osbuild.NewStatusScanner(rp)
@@ -381,6 +407,8 @@ func runOSBuildWithProgress(pb ProgressBar, manifest []byte, exports []string, o
 	var statusErrs []error
 	for {
 		st, err := osbuildStatus.Status()
+		// XXX: we cannot exit here, this would mean we lose error
+		// information if osbuild reading fails
 		if err != nil {
 			statusErrs = append(statusErrs, err)
 			continue
@@ -402,7 +430,7 @@ func runOSBuildWithProgress(pb ProgressBar, manifest []byte, exports []string, o
 	}
 
 	if err := cmd.Wait(); err != nil {
-		return fmt.Errorf("error running osbuild: %w\nOutput:\n%s", err, stdio.String())
+		return fmt.Errorf("error running osbuild: %w\nOutput:\n%s", err, internalBuildLog.String())
 	}
 	if len(statusErrs) > 0 {
 		return fmt.Errorf("errors parsing osbuild status:\n%w", errors.Join(statusErrs...))

bib/pkg/progress/progress_test.go

@@ -157,6 +157,38 @@ osbuild-stderr-output
 `)
 }
 
+func TestRunOSBuildWithBuildlog(t *testing.T) {
+	restore := progress.MockOsbuildCmd(makeFakeOsbuild(t, `
+echo osbuild-stdout-output
+>&2 echo osbuild-stderr-output
+`))
+	defer restore()
+
+	var fakeStdout, fakeStderr bytes.Buffer
+	restore = progress.MockOsStdout(&fakeStdout)
+	defer restore()
+	restore = progress.MockOsStderr(&fakeStderr)
+	defer restore()
+
+	for _, pbarType := range []string{"debug", "verbose"} {
+		t.Run(pbarType, func(t *testing.T) {
+			pbar, err := progress.New(pbarType)
+			assert.NoError(t, err)
+
+			var buildLog bytes.Buffer
+			opts := &progress.OSBuildOptions{
+				BuildLog: &buildLog,
+			}
+			err = progress.RunOSBuild(pbar, []byte(`{"fake":"manifest"}`), nil, opts)
+			assert.NoError(t, err)
+			expectedOutput := `osbuild-stdout-output
+osbuild-stderr-output
+`
+			assert.Equal(t, expectedOutput, buildLog.String())
+		})
+	}
+}
+
 func TestRunOSBuildWithProgressIncorrectJSON(t *testing.T) {
 	restore := progress.MockOsbuildCmd(makeFakeOsbuild(t, `echo osbuild-stdout-output
 >&2 echo osbuild-stderr-output
@@ -166,7 +198,8 @@ func TestRunOSBuildWithProgressIncorrectJSON(t *testing.T) {
 
 	pbar, err := progress.New("debug")
 	assert.NoError(t, err)
-	err = progress.RunOSBuild(pbar, []byte(`{"fake":"manifest"}`), "", "", nil, nil)
+
+	err = progress.RunOSBuild(pbar, []byte(`{"fake":"manifest"}`), nil, nil)
 	assert.EqualError(t, err, `errors parsing osbuild status:
 cannot scan line "invalid-json": invalid character 'i' looking for beginning of value`)
 }