From: Daniel Martí <mvdan@mvdan.cc>
Date: Wed, 16 Oct 2013 21:17:51 +0000 (+0200)
Subject: Finish large reworking of Popen calls
X-Git-Tag: 0.1~341
X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?a=commitdiff_plain;h=db80644fa65144400118202144d5e8b46aaacdf4;p=fdroidserver.git

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.
---

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)
-
-    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)
+        p = FDroidPopen(cmd, cwd=root_dir, verbose=verbose)
+
+    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)
-        
-        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()
+        p = FDroidPopen(['bash', '-x', '-c', cmd],
+                cwd=root_dir, verbose=verbose)
         
-        # 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