progress: immplement 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.

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

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,29 +77,22 @@ 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)
+	ppb := &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
+	ppb.spinnerPb = pb.New(0)
+	ppb.spinnerPb.SetTemplate(`[{{ (cycle . "|" "/" "-" "\\") }}] {{ string . "spinnerMsg" }}`)
+	ppb.msgPb = pb.New(0)
+	ppb.msgPb.SetTemplate(`Message: {{ string . "msg" }}`)
+	return ppb, nil
 }
 
 func (b *terminalProgressBar) SetProgress(subLevel int, msg string, done int, total int) error {
@@ -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,67 @@ 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.Fprintf(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()
+			close(b.shutdownCh)
+
+			// finally move cursor down again
+			fmt.Fprintf(b.out, "%s", CURSOR_SHOW)
+			fmt.Fprintf(b.out, strings.Repeat("\n", 2+len(b.subLevelPbs)))
+
+			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 {

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

test/test_progress.py

@@ -1,12 +1,15 @@
 import subprocess
 
-# pylint: disable=unused-import
+# pylint: disable=unused-import,duplicate-code
 from test_opts import container_storage_fixture
 from containerbuild import build_container_fixture, build_fake_container_fixture
 
 
-def bib_cmd(container_storage, output_path, build_fake_container):
-    return [
+def test_progress_debug(tmp_path, container_storage, build_fake_container):
+    output_path = tmp_path / "output"
+    output_path.mkdir(exist_ok=True)
+
+    cmdline = [
         "podman", "run", "--rm",
         "--privileged",
         "--security-opt", "label=type:unconfined_t",
@@ -16,13 +19,6 @@ def bib_cmd(container_storage, output_path, build_fake_container):
         "build",
         "quay.io/centos-bootc/centos-bootc:stream9",
     ]
-
-
-def test_progress_debug(tmp_path, container_storage, build_fake_container):
-    output_path = tmp_path / "output"
-    output_path.mkdir(exist_ok=True)
-
-    cmdline = bib_cmd(container_storage, output_path, build_fake_container)
     cmdline.append("--progress=debug")
     res = subprocess.run(cmdline, capture_output=True, check=True, text=True)
     assert res.stderr.count("Start progressbar") == 1
@@ -31,3 +27,24 @@ def test_progress_debug(tmp_path, container_storage, build_fake_container):
     assert res.stderr.count("Build complete") == 1
     assert res.stderr.count("Stop progressbar") == 1
     assert res.stdout.strip() == ""
+
+
+def test_progress_term(tmp_path, container_storage, build_fake_container):
+    output_path = tmp_path / "output"
+    output_path.mkdir(exist_ok=True)
+
+    cmdline = [
+        "podman", "run", "--rm",
+        "--privileged",
+        "--security-opt", "label=type:unconfined_t",
+        "-v", f"{container_storage}:/var/lib/containers/storage",
+        "-v", f"{output_path}:/output",
+        build_fake_container,
+        "build",
+        # explicitly select term progress
+        "--progress=term",
+        "quay.io/centos-bootc/centos-bootc:stream9",
+    ]
+    res = subprocess.run(cmdline, capture_output=True, text=True, check=False)
+    assert res.returncode == 0
+    assert "[|] Manifest generation step" in res.stderr