- renamed the '-j' option to '-k', such that we can use the former for parallel processing later on

- changed list(map()) to list comprehensions
- removed try/except block around os.environ.get() for SCONS_EXTERNAL_TEST
- fixed a potential deadlock for subprocess.Popen by using temporary files
- added the '--nopipefiles' option to switch off this temp file fix (not recommended though)

18 files changed, 139 insertions, 60 deletions

diff --git a/QMTest/TestCmd.py b/QMTest/TestCmd.py
index 8d382ce..8a465a0 100644
--- a/QMTest/TestCmd.py
+++ b/QMTest/TestCmd.py
@@ -897,11 +897,7 @@ class TestCmd(object):
                        combine = 0,
                        universal_newlines = 1,
                        timeout = None):
-        self.external = 0
-        try:
-            self.external = os.environ.get('SCONS_EXTERNAL_TEST', 0)
-        except KeyError:
-            pass
+        self.external = os.environ.get('SCONS_EXTERNAL_TEST', 0)
         self._cwd = os.getcwd()
         self.description_set(description)
         self.program_set(program)
diff --git a/QMTest/TestSCons.py b/QMTest/TestSCons.py
index e6e0c8c..86c0dec 100644
--- a/QMTest/TestSCons.py
+++ b/QMTest/TestSCons.py
@@ -231,11 +231,7 @@ class TestSCons(TestCommon):
         is not necessary.
         """
         self.orig_cwd = os.getcwd()
-        self.external = 0
-        try:
-            self.external = os.environ.get('SCONS_EXTERNAL_TEST', 0)
-        except KeyError:
-            pass
+        self.external = os.environ.get('SCONS_EXTERNAL_TEST', 0)
 
         if not self.external:
             try:
diff --git a/runtest.py b/runtest.py
index c1dbaab..89ec9ee 100644
--- a/runtest.py
+++ b/runtest.py
@@ -50,7 +50,7 @@
 #
 #       -h              Print the help and exit.
 #
-#       -j              Suppress printing of count and percent progress for
+#       -k              Suppress printing of count and percent progress for
 #                       the single tests.
 #
 #       -l              List available tests and exit.
@@ -160,6 +160,7 @@ spe = None
 print_progress = 1
 suppress_stdout = False
 suppress_stderr = False
+allow_pipe_files = True
 
 helpstr = """\
 Usage: runtest.py [OPTIONS] [TEST ...]
@@ -173,10 +174,15 @@ Options:
   -e, --external              Run the script in external mode (for testing separate Tools)
   -f FILE, --file FILE        Run tests in specified FILE.
   -h, --help                  Print this message and exit.
-  -j, --no-progress           Suppress count and percent progress messages.
+  -k, --no-progress           Suppress count and percent progress messages.
   -l, --list                  List available tests and exit.
   -n, --no-exec               No execute, just print command lines.
   --noqmtest                  Execute tests directly, not using QMTest.
+  --nopipefiles               Doesn't use the "file pipe" workaround for subprocess.Popen()
+                              for starting tests. WARNING: Only use this when too much file
+                              traffic is giving you trouble AND you can be sure that none of
+                              your tests create output that exceed 65K chars! You might
+                              run into some deadlocks else.
   -o FILE, --output FILE      Print test results to FILE.
   -P Python                   Use the specified Python interpreter.
   -p PACKAGE, --package PACKAGE
@@ -209,10 +215,10 @@ Environment Variables:
   TESTCMD_VERBOSE: turn on verbosity in TestCommand
 """
 
-opts, args = getopt.getopt(sys.argv[1:], "3ab:def:hjlno:P:p:qsv:Xx:t",
+opts, args = getopt.getopt(sys.argv[1:], "3ab:def:hklno:P:p:qsv:Xx:t",
                             ['all', 'aegis', 'baseline=', 'builddir=',
                              'debug', 'external', 'file=', 'help', 'no-progress',
-                             'list', 'no-exec', 'noqmtest', 'output=',
+                             'list', 'no-exec', 'noqmtest', 'nopipefiles', 'output=',
                              'package=', 'passed', 'python=', 'qmtest',
                              'quiet', 'short-progress', 'sp=', 'spe=', 'time',
                              'version=', 'exec=',
@@ -244,7 +250,7 @@ for o, a in opts:
     elif o in ['-h', '--help']:
         print helpstr
         sys.exit(0)
-    elif o in ['-j', '--no-progress']:
+    elif o in ['-k', '--no-progress']:
         print_progress = 0
     elif o in ['-l', '--list']:
         list_only = 1
@@ -252,6 +258,8 @@ for o, a in opts:
         execute_tests = None
     elif o in ['--noqmtest']:
         qmtest = None
+    elif o in ['--nopipefiles']:
+        allow_pipe_files = False
     elif o in ['-o', '--output']:
         if a != '-' and not os.path.isabs(a):
             a = os.path.join(cwd, a)
@@ -388,27 +396,77 @@ try:
 except:
     use_subprocess = False
     
-if (use_subprocess and
-    not suppress_stdout and
-    not suppress_stderr):
-    # If no suppress mode is selected, we still use the
-    # old spawn routines instead of the modern subprocess module.
-    # This is important for the test/runtest scripts, where we
-    # call runtest.py within the single tests. With subprocess the
-    # stderr of the subprocess lands in stdout of the top test script,
-    # which lets the test fail. :(
-    # TODO: find a way to use subprocess with proper stream redirection...
-    use_subprocess = False
-
 if use_subprocess:
-    def spawn_it(command_args):
-        p = subprocess.Popen(' '.join(command_args),
-                             stdout=subprocess.PIPE,
-                             stderr=subprocess.PIPE,
-                             shell=True)
-        spawned_stdout = p.stdout.read()
-        spawned_stderr = p.stderr.read()
-        return (spawned_stderr, spawned_stdout, p.wait())
+    if not suppress_stdout and not suppress_stderr:
+        # Without any output suppressed, we let the subprocess
+        # write its stuff freely to stdout/stderr.
+        def spawn_it(command_args):
+            p = subprocess.Popen(' '.join(command_args),
+                                 shell=True)
+            return (None, None, p.wait())
+    else:
+        # Else, we catch the output of both pipes...
+        if allow_pipe_files:
+            # The subprocess.Popen() suffers from a well-known
+            # problem. Data for stdout/stderr is read into a 
+            # memory buffer of fixed size, 65K which is not very much.
+            # When it fills up, it simply stops letting the child process
+            # write to it. The child will then sit and patiently wait to
+            # be able to write the rest of its output. Hang! 
+            # In order to work around this, we follow a suggestion
+            # by Anders Pearson in
+            #   http://http://thraxil.org/users/anders/posts/2008/03/13/Subprocess-Hanging-PIPE-is-your-enemy/
+            # and pass temp file objects to Popen() instead of the ubiquitous
+            # subprocess.PIPE. 
+            def spawn_it(command_args):
+                # Create temporary files
+                import tempfile
+                tmp_stdout = tempfile.TemporaryFile(mode='w+t')
+                tmp_stderr = tempfile.TemporaryFile(mode='w+t')
+                # Start subprocess...
+                p = subprocess.Popen(' '.join(command_args),
+                                     stdout=tmp_stdout,
+                                     stderr=tmp_stderr,
+                                     shell=True)
+                # ... and wait for it to finish.
+                ret = p.wait()
+                
+                try:
+                    # Rewind to start of files
+                    tmp_stdout.seek(0)
+                    tmp_stderr.seek(0)
+                    # Read output
+                    spawned_stdout = tmp_stdout.read()
+                    spawned_stderr = tmp_stderr.read()
+                finally:
+                    # Remove temp files by closing them
+                    tmp_stdout.close()
+                    tmp_stderr.close()
+                    
+                # Return values
+                return (spawned_stderr, spawned_stdout, ret)
+            
+        else:
+            # We get here only if the user gave the '--nopipefiles'
+            # option, meaning the "temp file" approach for
+            # subprocess.communicate() above shouldn't be used.
+            # He hopefully knows what he's doing, but again we have a
+            # potential deadlock situation in the following code:
+            #   If the subprocess writes a lot of data to its stderr,
+            #   the pipe will fill up (nobody's reading it yet) and the
+            #   subprocess will wait for someone to read it. 
+            #   But the parent process is trying to read from stdin
+            #   (but the subprocess isn't writing anything there).  
+            #   Hence a deadlock.
+            # Be dragons here! Better don't use this!
+            def spawn_it(command_args):
+                p = subprocess.Popen(' '.join(command_args),
+                                     stdout=subprocess.PIPE,
+                                     stderr=subprocess.PIPE,
+                                     shell=True)
+                spawned_stdout = p.stdout.read()
+                spawned_stderr = p.stderr.read()
+                return (spawned_stderr, spawned_stdout, p.wait())
 else:
     has_subprocess = False
     # Set up lowest-common-denominator spawning of a process on both Windows
@@ -426,7 +484,7 @@ else:
     else:
         def spawn_it(command_args):
             command = command_args[0]
-            command_args = list(map(escape, command_args))
+            command_args = [escape(c) for c in command_args]
             return (None, None, os.spawnv(os.P_WAIT, command, command_args))
 
 class Base(object):
@@ -476,14 +534,43 @@ if not use_subprocess:
                 self.status = self.status >> 8
 else:
     class PopenExecutor(Base):
-        def execute(self):
-            p = subprocess.Popen(self.command_str,
-                                 stdout=subprocess.PIPE,
-                                 stderr=subprocess.PIPE,
-                                 shell=True)
-            self.stdout = p.stdout.read()
-            self.stderr = p.stderr.read()
-            self.status = p.wait()
+        # For an explanation of the following 'if ... else'
+        # and the 'allow_pipe_files' option, please check out the
+        # use_subprocess path in the definition of spawn_it() above.
+        if allow_pipe_files:
+            def execute(self):
+                # Create temporary files
+                import tempfile
+                tmp_stdout = tempfile.TemporaryFile(mode='w+t')
+                tmp_stderr = tempfile.TemporaryFile(mode='w+t')
+                # Start subprocess...
+                p = subprocess.Popen(self.command_str,
+                                     stdout=tmp_stdout,
+                                     stderr=tmp_stderr,
+                                     shell=True)
+                # ... and wait for it to finish.
+                self.status = p.wait()
+                
+                try:
+                    # Rewind to start of files
+                    tmp_stdout.seek(0)
+                    tmp_stderr.seek(0)
+                    # Read output
+                    self.stdout = tmp_stdout.read()
+                    self.stderr = tmp_stderr.read()
+                finally:
+                    # Remove temp files by closing them
+                    tmp_stdout.close()
+                    tmp_stderr.close()
+        else:        
+            def execute(self):
+                p = subprocess.Popen(self.command_str,
+                                     stdout=subprocess.PIPE,
+                                     stderr=subprocess.PIPE,
+                                     shell=True)
+                self.stdout = p.stdout.read()
+                self.stderr = p.stderr.read()
+                self.status = p.wait()
 
 class Aegis(SystemExecutor):
     def header(self, f):
@@ -851,7 +938,7 @@ if qmtest:
 #except OSError:
 #    pass
 
-tests = list(map(Test, tests))
+tests = [Test(t) for t in tests]
 
 class Unbuffered(object):
     def __init__(self, file):
diff --git a/test/runtest/aegis/batch-output.py b/test/runtest/aegis/batch-output.py
index e29183d..e371def 100644
--- a/test/runtest/aegis/batch-output.py
+++ b/test/runtest/aegis/batch-output.py
@@ -52,7 +52,7 @@ NO RESULT TEST STDERR
 PASSING TEST STDERR
 """
 
-test.run(arguments = '-j -o aegis.out --aegis test', stderr=expect_stderr)
+test.run(arguments = '-k -o aegis.out --aegis test', stderr=expect_stderr)
 
 expect = """\
 test_result = [
diff --git a/test/runtest/baseline/combined.py b/test/runtest/baseline/combined.py
index 1983ae0..35c1796 100644
--- a/test/runtest/baseline/combined.py
+++ b/test/runtest/baseline/combined.py
@@ -69,7 +69,7 @@ NO RESULT TEST STDERR
 PASSING TEST STDERR
 """
 
-test.run(arguments='-j -b . test',
+test.run(arguments='-k -b . test',
          status=1,
          stdout=expect_stdout,
          stderr=expect_stderr)
diff --git a/test/runtest/baseline/fail.py b/test/runtest/baseline/fail.py
index a96ba80..5687160 100644
--- a/test/runtest/baseline/fail.py
+++ b/test/runtest/baseline/fail.py
@@ -47,7 +47,7 @@ expect_stderr = """\
 FAILING TEST STDERR
 """
 
-test.run(arguments='-j -b . test/fail.py',
+test.run(arguments='-k -b . test/fail.py',
          status=1,
          stdout=expect_stdout,
          stderr=expect_stderr)
diff --git a/test/runtest/baseline/no_result.py b/test/runtest/baseline/no_result.py
index 7a07c01..2149594 100644
--- a/test/runtest/baseline/no_result.py
+++ b/test/runtest/baseline/no_result.py
@@ -47,7 +47,7 @@ expect_stderr = """\
 NO RESULT TEST STDERR
 """
 
-test.run(arguments='-j -b . test/no_result.py',
+test.run(arguments='-k -b . test/no_result.py',
          status=2,
          stdout=expect_stdout,
          stderr=expect_stderr)
diff --git a/test/runtest/baseline/pass.py b/test/runtest/baseline/pass.py
index bcd87d7..affa486 100644
--- a/test/runtest/baseline/pass.py
+++ b/test/runtest/baseline/pass.py
@@ -50,7 +50,7 @@ expect_stderr = """\
 PASSING TEST STDERR
 """
 
-test.run(arguments='-j -b . test',
+test.run(arguments='-k -b . test',
          stdout=expect_stdout,
          stderr=expect_stderr)
 
diff --git a/test/runtest/fallback.py b/test/runtest/fallback.py
index ae7b814..1229b28 100644
--- a/test/runtest/fallback.py
+++ b/test/runtest/fallback.py
@@ -82,7 +82,7 @@ testlist = [
     test_pass_py,
 ]
 
-test.run(arguments = '-j '+' '.join(testlist),
+test.run(arguments = '-k '+' '.join(testlist),
          status = 1,
          stdout = expect_stdout,
          stderr = expect_stderr)
diff --git a/test/runtest/noqmtest.py b/test/runtest/noqmtest.py
index e2cb5f8..eb33223 100644
--- a/test/runtest/noqmtest.py
+++ b/test/runtest/noqmtest.py
@@ -74,7 +74,7 @@ testlist = [
     test_pass_py,
 ]
 
-test.run(arguments = '-j --noqmtest %s' % ' '.join(testlist),
+test.run(arguments = '-k --noqmtest %s' % ' '.join(testlist),
          status = 1,
          stdout = expect_stdout,
          stderr = expect_stderr)
diff --git a/test/runtest/print_time.py b/test/runtest/print_time.py
index dfce273..244c6f8 100644
--- a/test/runtest/print_time.py
+++ b/test/runtest/print_time.py
@@ -75,7 +75,7 @@ NO RESULT TEST STDERR
 PASSING TEST STDERR
 """
 
-test.run(arguments='-j -t test',
+test.run(arguments='-k -t test',
          status=1,
          stdout=expect_stdout,
          stderr=expect_stderr)
diff --git a/test/runtest/python.py b/test/runtest/python.py
index b45e8d4..d406be4 100644
--- a/test/runtest/python.py
+++ b/test/runtest/python.py
@@ -66,7 +66,7 @@ expect_stderr = """\
 PASSING TEST STDERR
 """
 
-test.run(arguments=['-j','-P', mypython, 'test'],
+test.run(arguments=['-k','-P', mypython, 'test'],
          stdout=expect_stdout,
          stderr=expect_stderr)
 
diff --git a/test/runtest/simple/combined.py b/test/runtest/simple/combined.py
index 6e0539a..616f4d5 100644
--- a/test/runtest/simple/combined.py
+++ b/test/runtest/simple/combined.py
@@ -70,7 +70,7 @@ NO RESULT TEST STDERR
 PASSING TEST STDERR
 """
 
-test.run(arguments='-j test',
+test.run(arguments='-k test',
          status=1,
          stdout=expect_stdout,
          stderr=expect_stderr)
diff --git a/test/runtest/simple/fail.py b/test/runtest/simple/fail.py
index d891597..8b800fb 100644
--- a/test/runtest/simple/fail.py
+++ b/test/runtest/simple/fail.py
@@ -47,7 +47,7 @@ expect_stderr = """\
 FAILING TEST STDERR
 """
 
-test.run(arguments='-j test/fail.py',
+test.run(arguments='-k test/fail.py',
          status=1,
          stdout=expect_stdout,
          stderr=expect_stderr)
diff --git a/test/runtest/simple/no_result.py b/test/runtest/simple/no_result.py
index 91ba50c..91af7e4 100644
--- a/test/runtest/simple/no_result.py
+++ b/test/runtest/simple/no_result.py
@@ -47,7 +47,7 @@ expect_stderr = """\
 NO RESULT TEST STDERR
 """
 
-test.run(arguments='-j test/no_result.py',
+test.run(arguments='-k test/no_result.py',
          status=2,
          stdout=expect_stdout,
          stderr=expect_stderr)
diff --git a/test/runtest/simple/pass.py b/test/runtest/simple/pass.py
index c954336..6e5b6b0 100644
--- a/test/runtest/simple/pass.py
+++ b/test/runtest/simple/pass.py
@@ -47,7 +47,7 @@ expect_stderr = """\
 PASSING TEST STDERR
 """
 
-test.run(arguments='-j test/pass.py', stdout=expect_stdout, stderr=expect_stderr)
+test.run(arguments='-k test/pass.py', stdout=expect_stdout, stderr=expect_stderr)
 
 test.pass_test()
 
diff --git a/test/runtest/src.py b/test/runtest/src.py
index 84b08a0..23894f9 100644
--- a/test/runtest/src.py
+++ b/test/runtest/src.py
@@ -62,7 +62,7 @@ PASSING TEST STDERR
 PASSING TEST STDERR
 """ % locals()
 
-test.run(arguments='-j src', stdout=expect_stdout, stderr=expect_stderr)
+test.run(arguments='-k src', stdout=expect_stdout, stderr=expect_stderr)
 
 test.pass_test()
 
diff --git a/test/runtest/testlistfile.py b/test/runtest/testlistfile.py
index 5a56bb3..b86b0f2 100644
--- a/test/runtest/testlistfile.py
+++ b/test/runtest/testlistfile.py
@@ -62,7 +62,7 @@ expect_stderr = """\
 PASSING TEST STDERR
 """
 
-test.run(arguments='-j -f t.txt', stdout=expect_stdout, stderr=expect_stderr)
+test.run(arguments='-k -f t.txt', stdout=expect_stdout, stderr=expect_stderr)
 
 test.pass_test()