From db80644fa65144400118202144d5e8b46aaacdf4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
Date: Wed, 16 Oct 2013 23:17:51 +0200
Subject: [PATCH] Finish large reworking of Popen calls

Now we handle the output pipes via Queues, which means that long and verbose
builds (like navit) won't hang mercilessly during any of init=, prebuild=
build= or the build itself. This also removes redundancies.
---
 fdroidserver/build.py  | 199 +++++++----------------------------------
 fdroidserver/common.py | 137 +++++++++++++++++-----------
 2 files changed, 118 insertions(+), 218 deletions(-)

diff --git a/fdroidserver/build.py b/fdroidserver/build.py
index e10b3b13..686c8382 100644
--- a/fdroidserver/build.py
+++ b/fdroidserver/build.py
@@ -27,36 +27,10 @@ import tarfile
 import traceback
 import time
 import json
-import Queue
-import threading
 from optparse import OptionParser
 
 import common
-from common import BuildException
-from common import VCSException
-
-class AsynchronousFileReader(threading.Thread):
-    '''
-    Helper class to implement asynchronous reading of a file
-    in a separate thread. Pushes read lines on a queue to
-    be consumed in another thread.
-    '''
- 
-    def __init__(self, fd, queue):
-        assert isinstance(queue, Queue.Queue)
-        assert callable(fd.readline)
-        threading.Thread.__init__(self)
-        self._fd = fd
-        self._queue = queue
- 
-    def run(self):
-        '''The body of the tread: read lines and put them on the queue.'''
-        for line in iter(self._fd.readline, ''):
-            self._queue.put(line)
- 
-    def eof(self):
-        '''Check whether there is no more content to expect.'''
-        return not self.is_alive() and self._queue.empty()
+from common import BuildException, VCSException, FDroidPopen
 
 def get_builder_vm_id():
     vd = os.path.join('builder', '.vagrant')
@@ -315,8 +289,6 @@ def build_server(app, thisbuild, vcs, build_dir, output_dir, sdk_path, force):
             cmdline += ' --force --test'
         cmdline += ' -p ' + app['id'] + ' --vercode ' + thisbuild['vercode']
         chan.exec_command('bash -c ". ~/.bsenv && ' + cmdline + '"')
-        output = ''
-        error = ''
         while not chan.exit_status_ready():
             while chan.recv_ready():
                 output += chan.recv(1024)
@@ -376,14 +348,11 @@ def build_local(app, thisbuild, vcs, build_dir, output_dir, srclib_dir, extlib_d
     # We need to clean via the build tool in case the binary dirs are
     # different from the default ones
     p = None
-    output = ''
-    error = ''
     if 'maven' in thisbuild:
         print "Cleaning Maven project..."
         cmd = [mvn3, 'clean', '-Dandroid.sdk.path=' + sdk_path]
 
-        p = subprocess.Popen(cmd, cwd=root_dir,
-                stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        p = FDroidPopen(cmd, cwd=root_dir, verbose=verbose)
     elif 'gradle' in thisbuild:
         print "Cleaning Gradle project..."
         cmd = [gradle, 'clean']
@@ -393,33 +362,15 @@ def build_local(app, thisbuild, vcs, build_dir, output_dir, srclib_dir, extlib_d
         else:
             gradle_dir = root_dir
 
-        p = subprocess.Popen(cmd, cwd=gradle_dir,
-                stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        p = FDroidPopen(cmd, cwd=gradle_dir, verbose=verbose)
     elif thisbuild.get('update', '.') != 'no':
         print "Cleaning Ant project..."
         cmd = ['ant', 'clean']
-        p = subprocess.Popen(cmd, cwd=root_dir,
-                stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        p = FDroidPopen(cmd, cwd=root_dir, verbose=verbose)
 
-    if p is not None:
-        for line in iter(p.stdout.readline, ''):
-            if verbose:
-                # Output directly to console
-                sys.stdout.write(line)
-                sys.stdout.flush()
-            else:
-                output += line
-        for line in iter(p.stderr.readline, ''):
-            if verbose:
-                # Output directly to console
-                sys.stdout.write(line)
-                sys.stdout.flush()
-            else:
-                error += line
-        p.communicate()
-        if p.returncode != 0:
-            raise BuildException("Error cleaning %s:%s" %
-                    (app['id'], thisbuild['version']), output, error)
+    if p is not None and p.returncode != 0:
+        raise BuildException("Error cleaning %s:%s" %
+                (app['id'], thisbuild['version']), p.stdout, p.stderr)
 
     # Also clean jni
     print "Cleaning jni dirs..."
@@ -456,8 +407,6 @@ def build_local(app, thisbuild, vcs, build_dir, output_dir, srclib_dir, extlib_d
     tarball.close()
 
     # Run a build command if one is required...
-    output = ''
-    error = ''
     if 'build' in thisbuild:
         cmd = thisbuild['build']
         # Substitute source library paths into commands...
@@ -470,42 +419,12 @@ def build_local(app, thisbuild, vcs, build_dir, output_dir, srclib_dir, extlib_d
         if verbose:
             print "Running 'build' commands in %s" % root_dir
 
-        p = subprocess.Popen(['bash', '-x', '-c', cmd], cwd=root_dir,
-                stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        p = FDroidPopen(['bash', '-x', '-c', cmd],
+                cwd=root_dir, verbose=verbose)
         
-        stdout_queue = Queue.Queue()
-        stdout_reader = AsynchronousFileReader(p.stdout, stdout_queue)
-        stdout_reader.start()
-        stderr_queue = Queue.Queue()
-        stderr_reader = AsynchronousFileReader(p.stderr, stderr_queue)
-        stderr_reader.start()
-        
-        # Check the queues if we received some output (until there is nothing more to get).
-        while not stdout_reader.eof() or not stderr_reader.eof():
-            # Show what we received from standard output.
-            while not stdout_queue.empty():
-                line = stdout_queue.get()
-                if verbose:
-                    # Output directly to console
-                    sys.stdout.write(line)
-                    sys.stdout.flush()
-                else:
-                    error += line
- 
-            # Show what we received from standard error.
-            while not stderr_queue.empty():
-                line = stderr_queue.get()
-                if verbose:
-                    # Output directly to console
-                    sys.stdout.write(line)
-                    sys.stdout.flush()
-                else:
-                    error += line
-
-        p.communicate()
         if p.returncode != 0:
             raise BuildException("Error running build command for %s:%s" %
-                    (app['id'], thisbuild['version']), output, error)
+                    (app['id'], thisbuild['version']), p.stdout, p.stderr)
 
     # Build native stuff if required...
     if thisbuild.get('buildjni') not in (None, 'no'):
@@ -530,21 +449,12 @@ def build_local(app, thisbuild, vcs, build_dir, output_dir, srclib_dir, extlib_d
                 open(manifest, 'w').write(manifest_text)
                 # In case the AM.xml read was big, free the memory
                 del manifest_text
-            p = subprocess.Popen([ndkbuild], cwd=root_dir + '/' + d,
-                    stdout=subprocess.PIPE)
-        output = p.communicate()[0]
-        if p.returncode != 0:
-            print output
-            raise BuildException("NDK build failed for %s:%s" % (app['id'], thisbuild['version']))
+            p = FDroidPopen([ndkbuild], cwd=os.path.join(root_dir,d),
+                    verbose=verbose)
+            if p.returncode != 0:
+                raise BuildException("NDK build failed for %s:%s" % (app['id'], thisbuild['version']), p.stdout, p.stderr)
 
     p = None
-    if verbose:
-        output = None
-        error = None
-    else:
-        output = ''
-        error = ''
-    output_apk = ''
     # Build the release...
     if 'maven' in thisbuild:
         print "Building Maven project..."
@@ -562,23 +472,7 @@ def build_local(app, thisbuild, vcs, build_dir, output_dir, srclib_dir, extlib_d
         if 'mvnflags' in thisbuild:
             mvncmd += thisbuild['mvnflags']
 
-        p = subprocess.Popen(mvncmd, cwd=root_dir, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        for line in iter(p.stdout.readline, ''):
-            if verbose:
-                # Output directly to console
-                sys.stdout.write(line)
-                sys.stdout.flush()
-            else:
-                output += line
-            if 'apk' in line:
-                output_apk += line
-        for line in iter(p.stderr.readline, ''):
-            if verbose:
-                # Output directly to console
-                sys.stdout.write(line)
-                sys.stdout.flush()
-            else:
-                error += line
+        p = FDroidPopen(mvncmd, cwd=root_dir, verbose=verbose, apkoutput=True)
 
     elif 'gradle' in thisbuild:
         print "Building Gradle project..."
@@ -622,58 +516,29 @@ def build_local(app, thisbuild, vcs, build_dir, output_dir, srclib_dir, extlib_d
         if verbose:
             print "Running %s on %s" % (" ".join(commands), gradle_dir)
 
-        p = subprocess.Popen(commands, cwd=gradle_dir, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        for line in iter(p.stdout.readline, ''):
-            if verbose:
-                # Output directly to console
-                sys.stdout.write(line)
-                sys.stdout.flush()
-            else:
-                output += line
-        for line in iter(p.stderr.readline, ''):
-            if verbose:
-                # Output directly to console
-                sys.stdout.write(line)
-                sys.stdout.flush()
-            else:
-                error += line
+        p = FDroidPopen(commands, cwd=gradle_dir, verbose=verbose)
 
     else:
         print "Building Ant project..."
-        antcommands = ['ant']
+        cmd = ['ant']
         if install:
-            antcommands += ['debug','install']
+            cmd += ['debug','install']
         elif 'antcommand' in thisbuild:
-            antcommands += [thisbuild['antcommand']]
+            cmd += [thisbuild['antcommand']]
         else:
-            antcommands += ['release']
-        p = subprocess.Popen(antcommands, cwd=root_dir, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        for line in iter(p.stdout.readline, ''):
-            if verbose:
-                # Output directly to console
-                sys.stdout.write(line)
-                sys.stdout.flush()
-            else:
-                output += line
-            if 'apk' in line:
-                output_apk += line
-        for line in iter(p.stderr.readline, ''):
-            if verbose:
-                # Output directly to console
-                sys.stdout.write(line)
-                sys.stdout.flush()
-            else:
-                error += line
-    p.communicate()
+            cmd += ['release']
+        p = FDroidPopen(cmd, cwd=root_dir, verbose=verbose, apkoutput=True)
     if p.returncode != 0:
-        raise BuildException("Build failed for %s:%s" % (app['id'], thisbuild['version']), output, error)
+        raise BuildException("Build failed for %s:%s" % (app['id'], thisbuild['version']), p.stdout, p.stderr)
     if install:
         if 'maven' in thisbuild:
-            p = subprocess.Popen([mvn3, 'android:deploy', '-Dandroid.sdk.path=' + sdk_path],
-                    cwd=root_dir, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-            output_, error_ = p.communicate()
+            p = FDroidPopen([mvn3, 'android:deploy',
+                '-Dandroid.sdk.path=' + sdk_path],
+                cwd=root_dir, verobse=verbose)
             if p.returncode != 0:
-                raise BuildException("Warning: Could not deploy %s:%s" % (app['id'], thisbuild['version']), output_, error_)
+                raise BuildException("Warning: Could not deploy %s:%s"
+                        % (app['id'], thisbuild['version']),
+                        p.stdout, p.stderr)
         return
     print "Successfully built version " + thisbuild['version'] + ' of ' + app['id']
 
@@ -691,14 +556,14 @@ def build_local(app, thisbuild, vcs, build_dir, output_dir, srclib_dir, extlib_d
         src = os.path.join(bindir, src)
     elif 'maven' in thisbuild:
         m = re.match(r".*^\[INFO\] .*apkbuilder.*/([^/]*)\.apk",
-                output_apk, re.S|re.M)
+                p.stdout_apk, re.S|re.M)
         if not m:
             m = re.match(r".*^\[INFO\] Creating additional unsigned apk file .*/([^/]+)\.apk",
-                    output_apk, re.S|re.M)
+                    p.stdout_apk, re.S|re.M)
         if not m:
             # This format is found in com.github.mobile, com.yubico.yubitotp and com.botbrew.basil for example...
             m = re.match(r'.*^\[INFO\] [^$]*aapt \[package,[^$]*' + bindir + '/([^/]+)\.ap[_k][,\]]',
-                    output_apk, re.S|re.M)
+                    p.stdout_apk, re.S|re.M)
         if not m:
             raise BuildException('Failed to find output')
         src = m.group(1)
@@ -712,7 +577,7 @@ def build_local(app, thisbuild, vcs, build_dir, output_dir, srclib_dir, extlib_d
             name = '-'.join([os.path.basename(build_dir), flavour, 'release', 'unsigned'])
         src = os.path.join(build_dir, 'build', 'apk', name+'.apk')
     else:
-        src = re.match(r".*^.*Creating (.+) for release.*$.*", output_apk,
+        src = re.match(r".*^.*Creating (.+) for release.*$.*", p.stdout_apk,
             re.S|re.M).group(1)
         src = os.path.join(bindir, src)
 
diff --git a/fdroidserver/common.py b/fdroidserver/common.py
index 2afbc248..73015552 100644
--- a/fdroidserver/common.py
+++ b/fdroidserver/common.py
@@ -23,6 +23,8 @@ import subprocess
 import time
 import operator
 import cgi
+import Queue
+import threading
 import magic
 
 def getvcs(vcstype, remote, local, sdk_path):
@@ -1135,14 +1137,10 @@ def getsrclib(spec, srclib_dir, sdk_path, ndk_path="", mvn3="", basepath=False,
             cmd = srclib["Prepare"].replace('$$SDK$$', sdk_path)
             cmd = cmd.replace('$$NDK$$', ndk_path).replace('$$MVN$$', mvn3)
 
-            print "******************************* PREPARE " + cmd + " **************"
-
-            p = subprocess.Popen(['bash', '-c', cmd], cwd=libdir,
-                    stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-            out, err = p.communicate()
+            p = FDroidPopen(['bash', '-x', '-c', cmd], cwd=libdir, verbose=verbose)
             if p.returncode != 0:
-                raise BuildException("Error running prepare command for srclib "
-                        + name, out, err)
+                raise BuildException("Error running prepare command for srclib %s"
+                        % name, p.stdout, p.stderr)
         
         if srclib["Update Project"] == "Yes":
             print "Updating srclib %s at path %s" % (name, libdir)
@@ -1198,8 +1196,6 @@ def prepare_source(vcs, app, build, build_dir, srclib_dir, extlib_dir, sdk_path,
 
     # Run an init command if one is required...
     if 'init' in build:
-        output = ''
-        error = ''
         cmd = build['init']
         cmd = cmd.replace('$$SDK$$', sdk_path)
         cmd = cmd.replace('$$NDK$$', ndk_path)
@@ -1207,26 +1203,10 @@ def prepare_source(vcs, app, build, build_dir, srclib_dir, extlib_dir, sdk_path,
         if verbose:
             print "Running 'init' commands in %s" % root_dir
 
-        p = subprocess.Popen(['bash', '-x', '-c', cmd], cwd=root_dir,
-                stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        for line in iter(p.stdout.readline, ''):
-            if verbose:
-                # Output directly to console
-                sys.stdout.write(line)
-                sys.stdout.flush()
-            else:
-                output += line
-        for line in iter(p.stderr.readline, ''):
-            if verbose:
-                # Output directly to console
-                sys.stdout.write(line)
-                sys.stdout.flush()
-            else:
-                error += line
-        p.communicate()
+        p = FDroidPopen(['bash', '-x', '-c', cmd], cwd=root_dir, verbose=verbose)
         if p.returncode != 0:
             raise BuildException("Error running init command for %s:%s" %
-                    (app['id'], build['version']), output, error)
+                    (app['id'], build['version']), p.stdout, p.stderr)
 
     # Generate (or update) the ant build file, build.xml...
     updatemode = build.get('update', '.')
@@ -1272,13 +1252,11 @@ def prepare_source(vcs, app, build, build_dir, srclib_dir, extlib_dir, sdk_path,
             if verbose:
                 print "Update of '%s': exec '%s' in '%s'"%\
                     (d," ".join(parms),cwd)
-            p = subprocess.Popen(parms, cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-            (out, err) = p.communicate()
-            if p.returncode != 0:
-                raise BuildException("Failed to update project with stdout '%s' and stderr '%s'"%(out,err))
+            p = FDroidPopen(parms, cwd=cwd, verbose=verbose)
             # check to see whether an error was returned without a proper exit code (this is the case for the 'no target set or target invalid' error)
-            if err != "" and err.startswith("Error: "):
-                raise BuildException("Failed to update project with stdout '%s' and stderr '%s'"%(out,err))
+            if p.returncode != 0 or (p.stderr != "" and p.stderr.startswith("Error: ")):
+                raise BuildException("Failed to update project at %s" % cwd,
+                        p.stdout, p.stderr)
 
     # If the app has ant set up to sign the release, we need to switch
     # that off, because we want the unsigned apk...
@@ -1429,26 +1407,10 @@ def prepare_source(vcs, app, build, build_dir, srclib_dir, extlib_dir, sdk_path,
         if verbose:
             print "Running 'prebuild' commands in %s" % root_dir
 
-        p = subprocess.Popen(['bash', '-x', '-c', cmd], cwd=root_dir,
-                stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        for line in iter(p.stdout.readline, ''):
-            if verbose:
-                # Output directly to console
-                sys.stdout.write(line)
-                sys.stdout.flush()
-            else:
-                output += line
-        for line in iter(p.stderr.readline, ''):
-            if verbose:
-                # Output directly to console
-                sys.stdout.write(line)
-                sys.stdout.flush()
-            else:
-                error += line
-        p.communicate()
+        p = FDroidPopen(['bash', '-x', '-c', cmd], cwd=root_dir, verbose=verbose)
         if p.returncode != 0:
             raise BuildException("Error running prebuild command for %s:%s" %
-                    (app['id'], build['version']), output, error)
+                    (app['id'], build['version']), p.stdout, p.stderr)
     print "Applying generic clean-ups..."
 
     if build.get('anal-tics', 'no') == 'yes':
@@ -1730,3 +1692,76 @@ def isApkDebuggable(apkfile):
     return False
 
 
+class AsynchronousFileReader(threading.Thread):
+    '''
+    Helper class to implement asynchronous reading of a file
+    in a separate thread. Pushes read lines on a queue to
+    be consumed in another thread.
+    '''
+ 
+    def __init__(self, fd, queue):
+        assert isinstance(queue, Queue.Queue)
+        assert callable(fd.readline)
+        threading.Thread.__init__(self)
+        self._fd = fd
+        self._queue = queue
+ 
+    def run(self):
+        '''The body of the tread: read lines and put them on the queue.'''
+        for line in iter(self._fd.readline, ''):
+            self._queue.put(line)
+ 
+    def eof(self):
+        '''Check whether there is no more content to expect.'''
+        return not self.is_alive() and self._queue.empty()
+
+class PopenResult:
+    returncode = None
+    stdout = ''
+    stderr = ''
+    stdout_apk = ''
+
+def FDroidPopen(commands, cwd,
+        stdout=subprocess.PIPE, stderr=subprocess.PIPE,
+        verbose=False, apkoutput=False):
+    """
+    Runs a command the FDroid way and returns return code and output
+
+    :param commands, cwd, stdout, stderr: like subprocess.Popen
+    :param verbose: whether to print output as it is saved
+    """
+    result = PopenResult()
+    p = subprocess.Popen(commands, cwd=cwd, stdout=stdout, stderr=stderr)
+    
+    stdout_queue = Queue.Queue()
+    stdout_reader = AsynchronousFileReader(p.stdout, stdout_queue)
+    stdout_reader.start()
+    stderr_queue = Queue.Queue()
+    stderr_reader = AsynchronousFileReader(p.stderr, stderr_queue)
+    stderr_reader.start()
+    
+    # Check the queues for output (until there is no more to get)
+    while not stdout_reader.eof() or not stderr_reader.eof():
+        # Show what we received from standard output
+        while not stdout_queue.empty():
+            line = stdout_queue.get()
+            if verbose:
+                # Output directly to console
+                sys.stdout.write(line)
+                sys.stdout.flush()
+            if apkoutput and 'apk' in line:
+                result.stdout_apk += line
+            result.stdout += line
+
+        # Show what we received from standard error
+        while not stderr_queue.empty():
+            line = stderr_queue.get()
+            if verbose:
+                # Output directly to console
+                sys.stderr.write(line)
+                sys.stderr.flush()
+            result.stderr += line
+
+    p.communicate()
+    result.returncode = p.returncode
+    return result