Unfinished code execution reports as `[finished with error]`

[dmackdev]

Firstly - thank you for building this fantastic project! I recently tried several other terminal presentation tools, and I found presenterm to be best by far. In particular, for its of out-of-the-box and non-blurred image support and how great the default themes and styling look. Well done!

I have encountered an issue with the bash code execution feature. On one of my machines, executing any simple bash script from within a presentation yields the [finished with error] message in the execution output, nearly every time. Very occasionally the message does not appear. The bash script executes successfully when I execute it directly in the terminal.

E.g. for the script

echo "hi"

In the presentation, after executing it, the output would be:

hi
[finished with error]

FWIW, on another machine, I very rarely see the [finished with error] message, but it still comes up occasionally. Both machines have similar setups - both use iterm2 and zsh.

I did some debugging: I believe the child code execution process had not exited by the time the output from the process is extracted - this try_wait was returning Ok(None). I also made a change to pipe stderr and display that, but there was no stderr output.

So my suggestions/questions from this are:

1. We should modify the ProcessReader::run method to not update the status to ProcessStatus::Failure if the child process has not exited yet. (We could potentially use a new ProcessStatus::NotComplete variant?)
2. Could the try_wait be replaced by the blocking wait to ensure the process has completed? ProcessReader::run is called in a new thread.
3. I think seeing stderr output would be preferable to the [finished with error] message. Seeing the actual stderr output would also help debugging. Any thoughts on this?

I would be happy to make contributions.

Cheers!

[dmackdev]

Could the try_wait be replaced by the blocking wait to ensure the process has completed? ProcessReader::run is called in a new thread.

Actually using blocking wait wait_with_output would mean the code execution output only appears in the presentation after all the code has finished executing - the presentation would not show the output line by line as soon as it is written to stdout, as it does currently.

[mfontanini]

Thanks for the report! I managed to reproduce consistently by doing

```bash +exec
echo "hi"
exec 1<&- sleep 1
```

I've never seen this locally unless I do that ^ but I guess it depends on how the OS handles things. I can imagine the race condition between closing stdout and the process dying is there and shouldn't be too rare to hit.

Anyway, talking about the fix, I'm not sure what you mean with using wait leading to the entire output being shown at once. I tried that and it seems to work just fine with the "echo / sleep" code snippet in examples/demo.md. The output is pushed into a shared vec so you can spit output and then wait for the program to finish. And this to me makes sense; this as you say runs in a separate thread so the presentation can keep going (you can change slides and all) and eventually the program will end and we'll get the done or "finished with error" depending on what happened. I'm not really sure why I used try_wait as to me wait makes more sense. Could you clarify how using wait doesn't work? I may be missing something.

As for showing the stderr output on failure, I think that makes sense. I'm just not sure how to show it "nicely". The best we can do is something like "finished with error" and then the error message in some specific configurable color (e.g. a shade of red likely).

[dmackdev]

I tried that and it seems to work just fine with the "echo / sleep" code snippet in examples/demo.md.

Actually, I was mistaken and you are correct - the behaviour I described only appears when using wait_with_output, not when using wait.

The behaviour I was describing is demonstrated by the following recording:

https://github.com/mfontanini/presenterm/assets/79006698/f0dcdb7e-6a53-4604-b3db-9efee23feb87

Instead of seeing the each hi output with pauses in between, you only see the entire output at the end, once the child process exits.

As for showing the stderr output on failure, I think that makes sense. I'm just not sure how to show it "nicely".

To me, the ideal would be that you see the output from both stdout and stderr exactly as you would see it from executing the code directly in the terminal. As an example, see the recording in https://github.com/dmackdev/presenterm/pull/3 where the final output shows the stdout and stderr outputs interleaved. But that approach on that PR currently suffers from the same aforementioned behaviour where any output only appears once the child process exits. I think showing output exactly as it would appear in the terminal is ideal because it could be confusing for viewers of the presentation to see it presented differently. Perhaps there might also be use cases where the presenter intends to execute code snippets that deliberately fail or have stderr output?

Failing that, as a compromise, I think showing stderr output after stdout output would still be valuable as it could help the presenter debug snippets and still provide a way to make stderr visible in the presentation, if so desired. See https://github.com/dmackdev/presenterm/pull/2 for an example of that. Colouring that output in a configurable colour also sounds like a good idea, and maybe even an option to control whether to show stderr at all?

[dmackdev]

I appreciate that our discussion (and my initial suggestions) have grown beyond the crux of this issue, so perhaps as next steps:

1. We replace try_wait with wait to fix this original issue of seeing [finished with error] only because the process had not exited yet. I am happy to raise a PR for this.
2. We devise an approach for showing stderr output - happy to raise a separate issue to focus on this, if you so wish!

Your call @mfontanini 😃

[mfontanini]

Sounds good! If you'd like to create a PR that'd be great. And yes, we should move the stderr handling to a separate issue. I like the idea of interleaving as long as it doesn't add other drawbacks (e.g. it still prints interactively). I left a comment on https://github.com/dmackdev/presenterm/pull/3 about that.

[dmackdev]

Great - I will raise that PR, raise a separate issue and have a look at your comment. Cheers!