From a0551c9d487e921fc96ea7a74eb0ddd56812c851 Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Fri, 17 Jan 2025 13:02:46 +0100
Subject: [PATCH 01/11] go.mod: run go get github.com/cheggaaa/pb/v3

---
 go.mod | 4 ++++
 go.sum | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/go.mod b/go.mod
index 4b43070823..a2ac57042b 100644
--- a/go.mod
+++ b/go.mod
@@ -13,6 +13,7 @@ require (
 	github.com/Azure/go-autorest/autorest/azure/auth v0.5.13
 	github.com/BurntSushi/toml v1.4.0
 	github.com/aws/aws-sdk-go v1.55.5
+	github.com/cheggaaa/pb/v3 v3.1.5
 	github.com/containers/common v0.60.4
 	github.com/containers/image/v5 v5.32.2
 	github.com/containers/storage v1.55.0
@@ -77,6 +78,7 @@ require (
 	github.com/docker/go-connections v0.5.0 // indirect
 	github.com/docker/go-units v0.5.0 // indirect
 	github.com/dougm/pretty v0.0.0-20171025230240-2ee9d7453c02 // indirect
+	github.com/fatih/color v1.16.0 // indirect
 	github.com/felixge/httpsnoop v1.0.4 // indirect
 	github.com/go-jose/go-jose/v4 v4.0.2 // indirect
 	github.com/go-logr/logr v1.4.2 // indirect
@@ -112,6 +114,8 @@ require (
 	github.com/kr/text v0.2.0 // indirect
 	github.com/letsencrypt/boulder v0.0.0-20240418210053-89b07f4543e0 // indirect
 	github.com/mailru/easyjson v0.7.7 // indirect
+	github.com/mattn/go-colorable v0.1.13 // indirect
+	github.com/mattn/go-isatty v0.0.20 // indirect
 	github.com/mattn/go-runewidth v0.0.16 // indirect
 	github.com/mattn/go-sqlite3 v1.14.22 // indirect
 	github.com/miekg/pkcs11 v1.1.1 // indirect
diff --git a/go.sum b/go.sum
index 0bc0465654..d258454546 100644
--- a/go.sum
+++ b/go.sum
@@ -82,6 +82,8 @@ github.com/cenkalti/backoff/v4 v4.2.1/go.mod h1:Y3VNntkOUPxTVeUxJ/G5vcM//AlwfmyY
 github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=
 github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs=
 github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
+github.com/cheggaaa/pb/v3 v3.1.5 h1:QuuUzeM2WsAqG2gMqtzaWithDJv0i+i6UlnwSCI4QLk=
+github.com/cheggaaa/pb/v3 v3.1.5/go.mod h1:CrxkeghYTXi1lQBEI7jSn+3svI3cuc19haAj6jM60XI=
 github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
 github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc=
 github.com/containerd/cgroups/v3 v3.0.3 h1:S5ByHZ/h9PMe5IOQoN7E+nMc2UcLEM/V48DGDJ9kip0=
@@ -270,6 +272,7 @@ github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0
 github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc=
 github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA=
 github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg=
+github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM=
 github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY=
 github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y=
 github.com/mattn/go-runewidth v0.0.16 h1:E5ScNMtiwvlvB5paMFdw9p4kSQzbXFikJ5SQO6TULQc=
@@ -482,7 +485,9 @@ golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.15.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 golang.org/x/sys v0.29.0 h1:TPYlXGxvx1MGTn2GiZDhnjPA9wZzZeGKHHmKhHYvgaU=

From e0439fdbdd8e926ec9e412f7aaaa4f75a94afd18 Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Wed, 4 Dec 2024 12:53:01 +0100
Subject: [PATCH 02/11] progress: add progressbar module

This commit adds a progressbar interface and implementations:
1. terminalProgressBar based on github.com/cheggaaa/pb/v3
2. plainProgressBar to just pass the message but no progress
3. debugProgressBar to print the osbuild/images status

The interface is very tailored to the needs of bib but it should
be similar enough to be reusable by `image-builder-cli`.

It contains a top level spinner that gives the current step:
"Make manifest" or "Building image". The the subprogress from
osbuild: the pipelines and the stages from the pipelines as
progress bars and the last line is the latest status message,
usually something from osbuild like "Starting pipeline qcow2".

The spinenr is a bit of a creative choice, it could also be a
progress bar because we know the steps (manifest+build). But the
runtime is totally dominated by the build step so having a first level
progress bar is odd as the UI will spend 90% of the time in it.
Plus the spinner gives it more a more "dynamic" appearnce.
---
 progress/bib/internal/progress/export_test.go |  29 ++
 progress/bib/internal/progress/progress.go    | 309 ++++++++++++++++++
 .../bib/internal/progress/progress_test.go    | 107 ++++++
 3 files changed, 445 insertions(+)
 create mode 100644 progress/bib/internal/progress/export_test.go
 create mode 100644 progress/bib/internal/progress/progress.go
 create mode 100644 progress/bib/internal/progress/progress_test.go

diff --git a/progress/bib/internal/progress/export_test.go b/progress/bib/internal/progress/export_test.go
new file mode 100644
index 0000000000..795b0246f3
--- /dev/null
+++ b/progress/bib/internal/progress/export_test.go
@@ -0,0 +1,29 @@
+package progress
+
+import (
+	"io"
+)
+
+type (
+	TerminalProgressBar = terminalProgressBar
+	DebugProgressBar    = debugProgressBar
+	PlainProgressBar    = plainProgressBar
+)
+
+func MockOsStderr(w io.Writer) (restore func()) {
+	saved := osStderr
+	osStderr = w
+	return func() {
+		osStderr = saved
+	}
+}
+
+func MockIsattyIsTerminal(b bool) (restore func()) {
+	saved := isattyIsTerminal
+	isattyIsTerminal = func(uintptr) bool {
+		return b
+	}
+	return func() {
+		isattyIsTerminal = saved
+	}
+}
diff --git a/progress/bib/internal/progress/progress.go b/progress/bib/internal/progress/progress.go
new file mode 100644
index 0000000000..88149aa9f1
--- /dev/null
+++ b/progress/bib/internal/progress/progress.go
@@ -0,0 +1,309 @@
+package progress
+
+import (
+	"bytes"
+	"fmt"
+	"io"
+	"os"
+	"os/exec"
+	"strings"
+
+	"github.com/cheggaaa/pb/v3"
+	"github.com/mattn/go-isatty"
+	"github.com/sirupsen/logrus"
+
+	"github.com/osbuild/images/pkg/osbuild"
+)
+
+var (
+	osStderr         io.Writer = os.Stderr
+	isattyIsTerminal           = isatty.IsTerminal
+)
+
+// ProgressBar is an interface for progress reporting when there is
+// an arbitrary amount of sub-progress information (like osbuild)
+type ProgressBar interface {
+	// SetProgress sets the progress details at the given "level".
+	// Levels should start with "0" and increase as the nesting
+	// gets deeper.
+	SetProgress(level int, msg string, done int, total int) error
+
+	// The high-level message that is displayed in a spinner
+	// that contains the current top level step, for bib this
+	// is really just "Manifest generation step" and
+	// "Image generation step". We could map this to a three-level
+	// progress as well but we spend 90% of the time in the
+	// "Image generation step" so the UI looks a bit odd.
+	SetPulseMsgf(fmt string, args ...interface{})
+
+	// A high level message with the last operation status.
+	// For us this usually comes from the stages and has information
+	// like "Starting module org.osbuild.selinux"
+	SetMessagef(fmt string, args ...interface{})
+	Start() error
+	Stop() error
+}
+
+// New creates a new progressbar based on the requested type
+func New(typ string) (ProgressBar, error) {
+	switch typ {
+	case "", "plain":
+		return NewPlainProgressBar()
+	case "term":
+		return NewTerminalProgressBar()
+	case "debug":
+		return NewDebugProgressBar()
+	default:
+		return nil, fmt.Errorf("unknown progress type: %q", typ)
+	}
+}
+
+type terminalProgressBar struct {
+	spinnerPb   *pb.ProgressBar
+	msgPb       *pb.ProgressBar
+	subLevelPbs []*pb.ProgressBar
+
+	pool        *pb.Pool
+	poolStarted bool
+}
+
+// NewTerminalProgressBar creates a new default pb3 based progressbar suitable for
+// most terminals.
+func NewTerminalProgressBar() (ProgressBar, error) {
+	f, ok := osStderr.(*os.File)
+	if !ok {
+		return nil, fmt.Errorf("cannot use %T as a terminal", f)
+	}
+	if !isattyIsTerminal(f.Fd()) {
+		return nil, fmt.Errorf("cannot use term progress without a terminal")
+	}
+
+	b := &terminalProgressBar{}
+	b.spinnerPb = pb.New(0)
+	b.spinnerPb.SetTemplate(`[{{ (cycle . "|" "/" "-" "\\") }}] {{ string . "spinnerMsg" }}`)
+	b.msgPb = pb.New(0)
+	b.msgPb.SetTemplate(`Message: {{ string . "msg" }}`)
+	b.pool = pb.NewPool(b.spinnerPb, b.msgPb)
+	b.pool.Output = osStderr
+	return b, nil
+}
+
+func (b *terminalProgressBar) SetProgress(subLevel int, msg string, done int, total int) error {
+	// auto-add as needed, requires sublevels to get added in order
+	// i.e. adding 0 and then 2 will fail
+	switch {
+	case subLevel == len(b.subLevelPbs):
+		apb := pb.New(0)
+		b.subLevelPbs = append(b.subLevelPbs, apb)
+		progressBarTmpl := `[{{ counters . }}] {{ string . "prefix" }} {{ bar .}} {{ percent . }}`
+		apb.SetTemplateString(progressBarTmpl)
+		b.pool.Add(apb)
+	case subLevel > len(b.subLevelPbs):
+		return fmt.Errorf("sublevel added out of order, have %v sublevels but want level %v", len(b.subLevelPbs), subLevel)
+	}
+	apb := b.subLevelPbs[subLevel]
+	apb.SetTotal(int64(total) + 1)
+	apb.SetCurrent(int64(done) + 1)
+	if msg != "" {
+		apb.Set("prefix", msg)
+	}
+	return nil
+}
+
+func shorten(msg string) string {
+	msg = strings.Replace(msg, "\n", " ", -1)
+	// XXX: make this smarter
+	if len(msg) > 60 {
+		return msg[:60] + "..."
+	}
+	return msg
+}
+
+func (b *terminalProgressBar) SetPulseMsgf(msg string, args ...interface{}) {
+	b.spinnerPb.Set("spinnerMsg", shorten(fmt.Sprintf(msg, args...)))
+}
+
+func (b *terminalProgressBar) SetMessagef(msg string, args ...interface{}) {
+	b.msgPb.Set("msg", shorten(fmt.Sprintf(msg, args...)))
+}
+
+func (b *terminalProgressBar) Start() error {
+	if err := b.pool.Start(); err != nil {
+		return fmt.Errorf("progress bar failed: %w", err)
+	}
+	b.poolStarted = true
+	return nil
+}
+
+func (b *terminalProgressBar) Stop() (err error) {
+	// pb.Stop() will deadlock if it was not started before
+	if b.poolStarted {
+		err = b.pool.Stop()
+	}
+	b.poolStarted = false
+	return err
+}
+
+type plainProgressBar struct {
+	w io.Writer
+}
+
+// NewPlainProgressBar starts a new "plain" progressbar that will just
+// prints message but does not show any progress.
+func NewPlainProgressBar() (ProgressBar, error) {
+	b := &plainProgressBar{w: osStderr}
+	return b, nil
+}
+
+func (b *plainProgressBar) SetPulseMsgf(msg string, args ...interface{}) {
+	fmt.Fprintf(b.w, msg, args...)
+}
+
+func (b *plainProgressBar) SetMessagef(msg string, args ...interface{}) {
+	fmt.Fprintf(b.w, msg, args...)
+}
+
+func (b *plainProgressBar) Start() (err error) {
+	return nil
+}
+
+func (b *plainProgressBar) Stop() (err error) {
+	return nil
+}
+
+func (b *plainProgressBar) SetProgress(subLevel int, msg string, done int, total int) error {
+	return nil
+}
+
+type debugProgressBar struct {
+	w io.Writer
+}
+
+// NewDebugProgressBar will create a progressbar aimed to debug the
+// lower level osbuild/images message. It will never clear the screen
+// so "glitches/weird" messages from the lower-layers can be inspected
+// easier.
+func NewDebugProgressBar() (ProgressBar, error) {
+	b := &debugProgressBar{w: osStderr}
+	return b, nil
+}
+
+func (b *debugProgressBar) SetPulseMsgf(msg string, args ...interface{}) {
+	fmt.Fprintf(b.w, "pulse: ")
+	fmt.Fprintf(b.w, msg, args...)
+	fmt.Fprintf(b.w, "\n")
+}
+
+func (b *debugProgressBar) SetMessagef(msg string, args ...interface{}) {
+	fmt.Fprintf(b.w, "msg: ")
+	fmt.Fprintf(b.w, msg, args...)
+	fmt.Fprintf(b.w, "\n")
+}
+
+func (b *debugProgressBar) Start() (err error) {
+	fmt.Fprintf(b.w, "Start progressbar\n")
+	return nil
+}
+
+func (b *debugProgressBar) Stop() (err error) {
+	fmt.Fprintf(b.w, "Stop progressbar\n")
+	return nil
+}
+
+func (b *debugProgressBar) SetProgress(subLevel int, msg string, done int, total int) error {
+	fmt.Fprintf(b.w, "%s[%v / %v] %s", strings.Repeat("  ", subLevel), done, total, msg)
+	fmt.Fprintf(b.w, "\n")
+	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 {
+	// 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.
+	switch pb.(type) {
+	case *terminalProgressBar, *debugProgressBar:
+		return runOSBuildWithProgress(pb, manifest, store, outputDirectory, exports, extraEnv)
+	default:
+		return runOSBuildNoProgress(pb, manifest, store, outputDirectory, exports, extraEnv)
+	}
+}
+
+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
+}
+
+func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirectory string, exports, extraEnv []string) error {
+	rp, wp, err := os.Pipe()
+	if err != nil {
+		return fmt.Errorf("cannot create pipe for osbuild: %w", err)
+	}
+	defer rp.Close()
+	defer wp.Close()
+
+	cmd := exec.Command(
+		"osbuild",
+		"--store", store,
+		"--output-directory", outputDirectory,
+		"--monitor=JSONSeqMonitor",
+		"--monitor-fd=3",
+		"-",
+	)
+	for _, export := range exports {
+		cmd.Args = append(cmd.Args, "--export", export)
+	}
+
+	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.ExtraFiles = []*os.File{wp}
+
+	osbuildStatus := osbuild.NewStatusScanner(rp)
+	if err := cmd.Start(); err != nil {
+		return fmt.Errorf("error starting osbuild: %v", err)
+	}
+	wp.Close()
+
+	var tracesMsgs []string
+	for {
+		st, err := osbuildStatus.Status()
+		if err != nil {
+			return fmt.Errorf("error reading osbuild status: %w", err)
+		}
+		if st == nil {
+			break
+		}
+		i := 0
+		for p := st.Progress; p != nil; p = p.SubProgress {
+			if err := pb.SetProgress(i, p.Message, p.Done, p.Total); err != nil {
+				logrus.Warnf("cannot set progress: %v", err)
+			}
+			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 nil
+}
diff --git a/progress/bib/internal/progress/progress_test.go b/progress/bib/internal/progress/progress_test.go
new file mode 100644
index 0000000000..ebff346a87
--- /dev/null
+++ b/progress/bib/internal/progress/progress_test.go
@@ -0,0 +1,107 @@
+package progress_test
+
+import (
+	"bytes"
+	"fmt"
+	"reflect"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/osbuild/bootc-image-builder/bib/internal/progress"
+)
+
+func TestProgressNew(t *testing.T) {
+	restore := progress.MockIsattyIsTerminal(true)
+	defer restore()
+
+	for _, tc := range []struct {
+		typ         string
+		expected    interface{}
+		expectedErr string
+	}{
+		{"term", &progress.TerminalProgressBar{}, ""},
+		{"debug", &progress.DebugProgressBar{}, ""},
+		{"plain", &progress.PlainProgressBar{}, ""},
+		{"bad", nil, `unknown progress type: "bad"`},
+	} {
+		pb, err := progress.New(tc.typ)
+		if tc.expectedErr == "" {
+			assert.NoError(t, err)
+			assert.Equal(t, reflect.TypeOf(pb), reflect.TypeOf(tc.expected), fmt.Sprintf("[%v] %T not the expected %T", tc.typ, pb, tc.expected))
+		} else {
+			assert.EqualError(t, err, tc.expectedErr)
+		}
+	}
+}
+
+func TestPlainProgress(t *testing.T) {
+	var buf bytes.Buffer
+	restore := progress.MockOsStderr(&buf)
+	defer restore()
+
+	// plain progress never generates progress output
+	pbar, err := progress.NewPlainProgressBar()
+	assert.NoError(t, err)
+	err = pbar.SetProgress(0, "set-progress", 1, 100)
+	assert.NoError(t, err)
+	assert.Equal(t, "", buf.String())
+
+	// but it shows the messages
+	pbar.SetPulseMsgf("pulse")
+	assert.Equal(t, "pulse", buf.String())
+	buf.Reset()
+
+	pbar.SetMessagef("message")
+	assert.Equal(t, "message", buf.String())
+	buf.Reset()
+
+	err = pbar.Start()
+	assert.NoError(t, err)
+	assert.Equal(t, "", buf.String())
+	err = pbar.Stop()
+	assert.NoError(t, err)
+	assert.Equal(t, "", buf.String())
+}
+
+func TestDebugProgress(t *testing.T) {
+	var buf bytes.Buffer
+	restore := progress.MockOsStderr(&buf)
+	defer restore()
+
+	pbar, err := progress.NewDebugProgressBar()
+	assert.NoError(t, err)
+	err = pbar.SetProgress(0, "set-progress-msg", 1, 100)
+	assert.NoError(t, err)
+	assert.Equal(t, "[1 / 100] set-progress-msg\n", buf.String())
+	buf.Reset()
+
+	pbar.SetPulseMsgf("pulse-msg")
+	assert.Equal(t, "pulse: pulse-msg\n", buf.String())
+	buf.Reset()
+
+	pbar.SetMessagef("some-message")
+	assert.Equal(t, "msg: some-message\n", buf.String())
+	buf.Reset()
+
+	err = pbar.Start()
+	assert.NoError(t, err)
+	assert.Equal(t, "Start progressbar\n", buf.String())
+	buf.Reset()
+
+	err = pbar.Stop()
+	assert.NoError(t, err)
+	assert.Equal(t, "Stop progressbar\n", buf.String())
+	buf.Reset()
+}
+
+func TestTermProgressNoTerm(t *testing.T) {
+	var buf bytes.Buffer
+	restore := progress.MockOsStderr(&buf)
+	defer restore()
+
+	// TODO: use something like "github.com/creack/pty" to create
+	// a real pty to test this for real
+	_, err := progress.NewTerminalProgressBar()
+	assert.EqualError(t, err, "cannot use *os.File as a terminal")
+}

From 4c21e19ed9bf5e497d9de1f860a85174bf00cefd Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Wed, 11 Dec 2024 09:57:49 +0100
Subject: [PATCH 03/11] progress: implement pb.Pool without raw terminal access

This commit implements enough of `pb.Pool` for our needs
without the need to implement raw terminal access.

It turns out that by default podman does not connect the
full tty, even in `--privileged` mode. This is a sensible
security default but it means `pb.Pool` does not work as
it wants to set the terminal into "raw" mode and will fail
with an ioctl() error when not running on a terminal.

However we really just need simple ANSI sequences to
render the pool so this seems unnecessary. The initial
idea was to just use  `--log-driver=passthrough-tty` but
that is not available on podman 4.x. (which is part of
Ubuntu 24.04 LTS and the GH actions).

So this commit just implemnts a custom pool like renderer.
---
 progress/bib/internal/progress/export_test.go |  10 --
 progress/bib/internal/progress/progress.go    | 103 +++++++++++++-----
 .../bib/internal/progress/progress_test.go    |  27 +++--
 3 files changed, 95 insertions(+), 45 deletions(-)

diff --git a/progress/bib/internal/progress/export_test.go b/progress/bib/internal/progress/export_test.go
index 795b0246f3..c831824264 100644
--- a/progress/bib/internal/progress/export_test.go
+++ b/progress/bib/internal/progress/export_test.go
@@ -17,13 +17,3 @@ func MockOsStderr(w io.Writer) (restore func()) {
 		osStderr = saved
 	}
 }
-
-func MockIsattyIsTerminal(b bool) (restore func()) {
-	saved := isattyIsTerminal
-	isattyIsTerminal = func(uintptr) bool {
-		return b
-	}
-	return func() {
-		isattyIsTerminal = saved
-	}
-}
diff --git a/progress/bib/internal/progress/progress.go b/progress/bib/internal/progress/progress.go
index 88149aa9f1..2c32318d30 100644
--- a/progress/bib/internal/progress/progress.go
+++ b/progress/bib/internal/progress/progress.go
@@ -7,19 +7,31 @@ import (
 	"os"
 	"os/exec"
 	"strings"
+	"time"
 
 	"github.com/cheggaaa/pb/v3"
-	"github.com/mattn/go-isatty"
 	"github.com/sirupsen/logrus"
 
 	"github.com/osbuild/images/pkg/osbuild"
 )
 
 var (
-	osStderr         io.Writer = os.Stderr
-	isattyIsTerminal           = isatty.IsTerminal
+	osStderr io.Writer = os.Stderr
+
+	// This is only needed because pb.Pool require a real terminal.
+	// It sets it into "raw-mode" but there is really no need for
+	// this (see "func render()" below) so once this is fixed
+	// upstream we should remove this.
+	ESC         = "\x1b"
+	ERASE_LINE  = ESC + "[2K"
+	CURSOR_HIDE = ESC + "[?25l"
+	CURSOR_SHOW = ESC + "[?25h"
 )
 
+func cursorUp(i int) string {
+	return fmt.Sprintf("%s[%dA", ESC, i)
+}
+
 // ProgressBar is an interface for progress reporting when there is
 // an arbitrary amount of sub-progress information (like osbuild)
 type ProgressBar interface {
@@ -47,6 +59,8 @@ type ProgressBar interface {
 // New creates a new progressbar based on the requested type
 func New(typ string) (ProgressBar, error) {
 	switch typ {
+	// XXX: autoseelct based on PS1 value (i.e. use term in
+	// interactive shells only?)
 	case "", "plain":
 		return NewPlainProgressBar()
 	case "term":
@@ -63,28 +77,21 @@ type terminalProgressBar struct {
 	msgPb       *pb.ProgressBar
 	subLevelPbs []*pb.ProgressBar
 
-	pool        *pb.Pool
-	poolStarted bool
+	shutdownCh chan bool
+
+	out io.Writer
 }
 
 // NewTerminalProgressBar creates a new default pb3 based progressbar suitable for
 // most terminals.
 func NewTerminalProgressBar() (ProgressBar, error) {
-	f, ok := osStderr.(*os.File)
-	if !ok {
-		return nil, fmt.Errorf("cannot use %T as a terminal", f)
+	b := &terminalProgressBar{
+		out: osStderr,
 	}
-	if !isattyIsTerminal(f.Fd()) {
-		return nil, fmt.Errorf("cannot use term progress without a terminal")
-	}
-
-	b := &terminalProgressBar{}
 	b.spinnerPb = pb.New(0)
 	b.spinnerPb.SetTemplate(`[{{ (cycle . "|" "/" "-" "\\") }}] {{ string . "spinnerMsg" }}`)
 	b.msgPb = pb.New(0)
 	b.msgPb.SetTemplate(`Message: {{ string . "msg" }}`)
-	b.pool = pb.NewPool(b.spinnerPb, b.msgPb)
-	b.pool.Output = osStderr
 	return b, nil
 }
 
@@ -97,16 +104,13 @@ func (b *terminalProgressBar) SetProgress(subLevel int, msg string, done int, to
 		b.subLevelPbs = append(b.subLevelPbs, apb)
 		progressBarTmpl := `[{{ counters . }}] {{ string . "prefix" }} {{ bar .}} {{ percent . }}`
 		apb.SetTemplateString(progressBarTmpl)
-		b.pool.Add(apb)
 	case subLevel > len(b.subLevelPbs):
 		return fmt.Errorf("sublevel added out of order, have %v sublevels but want level %v", len(b.subLevelPbs), subLevel)
 	}
 	apb := b.subLevelPbs[subLevel]
 	apb.SetTotal(int64(total) + 1)
 	apb.SetCurrent(int64(done) + 1)
-	if msg != "" {
-		apb.Set("prefix", msg)
-	}
+	apb.Set("prefix", msg)
 	return nil
 }
 
@@ -127,21 +131,66 @@ func (b *terminalProgressBar) SetMessagef(msg string, args ...interface{}) {
 	b.msgPb.Set("msg", shorten(fmt.Sprintf(msg, args...)))
 }
 
+func (b *terminalProgressBar) render() {
+	var renderedLines int
+	fmt.Fprintf(b.out, "%s%s\n", ERASE_LINE, b.spinnerPb.String())
+	renderedLines++
+	for _, prog := range b.subLevelPbs {
+		fmt.Fprintf(b.out, "%s%s\n", ERASE_LINE, prog.String())
+		renderedLines++
+	}
+	fmt.Fprintf(b.out, "%s%s\n", ERASE_LINE, b.msgPb.String())
+	renderedLines++
+	fmt.Fprint(b.out, cursorUp(renderedLines))
+}
+
+// Workaround for the pb.Pool requiring "raw-mode" - see here how to avoid
+// it. Once fixes upstream we should remove this.
+func (b *terminalProgressBar) renderLoop() {
+	for {
+		select {
+		case <-b.shutdownCh:
+			b.render()
+			// finally move cursor down again
+			fmt.Fprint(b.out, CURSOR_SHOW)
+			fmt.Fprint(b.out, strings.Repeat("\n", 2+len(b.subLevelPbs)))
+			// close last to avoid race with b.out
+			close(b.shutdownCh)
+			return
+		case <-time.After(200 * time.Millisecond):
+			// break to redraw the screen
+		}
+		b.render()
+	}
+}
+
 func (b *terminalProgressBar) Start() error {
-	if err := b.pool.Start(); err != nil {
-		return fmt.Errorf("progress bar failed: %w", err)
+	// render() already running
+	if b.shutdownCh != nil {
+		return nil
 	}
-	b.poolStarted = true
+	fmt.Fprintf(b.out, "%s", CURSOR_HIDE)
+	b.shutdownCh = make(chan bool)
+	go b.renderLoop()
+
 	return nil
 }
 
 func (b *terminalProgressBar) Stop() (err error) {
-	// pb.Stop() will deadlock if it was not started before
-	if b.poolStarted {
-		err = b.pool.Stop()
+	if b.shutdownCh == nil {
+		return nil
 	}
-	b.poolStarted = false
-	return err
+	// request shutdown
+	b.shutdownCh <- true
+	// wait for ack
+	select {
+	case <-b.shutdownCh:
+	// shudown complete
+	case <-time.After(1 * time.Second):
+		logrus.Warnf("no progress channel shutdown after 1sec")
+	}
+	b.shutdownCh = nil
+	return nil
 }
 
 type plainProgressBar struct {
diff --git a/progress/bib/internal/progress/progress_test.go b/progress/bib/internal/progress/progress_test.go
index ebff346a87..364d96a958 100644
--- a/progress/bib/internal/progress/progress_test.go
+++ b/progress/bib/internal/progress/progress_test.go
@@ -12,9 +12,6 @@ import (
 )
 
 func TestProgressNew(t *testing.T) {
-	restore := progress.MockIsattyIsTerminal(true)
-	defer restore()
-
 	for _, tc := range []struct {
 		typ         string
 		expected    interface{}
@@ -23,6 +20,7 @@ func TestProgressNew(t *testing.T) {
 		{"term", &progress.TerminalProgressBar{}, ""},
 		{"debug", &progress.DebugProgressBar{}, ""},
 		{"plain", &progress.PlainProgressBar{}, ""},
+		// unknown progress type
 		{"bad", nil, `unknown progress type: "bad"`},
 	} {
 		pb, err := progress.New(tc.typ)
@@ -95,13 +93,26 @@ func TestDebugProgress(t *testing.T) {
 	buf.Reset()
 }
 
-func TestTermProgressNoTerm(t *testing.T) {
+func TestTermProgress(t *testing.T) {
 	var buf bytes.Buffer
 	restore := progress.MockOsStderr(&buf)
 	defer restore()
 
-	// TODO: use something like "github.com/creack/pty" to create
-	// a real pty to test this for real
-	_, err := progress.NewTerminalProgressBar()
-	assert.EqualError(t, err, "cannot use *os.File as a terminal")
+	pbar, err := progress.NewTerminalProgressBar()
+	assert.NoError(t, err)
+
+	err = pbar.Start()
+	assert.NoError(t, err)
+	pbar.SetPulseMsgf("pulse-msg")
+	pbar.SetMessagef("some-message")
+	err = pbar.SetProgress(0, "set-progress-msg", 0, 5)
+	assert.NoError(t, err)
+	err = pbar.Stop()
+	assert.NoError(t, err)
+
+	assert.Contains(t, buf.String(), "[1 / 6] set-progress-msg")
+	assert.Contains(t, buf.String(), "[|] pulse-msg\n")
+	assert.Contains(t, buf.String(), "Message: some-message\n")
+	// check shutdown
+	assert.Contains(t, buf.String(), progress.CURSOR_SHOW)
 }

From aa26b62e1229e6b04c559de16e82c9324f43c293 Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Thu, 12 Dec 2024 17:59:39 +0100
Subject: [PATCH 04/11] progress: simplify/tweak Progress interface

The Start/Stop methods can no longer error so remove the error
from the signature. The commit also tweaks the doc strings and
comments slightly.
---
 progress/bib/internal/progress/progress.go    | 39 +++++++++++--------
 .../bib/internal/progress/progress_test.go    | 18 +++------
 2 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/progress/bib/internal/progress/progress.go b/progress/bib/internal/progress/progress.go
index 2c32318d30..6493655194 100644
--- a/progress/bib/internal/progress/progress.go
+++ b/progress/bib/internal/progress/progress.go
@@ -38,6 +38,10 @@ type ProgressBar interface {
 	// SetProgress sets the progress details at the given "level".
 	// Levels should start with "0" and increase as the nesting
 	// gets deeper.
+	//
+	// Note that reducing depth is currently not supported, once
+	// a sub-progress is added it cannot be removed/hidden
+	// (but if required it can be added, its a SMOP)
 	SetProgress(level int, msg string, done int, total int) error
 
 	// The high-level message that is displayed in a spinner
@@ -52,8 +56,13 @@ type ProgressBar interface {
 	// For us this usually comes from the stages and has information
 	// like "Starting module org.osbuild.selinux"
 	SetMessagef(fmt string, args ...interface{})
-	Start() error
-	Stop() error
+
+	// Start will start rendering the progress information
+	Start()
+
+	// Stop will stop rendering the progress information, the
+	// screen is not cleared, the last few lines will be visible
+	Stop()
 }
 
 // New creates a new progressbar based on the requested type
@@ -164,21 +173,19 @@ func (b *terminalProgressBar) renderLoop() {
 	}
 }
 
-func (b *terminalProgressBar) Start() error {
+func (b *terminalProgressBar) Start() {
 	// render() already running
 	if b.shutdownCh != nil {
-		return nil
+		return
 	}
 	fmt.Fprintf(b.out, "%s", CURSOR_HIDE)
 	b.shutdownCh = make(chan bool)
 	go b.renderLoop()
-
-	return nil
 }
 
-func (b *terminalProgressBar) Stop() (err error) {
+func (b *terminalProgressBar) Stop() {
 	if b.shutdownCh == nil {
-		return nil
+		return
 	}
 	// request shutdown
 	b.shutdownCh <- true
@@ -187,10 +194,12 @@ func (b *terminalProgressBar) Stop() (err error) {
 	case <-b.shutdownCh:
 	// shudown complete
 	case <-time.After(1 * time.Second):
+		// I cannot think of how this could happen, i.e. why
+		// closing would not work but lets be conservative -
+		// without a timeout we hang here forever
 		logrus.Warnf("no progress channel shutdown after 1sec")
 	}
 	b.shutdownCh = nil
-	return nil
 }
 
 type plainProgressBar struct {
@@ -212,12 +221,10 @@ func (b *plainProgressBar) SetMessagef(msg string, args ...interface{}) {
 	fmt.Fprintf(b.w, msg, args...)
 }
 
-func (b *plainProgressBar) Start() (err error) {
-	return nil
+func (b *plainProgressBar) Start() {
 }
 
-func (b *plainProgressBar) Stop() (err error) {
-	return nil
+func (b *plainProgressBar) Stop() {
 }
 
 func (b *plainProgressBar) SetProgress(subLevel int, msg string, done int, total int) error {
@@ -249,14 +256,12 @@ func (b *debugProgressBar) SetMessagef(msg string, args ...interface{}) {
 	fmt.Fprintf(b.w, "\n")
 }
 
-func (b *debugProgressBar) Start() (err error) {
+func (b *debugProgressBar) Start() {
 	fmt.Fprintf(b.w, "Start progressbar\n")
-	return nil
 }
 
-func (b *debugProgressBar) Stop() (err error) {
+func (b *debugProgressBar) Stop() {
 	fmt.Fprintf(b.w, "Stop progressbar\n")
-	return nil
 }
 
 func (b *debugProgressBar) SetProgress(subLevel int, msg string, done int, total int) error {
diff --git a/progress/bib/internal/progress/progress_test.go b/progress/bib/internal/progress/progress_test.go
index 364d96a958..b7efca1322 100644
--- a/progress/bib/internal/progress/progress_test.go
+++ b/progress/bib/internal/progress/progress_test.go
@@ -54,11 +54,9 @@ func TestPlainProgress(t *testing.T) {
 	assert.Equal(t, "message", buf.String())
 	buf.Reset()
 
-	err = pbar.Start()
-	assert.NoError(t, err)
+	pbar.Start()
 	assert.Equal(t, "", buf.String())
-	err = pbar.Stop()
-	assert.NoError(t, err)
+	pbar.Stop()
 	assert.Equal(t, "", buf.String())
 }
 
@@ -82,13 +80,11 @@ func TestDebugProgress(t *testing.T) {
 	assert.Equal(t, "msg: some-message\n", buf.String())
 	buf.Reset()
 
-	err = pbar.Start()
-	assert.NoError(t, err)
+	pbar.Start()
 	assert.Equal(t, "Start progressbar\n", buf.String())
 	buf.Reset()
 
-	err = pbar.Stop()
-	assert.NoError(t, err)
+	pbar.Stop()
 	assert.Equal(t, "Stop progressbar\n", buf.String())
 	buf.Reset()
 }
@@ -101,14 +97,12 @@ func TestTermProgress(t *testing.T) {
 	pbar, err := progress.NewTerminalProgressBar()
 	assert.NoError(t, err)
 
-	err = pbar.Start()
-	assert.NoError(t, err)
+	pbar.Start()
 	pbar.SetPulseMsgf("pulse-msg")
 	pbar.SetMessagef("some-message")
 	err = pbar.SetProgress(0, "set-progress-msg", 0, 5)
 	assert.NoError(t, err)
-	err = pbar.Stop()
-	assert.NoError(t, err)
+	pbar.Stop()
 
 	assert.Contains(t, buf.String(), "[1 / 6] set-progress-msg")
 	assert.Contains(t, buf.String(), "[|] pulse-msg\n")

From e60ccafa5ae2813d4cf3bb72119ab31edeeec401 Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Mon, 16 Dec 2024 10:59:38 +0100
Subject: [PATCH 05/11] progress: add extra error checks after pb.ProgressBar
 shutdown

This commit adds some extra checks to ensure that the pb.ProgressBar
does not contain any errors. This should not happen but we want
to be paranoid (especially since the error handling of pb.ProgressBar
is a bit indirect).
---
 progress/bib/internal/progress/progress.go    | 26 +++++++++++++++++++
 .../bib/internal/progress/progress_test.go    |  1 +
 2 files changed, 27 insertions(+)

diff --git a/progress/bib/internal/progress/progress.go b/progress/bib/internal/progress/progress.go
index 6493655194..a09eefc90b 100644
--- a/progress/bib/internal/progress/progress.go
+++ b/progress/bib/internal/progress/progress.go
@@ -2,6 +2,7 @@ package progress
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -113,6 +114,9 @@ func (b *terminalProgressBar) SetProgress(subLevel int, msg string, done int, to
 		b.subLevelPbs = append(b.subLevelPbs, apb)
 		progressBarTmpl := `[{{ counters . }}] {{ string . "prefix" }} {{ bar .}} {{ percent . }}`
 		apb.SetTemplateString(progressBarTmpl)
+		if err := apb.Err(); err != nil {
+			return fmt.Errorf("error setting the progressbarTemplat: %w", err)
+		}
 	case subLevel > len(b.subLevelPbs):
 		return fmt.Errorf("sublevel added out of order, have %v sublevels but want level %v", len(b.subLevelPbs), subLevel)
 	}
@@ -183,6 +187,22 @@ func (b *terminalProgressBar) Start() {
 	go b.renderLoop()
 }
 
+func (b *terminalProgressBar) Err() error {
+	var errs []error
+	if err := b.spinnerPb.Err(); err != nil {
+		errs = append(errs, fmt.Errorf("error on spinner progressbar: %w", err))
+	}
+	if err := b.msgPb.Err(); err != nil {
+		errs = append(errs, fmt.Errorf("error on spinner progressbar: %w", err))
+	}
+	for _, pb := range b.subLevelPbs {
+		if err := pb.Err(); err != nil {
+			errs = append(errs, fmt.Errorf("error on spinner progressbar: %w", err))
+		}
+	}
+	return errors.Join(errs...)
+}
+
 func (b *terminalProgressBar) Stop() {
 	if b.shutdownCh == nil {
 		return
@@ -200,6 +220,12 @@ func (b *terminalProgressBar) Stop() {
 		logrus.Warnf("no progress channel shutdown after 1sec")
 	}
 	b.shutdownCh = nil
+	// This should never happen but be paranoid, this should
+	// never happen but ensure we did not accumulate error while
+	// running
+	if err := b.Err(); err != nil {
+		fmt.Fprintf(b.out, "error from pb.ProgressBar: %v", err)
+	}
 }
 
 type plainProgressBar struct {
diff --git a/progress/bib/internal/progress/progress_test.go b/progress/bib/internal/progress/progress_test.go
index b7efca1322..6b11d6f63e 100644
--- a/progress/bib/internal/progress/progress_test.go
+++ b/progress/bib/internal/progress/progress_test.go
@@ -103,6 +103,7 @@ func TestTermProgress(t *testing.T) {
 	err = pbar.SetProgress(0, "set-progress-msg", 0, 5)
 	assert.NoError(t, err)
 	pbar.Stop()
+	assert.NoError(t, pbar.(*progress.TerminalProgressBar).Err())
 
 	assert.Contains(t, buf.String(), "[1 / 6] set-progress-msg")
 	assert.Contains(t, buf.String(), "[|] pulse-msg\n")

From d1d7b4fa81425d183c62cc4ce1dcfe57722486bb Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Mon, 16 Dec 2024 14:50:29 +0100
Subject: [PATCH 06/11] progress: set defaultWidth if no width can be found

This commit fixes a bug in the progress rendering when no terminal
width can be identified. In this case pb.ProgressBar will strip
bars with dynamic elements. This is undesirable so when no size
can be found we just use the pb.defaultBarWidth (which is not
exported sadly). This should get fixed upstream ideally.
---
 progress/bib/internal/progress/progress.go | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/progress/bib/internal/progress/progress.go b/progress/bib/internal/progress/progress.go
index a09eefc90b..3e7bf4cea4 100644
--- a/progress/bib/internal/progress/progress.go
+++ b/progress/bib/internal/progress/progress.go
@@ -111,12 +111,17 @@ func (b *terminalProgressBar) SetProgress(subLevel int, msg string, done int, to
 	switch {
 	case subLevel == len(b.subLevelPbs):
 		apb := pb.New(0)
-		b.subLevelPbs = append(b.subLevelPbs, apb)
 		progressBarTmpl := `[{{ counters . }}] {{ string . "prefix" }} {{ bar .}} {{ percent . }}`
 		apb.SetTemplateString(progressBarTmpl)
 		if err := apb.Err(); err != nil {
 			return fmt.Errorf("error setting the progressbarTemplat: %w", err)
 		}
+		// workaround bug when running tests in tmt
+		if apb.Width() == 0 {
+			// this is pb.defaultBarWidth
+			apb.SetWidth(100)
+		}
+		b.subLevelPbs = append(b.subLevelPbs, apb)
 	case subLevel > len(b.subLevelPbs):
 		return fmt.Errorf("sublevel added out of order, have %v sublevels but want level %v", len(b.subLevelPbs), subLevel)
 	}

From 57b7b478332238aa6ccd599aeb21b046cf8c3077 Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Thu, 19 Dec 2024 13:06:04 +0100
Subject: [PATCH 07/11] progress: fix missing "\n" in plain progress

This commit fixes the missing \n when plain progress is used. Without
the output looks rather silly :/
---
 progress/bib/internal/progress/progress.go      | 2 ++
 progress/bib/internal/progress/progress_test.go | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/progress/bib/internal/progress/progress.go b/progress/bib/internal/progress/progress.go
index 3e7bf4cea4..7e6eefa930 100644
--- a/progress/bib/internal/progress/progress.go
+++ b/progress/bib/internal/progress/progress.go
@@ -246,10 +246,12 @@ func NewPlainProgressBar() (ProgressBar, error) {
 
 func (b *plainProgressBar) SetPulseMsgf(msg string, args ...interface{}) {
 	fmt.Fprintf(b.w, msg, args...)
+	fmt.Fprintf(b.w, "\n")
 }
 
 func (b *plainProgressBar) SetMessagef(msg string, args ...interface{}) {
 	fmt.Fprintf(b.w, msg, args...)
+	fmt.Fprintf(b.w, "\n")
 }
 
 func (b *plainProgressBar) Start() {
diff --git a/progress/bib/internal/progress/progress_test.go b/progress/bib/internal/progress/progress_test.go
index 6b11d6f63e..211fc2215c 100644
--- a/progress/bib/internal/progress/progress_test.go
+++ b/progress/bib/internal/progress/progress_test.go
@@ -47,11 +47,11 @@ func TestPlainProgress(t *testing.T) {
 
 	// but it shows the messages
 	pbar.SetPulseMsgf("pulse")
-	assert.Equal(t, "pulse", buf.String())
+	assert.Equal(t, "pulse\n", buf.String())
 	buf.Reset()
 
 	pbar.SetMessagef("message")
-	assert.Equal(t, "message", buf.String())
+	assert.Equal(t, "message\n", buf.String())
 	buf.Reset()
 
 	pbar.Start()

From 63c7f91abb06f8dbc85658a4d40e02eb38c754a1 Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Thu, 12 Dec 2024 19:45:44 +0100
Subject: [PATCH 08/11] progress: auto-select progress based on if we run on a
 terminal

This commit adds automatic progress bar selection based on checking
if we run on a terminal or not. When running on a terminal we use
the nice "terminalProgressBar". If that is not set we assuem
we run in a script or CI/CD environment and select plainProgressBar.

Thanks Colin for the hint about the bad integration test.
---
 progress/bib/internal/progress/export_test.go |  8 ++++++++
 progress/bib/internal/progress/progress.go    | 14 +++++++++++---
 .../bib/internal/progress/progress_test.go    | 19 +++++++++++++++++++
 3 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/progress/bib/internal/progress/export_test.go b/progress/bib/internal/progress/export_test.go
index c831824264..c6d8e17e8e 100644
--- a/progress/bib/internal/progress/export_test.go
+++ b/progress/bib/internal/progress/export_test.go
@@ -17,3 +17,11 @@ func MockOsStderr(w io.Writer) (restore func()) {
 		osStderr = saved
 	}
 }
+
+func MockIsattyIsTerminal(fn func(uintptr) bool) (restore func()) {
+	saved := isattyIsTerminal
+	isattyIsTerminal = fn
+	return func() {
+		isattyIsTerminal = saved
+	}
+}
diff --git a/progress/bib/internal/progress/progress.go b/progress/bib/internal/progress/progress.go
index 7e6eefa930..66313078d1 100644
--- a/progress/bib/internal/progress/progress.go
+++ b/progress/bib/internal/progress/progress.go
@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"github.com/cheggaaa/pb/v3"
+	"github.com/mattn/go-isatty"
 	"github.com/sirupsen/logrus"
 
 	"github.com/osbuild/images/pkg/osbuild"
@@ -66,12 +67,19 @@ type ProgressBar interface {
 	Stop()
 }
 
+var isattyIsTerminal = isatty.IsTerminal
+
 // New creates a new progressbar based on the requested type
 func New(typ string) (ProgressBar, error) {
 	switch typ {
-	// XXX: autoseelct based on PS1 value (i.e. use term in
-	// interactive shells only?)
-	case "", "plain":
+	case "":
+		// autoselect based on if we are on an interactive
+		// terminal, use plain progress for scripts
+		if isattyIsTerminal(os.Stdin.Fd()) {
+			return NewTerminalProgressBar()
+		}
+		return NewPlainProgressBar()
+	case "plain":
 		return NewPlainProgressBar()
 	case "term":
 		return NewTerminalProgressBar()
diff --git a/progress/bib/internal/progress/progress_test.go b/progress/bib/internal/progress/progress_test.go
index 211fc2215c..326513f0be 100644
--- a/progress/bib/internal/progress/progress_test.go
+++ b/progress/bib/internal/progress/progress_test.go
@@ -111,3 +111,22 @@ func TestTermProgress(t *testing.T) {
 	// check shutdown
 	assert.Contains(t, buf.String(), progress.CURSOR_SHOW)
 }
+
+func TestProgressNewAutoselect(t *testing.T) {
+	for _, tc := range []struct {
+		onTerm   bool
+		expected interface{}
+	}{
+		{false, &progress.PlainProgressBar{}},
+		{true, &progress.TerminalProgressBar{}},
+	} {
+		restore := progress.MockIsattyIsTerminal(func(uintptr) bool {
+			return tc.onTerm
+		})
+		defer restore()
+
+		pb, err := progress.New("")
+		assert.NoError(t, err)
+		assert.Equal(t, reflect.TypeOf(pb), reflect.TypeOf(tc.expected), fmt.Sprintf("[%v] %T not the expected %T", tc.onTerm, pb, tc.expected))
+	}
+}

From 114299f8c23d2150615282706c9741001cab662d Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Mon, 6 Jan 2025 13:15:59 +0100
Subject: [PATCH 09/11] bib: make `--progress=auto` the default

Previously an empty string in `--progress` meant to "auto-select".
But that is not super intuitive so this commit makes it explicit
using "auto".

Thanks to Achilleas for the suggestion.
---
 progress/bib/internal/progress/progress.go      | 2 +-
 progress/bib/internal/progress/progress_test.go | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/progress/bib/internal/progress/progress.go b/progress/bib/internal/progress/progress.go
index 66313078d1..98fb2acbd2 100644
--- a/progress/bib/internal/progress/progress.go
+++ b/progress/bib/internal/progress/progress.go
@@ -72,7 +72,7 @@ var isattyIsTerminal = isatty.IsTerminal
 // New creates a new progressbar based on the requested type
 func New(typ string) (ProgressBar, error) {
 	switch typ {
-	case "":
+	case "", "auto":
 		// autoselect based on if we are on an interactive
 		// terminal, use plain progress for scripts
 		if isattyIsTerminal(os.Stdin.Fd()) {
diff --git a/progress/bib/internal/progress/progress_test.go b/progress/bib/internal/progress/progress_test.go
index 326513f0be..4023565fb9 100644
--- a/progress/bib/internal/progress/progress_test.go
+++ b/progress/bib/internal/progress/progress_test.go
@@ -125,7 +125,7 @@ func TestProgressNewAutoselect(t *testing.T) {
 		})
 		defer restore()
 
-		pb, err := progress.New("")
+		pb, err := progress.New("auto")
 		assert.NoError(t, err)
 		assert.Equal(t, reflect.TypeOf(pb), reflect.TypeOf(tc.expected), fmt.Sprintf("[%v] %T not the expected %T", tc.onTerm, pb, tc.expected))
 	}

From 4c3a26a0ce4c112c9729088d2fd79e6034efb4c0 Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Mon, 6 Jan 2025 13:22:42 +0100
Subject: [PATCH 10/11] bib: rename "plain" progress to "verbose" progress

This commit renames the "plain" progress to "verbose" progress. The
new name is closer to what the intention of the this progress is
and easier for the users to understand.

Thanks to Ondrej for the suggestion.
---
 progress/bib/internal/progress/export_test.go |  2 +-
 progress/bib/internal/progress/progress.go    | 26 +++++++++----------
 .../bib/internal/progress/progress_test.go    | 10 +++----
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/progress/bib/internal/progress/export_test.go b/progress/bib/internal/progress/export_test.go
index c6d8e17e8e..252d5329f9 100644
--- a/progress/bib/internal/progress/export_test.go
+++ b/progress/bib/internal/progress/export_test.go
@@ -7,7 +7,7 @@ import (
 type (
 	TerminalProgressBar = terminalProgressBar
 	DebugProgressBar    = debugProgressBar
-	PlainProgressBar    = plainProgressBar
+	VerboseProgressBar  = verboseProgressBar
 )
 
 func MockOsStderr(w io.Writer) (restore func()) {
diff --git a/progress/bib/internal/progress/progress.go b/progress/bib/internal/progress/progress.go
index 98fb2acbd2..2112b39765 100644
--- a/progress/bib/internal/progress/progress.go
+++ b/progress/bib/internal/progress/progress.go
@@ -74,13 +74,13 @@ func New(typ string) (ProgressBar, error) {
 	switch typ {
 	case "", "auto":
 		// autoselect based on if we are on an interactive
-		// terminal, use plain progress for scripts
+		// terminal, use verbose progress for scripts
 		if isattyIsTerminal(os.Stdin.Fd()) {
 			return NewTerminalProgressBar()
 		}
-		return NewPlainProgressBar()
-	case "plain":
-		return NewPlainProgressBar()
+		return NewVerboseProgressBar()
+	case "verbose":
+		return NewVerboseProgressBar()
 	case "term":
 		return NewTerminalProgressBar()
 	case "debug":
@@ -241,34 +241,34 @@ func (b *terminalProgressBar) Stop() {
 	}
 }
 
-type plainProgressBar struct {
+type verboseProgressBar struct {
 	w io.Writer
 }
 
-// NewPlainProgressBar starts a new "plain" progressbar that will just
+// NewVerboseProgressBar starts a new "verbose" progressbar that will just
 // prints message but does not show any progress.
-func NewPlainProgressBar() (ProgressBar, error) {
-	b := &plainProgressBar{w: osStderr}
+func NewVerboseProgressBar() (ProgressBar, error) {
+	b := &verboseProgressBar{w: osStderr}
 	return b, nil
 }
 
-func (b *plainProgressBar) SetPulseMsgf(msg string, args ...interface{}) {
+func (b *verboseProgressBar) SetPulseMsgf(msg string, args ...interface{}) {
 	fmt.Fprintf(b.w, msg, args...)
 	fmt.Fprintf(b.w, "\n")
 }
 
-func (b *plainProgressBar) SetMessagef(msg string, args ...interface{}) {
+func (b *verboseProgressBar) SetMessagef(msg string, args ...interface{}) {
 	fmt.Fprintf(b.w, msg, args...)
 	fmt.Fprintf(b.w, "\n")
 }
 
-func (b *plainProgressBar) Start() {
+func (b *verboseProgressBar) Start() {
 }
 
-func (b *plainProgressBar) Stop() {
+func (b *verboseProgressBar) Stop() {
 }
 
-func (b *plainProgressBar) SetProgress(subLevel int, msg string, done int, total int) error {
+func (b *verboseProgressBar) SetProgress(subLevel int, msg string, done int, total int) error {
 	return nil
 }
 
diff --git a/progress/bib/internal/progress/progress_test.go b/progress/bib/internal/progress/progress_test.go
index 4023565fb9..97f048cecc 100644
--- a/progress/bib/internal/progress/progress_test.go
+++ b/progress/bib/internal/progress/progress_test.go
@@ -19,7 +19,7 @@ func TestProgressNew(t *testing.T) {
 	}{
 		{"term", &progress.TerminalProgressBar{}, ""},
 		{"debug", &progress.DebugProgressBar{}, ""},
-		{"plain", &progress.PlainProgressBar{}, ""},
+		{"verbose", &progress.VerboseProgressBar{}, ""},
 		// unknown progress type
 		{"bad", nil, `unknown progress type: "bad"`},
 	} {
@@ -33,13 +33,13 @@ func TestProgressNew(t *testing.T) {
 	}
 }
 
-func TestPlainProgress(t *testing.T) {
+func TestVerboseProgress(t *testing.T) {
 	var buf bytes.Buffer
 	restore := progress.MockOsStderr(&buf)
 	defer restore()
 
-	// plain progress never generates progress output
-	pbar, err := progress.NewPlainProgressBar()
+	// verbose progress never generates progress output
+	pbar, err := progress.NewVerboseProgressBar()
 	assert.NoError(t, err)
 	err = pbar.SetProgress(0, "set-progress", 1, 100)
 	assert.NoError(t, err)
@@ -117,7 +117,7 @@ func TestProgressNewAutoselect(t *testing.T) {
 		onTerm   bool
 		expected interface{}
 	}{
-		{false, &progress.PlainProgressBar{}},
+		{false, &progress.VerboseProgressBar{}},
 		{true, &progress.TerminalProgressBar{}},
 	} {
 		restore := progress.MockIsattyIsTerminal(func(uintptr) bool {

From ed3c4de7adcda3d20388eb3914cb5b12ef33c762 Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Fri, 17 Jan 2025 13:18:46 +0100
Subject: [PATCH 11/11] progress: move into the final place in images and fix
 imports

This commit finishes moving the bootc-image-builder "progress"
package into "images". This allows code-sharing between bib
and ibcli.

Note that further work will happen to unify `osbuild.RunOsbuild`
and `progress.RunOSBuild()`.
---
 {progress/bib/internal => pkg/osbuild}/progress/export_test.go  | 0
 {progress/bib/internal => pkg/osbuild}/progress/progress.go     | 0
 .../bib/internal => pkg/osbuild}/progress/progress_test.go      | 2 +-
 3 files changed, 1 insertion(+), 1 deletion(-)
 rename {progress/bib/internal => pkg/osbuild}/progress/export_test.go (100%)
 rename {progress/bib/internal => pkg/osbuild}/progress/progress.go (100%)
 rename {progress/bib/internal => pkg/osbuild}/progress/progress_test.go (98%)

diff --git a/progress/bib/internal/progress/export_test.go b/pkg/osbuild/progress/export_test.go
similarity index 100%
rename from progress/bib/internal/progress/export_test.go
rename to pkg/osbuild/progress/export_test.go
diff --git a/progress/bib/internal/progress/progress.go b/pkg/osbuild/progress/progress.go
similarity index 100%
rename from progress/bib/internal/progress/progress.go
rename to pkg/osbuild/progress/progress.go
diff --git a/progress/bib/internal/progress/progress_test.go b/pkg/osbuild/progress/progress_test.go
similarity index 98%
rename from progress/bib/internal/progress/progress_test.go
rename to pkg/osbuild/progress/progress_test.go
index 97f048cecc..f7b13afc50 100644
--- a/progress/bib/internal/progress/progress_test.go
+++ b/pkg/osbuild/progress/progress_test.go
@@ -8,7 +8,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 
-	"github.com/osbuild/bootc-image-builder/bib/internal/progress"
+	"github.com/osbuild/images/pkg/osbuild/progress"
 )
 
 func TestProgressNew(t *testing.T) {