From 9bc6d76a0fc09de31688cc898630de39cfcc31c9 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 4 Jul 2024 17:12:45 +0200 Subject: [PATCH 1/5] main,osbuildprogress: add `--progress=text` support This adds a new `bootc-image-builder` mode that makes use of the osbuild json-seq progress information. It will display some user friendly progress information. --- bib/cmd/bootc-image-builder/main.go | 12 +- bib/internal/osbuildprogress/progress.go | 176 +++++++++++++++++++++++ 2 files changed, 186 insertions(+), 2 deletions(-) create mode 100644 bib/internal/osbuildprogress/progress.go diff --git a/bib/cmd/bootc-image-builder/main.go b/bib/cmd/bootc-image-builder/main.go index 035e4c0d3..45fefa5f4 100644 --- a/bib/cmd/bootc-image-builder/main.go +++ b/bib/cmd/bootc-image-builder/main.go @@ -27,6 +27,7 @@ import ( "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/osbuildprogress" "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" @@ -383,6 +384,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") + progress, _ := cmd.Flags().GetString("progress") logrus.Debug("Validating environment") if err := setup.Validate(targetArch); err != nil { @@ -453,7 +455,12 @@ func cmdBuild(cmd *cobra.Command, args []string) error { osbuildEnv = append(osbuildEnv, envVars...) } - _, err = osbuild.RunOSBuild(mf, osbuildStore, outputDir, exports, nil, osbuildEnv, false, os.Stderr) + switch progress { + case "text": + err = osbuildprogress.RunOSBuild(mf, osbuildStore, outputDir, exports, osbuildEnv) + default: + _, err = osbuild.RunOSBuild(mf, osbuildStore, outputDir, exports, nil, osbuildEnv, false, os.Stderr) + } if err != nil { return fmt.Errorf("cannot run osbuild: %w", err) } @@ -607,7 +614,8 @@ 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") + // XXX: make this a proper type + buildCmd.Flags().String("progress", "none", "type of progress bar to use") buildCmd.Flags().String("store", "/store", "osbuild store for intermediate pipeline trees") // flag rules for _, dname := range []string{"output", "store", "rpmmd"} { diff --git a/bib/internal/osbuildprogress/progress.go b/bib/internal/osbuildprogress/progress.go new file mode 100644 index 000000000..6988598c3 --- /dev/null +++ b/bib/internal/osbuildprogress/progress.go @@ -0,0 +1,176 @@ +package osbuildprogress + +import ( + "bufio" + "bytes" + "encoding/json" + "fmt" + "io" + "os" + "os/exec" + "strings" + "time" +) + +type OsbuildJsonProgress struct { + ID string `json:"id"` + Context struct { + Origin string `json:"origin"` + Pipeline struct { + Name string `json:"name"` + Stage struct { + Name string `json:"name"` + ID string `json:"id"` + } `json:"stage"` + ID string `json:"id"` + } `json:"pipeline"` + } `json:"context"` + Progress struct { + Name string `json:"name"` + Total int `json:"total"` + Done int `json:"done"` + // XXX: there are currently only two levels but it should be + // deeper nested in theory + SubProgress struct { + Name string `json:"name"` + Total int `json:"total"` + Done int `json:"done"` + // XXX: in theory this could be more nested but it's not + + } `json:"progress"` + } `json:"progress"` + + Message string `json:"message"` +} + +func scanJsonSeq(r io.Reader, ch chan OsbuildJsonProgress) { + var progress OsbuildJsonProgress + + scanner := bufio.NewScanner(r) + for scanner.Scan() { + // XXX: use a proper jsonseq reader? + line := scanner.Bytes() + line = bytes.Trim(line, "\x1e") + if err := json.Unmarshal(line, &progress); err != nil { + // XXX: provide an invalid lines chan + fmt.Fprintf(os.Stderr, "json decode err for line %q: %v\n", line, err) + continue + } + ch <- progress + } + if err := scanner.Err(); err != nil && err != io.EOF { + // log error here + } +} + +func AttachProgress(r io.Reader, w io.Writer) { + var progress OsbuildJsonProgress + spinner := []string{"|", "/", "-", "\\"} + i := 0 + + ch := make(chan OsbuildJsonProgress) + go scanJsonSeq(r, ch) + + mainProgress := "unknown" + subProgress := "" + message := "-" + + contextMap := map[string]string{} + + fmt.Fprintf(w, "\n") + for { + select { + case progress = <-ch: + id := progress.Context.Pipeline.ID + pipelineName := contextMap[id] + if pipelineName == "" { + pipelineName = progress.Context.Pipeline.Name + contextMap[id] = pipelineName + } + // XXX: use differentmap? + id = "stage-" + progress.Context.Pipeline.Stage.ID + stageName := contextMap[id] + if stageName == "" { + stageName = progress.Context.Pipeline.Stage.Name + contextMap[id] = stageName + } + + if progress.Progress.Total > 0 { + mainProgress = fmt.Sprintf("step: %v [%v/%v]", pipelineName, progress.Progress.Done+1, progress.Progress.Total+1) + } + // XXX: use context instead of name here too + if progress.Progress.SubProgress.Total > 0 { + subProgress = fmt.Sprintf("%v [%v/%v]", stageName, progress.Progress.SubProgress.Done+1, progress.Progress.SubProgress.Total+1) + } + + // todo: make message more structured in osbuild? + // message from the stages themselfs are very noisy + // best not to show to the user (only for failures) + if progress.Context.Origin == "osbuild.monitor" { + message = progress.Message + } + //message = strings.TrimSpace(strings.SplitN(progress.Message, "\n", 2)[0]) + // todo: fix in osbuild? + /* + l := strings.SplitN(message, ":", 2) + if len(l) > 1 { + message = strings.TrimSpace(l[1]) + } + */ + if len(message) > 60 { + message = message[:60] + "..." + } + case <-time.After(200 * time.Millisecond): + // nothing + } + + // XXX: use real progressbar *or* use helper to get terminal + // size for proper length checks etc + // + // poor man progress, we need multiple progress bars and + // a message that keeps getting updated (or maybe not the + // message) + fmt.Fprintf(w, "\x1b[2KBuilding [%s]\n", spinner[i]) + fmt.Fprintf(w, "\x1b[2Kstep : %s\n", mainProgress) + if subProgress != "" { + fmt.Fprintf(w, "\x1b[2Kmodule : %s\n", strings.TrimPrefix(subProgress, "org.osbuild.")) + } + fmt.Fprintf(w, "\x1b[3Kmessage: %s\n", message) + if subProgress != "" { + fmt.Fprintf(w, "\x1b[%dA", 4) + } else { + fmt.Fprintf(w, "\x1b[%dA", 3) + } + // spin + i = (i + 1) % len(spinner) + } +} + +// XXX: merge back into images/pkg/osbuild/osbuild-exec.go(?) +func RunOSBuild(manifest []byte, store, outputDirectory string, exports, extraEnv []string) error { + cmd := exec.Command( + "osbuild", + "--store", store, + "--output-directory", outputDirectory, + "--monitor=JSONSeqMonitor", + "-", + ) + cmd.Env = append(os.Environ(), extraEnv...) + cmd.Stdin = bytes.NewBuffer(manifest) + cmd.Stderr = os.Stderr + + for _, export := range exports { + cmd.Args = append(cmd.Args, "--export", export) + } + + stdout, err := cmd.StdoutPipe() + if err != nil { + return err + } + go AttachProgress(stdout, os.Stdout) + + if err := cmd.Start(); err != nil { + return fmt.Errorf("error starting osbuild: %v", err) + } + return cmd.Wait() +} From 31b653c934c7db33e17bcd772731586de1337781 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 4 Jul 2024 17:54:11 +0200 Subject: [PATCH 2/5] osbuildprogress: switch to `chegggaaa/pb/v3` for progress Switch the progress output to `chegggaaa/pb/v3` and tweak the progress. Still open questions about the message output and error handling. --- bib/internal/osbuildprogress/progress.go | 92 ++++++++++++------------ 1 file changed, 47 insertions(+), 45 deletions(-) diff --git a/bib/internal/osbuildprogress/progress.go b/bib/internal/osbuildprogress/progress.go index 6988598c3..6d49881a9 100644 --- a/bib/internal/osbuildprogress/progress.go +++ b/bib/internal/osbuildprogress/progress.go @@ -10,6 +10,8 @@ import ( "os/exec" "strings" "time" + + "github.com/cheggaaa/pb/v3" ) type OsbuildJsonProgress struct { @@ -27,14 +29,14 @@ type OsbuildJsonProgress struct { } `json:"context"` Progress struct { Name string `json:"name"` - Total int `json:"total"` - Done int `json:"done"` + Total int64 `json:"total"` + Done int64 `json:"done"` // XXX: there are currently only two levels but it should be // deeper nested in theory SubProgress struct { Name string `json:"name"` - Total int `json:"total"` - Done int `json:"done"` + Total int64 `json:"total"` + Done int64 `json:"done"` // XXX: in theory this could be more nested but it's not } `json:"progress"` @@ -43,7 +45,7 @@ type OsbuildJsonProgress struct { Message string `json:"message"` } -func scanJsonSeq(r io.Reader, ch chan OsbuildJsonProgress) { +func scanJsonSeq(r io.Reader, ch chan OsbuildJsonProgress, errCh chan error) { var progress OsbuildJsonProgress scanner := bufio.NewScanner(r) @@ -53,33 +55,48 @@ func scanJsonSeq(r io.Reader, ch chan OsbuildJsonProgress) { line = bytes.Trim(line, "\x1e") if err := json.Unmarshal(line, &progress); err != nil { // XXX: provide an invalid lines chan - fmt.Fprintf(os.Stderr, "json decode err for line %q: %v\n", line, err) + errCh <- err continue } ch <- progress } if err := scanner.Err(); err != nil && err != io.EOF { - // log error here + errCh <- err } } func AttachProgress(r io.Reader, w io.Writer) { var progress OsbuildJsonProgress - spinner := []string{"|", "/", "-", "\\"} - i := 0 ch := make(chan OsbuildJsonProgress) - go scanJsonSeq(r, ch) - - mainProgress := "unknown" - subProgress := "" - message := "-" + errCh := make(chan error) + go scanJsonSeq(r, ch, errCh) + + lastMessage := "-" + + spinnerPb := pb.New(0) + spinnerPb.SetTemplate(`Building [{{ (cycle . "|" "/" "-" "\\") }}]`) + mainPb := pb.New(0) + progressBarTmplFmt := `[{{ counters . }}] %s: {{ string . "prefix" }} {{ bar .}} {{ percent . }}` + mainPb.SetTemplateString(fmt.Sprintf(progressBarTmplFmt, "step")) + subPb := pb.New(0) + subPb.SetTemplateString(fmt.Sprintf(progressBarTmplFmt, "module")) + msgPb := pb.New(0) + msgPb.SetTemplate(`last msg: {{ string . "msg" }}`) + + pool, err := pb.StartPool(spinnerPb, mainPb, subPb, msgPb) + if err != nil { + fmt.Fprintf(os.Stderr, "progress failed: %v\n", err) + return + } contextMap := map[string]string{} - fmt.Fprintf(w, "\n") for { select { + case err := <-errCh: + fmt.Fprintf(os.Stderr, "error: %v", err) + break case progress = <-ch: id := progress.Context.Pipeline.ID pipelineName := contextMap[id] @@ -96,54 +113,38 @@ func AttachProgress(r io.Reader, w io.Writer) { } if progress.Progress.Total > 0 { - mainProgress = fmt.Sprintf("step: %v [%v/%v]", pipelineName, progress.Progress.Done+1, progress.Progress.Total+1) + mainPb.SetTotal(progress.Progress.Total + 1) + mainPb.SetCurrent(progress.Progress.Done + 1) + mainPb.Set("prefix", pipelineName) } // XXX: use context instead of name here too if progress.Progress.SubProgress.Total > 0 { - subProgress = fmt.Sprintf("%v [%v/%v]", stageName, progress.Progress.SubProgress.Done+1, progress.Progress.SubProgress.Total+1) + subPb.SetTotal(progress.Progress.SubProgress.Total + 1) + subPb.SetCurrent(progress.Progress.SubProgress.Done + 1) + subPb.Set("prefix", strings.TrimPrefix(stageName, "org.osbuild.")) } // todo: make message more structured in osbuild? // message from the stages themselfs are very noisy // best not to show to the user (only for failures) if progress.Context.Origin == "osbuild.monitor" { - message = progress.Message + lastMessage = progress.Message } - //message = strings.TrimSpace(strings.SplitN(progress.Message, "\n", 2)[0]) - // todo: fix in osbuild? /* - l := strings.SplitN(message, ":", 2) + // todo: fix in osbuild? + lastMessage = strings.TrimSpace(strings.SplitN(progress.Message, "\n", 2)[0]) + l := strings.SplitN(lastMessage, ":", 2) if len(l) > 1 { - message = strings.TrimSpace(l[1]) + lastMessage = strings.TrimSpace(l[1]) } */ - if len(message) > 60 { - message = message[:60] + "..." - } + msgPb.Set("msg", lastMessage) + case <-time.After(200 * time.Millisecond): // nothing } - - // XXX: use real progressbar *or* use helper to get terminal - // size for proper length checks etc - // - // poor man progress, we need multiple progress bars and - // a message that keeps getting updated (or maybe not the - // message) - fmt.Fprintf(w, "\x1b[2KBuilding [%s]\n", spinner[i]) - fmt.Fprintf(w, "\x1b[2Kstep : %s\n", mainProgress) - if subProgress != "" { - fmt.Fprintf(w, "\x1b[2Kmodule : %s\n", strings.TrimPrefix(subProgress, "org.osbuild.")) - } - fmt.Fprintf(w, "\x1b[3Kmessage: %s\n", message) - if subProgress != "" { - fmt.Fprintf(w, "\x1b[%dA", 4) - } else { - fmt.Fprintf(w, "\x1b[%dA", 3) - } - // spin - i = (i + 1) % len(spinner) } + pool.Stop() } // XXX: merge back into images/pkg/osbuild/osbuild-exec.go(?) @@ -172,5 +173,6 @@ func RunOSBuild(manifest []byte, store, outputDirectory string, exports, extraEn if err := cmd.Start(); err != nil { return fmt.Errorf("error starting osbuild: %v", err) } + // XXX: add WaitGroup return cmd.Wait() } From 68b8b700729173fe1fa13664bc4f1063ad303c92 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Tue, 30 Jul 2024 10:59:04 +0200 Subject: [PATCH 3/5] osbuildprogress: use `--monitor-fd=` to get progress from osbuild osbuild will send the build result to stdout in a non-json format. By using the --monitor-fd this won't affect the progress handling. --- bib/internal/osbuildprogress/progress.go | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/bib/internal/osbuildprogress/progress.go b/bib/internal/osbuildprogress/progress.go index 6d49881a9..f574e37db 100644 --- a/bib/internal/osbuildprogress/progress.go +++ b/bib/internal/osbuildprogress/progress.go @@ -149,30 +149,38 @@ func AttachProgress(r io.Reader, w io.Writer) { // XXX: merge back into images/pkg/osbuild/osbuild-exec.go(?) func RunOSBuild(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", "-", ) 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} + go AttachProgress(rp, os.Stdout) for _, export := range exports { cmd.Args = append(cmd.Args, "--export", export) } - - stdout, err := cmd.StdoutPipe() - if err != nil { - return err - } - go AttachProgress(stdout, os.Stdout) - if err := cmd.Start(); err != nil { return fmt.Errorf("error starting osbuild: %v", err) } + wp.Close() + // XXX: add WaitGroup return cmd.Wait() } From c3ea488fe5b189d7ff97ddbffb969097a92af058 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Tue, 30 Jul 2024 11:16:09 +0200 Subject: [PATCH 4/5] osbuildprogress: use a WaitGroup to syncronise goroutine --- bib/internal/osbuildprogress/progress.go | 38 +++++++++++++++++------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/bib/internal/osbuildprogress/progress.go b/bib/internal/osbuildprogress/progress.go index f574e37db..bdd738d88 100644 --- a/bib/internal/osbuildprogress/progress.go +++ b/bib/internal/osbuildprogress/progress.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "strings" + "sync" "time" "github.com/cheggaaa/pb/v3" @@ -45,16 +46,18 @@ type OsbuildJsonProgress struct { Message string `json:"message"` } -func scanJsonSeq(r io.Reader, ch chan OsbuildJsonProgress, errCh chan error) { - var progress OsbuildJsonProgress +func scanJsonSeq(wg *sync.WaitGroup, r io.Reader, ch chan OsbuildJsonProgress, errCh chan error) { + wg.Add(1) + defer wg.Done() + var progress OsbuildJsonProgress scanner := bufio.NewScanner(r) for scanner.Scan() { // XXX: use a proper jsonseq reader? line := scanner.Bytes() line = bytes.Trim(line, "\x1e") if err := json.Unmarshal(line, &progress); err != nil { - // XXX: provide an invalid lines chan + // XXX: provide an invalid lines chan here? errCh <- err continue } @@ -63,14 +66,17 @@ func scanJsonSeq(r io.Reader, ch chan OsbuildJsonProgress, errCh chan error) { if err := scanner.Err(); err != nil && err != io.EOF { errCh <- err } + errCh <- io.EOF } -func AttachProgress(r io.Reader, w io.Writer) { - var progress OsbuildJsonProgress +func AttachProgress(wg *sync.WaitGroup, r io.Reader, w io.Writer) { + wg.Add(1) + defer wg.Done() + var progress OsbuildJsonProgress ch := make(chan OsbuildJsonProgress) errCh := make(chan error) - go scanJsonSeq(r, ch, errCh) + go scanJsonSeq(wg, r, ch, errCh) lastMessage := "-" @@ -89,14 +95,17 @@ func AttachProgress(r io.Reader, w io.Writer) { fmt.Fprintf(os.Stderr, "progress failed: %v\n", err) return } + defer pool.Stop() contextMap := map[string]string{} for { select { case err := <-errCh: + if err == io.EOF { + return + } fmt.Fprintf(os.Stderr, "error: %v", err) - break case progress = <-ch: id := progress.Context.Pipeline.ID pipelineName := contextMap[id] @@ -144,11 +153,12 @@ func AttachProgress(r io.Reader, w io.Writer) { // nothing } } - pool.Stop() } // XXX: merge back into images/pkg/osbuild/osbuild-exec.go(?) func RunOSBuild(manifest []byte, store, outputDirectory string, exports, extraEnv []string) error { + wg := &sync.WaitGroup{} + rp, wp, err := os.Pipe() if err != nil { return fmt.Errorf("cannot create pipe for osbuild: %w", err) @@ -171,7 +181,7 @@ func RunOSBuild(manifest []byte, store, outputDirectory string, exports, extraEn // exported here cmd.Stdout = nil cmd.ExtraFiles = []*os.File{wp} - go AttachProgress(rp, os.Stdout) + go AttachProgress(wg, rp, os.Stdout) for _, export := range exports { cmd.Args = append(cmd.Args, "--export", export) @@ -181,6 +191,12 @@ func RunOSBuild(manifest []byte, store, outputDirectory string, exports, extraEn } wp.Close() - // XXX: add WaitGroup - return cmd.Wait() + if err := cmd.Wait(); err != nil { + return fmt.Errorf("error running osbuild: %w", err) + } + + // wait until the goroutines are finished too or we get premature + // exit of the progress reading and half finished progress bars + wg.Wait() + return nil } From a93c5a6a37b32c7cd538c6347f4918b795f7279f Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Tue, 30 Jul 2024 11:47:19 +0200 Subject: [PATCH 5/5] osbuildprogress: reorg lines a bit --- bib/internal/osbuildprogress/progress.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/bib/internal/osbuildprogress/progress.go b/bib/internal/osbuildprogress/progress.go index bdd738d88..067fcf7d2 100644 --- a/bib/internal/osbuildprogress/progress.go +++ b/bib/internal/osbuildprogress/progress.go @@ -174,6 +174,10 @@ func RunOSBuild(manifest []byte, store, outputDirectory string, exports, extraEn "--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 @@ -181,11 +185,8 @@ func RunOSBuild(manifest []byte, store, outputDirectory string, exports, extraEn // exported here cmd.Stdout = nil cmd.ExtraFiles = []*os.File{wp} - go AttachProgress(wg, rp, os.Stdout) - for _, export := range exports { - cmd.Args = append(cmd.Args, "--export", export) - } + go AttachProgress(wg, rp, os.Stdout) if err := cmd.Start(); err != nil { return fmt.Errorf("error starting osbuild: %v", err) }