Convert Linux backends to use app log streaming #967
Conversation
| self.tools.subprocess.stream_output( | ||
| app.app_name, | ||
| log_popen, | ||
| stop_func=lambda: log_popen.poll() is not None, | ||
| ) |
There was a problem hiding this comment.
curious....did you find the stop_func was necessary for log streaming to exit properly?
I'd expect checking poll() is functionally equivalent to the log streamer exiting when readline() returns an empty string....since that'll happen once the PIPE is closed...and that'll happen when the underlying process exits.
There was a problem hiding this comment.
Good point - this was mostly done for consistency with the iOS/macOS/etc backends, but you're right - as we're streaming the actual process, and the actual process will exit, there's no need to poll here.
| try: | ||
| # Start streaming logs for the app. | ||
| self.logger.info( | ||
| "Following log output (type CTRL-C to stop log)...", |
There was a problem hiding this comment.
Oh yeah - I forgot we made a conscious decision to pull those out.
There was a problem hiding this comment.
In general, I think I have concerns about how much of work that's typically been encapsulated inside Subprocess is now being implemented in platform code.
More or less, this re-implements Subprocess._run_and_stream_output() in multiple places....but really _run_and_stream_output() is just an implementation of Subprocess.run().
Would it be possible to augment the Subprocess.run() signature to achieve the same outcome?
I've reviewed #962 and AFAICT, the primary goal is to monitor the output for sentinels. I think that would be achievable by exposing filter_func in Subprocess.run() (although, I think it should renamed since it isn't really filtering anything but we can bikeshed later).
All that said, macOS and iOS are fundamentally different since the App process and the process making the App output available are independent; so, they require a different architecture from the Linux backends here.
Let me know if I haven't taken in the whole picture properly.
You're not wrong - I've had very similar thoughts. The only reason I didn't do it as part of this PR is that I wanted to confirm that the run sequence in fact was identical on every platform, and could be abstracted consistently, especially in the light of the changes we're going to need to make to stream filters for #962. As you note, macOS has a quite different approach; I wasn't 100% sure if we were going to end up with similar differences once we resolved the issues like the Android startup problem (#966) or Windows logging problem (#970). Think of it as an incremental approach - get everything into a stream-based approach on a platform-by-platform basis (and fix some of the other log-related issues); then accomodate the testing changes; then refactor to pull out the common "streaming execution" code. Smaller focussed PRs tend to be easier to review. Based on where I am right now, it seems very likely we'll be able to factor out some common code; the "run this app" code workflow is very similar across every platform. I'm not completely sure if it will just be adding a |
In order to support test mode (Refs #962), we need to convert app runners to use streamers, rather than just outputting logs directly to stdout. To make the code easier to review, I've split the introduction of streaming to the linux backends as a standalone PR, similar to what has been done with #966 for Android.
PR Checklist: