progress: set --cache-max-size in osbuild

bib/cmd/bootc-image-builder/main.go

@@ -488,7 +488,12 @@ func cmdBuild(cmd *cobra.Command, args []string) error {
 		osbuildEnv = append(osbuildEnv, envVars...)
 	}
 
-	if err = progress.RunOSBuild(pbar, mf, osbuildStore, outputDir, exports, osbuildEnv); err != nil {
+	osbuildOpts := progress.OSBuildOptions{
+		StoreDir:  osbuildStore,
+		OutputDir: outputDir,
+		ExtraEnv:  osbuildEnv,
+	}
+	if err = progress.RunOSBuild(pbar, mf, exports, &osbuildOpts); err != nil {
 		return fmt.Errorf("cannot run osbuild: %w", err)
 	}
 

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
@@ -25,3 +33,11 @@ func MockIsattyIsTerminal(fn func(uintptr) bool) (restore func()) {
 		isattyIsTerminal = saved
 	}
 }
+
+func MockOsbuildCmd(s string) (restore func()) {
+	saved := osbuildCmd
+	osbuildCmd = s
+	return func() {
+		osbuildCmd = saved
+	}
+}

bib/pkg/progress/progress.go

@@ -14,10 +14,12 @@ import (
 	"github.com/mattn/go-isatty"
 	"github.com/sirupsen/logrus"
 
+	"github.com/osbuild/images/pkg/datasizes"
 	"github.com/osbuild/images/pkg/osbuild"
 )
 
 var (
+	osStdout io.Writer = os.Stdout
 	osStderr io.Writer = os.Stderr
 
 	// This is only needed because pb.Pool require a real terminal.
@@ -311,54 +313,94 @@ func (b *debugProgressBar) SetProgress(subLevel int, msg string, done int, total
 	return nil
 }
 
-// XXX: merge variant back into images/pkg/osbuild/osbuild-exec.go
-func RunOSBuild(pb ProgressBar, manifest []byte, store, outputDirectory string, exports, extraEnv []string) error {
+type OSBuildOptions struct {
+	StoreDir  string
+	OutputDir string
+	ExtraEnv  []string
+
+	// BuildLog writes the osbuild output to the given writer
+	BuildLog io.Writer
+
+	CacheMaxSize int64
+}
+
+func writersForOSBuild(pb ProgressBar, opts *OSBuildOptions, internalBuildLog io.Writer) (osbuildStdout io.Writer, osbuildStderr 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.
+	// is currently expecting the raw osbuild output.
 	switch pb.(type) {
-	case *terminalProgressBar, *debugProgressBar:
-		return runOSBuildWithProgress(pb, manifest, store, outputDirectory, exports, extraEnv)
+	case *verboseProgressBar:
+		// 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 osStdout, osStderr
+		}
+		// With a build log we need a single output stream
+		osbuildStdout = osStdout
 	default:
-		return runOSBuildNoProgress(pb, manifest, store, outputDirectory, exports, extraEnv)
+		// hide the direct osbuild output by default
+		osbuildStdout = io.Discard
 	}
-}
 
-func runOSBuildNoProgress(pb ProgressBar, manifest []byte, store, outputDirectory string, exports, extraEnv []string) error {
-	_, err := osbuild.RunOSBuild(manifest, store, outputDirectory, exports, nil, 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
 }
 
-func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirectory string, exports, extraEnv []string) error {
+var osbuildCmd = "osbuild"
+
+// 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)
+
 	rp, wp, err := os.Pipe()
 	if err != nil {
 		return fmt.Errorf("cannot create pipe for osbuild: %w", err)
 	}
 	defer rp.Close()
 	defer wp.Close()
 
+	cacheMaxSize := int64(20 * datasizes.GiB)
+	if opts.CacheMaxSize != 0 {
+		cacheMaxSize = opts.CacheMaxSize
+	}
 	cmd := exec.Command(
-		"osbuild",
-		"--store", store,
-		"--output-directory", outputDirectory,
+		osbuildCmd,
+		"--store", opts.StoreDir,
+		"--output-directory", opts.OutputDir,
 		"--monitor=JSONSeqMonitor",
 		"--monitor-fd=3",
+		fmt.Sprintf("--cache-max-size=%v", cacheMaxSize),
 		"-",
 	)
 	for _, export := range exports {
 		cmd.Args = append(cmd.Args, "--export", export)
 	}
 
-	cmd.Env = append(os.Environ(), extraEnv...)
+	cmd.Env = append(os.Environ(), opts.ExtraEnv...)
 	cmd.Stdin = bytes.NewBuffer(manifest)
-	cmd.Stderr = os.Stderr
-	// we could use "--json" here and would get the build-result
-	// exported here
-	cmd.Stdout = nil
+	cmd.Stdout = osbuildStdout
+	cmd.Stderr = osbuildStderr
 	cmd.ExtraFiles = []*os.File{wp}
 
 	osbuildStatus := osbuild.NewStatusScanner(rp)
@@ -367,11 +409,14 @@ func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirect
 	}
 	wp.Close()
 
-	var tracesMsgs []string
+	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 {
-			return fmt.Errorf("error reading osbuild status: %w", err)
+			statusErrs = append(statusErrs, err)
+			continue
 		}
 		if st == nil {
 			break
@@ -383,21 +428,17 @@ func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirect
 			}
 			i++
 		}
-		// keep the messages/traces for better error reporting
-		if st.Message != "" {
-			tracesMsgs = append(tracesMsgs, st.Message)
-		}
-		if st.Trace != "" {
-			tracesMsgs = append(tracesMsgs, st.Trace)
-		}
 		// forward to user
 		if st.Message != "" {
 			pb.SetMessagef(st.Message)
 		}
 	}
 
 	if err := cmd.Wait(); err != nil {
-		return fmt.Errorf("error running osbuild: %w\nLog:\n%s", err, strings.Join(tracesMsgs, "\n"))
+		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...))
 	}
 
 	return nil

bib/pkg/progress/progress_test.go

@@ -3,6 +3,8 @@ package progress_test
 import (
 	"bytes"
 	"fmt"
+	"os"
+	"path/filepath"
 	"reflect"
 	"testing"
 
@@ -130,3 +132,92 @@ func TestProgressNewAutoselect(t *testing.T) {
 		assert.Equal(t, reflect.TypeOf(pb), reflect.TypeOf(tc.expected), fmt.Sprintf("[%v] %T not the expected %T", tc.onTerm, pb, tc.expected))
 	}
 }
+
+func makeFakeOsbuild(t *testing.T, content string) string {
+	p := filepath.Join(t.TempDir(), "fake-osbuild")
+	err := os.WriteFile(p, []byte("#!/bin/sh\n"+content), 0755)
+	assert.NoError(t, err)
+	return p
+}
+
+func TestRunOSBuildWithProgressErrorReporting(t *testing.T) {
+	restore := progress.MockOsbuildCmd(makeFakeOsbuild(t, `echo osbuild-stdout-output
+>&2 echo osbuild-stderr-output
+exit 112
+`))
+	defer restore()
+
+	pbar, err := progress.New("debug")
+	assert.NoError(t, err)
+	err = progress.RunOSBuild(pbar, []byte(`{"fake":"manifest"}`), nil, nil)
+	assert.EqualError(t, err, `error running osbuild: exit status 112
+Output:
+osbuild-stdout-output
+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
+>&3 echo invalid-json
+`))
+	defer restore()
+
+	pbar, err := progress.New("debug")
+	assert.NoError(t, err)
+
+	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`)
+}
+
+func TestRunOSBuildCacheMaxSize(t *testing.T) {
+	fakeOsbuildBinary := makeFakeOsbuild(t, `echo "$@" > "$0".cmdline`)
+	restore := progress.MockOsbuildCmd(fakeOsbuildBinary)
+	defer restore()
+
+	pbar, err := progress.New("debug")
+	assert.NoError(t, err)
+
+	osbuildOpts := &progress.OSBuildOptions{
+		CacheMaxSize: 77,
+	}
+	err = progress.RunOSBuild(pbar, []byte(`{"fake":"manifest"}`), nil, osbuildOpts)
+	assert.NoError(t, err)
+	cmdline, err := os.ReadFile(fakeOsbuildBinary + ".cmdline")
+	assert.NoError(t, err)
+	assert.Contains(t, string(cmdline), "--cache-max-size=77")
+}