launchpad-reviewers team mailing list archive

[Jelmer Vernooij <jelmer@xxxxxxxxx>]

Jelmer Vernooij has proposed merging lp:~jelmer/launchpad/637507-subprocess-sigpipe into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
  #637507 Uploads with extra data in the .tar.gz rejected unnecessarily
  https://bugs.launchpad.net/bugs/637507


Reset the SIGPIPE handler before calling external processes. Some processes rely on this behaviour and it was breaking our use of "dpkg-source -x".

I've added a convenience function for calling dpkg-source -x, as there were three places where it was being used.
-- 
https://code.launchpad.net/~jelmer/launchpad/637507-subprocess-sigpipe/+merge/37940
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jelmer/launchpad/637507-subprocess-sigpipe into lp:launchpad/devel.

=== modified file 'lib/lp/archiveuploader/dscfile.py'
--- lib/lp/archiveuploader/dscfile.py	2010-10-03 15:30:06 +0000
+++ lib/lp/archiveuploader/dscfile.py	2010-10-08 09:36:50 +0000
@@ -24,7 +24,6 @@
 import glob
 import os
 import shutil
-import subprocess
 import tempfile
 
 from zope.component import getUtility
@@ -49,20 +48,17 @@
     TagFileParseError,
     )
 from lp.archiveuploader.utils import (
+    DpkgSourceError,
     determine_source_file_type,
+    extract_dpkg_source,
     get_source_file_extension,
     ParseMaintError,
-    prefix_multi_line_string,
     re_is_component_orig_tar_ext,
     re_issource,
     re_valid_pkg_name,
     re_valid_version,
     safe_fix_maintainer,
     )
-from lp.buildmaster.enums import BuildStatus
-from lp.code.interfaces.sourcepackagerecipebuild import (
-    ISourcePackageRecipeBuildSource,
-    )
 from lp.registry.interfaces.person import (
     IPersonSet,
     PersonCreationRationale,
@@ -77,17 +73,6 @@
     )
 
 
-class DpkgSourceError(Exception):
-
-    _fmt = "Unable to unpack source package (%(result)s): %(output)s"
-
-    def __init__(self, output, result):
-        Exception.__init__(
-            self, self._fmt % {"output": output, "result": result})
-        self.output = output
-        self.result = result
-
-
 def unpack_source(dsc_filepath):
     """Unpack a source package into a temporary directory
 
@@ -97,24 +82,7 @@
     # Get a temporary dir together.
     unpacked_dir = tempfile.mkdtemp()
     try:
-        # chdir into it
-        cwd = os.getcwd()
-        os.chdir(unpacked_dir)
-        try:
-            args = ["dpkg-source", "-sn", "-x", dsc_filepath]
-            dpkg_source = subprocess.Popen(args, stdout=subprocess.PIPE,
-                                           stderr=subprocess.PIPE)
-            output, unused = dpkg_source.communicate()
-            result = dpkg_source.wait()
-        finally:
-            # When all is said and done, chdir out again so that we can
-            # clean up the tree with shutil.rmtree without leaving the
-            # process in a directory we're trying to remove.
-            os.chdir(cwd)
-
-        if result != 0:
-            dpkg_output = prefix_multi_line_string(output, "  ")
-            raise DpkgSourceError(result=result, output=dpkg_output)
+        extract_dpkg_source(dsc_filepath, unpacked_dir)
     except:
         shutil.rmtree(unpacked_dir)
         raise

=== modified file 'lib/lp/archiveuploader/tests/test_utils.py'
--- lib/lp/archiveuploader/tests/test_utils.py	2010-08-27 12:21:25 +0000
+++ lib/lp/archiveuploader/tests/test_utils.py	2010-10-08 09:36:50 +0000
@@ -5,17 +5,20 @@
 
 # arch-tag: 90e6eb79-83a2-47e8-9f8b-3c687079c923
 
-from testtools import TestCase
+import os
 
 from lp.archiveuploader.tests import datadir
 from lp.archiveuploader.utils import (
+    DpkgSourceError,
     determine_binary_file_type,
     determine_source_file_type,
+    extract_dpkg_source,
     re_isadeb,
     re_issource,
     )
 from lp.registry.interfaces.sourcepackage import SourcePackageFileType
 from lp.soyuz.enums import BinaryPackageFileType
+from lp.testing import TestCase
 
 
 class TestUtilities(TestCase):
@@ -254,3 +257,25 @@
 
         # bzip2 compression for files which must be gzipped is invalid.
         self.assertIs(None, re_issource.match('foo-bar_1.0.diff.bz2'))
+
+
+class DdpkgExtractSourceTests(TestCase):
+    """Tests for dpkg_extract_source."""
+
+    def test_simple(self):
+        # unpack_source unpacks in a temporary directory and returns the
+        # path.
+        temp_dir = self.makeTemporaryDirectory()
+        extract_dpkg_source(
+            datadir(os.path.join('suite', 'bar_1.0-1', 'bar_1.0-1.dsc')),
+            temp_dir)
+        self.assertEquals(["bar-1.0"], os.listdir(temp_dir))
+        self.assertContentEqual(
+            ["THIS_IS_BAR", "debian"],
+            os.listdir(os.path.join(temp_dir, "bar-1.0")))
+
+    def test_nonexistant(self):
+        temp_dir = self.makeTemporaryDirectory()
+        err = self.assertRaises(
+            DpkgSourceError, extract_dpkg_source, "thispathdoesntexist", temp_dir)
+        self.assertEquals(2, err.result)

=== modified file 'lib/lp/archiveuploader/utils.py'
--- lib/lp/archiveuploader/utils.py	2010-08-26 17:36:08 +0000
+++ lib/lp/archiveuploader/utils.py	2010-10-08 09:36:50 +0000
@@ -6,6 +6,8 @@
 __metaclass__ = type
 
 __all__ = [
+    'DpkgSourceError',
+    'extract_dpkg_source',
     're_taint_free',
     're_isadeb',
     're_issource',
@@ -27,15 +29,28 @@
 
 import email.Header
 import re
+import signal
+import subprocess
 
 from canonical.encoding import (
     ascii_smash,
     guess as guess_encoding,
     )
-from lp.archiveuploader.tagfiles import TagFileParseError
 from lp.soyuz.enums import BinaryPackageFileType
 
 
+class DpkgSourceError(Exception):
+
+    _fmt = "Unable to unpack source package (%(result)s): %(output)s"
+
+    def __init__(self, command, output, result):
+        Exception.__init__(
+            self, self._fmt % {"output": output, "result": result, "command": command})
+        self.output = output
+        self.result = result
+        self.command = command
+
+
 re_taint_free = re.compile(r"^[-+~/\.\w]+$")
 
 re_isadeb = re.compile(r"(.+?)_(.+?)_(.+)\.(u?d?deb)$")
@@ -249,3 +264,24 @@
     content = ascii_smash(content)
 
     return fix_maintainer(content, fieldname)
+
+
+def extract_dpkg_source(dsc_filepath, target):
+    """Extract a source package by dsc file path.
+
+    :param dsc_filepath: Path of the DSC file
+    :param target: Target directory
+    """
+    def subprocess_setup():
+        # Python installs a SIGPIPE handler by default. This is usually not what
+        # non-Python subprocesses expect.
+        signal.signal(signal.SIGPIPE, signal.SIG_DFL)
+    args = ["dpkg-source", "-sn", "-x", dsc_filepath]
+    dpkg_source = subprocess.Popen(
+        args, stdout=subprocess.PIPE, cwd=target, stderr=subprocess.PIPE,
+        preexec_fn=subprocess_setup)
+    output, unused = dpkg_source.communicate()
+    result = dpkg_source.wait()
+    if result != 0:
+        dpkg_output = prefix_multi_line_string(output, "  ")
+        raise DpkgSourceError(result=result, output=dpkg_output, command=args)

=== modified file 'lib/lp/soyuz/scripts/gina/packages.py'
--- lib/lp/soyuz/scripts/gina/packages.py	2010-08-25 11:00:07 +0000
+++ lib/lp/soyuz/scripts/gina/packages.py	2010-10-08 09:36:50 +0000
@@ -34,6 +34,7 @@
 from canonical.launchpad.scripts import log
 from canonical.launchpad.validators.version import valid_debian_version
 from lp.archivepublisher.diskpool import poolify
+from lp.archiveuploader.utils import extract_dpkg_source
 from lp.registry.interfaces.gpg import GPGKeyAlgorithm
 from lp.registry.interfaces.sourcepackage import SourcePackageUrgency
 from lp.soyuz.enums import PackagePublishingPriority
@@ -104,7 +105,7 @@
 def unpack_dsc(package, version, component, archive_root):
     dsc_name, dsc_path, component = get_dsc_path(package, version,
                                                  component, archive_root)
-    call("dpkg-source -sn -x %s" % dsc_path)
+    extract_dpkg_source(dsc_path, ".")
 
     version = re.sub("^\d+:", "", version)
     version = re.sub("-[^-]+$", "", version)

=== modified file 'scripts/ftpmaster-tools/sync-source.py'
--- scripts/ftpmaster-tools/sync-source.py	2010-10-03 15:30:06 +0000
+++ scripts/ftpmaster-tools/sync-source.py	2010-10-08 09:36:50 +0000
@@ -247,22 +247,19 @@
 
 
 def extract_source(dsc_filename):
+    oldcwd = os.getcwd()
     # Create and move into a temporary directory
     tmpdir = tempfile.mktemp()
-    os.mkdir(tmpdir)
-    old_cwd = os.getcwd()
-    os.chdir(tmpdir)
 
     # Extract the source package
-    cmd = "dpkg-source -sn -x %s" % (dsc_filename)
-    (result, output) = commands.getstatusoutput(cmd)
-    if (result != 0):
-        print " * command was '%s'" % (cmd)
-        print dak_utils.prefix_multi_line_string(
-            output, " [dpkg-source output:] "), ""
+    try:
+        extract_dpkg_source(dsc_filename, tmpdir)
+    except DpkgSourceError, e:
+        print " * command was '%s'" % (e.command)
+        print e.output
         dak_utils.fubar(
             "'dpkg-source -x' failed for %s [return code: %s]." %
-            (dsc_filename, result))
+            (dsc_filename, e.result))
 
     return (old_cwd, tmpdir)