diff --git a/bib/pkg/progress/progress.go b/bib/pkg/progress/progress.go index 508fed4..2326153 100644 --- a/bib/pkg/progress/progress.go +++ b/bib/pkg/progress/progress.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "strings" + "syscall" "time" "github.com/cheggaaa/pb/v3" @@ -340,7 +341,7 @@ func runOSBuildNoProgress(pb ProgressBar, manifest []byte, store, outputDirector var osbuildCmd = "osbuild" -func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirectory string, exports, extraEnv []string) error { +func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirectory string, exports, extraEnv []string) (err error) { rp, wp, err := os.Pipe() if err != nil { return fmt.Errorf("cannot create pipe for osbuild: %w", err) @@ -372,14 +373,32 @@ func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirect return fmt.Errorf("error starting osbuild: %v", err) } wp.Close() + defer func() { + // Try to stop osbuild if we exit early, we are gentle + // here to give osbuild the chance to release its + // resources (like mounts in the buildroot). This is + // best effort only (but also a pretty uncommon error + // condition). If ProcessState is set the process has + // already exited and we have nothing to do. + if err != nil && cmd.Process != nil && cmd.ProcessState == nil { + sigErr := cmd.Process.Signal(syscall.SIGINT) + err = errors.Join(err, sigErr) + } + }() var tracesMsgs []string - var statusErrs []error for { st, err := osbuildStatus.Status() if err != nil { - statusErrs = append(statusErrs, err) - continue + // This should never happen but if it does we try + // to be helpful. We need to exit here (and kill + // osbuild in the defer) or we would appear to be + // handing as cmd.Wait() would wait to finish but + // no progress or other message is reported. We + // can also not (in the general case) recover as + // the underlying osbuildStatus.scanner maybe in + // an unrecoverable state (like ErrTooBig). + return fmt.Errorf(`error parsing osbuild status, please report a bug and try with "--progress=verbose": %w`, err) } if st == nil { break @@ -408,9 +427,6 @@ func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirect if err := cmd.Wait(); err != nil { return fmt.Errorf("error running osbuild: %w\nBuildLog:\n%s\nOutput:\n%s", err, strings.Join(tracesMsgs, "\n"), stdio.String()) } - if len(statusErrs) > 0 { - return fmt.Errorf("errors parsing osbuild status:\n%w", errors.Join(statusErrs...)) - } return nil } diff --git a/bib/pkg/progress/progress_test.go b/bib/pkg/progress/progress_test.go index 4343c98..1daa21c 100644 --- a/bib/pkg/progress/progress_test.go +++ b/bib/pkg/progress/progress_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "reflect" "testing" + "time" "github.com/stretchr/testify/assert" @@ -167,15 +168,35 @@ osbuild-stderr-output } func TestRunOSBuildWithProgressIncorrectJSON(t *testing.T) { - restore := progress.MockOsbuildCmd(makeFakeOsbuild(t, `echo osbuild-stdout-output ->&2 echo osbuild-stderr-output + signalDeliveredMarkerPath := filepath.Join(t.TempDir(), "sigint-delivered") + + restore := progress.MockOsbuildCmd(makeFakeOsbuild(t, fmt.Sprintf(` +trap 'touch "%s";exit 2' INT + >&3 echo invalid-json -`)) + +# we cannot sleep infinity here or the shell script trap is never run +while true; do + sleep 0.1 +done +`, signalDeliveredMarkerPath))) defer restore() pbar, err := progress.New("debug") assert.NoError(t, err) err = progress.RunOSBuild(pbar, []byte(`{"fake":"manifest"}`), "", "", nil, nil) - assert.EqualError(t, err, `errors parsing osbuild status: -cannot scan line "invalid-json": invalid character 'i' looking for beginning of value`) + assert.EqualError(t, err, `error parsing osbuild status, please report a bug and try with "--progress=verbose": cannot scan line "invalid-json": invalid character 'i' looking for beginning of value`) + + // ensure the SIGINT got delivered + var pathExists = func(p string) bool { + _, err := os.Stat(p) + return err == nil + } + for i := 0; i < 20; i++ { + time.Sleep(100 * time.Millisecond) + if pathExists(signalDeliveredMarkerPath) { + break + } + } + assert.True(t, pathExists(signalDeliveredMarkerPath)) }