From d82e65e0e2becaa579efdc25cebfde8dd4672049 Mon Sep 17 00:00:00 2001 From: Evan Broder Date: Tue, 24 May 2011 20:22:37 +0200 Subject: [PATCH 1/2] * 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) --- 404main | 3 +- ack-sync | 2 +- backportpackage | 2 +- debian/changelog | 9 +- dgetlp | 3 +- get-branches | 2 +- import-bug-from-debian | 2 +- lp-project-upload | 3 +- pbuilder-dist | 2 +- syncpackage | 2 +- ubuntu-iso | 3 +- ubuntutools/archive.py | 2 +- ubuntutools/builder.py | 2 +- ubuntutools/misc.py | 2 +- ubuntutools/requestsync/common.py | 3 +- ubuntutools/requestsync/mail.py | 2 +- ubuntutools/sponsor_patch/patch.py | 3 +- ubuntutools/sponsor_patch/sponsor_patch.py | 2 +- ubuntutools/subprocess.py | 108 +++++++++++++++++++++ ubuntutools/test/test_help.py | 2 +- ubuntutools/test/test_pylint.py | 4 +- 21 files changed, 143 insertions(+), 20 deletions(-) create mode 100644 ubuntutools/subprocess.py diff --git a/404main b/404main index 152c975..bef3227 100755 --- a/404main +++ b/404main @@ -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.""" diff --git a/ack-sync b/ack-sync index faafdc7..b267806 100755 --- a/ack-sync +++ b/ack-sync @@ -22,7 +22,6 @@ import getopt import lazr.restfulclient import os import re -import subprocess import sys import logging import glob @@ -31,6 +30,7 @@ import fnmatch from launchpadlib.launchpad import Launchpad from ubuntutools.config import UDTConfig +from ubuntutools import subprocess COMMAND_LINE_SYNTAX_ERROR = 1 VERSION_DETECTION_FAILED = 2 diff --git a/backportpackage b/backportpackage index 8f68ee5..de4be77 100755 --- a/backportpackage +++ b/backportpackage @@ -21,7 +21,6 @@ import optparse import os import shutil -import subprocess import sys import tempfile @@ -34,6 +33,7 @@ from ubuntutools.archive import UbuntuSourcePackage, DownloadError 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) diff --git a/debian/changelog b/debian/changelog index 3742536..253c4d2 100644 --- a/debian/changelog +++ b/debian/changelog @@ -15,7 +15,14 @@ ubuntu-dev-tools (0.124) UNRELEASED; urgency=low * mk-sbuild: - maintainer_name isn't mandatory any more (LP: #787051) - -- Benjamin Drung Mon, 23 May 2011 23:38:51 +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 Tue, 24 May 2011 20:21:47 +0200 ubuntu-dev-tools (0.123) unstable; urgency=low diff --git a/dgetlp b/dgetlp index 3254e74..c6ded51 100755 --- a/dgetlp +++ b/dgetlp @@ -32,7 +32,6 @@ import email.feedparser import hashlib import optparse import os -import subprocess import sys import urllib2 @@ -43,6 +42,8 @@ except ImportError: "use this utility.") sys.exit(1) +from ubuntutools import subprocess + USAGE = u"""Usage: %prog [-d|(-v|-q)] This scripts simulates «dget»'s behaviour for files hosted at diff --git a/get-branches b/get-branches index df4cade..57a0e0b 100755 --- a/get-branches +++ b/get-branches @@ -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 ] -t [-o ]" diff --git a/import-bug-from-debian b/import-bug-from-debian index 0b01036..024ac72 100755 --- a/import-bug-from-debian +++ b/import-bug-from-debian @@ -25,7 +25,6 @@ from optparse import OptionParser, SUPPRESS_HELP import re import sys -import subprocess try: import SOAPpy @@ -37,6 +36,7 @@ except ImportError: from launchpadlib.launchpad import Launchpad from ubuntutools.config import UDTConfig +from ubuntutools import subprocess def main(): bug_re = re.compile(r"bug=(\d+)") diff --git a/lp-project-upload b/lp-project-upload index b7ccb86..851ab69 100755 --- a/lp-project-upload +++ b/lp-project-upload @@ -20,13 +20,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.''' diff --git a/pbuilder-dist b/pbuilder-dist index 990bce0..f80945a 100755 --- a/pbuilder-dist +++ b/pbuilder-dist @@ -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): diff --git a/syncpackage b/syncpackage index 8ec7fb6..a47a182 100755 --- a/syncpackage +++ b/syncpackage @@ -25,7 +25,6 @@ import debian.debian_support import optparse import os import shutil -import subprocess import sys from devscripts.logger import Logger @@ -38,6 +37,7 @@ from ubuntutools.requestsync.mail import (getDebianSrcPkg 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): diff --git a/ubuntu-iso b/ubuntu-iso index c93746e..5c1309d 100755 --- a/ubuntu-iso +++ b/ubuntu-iso @@ -21,7 +21,8 @@ # ################################################################## import sys -import subprocess + +from ubuntutools import subprocess def extract(iso, path): command = ['isoinfo', '-R', '-i', iso, '-x', path] diff --git a/ubuntutools/archive.py b/ubuntutools/archive.py index 8eb473d..cc91027 100644 --- a/ubuntutools/archive.py +++ b/ubuntutools/archive.py @@ -31,7 +31,6 @@ from __future__ import with_statement import hashlib import os.path -import subprocess import urllib2 import urlparse import re @@ -45,6 +44,7 @@ from devscripts.logger import Logger 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" diff --git a/ubuntutools/builder.py b/ubuntutools/builder.py index fc5795d..ef00e22 100644 --- a/ubuntutools/builder.py +++ b/ubuntutools/builder.py @@ -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""" diff --git a/ubuntutools/misc.py b/ubuntutools/misc.py index 55c6551..f8b4848 100644 --- a/ubuntutools/misc.py +++ b/ubuntutools/misc.py @@ -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(): diff --git a/ubuntutools/requestsync/common.py b/ubuntutools/requestsync/common.py index ec3cbce..e9f7c12 100644 --- a/ubuntutools/requestsync/common.py +++ b/ubuntutools/requestsync/common.py @@ -24,9 +24,10 @@ import sys 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 diff --git a/ubuntutools/requestsync/mail.py b/ubuntutools/requestsync/mail.py index 923907f..4fbb728 100644 --- a/ubuntutools/requestsync/mail.py +++ b/ubuntutools/requestsync/mail.py @@ -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__ = [ diff --git a/ubuntutools/sponsor_patch/patch.py b/ubuntutools/sponsor_patch/patch.py index 6d1c34a..12d3787 100644 --- a/ubuntutools/sponsor_patch/patch.py +++ b/ubuntutools/sponsor_patch/patch.py @@ -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): diff --git a/ubuntutools/sponsor_patch/sponsor_patch.py b/ubuntutools/sponsor_patch/sponsor_patch.py index 4c5626f..0f05e8d 100644 --- a/ubuntutools/sponsor_patch/sponsor_patch.py +++ b/ubuntutools/sponsor_patch/sponsor_patch.py @@ -19,7 +19,6 @@ import os import pwd import re import shutil -import subprocess import sys import debian.changelog @@ -28,6 +27,7 @@ import launchpadlib.launchpad from devscripts.logger import Logger +from ubuntutools import subprocess from ubuntutools.update_maintainer import update_maintainer from ubuntutools.question import Question, YesNoQuestion, input_number diff --git a/ubuntutools/subprocess.py b/ubuntutools/subprocess.py new file mode 100644 index 0000000..688d607 --- /dev/null +++ b/ubuntutools/subprocess.py @@ -0,0 +1,108 @@ +"""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'] + + +def Popen(*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 + + return subprocess.Popen(*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 diff --git a/ubuntutools/test/test_help.py b/ubuntutools/test/test_help.py index 221a84f..c3bb9c5 100644 --- a/ubuntutools/test/test_help.py +++ b/ubuntutools/test/test_help.py @@ -18,10 +18,10 @@ import fcntl import os import select import signal -import subprocess import time import setup +from ubuntutools import subprocess from ubuntutools.test import unittest BLACKLIST = { diff --git a/ubuntutools/test/test_pylint.py b/ubuntutools/test/test_pylint.py index 60b81d3..8a1c650 100644 --- a/ubuntutools/test/test_pylint.py +++ b/ubuntutools/test/test_pylint.py @@ -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 @@ WHITELIST = [re.compile(': %s$' % x) for x in ( # mox: r"Instance of '.+' has no '(WithSideEffects|MultipleTimes|AndReturn)' " r"member", + # Wrong Popen + r"Function 'Popen' has no '__init__' member", )] class PylintTestCase(unittest.TestCase): From 5c96b78d72022d193a47c645be2598418175959d Mon Sep 17 00:00:00 2001 From: Evan Broder Date: Sun, 19 Jun 2011 14:20:47 -0700 Subject: [PATCH 2/2] Reimplement ubuntutools.subprocess.Popen as a class instead of a factory function. --- ubuntutools/subprocess.py | 31 ++++++++++++++++--------------- ubuntutools/test/test_pylint.py | 4 ++-- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/ubuntutools/subprocess.py b/ubuntutools/subprocess.py index 688d607..2fc4b37 100644 --- a/ubuntutools/subprocess.py +++ b/ubuntutools/subprocess.py @@ -21,24 +21,25 @@ from subprocess import PIPE, STDOUT, CalledProcessError __all__ = ['Popen', 'call', 'check_call', 'check_output', 'CalledProcessError', 'PIPE', 'STDOUT'] -def Popen(*args, **kwargs): - kwargs.setdefault('close_fds', True) +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 '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 + if given_preexec_fn: + given_preexec_fn() + kwargs['preexec_fn'] = preexec_fn - return subprocess.Popen(*args, **kwargs) + subprocess.Popen.__init__(self, *args, **kwargs) # call, check_call, and check_output are copied directly from the diff --git a/ubuntutools/test/test_pylint.py b/ubuntutools/test/test_pylint.py index 8a1c650..7806abf 100644 --- a/ubuntutools/test/test_pylint.py +++ b/ubuntutools/test/test_pylint.py @@ -28,8 +28,8 @@ WHITELIST = [re.compile(': %s$' % x) for x in ( # mox: r"Instance of '.+' has no '(WithSideEffects|MultipleTimes|AndReturn)' " r"member", - # Wrong Popen - r"Function 'Popen' has no '__init__' member", + # pylint doesn't like *args/**kwargs + r"Instance of 'Popen' has no '.*' member", )] class PylintTestCase(unittest.TestCase):