runcmd: Collect stderr of whole pipelinebaserock/richardmaw/pipeline-stderr-pipe-v2

When pipe_stderr is None (inherit parent fd), subprocess.STDOUT (stderr to
stdout) or a specific file, it would put everything to the right place, but
when pipe_stderr is subprocess.PIPE the behaviour is less well defined.

Before this change, it would happen to return the stderr of the last element of
the pipeline, behaving similar to this shell snippet, except it does not need
an intermediate file to store the contents of stderr in memory.

    stdout="$(foo 2>/dev/null | bar 2>/dev/null | baz 2>tmp)"
    stderr="$(cat tmp)"

With this patch, it now behaves more like:

    stdout="$((foo | bar | baz) 2>tmp)"
    stderr="$(cat tmp)"

It works by duplicating some of the subprocess.PIPE behaviour outside of
subprocess.Popen, as there's no better way to make every subprocess have the
same stderr, since there's no way to get the write end out of the Popen object
after it has been constructed, since it is closed in the constructor, only
leaving the read-end available as p.stderr.

Only the last element of the pipeline is given the read end of the pipe, since
it ought to have only one read pipe, and it makes the most sense for the last
element to have the pipe, since you can then think of the pipeline as having a
pipe that joins every element for stderr, that comes out at the end, with the
stdout of the pipeline.

1 files changed, 16 insertions, 4 deletions

diff --git a/cliapp/runcmd.py b/cliapp/runcmd.py
index 2757068..5ac8da5 100644
--- a/cliapp/runcmd.py
+++ b/cliapp/runcmd.py
@@ -112,23 +112,27 @@ def runcmd_unchecked(argv, *argvs, **kwargs):
 
 def _build_pipeline(argvs, pipe_stdin, pipe_stdout, pipe_stderr, kwargs):
     procs = []
+
+    if pipe_stderr == subprocess.PIPE:
+        # Make pipe for all subprocesses to share
+        rpipe, wpipe = os.pipe()
+        stderr = wpipe
+    else:
+        stderr = pipe_stderr
+
     for i, argv in enumerate(argvs):
         if i == 0 and i == len(argvs) - 1:
             stdin = pipe_stdin
             stdout = pipe_stdout
-            stderr = pipe_stderr
         elif i == 0:
             stdin = pipe_stdin
             stdout = subprocess.PIPE
-            stderr = pipe_stderr
         elif i == len(argvs) - 1:
             stdin = procs[-1].stdout
             stdout = pipe_stdout
-            stderr = pipe_stderr
         else:
             stdin = procs[-1].stdout
             stdout = subprocess.PIPE
-            stderr = pipe_stderr
         p = subprocess.Popen(argv, stdin=stdin, stdout=stdout,
                              stderr=stderr, close_fds=True, **kwargs)
 
@@ -142,6 +146,14 @@ def _build_pipeline(argvs, pipe_stdin, pipe_stdout, pipe_stderr, kwargs):
 
         procs.append(p)
 
+    if pipe_stderr == subprocess.PIPE:
+        # Ensure only subprocesses hold the write end of the pipe, so we get
+        # EOF when they all terminate
+        os.close(wpipe)
+        # Allow reading of the stderr of every process as the stderr of
+        # the last element
+        procs[-1].stderr = os.fdopen(rpipe)
+
     return procs
 
 def _run_pipeline(procs, feed_stdin, pipe_stdin, pipe_stdout, pipe_stderr,