From 655b6bbd0f433282ca8952c68c2ec0d5c9bc4d72 Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Sat, 25 Jan 2025 22:16:59 +0100
Subject: [PATCH] progress: tweak progress error reporting

This commit adds catpure of os.Std{out,err} when running osbuild so
that we capture the error log and can display it as part of
an error from osbuild, e.g. when osbuild itself crashes.

This gives us more accurate error reporting if anything fails
during the osbuild building.
---
 bib/pkg/progress/export_test.go   |  8 ++++++++
 bib/pkg/progress/progress.go      | 21 +++++++--------------
 bib/pkg/progress/progress_test.go | 26 ++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/bib/pkg/progress/export_test.go b/bib/pkg/progress/export_test.go
index 252d532..ae12d16 100644
--- a/bib/pkg/progress/export_test.go
+++ b/bib/pkg/progress/export_test.go
@@ -25,3 +25,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
+	}
+}
diff --git a/bib/pkg/progress/progress.go b/bib/pkg/progress/progress.go
index 2112b39..1e1a3a5 100644
--- a/bib/pkg/progress/progress.go
+++ b/bib/pkg/progress/progress.go
@@ -333,6 +333,8 @@ func runOSBuildNoProgress(pb ProgressBar, manifest []byte, store, outputDirector
 	return err
 }
 
+var osbuildCmd = "osbuild"
+
 func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirectory string, exports, extraEnv []string) error {
 	rp, wp, err := os.Pipe()
 	if err != nil {
@@ -342,7 +344,7 @@ func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirect
 	defer wp.Close()
 
 	cmd := exec.Command(
-		"osbuild",
+		osbuildCmd,
 		"--store", store,
 		"--output-directory", outputDirectory,
 		"--monitor=JSONSeqMonitor",
@@ -353,12 +355,11 @@ func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirect
 		cmd.Args = append(cmd.Args, "--export", export)
 	}
 
+	var stdio bytes.Buffer
 	cmd.Env = append(os.Environ(), 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 = &stdio
+	cmd.Stderr = &stdio
 	cmd.ExtraFiles = []*os.File{wp}
 
 	osbuildStatus := osbuild.NewStatusScanner(rp)
@@ -367,7 +368,6 @@ func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirect
 	}
 	wp.Close()
 
-	var tracesMsgs []string
 	for {
 		st, err := osbuildStatus.Status()
 		if err != nil {
@@ -383,13 +383,6 @@ 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)
@@ -397,7 +390,7 @@ func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirect
 	}
 
 	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, stdio.String())
 	}
 
 	return nil
diff --git a/bib/pkg/progress/progress_test.go b/bib/pkg/progress/progress_test.go
index 67a720d..31734ec 100644
--- a/bib/pkg/progress/progress_test.go
+++ b/bib/pkg/progress/progress_test.go
@@ -3,6 +3,8 @@ package progress_test
 import (
 	"bytes"
 	"fmt"
+	"os"
+	"path/filepath"
 	"reflect"
 	"testing"
 
@@ -130,3 +132,27 @@ 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 TestRunOSBuildWithProgress(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
+`)
+}