From 238b22a3de94c51e71af44d40da6400108ed7098 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 4 Dec 2024 12:53:01 +0100 Subject: [PATCH 1/8] 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. --- bib/go.mod | 2 +- bib/go.sum | 2 + bib/internal/progress/export_test.go | 29 +++ bib/internal/progress/progress.go | 309 +++++++++++++++++++++++++ bib/internal/progress/progress_test.go | 107 +++++++++ 5 files changed, 448 insertions(+), 1 deletion(-) create mode 100644 bib/internal/progress/export_test.go create mode 100644 bib/internal/progress/progress.go create mode 100644 bib/internal/progress/progress_test.go diff --git a/bib/go.mod b/bib/go.mod index be383887..44e03adc 100644 --- a/bib/go.mod +++ b/bib/go.mod @@ -75,7 +75,7 @@ require ( 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.19 // 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/bib/go.sum b/bib/go.sum index 27b763a9..ccb8131f 100644 --- a/bib/go.sum +++ b/bib/go.sum @@ -190,6 +190,8 @@ github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovk github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM= github.com/mattn/go-isatty v0.0.19 h1:JITubQf0MOLdlGRuRq+jtsDlekdYPia9ZFsB8h/APPA= github.com/mattn/go-isatty v0.0.19/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= +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= github.com/mattn/go-runewidth v0.0.16/go.mod h1:Jdepj2loyihRzMpdS35Xk/zdY8IAYHsh153qUoGf23w= github.com/mattn/go-sqlite3 v1.14.22 h1:2gZY6PC6kBnID23Tichd1K+Z0oS6nE/XwU+Vz/5o4kU= diff --git a/bib/internal/progress/export_test.go b/bib/internal/progress/export_test.go new file mode 100644 index 00000000..795b0246 --- /dev/null +++ b/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/bib/internal/progress/progress.go b/bib/internal/progress/progress.go new file mode 100644 index 00000000..88149aa9 --- /dev/null +++ b/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/bib/internal/progress/progress_test.go b/bib/internal/progress/progress_test.go new file mode 100644 index 00000000..ebff346a --- /dev/null +++ b/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 9c5f308e2beb14aeeb8d52da725dcdba885fc900 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 5 Dec 2024 11:03:30 +0100 Subject: [PATCH 2/8] main,osbuildprogress: add `--progress=term,plain,debug` support This adds a new `progress` flag that makes use of the osbuild jsonseq progress information to show progress and hide the low-level details from the user. --- bib/cmd/bootc-image-builder/main.go | 50 ++++++++++++++++++++++------- test/containerbuild.py | 5 ++- test/test_progress.py | 33 +++++++++++++++++++ 3 files changed, 73 insertions(+), 15 deletions(-) create mode 100644 test/test_progress.py diff --git a/bib/cmd/bootc-image-builder/main.go b/bib/cmd/bootc-image-builder/main.go index a49afed3..80e39dc2 100644 --- a/bib/cmd/bootc-image-builder/main.go +++ b/bib/cmd/bootc-image-builder/main.go @@ -21,12 +21,12 @@ import ( "github.com/osbuild/images/pkg/container" "github.com/osbuild/images/pkg/dnfjson" "github.com/osbuild/images/pkg/manifest" - "github.com/osbuild/images/pkg/osbuild" "github.com/osbuild/images/pkg/rpmmd" "github.com/osbuild/bootc-image-builder/bib/internal/buildconfig" podman_container "github.com/osbuild/bootc-image-builder/bib/internal/container" "github.com/osbuild/bootc-image-builder/bib/internal/imagetypes" + "github.com/osbuild/bootc-image-builder/bib/internal/progress" "github.com/osbuild/bootc-image-builder/bib/internal/setup" "github.com/osbuild/bootc-image-builder/bib/internal/source" "github.com/osbuild/bootc-image-builder/bib/internal/util" @@ -171,7 +171,16 @@ func saveManifest(ms manifest.OSBuildManifest, fpath string) error { return nil } -func manifestFromCobra(cmd *cobra.Command, args []string) ([]byte, *mTLSConfig, error) { +func manifestFromCobra(cmd *cobra.Command, args []string, pbar progress.ProgressBar) ([]byte, *mTLSConfig, error) { + if pbar == nil { + var err error + pbar, err = progress.New("plain") + // this should never happen + if err != nil { + return nil, nil, err + } + } + cntArch := arch.Current() imgref := args[0] @@ -234,6 +243,11 @@ func manifestFromCobra(cmd *cobra.Command, args []string) ([]byte, *mTLSConfig, logrus.Debug("Using local container") } + pbar.SetPulseMsgf("Manifest generation step") + if err := pbar.Start(); err != nil { + return nil, nil, fmt.Errorf("cannot start progress: %v", err) + } + if err := setup.ValidateHasContainerTags(imgref); err != nil { return nil, nil, err } @@ -320,7 +334,7 @@ func manifestFromCobra(cmd *cobra.Command, args []string) ([]byte, *mTLSConfig, } func cmdManifest(cmd *cobra.Command, args []string) error { - mf, _, err := manifestFromCobra(cmd, args) + mf, _, err := manifestFromCobra(cmd, args, nil) if err != nil { return fmt.Errorf("cannot generate manifest: %w", err) } @@ -383,6 +397,7 @@ func cmdBuild(cmd *cobra.Command, args []string) error { osbuildStore, _ := cmd.Flags().GetString("store") outputDir, _ := cmd.Flags().GetString("output") targetArch, _ := cmd.Flags().GetString("target-arch") + progressType, _ := cmd.Flags().GetString("progress") logrus.Debug("Validating environment") if err := setup.Validate(targetArch); err != nil { @@ -415,13 +430,23 @@ func cmdBuild(cmd *cobra.Command, args []string) error { return fmt.Errorf("chowning is not allowed in output directory") } + pbar, err := progress.New(progressType) + if err != nil { + return fmt.Errorf("cannto create progress bar: %w", err) + } + defer func() { + if err := pbar.Stop(); err != nil { + logrus.Warnf("progressbar stopping failed: %v", err) + } + }() + manifest_fname := fmt.Sprintf("manifest-%s.json", strings.Join(imgTypes, "-")) - fmt.Printf("Generating manifest %s\n", manifest_fname) - mf, mTLS, err := manifestFromCobra(cmd, args) + pbar.SetMessagef("Generating manifest %s", manifest_fname) + mf, mTLS, err := manifestFromCobra(cmd, args, pbar) if err != nil { return fmt.Errorf("cannot build manifest: %w", err) } - fmt.Print("DONE\n") + pbar.SetMessagef("Done generating manifest") // collect pipeline exports for each image type imageTypes, err := imagetypes.New(imgTypes...) @@ -434,7 +459,8 @@ func cmdBuild(cmd *cobra.Command, args []string) error { return fmt.Errorf("cannot save manifest: %w", err) } - fmt.Printf("Building %s\n", manifest_fname) + pbar.SetPulseMsgf("Image building step") + pbar.SetMessagef("Building %s", manifest_fname) var osbuildEnv []string if !canChown { @@ -453,12 +479,11 @@ func cmdBuild(cmd *cobra.Command, args []string) error { osbuildEnv = append(osbuildEnv, envVars...) } - _, err = osbuild.RunOSBuild(mf, osbuildStore, outputDir, exports, nil, osbuildEnv, false, os.Stderr) - if err != nil { + if err = progress.RunOSBuild(pbar, mf, osbuildStore, outputDir, exports, osbuildEnv); err != nil { return fmt.Errorf("cannot run osbuild: %w", err) } - fmt.Println("Build complete!") + pbar.SetMessagef("Build complete!") if upload { for idx, imgType := range imgTypes { switch imgType { @@ -472,7 +497,7 @@ func cmdBuild(cmd *cobra.Command, args []string) error { } } } else { - fmt.Printf("Results saved in\n%s\n", outputDir) + pbar.SetMessagef("Results saved in %s", outputDir) } if err := chownR(outputDir, chown); err != nil { @@ -607,8 +632,9 @@ func buildCobraCmdline() (*cobra.Command, error) { buildCmd.Flags().String("aws-region", "", "target region for AWS uploads (only for type=ami)") buildCmd.Flags().String("chown", "", "chown the ouput directory to match the specified UID:GID") buildCmd.Flags().String("output", ".", "artifact output directory") - buildCmd.Flags().String("progress", "text", "type of progress bar to use") buildCmd.Flags().String("store", "/store", "osbuild store for intermediate pipeline trees") + //TODO: add json progress for higher level tools like "podman bootc" + buildCmd.Flags().String("progress", "", "type of progress bar to use") // flag rules for _, dname := range []string{"output", "store", "rpmmd"} { if err := buildCmd.MarkFlagDirname(dname); err != nil { diff --git a/test/containerbuild.py b/test/containerbuild.py index d751ccfb..44154c75 100644 --- a/test/containerbuild.py +++ b/test/containerbuild.py @@ -68,16 +68,15 @@ def build_fake_container_fixture(tmpdir_factory, build_container): fake_osbuild_path = tmp_path / "fake-osbuild" fake_osbuild_path.write_text(textwrap.dedent("""\ - #!/bin/sh -e + #!/bin/bash -e # injest generated manifest from the images library, if we do not # do this images may fail with "broken" pipe errors - cat - + cat - >/dev/null mkdir -p /output/qcow2 echo "fake-disk.qcow2" > /output/qcow2/disk.qcow2 - echo "Done" """), encoding="utf8") cntf_path = tmp_path / "Containerfile" diff --git a/test/test_progress.py b/test/test_progress.py new file mode 100644 index 00000000..2c76ff2a --- /dev/null +++ b/test/test_progress.py @@ -0,0 +1,33 @@ +import subprocess + +# pylint: disable=unused-import +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 [ + "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", + "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 + assert res.stderr.count("Manifest generation step") == 1 + assert res.stderr.count("Image building step") == 1 + assert res.stderr.count("Build complete") == 1 + assert res.stderr.count("Stop progressbar") == 1 + assert res.stdout.strip() == "" From 5eccb3abe583b8b1cc52318efba29b69362cabfd Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 11 Dec 2024 09:57:49 +0100 Subject: [PATCH 3/8] 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. --- bib/internal/progress/export_test.go | 10 --- bib/internal/progress/progress.go | 103 ++++++++++++++++++------- bib/internal/progress/progress_test.go | 27 +++++-- test/test_progress.py | 37 ++++++--- 4 files changed, 122 insertions(+), 55 deletions(-) diff --git a/bib/internal/progress/export_test.go b/bib/internal/progress/export_test.go index 795b0246..c8318242 100644 --- a/bib/internal/progress/export_test.go +++ b/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/bib/internal/progress/progress.go b/bib/internal/progress/progress.go index 88149aa9..2c32318d 100644 --- a/bib/internal/progress/progress.go +++ b/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/bib/internal/progress/progress_test.go b/bib/internal/progress/progress_test.go index ebff346a..364d96a9 100644 --- a/bib/internal/progress/progress_test.go +++ b/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) } diff --git a/test/test_progress.py b/test/test_progress.py index 2c76ff2a..b531eb92 100644 --- a/test/test_progress.py +++ b/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 From 87905edc361fe6ab752b39cb72b7af377fd5d631 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 12 Dec 2024 17:59:39 +0100 Subject: [PATCH 4/8] 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. --- bib/cmd/bootc-image-builder/main.go | 10 ++----- bib/internal/progress/progress.go | 39 +++++++++++++++----------- bib/internal/progress/progress_test.go | 18 ++++-------- 3 files changed, 30 insertions(+), 37 deletions(-) diff --git a/bib/cmd/bootc-image-builder/main.go b/bib/cmd/bootc-image-builder/main.go index 80e39dc2..e466a004 100644 --- a/bib/cmd/bootc-image-builder/main.go +++ b/bib/cmd/bootc-image-builder/main.go @@ -244,9 +244,7 @@ func manifestFromCobra(cmd *cobra.Command, args []string, pbar progress.Progress } pbar.SetPulseMsgf("Manifest generation step") - if err := pbar.Start(); err != nil { - return nil, nil, fmt.Errorf("cannot start progress: %v", err) - } + pbar.Start() if err := setup.ValidateHasContainerTags(imgref); err != nil { return nil, nil, err @@ -434,11 +432,7 @@ func cmdBuild(cmd *cobra.Command, args []string) error { if err != nil { return fmt.Errorf("cannto create progress bar: %w", err) } - defer func() { - if err := pbar.Stop(); err != nil { - logrus.Warnf("progressbar stopping failed: %v", err) - } - }() + defer pbar.Stop() manifest_fname := fmt.Sprintf("manifest-%s.json", strings.Join(imgTypes, "-")) pbar.SetMessagef("Generating manifest %s", manifest_fname) diff --git a/bib/internal/progress/progress.go b/bib/internal/progress/progress.go index 2c32318d..64936551 100644 --- a/bib/internal/progress/progress.go +++ b/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/bib/internal/progress/progress_test.go b/bib/internal/progress/progress_test.go index 364d96a9..b7efca13 100644 --- a/bib/internal/progress/progress_test.go +++ b/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 a2ba0933e37872b2538700037e5acd55c9aa9e94 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Fri, 13 Dec 2024 16:50:58 +0100 Subject: [PATCH 5/8] cmd: tweak/clarify the lifecycle of the progressbar manifestFromCobra generate an osbuild manifest from a cobra commandline and it also takes a progress.ProgressBar as input. It needs an unstarted progres bar and will start it at the right point (it cannot be started yet to avoid the "podman pull" progress and our progress fighting). The caller is responsible for stopping the progress bar (this functtion cannot know what else needs to happen after manifest generation). TODO: provide a podman progress reader to integrate the podman progress into our progress. Thanks to Ondrej for the suggestion to clarify the relationship further. --- bib/cmd/bootc-image-builder/main.go | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/bib/cmd/bootc-image-builder/main.go b/bib/cmd/bootc-image-builder/main.go index e466a004..693d2382 100644 --- a/bib/cmd/bootc-image-builder/main.go +++ b/bib/cmd/bootc-image-builder/main.go @@ -171,16 +171,17 @@ func saveManifest(ms manifest.OSBuildManifest, fpath string) error { return nil } +// manifestFromCobra generate an osbuild manifest from a cobra commandline. +// +// It takes an unstarted progres bar and will start it at the right +// point (it cannot be started yet to avoid the "podman pull" progress +// and our progress fighting). The caller is responsible for stopping +// the progress bar (this function cannot know what else needs to happen +// after manifest generation). +// +// TODO: provide a podman progress reader to integrate the podman progress +// into our progress. func manifestFromCobra(cmd *cobra.Command, args []string, pbar progress.ProgressBar) ([]byte, *mTLSConfig, error) { - if pbar == nil { - var err error - pbar, err = progress.New("plain") - // this should never happen - if err != nil { - return nil, nil, err - } - } - cntArch := arch.Current() imgref := args[0] @@ -332,7 +333,14 @@ func manifestFromCobra(cmd *cobra.Command, args []string, pbar progress.Progress } func cmdManifest(cmd *cobra.Command, args []string) error { - mf, _, err := manifestFromCobra(cmd, args, nil) + pbar, err := progress.New("") + if err != nil { + // this should never happen + return fmt.Errorf("cannot create progress bar: %w", err) + } + defer pbar.Stop() + + mf, _, err := manifestFromCobra(cmd, args, pbar) if err != nil { return fmt.Errorf("cannot generate manifest: %w", err) } From 0657c54849bdd0fa4ce37dfcfd50c44514ef73cf Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 16 Dec 2024 10:59:38 +0100 Subject: [PATCH 6/8] 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). --- bib/internal/progress/progress.go | 26 ++++++++++++++++++++++++++ bib/internal/progress/progress_test.go | 1 + 2 files changed, 27 insertions(+) diff --git a/bib/internal/progress/progress.go b/bib/internal/progress/progress.go index 64936551..a09eefc9 100644 --- a/bib/internal/progress/progress.go +++ b/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/bib/internal/progress/progress_test.go b/bib/internal/progress/progress_test.go index b7efca13..6b11d6f6 100644 --- a/bib/internal/progress/progress_test.go +++ b/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 af4e67006bf497b9b561399adfae3f9517ef62b1 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 16 Dec 2024 14:50:29 +0100 Subject: [PATCH 7/8] 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. --- bib/internal/progress/progress.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/bib/internal/progress/progress.go b/bib/internal/progress/progress.go index a09eefc9..3e7bf4ce 100644 --- a/bib/internal/progress/progress.go +++ b/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 b93521a3fb22c9a77a574d0693540fd872feaccd Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 18 Dec 2024 13:41:55 +0100 Subject: [PATCH 8/8] cloud: temporary stop progress before uploading The upload code is currently using a plain "pb.ProgressBar" and this is tested as part of our integration test. This will collide with our own progress implementation. For now workaround this by stopping our own progress and let plain "pb" takeover. I was tempted to fix it right away but the PR with the progress is already relatively big so I opted for a followup. --- bib/cmd/bootc-image-builder/cloud.go | 2 +- bib/cmd/bootc-image-builder/main.go | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/bib/cmd/bootc-image-builder/cloud.go b/bib/cmd/bootc-image-builder/cloud.go index 8b94b385..98de143b 100644 --- a/bib/cmd/bootc-image-builder/cloud.go +++ b/bib/cmd/bootc-image-builder/cloud.go @@ -34,7 +34,7 @@ func uploadAMI(path, targetArch string, flags *pflag.FlagSet) error { // similar. Eventually we may provide json progress here too. var pbar *pb.ProgressBar switch progress { - case "text": + case "", "plain", "term": pbar = pb.New(0) } diff --git a/bib/cmd/bootc-image-builder/main.go b/bib/cmd/bootc-image-builder/main.go index 693d2382..9a5be5ea 100644 --- a/bib/cmd/bootc-image-builder/main.go +++ b/bib/cmd/bootc-image-builder/main.go @@ -487,6 +487,11 @@ func cmdBuild(cmd *cobra.Command, args []string) error { pbar.SetMessagef("Build complete!") if upload { + // XXX: pass our own progress.ProgressBar here + // *for now* just stop our own progress and let the uploadAMI + // progress take over - but we really need to fix this in a + // followup + pbar.Stop() for idx, imgType := range imgTypes { switch imgType { case "ami":