Rework progress #352
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,30 +7,17 @@ import ( | |
| "io" | ||
| "log" | ||
| "os" | ||
| "strings" | ||
| "sync" | ||
| "syscall" | ||
|
|
||
| "github.com/osbuild/images/pkg/osbuild" | ||
| ) | ||
|
|
||
| // XXX: merge variant back into images/pkg/osbuild/osbuild-exec.go | ||
| func RunOSBuild(pb ProgressBar, manifest []byte, exports []string, opts *osbuild.OSBuildOptions) error { | ||
| if opts == nil { | ||
| opts = &osbuild.OSBuildOptions{} | ||
| } | ||
|
|
||
| // To keep maximum compatibility keep the old behavior to run osbuild | ||
|
|
@@ -48,92 +35,39 @@ func RunOSBuild(pb ProgressBar, manifest []byte, exports []string, opts *OSBuild | |
| } | ||
| } | ||
|
|
||
| func runOSBuildNoProgress(pb ProgressBar, manifest []byte, exports []string, opts *osbuild.OSBuildOptions) error { | ||
| cmd := osbuild.NewOSBuildCmd(manifest, exports, opts) | ||
| if err := cmd.Run(); err != nil { | ||
| return fmt.Errorf("error running osbuild: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func runOSBuildWithProgress(pb ProgressBar, manifest []byte, exports []string, opts *osbuild.OSBuildOptions) (err 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() | ||
|
|
||
|
|
||
| opts.Monitor = osbuild.MonitorJSONSeq | ||
| opts.MonitorFD = 3 | ||
| opts.MonitorFile = wp | ||
| var stdio bytes.Buffer | ||
| var mu sync.Mutex | ||
| buildLog := opts.BuildLog | ||
| opts.BuildLogMu = &mu | ||
|
There was a problem hiding this comment. Can someone explain why type SyncedWriter struct {
mu sync.Mutex
w io.Writer
}
func NewSyncedWriter(w io.Writer) *SyncedWriter {
return &SyncedWriter{w: w}
}
func (sw *SyncedWriter) Write(p []byte) (n int, err error) {
sw.mu.Lock()
defer sw.mu.Unlock()
return sw.w.Write(p)
}Than we could drop this passing of mutex, it is created on stack either way. There was a problem hiding this comment. 2 reasons:
There was a problem hiding this comment. The 2nd reason is actually no longer the case with osbuild/images#1966 ; but we still need to pass along the mutex, unless we don't care about the ordering for these extra statements ibcli adds to the buildlog. There was a problem hiding this comment. Hmmm, right. Well, how about to tee before sync so each writer has its individual mutex that can be shared? Something like: syncedStdio := newSyncedWriter(&stdio)
syncedBuildLog := newSyncedWriter(opts.BuildLog)
io.MultiWriter(syncedStdio, syncedBuildLog)Then use There was a problem hiding this comment. So the flow should be If we create the syncw after the mw we get I think by having both stdout and stderr go via the multiw into syncw we're already out of order? Unsure. There was a problem hiding this comment. Oh actually this works in images, I guess io.MultiWriter isn't racy? The only problem is still writing the the buildlog from ibcli if we actually wanna move the synced writer over to images. There was a problem hiding this comment. Reversing multiwriter and syncedwriter can work, test pass with go test -race iwth this in images: But this doesn't solve problem 2 :/ |
||
| if opts.BuildLog == nil { | ||
| mw := &stdio | ||
| opts.Stdout = mw | ||
| opts.Stderr = mw | ||
| buildLog = io.Discard | ||
| } else { | ||
| opts.Stdout = &stdio | ||
| } | ||
| cmd := osbuild.NewOSBuildCmd(manifest, exports, opts) | ||
|
|
||
| osbuildStatus := osbuild.NewStatusScanner(rp) | ||
| if err := cmd.Start(); err != nil { | ||
|
|
@@ -186,11 +120,15 @@ func runOSBuildWithProgress(pb ProgressBar, manifest []byte, exports []string, o | |
| // external build log | ||
| if st.Message != "" { | ||
| tracesMsgs = append(tracesMsgs, st.Message) | ||
| mu.Lock() | ||
| fmt.Fprintln(buildLog, st.Message) | ||
| mu.Unlock() | ||
| } | ||
| if st.Trace != "" { | ||
| tracesMsgs = append(tracesMsgs, st.Trace) | ||
| mu.Lock() | ||
| fmt.Fprintln(buildLog, st.Trace) | ||
| mu.Unlock() | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is still draft, this is just to show what it would look like with osbuild/images#1966. Not actually intending to merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure was just highlighting it, I guess we have a linter for that. Or not, depends on the repo :-)