Merge lp:~broder/ubuntu-dev-tools/fix-785854 into lp:~ubuntu-dev/ubuntu-dev-tools/trunk

[Evan Broder]

[Barry Warsaw (barry)]

Hi Evan,

I totally see why you've added this, and changed the code to use it. Thanks for making ubuntu-dev-tools better! Of course, once we migrate u-d-t to Python 3.2 this change won't be necessary <wink>.

I'm a little concerned that what you've written isn't entirely API compatible with Python's version. Python's subprocess.Popen is a class while ubuntutools.subprocess.Popen is a function. I have no idea whether people actually try to subclass subprocess.Popen, and within u-d-t it's easy enough to guarantee this, but is it possible there are would be external clients that could be broken by this? It makes me uncomfortable that your docs claim API compatibility when it's really not.

Two thoughts: could this be implemented as a subprocess.Popen subclass?

Or, would it make sense to just use a different API rather than promoting something that looks like stdlib subprocess.Popen but isn't quite? The way you have it does mean that existing code in u-d-t doesn't have to change, except for the import statement. I'm not sure whether that's good or not ;).

You might also call inspect.getargspec() at module scope and then not redefine, but simply expose the stdlib subprocess functions directly in ubuntutools.subprocess.Popen.

Anyway, just some thoughts. At the least, please do indicate in the docs about this difference. I'm happy to chat about alternatives in IRC if you want.

review: Needs Fixing

[lp:~broder/ubuntu-dev-tools/fix-785854]

1093. By Evan Broder on 2011-06-19

Reimplement ubuntutools.subprocess.Popen as a class instead of a factory function.

Unmerged revisions

=== modified file '404main'
--- 404main	2010-12-27 19:32:07 +0000
+++ 404main	2011-06-19 21:23:31 +0000
@@ -24,12 +24,13 @@
 # This script is used to check if a package and all its build
 # dependencies are in main or not.
 
-import subprocess
 import sys
 
 import apt_pkg
 import apt
 
+from ubuntutools import subprocess
+
 def process_deps(cache, deps):
     """Takes a list of (build) dependencies and processes it."""
 

=== modified file 'ack-sync'
--- ack-sync	2011-05-23 21:44:45 +0000
+++ ack-sync	2011-06-19 21:23:31 +0000
@@ -22,7 +22,6 @@
 import lazr.restfulclient
 import os
 import re
-import subprocess
 import sys
 import logging
 import glob
@@ -31,6 +30,7 @@
 from launchpadlib.launchpad import Launchpad
 
 from ubuntutools.config import UDTConfig
+from ubuntutools import subprocess
 
 COMMAND_LINE_SYNTAX_ERROR = 1
 VERSION_DETECTION_FAILED = 2

=== modified file 'backportpackage'
--- backportpackage	2011-05-28 17:41:07 +0000
+++ backportpackage	2011-06-19 21:23:31 +0000
@@ -21,7 +21,6 @@
 import optparse
 import os
 import shutil
-import subprocess
 import sys
 import tempfile
 
@@ -34,6 +33,7 @@
 from ubuntutools.config import UDTConfig, ubu_email
 from ubuntutools.builder import get_builder
 from ubuntutools.question import YesNoQuestion
+from ubuntutools import subprocess
 
 def error(msg):
     Logger.error(msg)

=== modified file 'debian/changelog'
--- debian/changelog	2011-06-16 19:02:04 +0000
+++ debian/changelog	2011-06-19 21:23:31 +0000
@@ -47,7 +47,18 @@
   * mk-sbuild:
     - maintainer_name isn't mandatory any more (LP: #787051)
 
+<<<<<<< TREE
  -- Benjamin Drung <[e-mail]>  Wed, 25 May 2011 18:27:46 +0200
+=======
+  [ Evan Broder ]
+  * ubuntutools.subprocess:
+    - New drop-in replacement wrapper module around subprocess that
+      backports the restore_signals kwarg and defaults close_fds=True
+    - Switch everything previously using subprocess to use
+      ubuntutools.subprocess instead (LP: #785854)
+
+ -- Evan Broder <[e-mail]>  Tue, 24 May 2011 20:21:47 +0200
+>>>>>>> MERGE-SOURCE
 
 ubuntu-dev-tools (0.123) unstable; urgency=low
 

=== modified file 'dgetlp'
--- dgetlp	2011-02-12 22:08:15 +0000
+++ dgetlp	2011-06-19 21:23:31 +0000
@@ -32,7 +32,6 @@
 import hashlib
 import optparse
 import os
-import subprocess
 import sys
 import urllib2
 
@@ -43,6 +42,8 @@
                           "use this utility.")
     sys.exit(1)
 
+from ubuntutools import subprocess
+
 USAGE = u"""Usage: %prog [-d|(-v|-q)] <Launchpad URL>
 
 This scripts simulates «dget»'s behaviour for files hosted at

=== modified file 'get-branches'
--- get-branches	2010-12-27 19:49:00 +0000
+++ get-branches	2011-06-19 21:23:31 +0000
@@ -25,10 +25,10 @@
 #
 
 import os
-import subprocess
 import sys
 from optparse import OptionParser
 from ubuntutools.lp.lpapicache import PersonTeam
+from ubuntutools import subprocess
 
 def main():
     usage = "Usage: %prog [-d <directory>] -t <team> [-o <operation>]"

=== modified file 'import-bug-from-debian'
--- import-bug-from-debian	2011-02-28 22:30:54 +0000
+++ import-bug-from-debian	2011-06-19 21:23:31 +0000
@@ -25,7 +25,6 @@
 from optparse import OptionParser, SUPPRESS_HELP
 import re
 import sys
-import subprocess
 
 try:
     import SOAPpy
@@ -37,6 +36,7 @@
 from launchpadlib.launchpad import Launchpad
 
 from ubuntutools.config import UDTConfig
+from ubuntutools import subprocess
 
 def main():
     bug_re = re.compile(r"bug=(\d+)")

=== modified file 'lp-project-upload'
--- lp-project-upload	2011-06-09 11:36:06 +0000
+++ lp-project-upload	2011-06-19 21:23:31 +0000
@@ -22,13 +22,14 @@
 
 import datetime
 import os
-import subprocess
 import sys
 import tempfile
 
 from launchpadlib.launchpad import Launchpad
 from launchpadlib.errors import HTTPError
 
+from ubuntutools import subprocess
+
 def create_release(project, version):
     '''Create new release and milestone for LP project.'''
 

=== modified file 'pbuilder-dist'
--- pbuilder-dist	2011-05-23 21:41:00 +0000
+++ pbuilder-dist	2011-06-19 21:23:31 +0000
@@ -30,13 +30,13 @@
 # that the target distribution is always meant to be Ubuntu Hardy.
 
 import os
-import subprocess
 import sys
 
 from devscripts.logger import Logger
 
 from ubuntutools.distro_info import DebianDistroInfo
 import ubuntutools.misc
+from ubuntutools import subprocess
 
 class PbuilderDist:
     def __init__(self, builder):

=== modified file 'syncpackage'
--- syncpackage	2011-05-23 21:41:00 +0000
+++ syncpackage	2011-06-19 21:23:31 +0000
@@ -25,7 +25,6 @@
 import optparse
 import os
 import shutil
-import subprocess
 import sys
 
 from devscripts.logger import Logger
@@ -38,6 +37,7 @@
 from ubuntutools.requestsync.lp import getDebianSrcPkg, getUbuntuSrcPkg
 from ubuntutools.lp import udtexceptions
 from ubuntutools.lp.lpapicache import Launchpad
+from ubuntutools import subprocess
 
 
 class Version(debian.debian_support.Version):

=== modified file 'ubuntu-iso'
--- ubuntu-iso	2010-12-27 13:01:03 +0000
+++ ubuntu-iso	2011-06-19 21:23:31 +0000
@@ -21,7 +21,8 @@
 # ##################################################################
 
 import sys
-import subprocess
+
+from ubuntutools import subprocess
 
 def extract(iso, path):
     command = ['isoinfo', '-R', '-i', iso, '-x', path]

=== modified file 'ubuntutools/archive.py'
--- ubuntutools/archive.py	2011-05-30 13:55:15 +0000
+++ ubuntutools/archive.py	2011-06-19 21:23:31 +0000
@@ -31,7 +31,6 @@
 
 import hashlib
 import os.path
-import subprocess
 import urllib2
 import urlparse
 import re
@@ -45,6 +44,7 @@
 from ubuntutools.config import UDTConfig
 from ubuntutools.lp.lpapicache import (Launchpad, Distribution,
                                        SourcePackagePublishingHistory)
+from ubuntutools import subprocess
 
 class DownloadError(Exception):
     "Unable to pull a source package"

=== modified file 'ubuntutools/builder.py'
--- ubuntutools/builder.py	2011-05-23 21:41:00 +0000
+++ ubuntutools/builder.py	2011-06-19 21:23:31 +0000
@@ -19,9 +19,9 @@
 #
 
 import os
-import subprocess
 
 from devscripts.logger import Logger
+from ubuntutools import subprocess
 
 def _build_preparation(result_directory):
     """prepares the builder for building a package"""

=== modified file 'ubuntutools/misc.py'
--- ubuntutools/misc.py	2011-02-13 14:15:24 +0000
+++ ubuntutools/misc.py	2011-06-19 21:23:31 +0000
@@ -25,10 +25,10 @@
 import locale
 import os
 import os.path
-from subprocess import Popen, PIPE
 import sys
 
 from ubuntutools.lp.udtexceptions import PocketDoesNotExistError
+from ubuntutools.subprocess import Popen, PIPE
 
 _system_distribution = None
 def system_distribution():

=== modified file 'ubuntutools/requestsync/common.py'
--- ubuntutools/requestsync/common.py	2010-12-22 22:01:39 +0000
+++ ubuntutools/requestsync/common.py	2011-06-19 21:23:31 +0000
@@ -24,9 +24,10 @@
 import urllib2
 import re
 import tempfile
-import subprocess
 from debian.changelog import Changelog
 
+from ubuntutools import subprocess
+
 def raw_input_exit_on_ctrlc(*args, **kwargs):
     '''
     A wrapper around raw_input() to exit with a normalized message on Control-C

=== modified file 'ubuntutools/requestsync/mail.py'
--- ubuntutools/requestsync/mail.py	2011-03-05 22:48:24 +0000
+++ ubuntutools/requestsync/mail.py	2011-06-19 21:23:31 +0000
@@ -22,13 +22,13 @@
 
 import os
 import sys
-import subprocess
 import smtplib
 import socket
 from debian.changelog import Version
 from ubuntutools.archive import rmadison, FakeSPPH
 from ubuntutools.distro_info import DebianDistroInfo
 from ubuntutools.requestsync.common import raw_input_exit_on_ctrlc
+from ubuntutools import subprocess
 from ubuntutools.lp.udtexceptions import PackageNotFoundException
 
 __all__ = [

=== modified file 'ubuntutools/sponsor_patch/patch.py'
--- ubuntutools/sponsor_patch/patch.py	2010-12-27 14:20:49 +0000
+++ ubuntutools/sponsor_patch/patch.py	2011-06-19 21:23:31 +0000
@@ -16,7 +16,8 @@
 # OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
 import os
-import subprocess
+
+from ubuntutools import subprocess
 
 class Patch(object):
     def __init__(self, patch_file):

=== modified file 'ubuntutools/sponsor_patch/sponsor_patch.py'
--- ubuntutools/sponsor_patch/sponsor_patch.py	2011-06-03 17:18:38 +0000
+++ ubuntutools/sponsor_patch/sponsor_patch.py	2011-06-19 21:23:31 +0000
@@ -19,7 +19,6 @@
 import pwd
 import re
 import shutil
-import subprocess
 import sys
 
 import debian.changelog
@@ -28,6 +27,7 @@
 
 from devscripts.logger import Logger
 
+from ubuntutools import subprocess
 from ubuntutools.update_maintainer import update_maintainer
 from ubuntutools.question import Question, YesNoQuestion, input_number
 

=== added file 'ubuntutools/subprocess.py'
--- ubuntutools/subprocess.py	1970-01-01 00:00:00 +0000
+++ ubuntutools/subprocess.py	2011-06-19 21:23:31 +0000
@@ -0,0 +1,109 @@
+"""Drop-in replacement for subprocess with better defaults
+
+This is an API-compatible replacement for the built-in subprocess
+module whose defaults better line up with our tastes.
+
+In particular, it:
+ - Adds support for the restore_signals flag if subprocess itself
+   doesn't support it
+ - Defaults close_fds to True
+"""
+
+
+from __future__ import absolute_import
+
+import inspect
+import signal
+import subprocess
+
+from subprocess import PIPE, STDOUT, CalledProcessError
+
+__all__ = ['Popen', 'call', 'check_call', 'check_output', 'CalledProcessError', 'PIPE', 'STDOUT']
+
+
+class Popen(subprocess.Popen):
+    def __init__(self, *args, **kwargs):
+        kwargs.setdefault('close_fds', True)
+
+        if 'restore_signals' not in inspect.getargspec(subprocess.Popen.__init__)[0]:
+            given_preexec_fn = kwargs.pop('preexec_fn', None)
+            restore_signals = kwargs.pop('restore_signals', True)
+            def preexec_fn():
+                if restore_signals:
+                    for sig in ('SIGPIPE', 'SIGXFZ', 'SIGXFSZ'):
+                        if hasattr(signal, sig):
+                            signal.signal(getattr(signal, sig),
+                                          signal.SIG_DFL)
+
+                if given_preexec_fn:
+                    given_preexec_fn()
+            kwargs['preexec_fn'] = preexec_fn
+
+        subprocess.Popen.__init__(self, *args, **kwargs)
+
+
+# call, check_call, and check_output are copied directly from the
+# subprocess module shipped with Python 2.7.1-5ubuntu2
+
+
+def call(*popenargs, **kwargs):
+    """Run command with arguments.  Wait for command to complete, then
+    return the returncode attribute.
+
+    The arguments are the same as for the Popen constructor.  Example:
+
+    retcode = call(["ls", "-l"])
+    """
+    return Popen(*popenargs, **kwargs).wait()
+
+
+def check_call(*popenargs, **kwargs):
+    """Run command with arguments.  Wait for command to complete.  If
+    the exit code was zero then return, otherwise raise
+    CalledProcessError.  The CalledProcessError object will have the
+    return code in the returncode attribute.
+
+    The arguments are the same as for the Popen constructor.  Example:
+
+    check_call(["ls", "-l"])
+    """
+    retcode = call(*popenargs, **kwargs)
+    if retcode:
+        cmd = kwargs.get("args")
+        if cmd is None:
+            cmd = popenargs[0]
+        raise CalledProcessError(retcode, cmd)
+    return 0
+
+
+def check_output(*popenargs, **kwargs):
+    r"""Run command with arguments and return its output as a byte string.
+
+    If the exit code was non-zero it raises a CalledProcessError.  The
+    CalledProcessError object will have the return code in the returncode
+    attribute and output in the output attribute.
+
+    The arguments are the same as for the Popen constructor.  Example:
+
+    >>> check_output(["ls", "-l", "/dev/null"])
+    'crw-rw-rw- 1 root root 1, 3 Oct 18  2007 /dev/null\n'
+
+    The stdout argument is not allowed as it is used internally.
+    To capture standard error in the result, use stderr=STDOUT.
+
+    >>> check_output(["/bin/sh", "-c",
+    ...               "ls -l non_existent_file ; exit 0"],
+    ...              stderr=STDOUT)
+    'ls: non_existent_file: No such file or directory\n'
+    """
+    if 'stdout' in kwargs:
+        raise ValueError('stdout argument not allowed, it will be overridden.')
+    process = Popen(stdout=PIPE, *popenargs, **kwargs)
+    output, unused_err = process.communicate()
+    retcode = process.poll()
+    if retcode:
+        cmd = kwargs.get("args")
+        if cmd is None:
+            cmd = popenargs[0]
+        raise CalledProcessError(retcode, cmd, output=output)
+    return output

=== modified file 'ubuntutools/test/test_help.py'
--- ubuntutools/test/test_help.py	2011-05-21 15:53:16 +0000
+++ ubuntutools/test/test_help.py	2011-06-19 21:23:31 +0000
@@ -18,10 +18,10 @@
 import os
 import select
 import signal
-import subprocess
 import time
 
 import setup
+from ubuntutools import subprocess
 from ubuntutools.test import unittest
 
 BLACKLIST = {

=== modified file 'ubuntutools/test/test_pylint.py'
--- ubuntutools/test/test_pylint.py	2011-05-30 13:55:15 +0000
+++ ubuntutools/test/test_pylint.py	2011-06-19 21:23:31 +0000
@@ -15,10 +15,10 @@
 # PERFORMANCE OF THIS SOFTWARE.
 
 import re
-import subprocess
 
 import setup
 from ubuntutools.test import unittest
+from ubuntutools import subprocess
 
 WHITELIST = [re.compile(': %s$' % x) for x in (
     # Wildcard import:
@@ -28,6 +28,8 @@
     # mox:
     r"Instance of '.+' has no '(WithSideEffects|MultipleTimes|AndReturn)' "
     r"member",
+    # pylint doesn't like *args/**kwargs
+    r"Instance of 'Popen' has no '.*' member",
 )]
 
 class PylintTestCase(unittest.TestCase):