launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/archiveuploader-binary-error into lp:launchpad.

Commit message:
Produce a more comprehensible upload error if apt_inst.DebFile raises SystemError.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/archiveuploader-binary-error/+merge/247872

A recent apt upgrade in production brought it out of sync with python-apt and exposed a bug in archiveuploader's error handling: it tries to wrap a SystemError directly in an UploadError, but it needs to stringify it first.  The result is visible here and is very confusing:

  https://answers.launchpad.net/launchpad/+question/260244
  https://lists.launchpad.net/launchpad-users/msg06744.html

This branch doesn't fix the underlying problem with python-apt, but it does at least remove one level of debugging work from figuring out what the error message means.  (Given that the error now happens to be "E:Could not open file descriptor -1, E:Could not open file descriptor -1", that's not much of a user-facing improvement in this case, but it might help with other SystemError exceptions from apt_inst.DebFile in future.)
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/archiveuploader-binary-error into lp:launchpad.

=== modified file 'lib/lp/archiveuploader/nascentuploadfile.py'
--- lib/lp/archiveuploader/nascentuploadfile.py	2014-11-05 11:20:23 +0000
+++ lib/lp/archiveuploader/nascentuploadfile.py	2015-01-28 17:38:58 +0000
@@ -742,7 +742,8 @@
             # We get an error from the constructor if the .deb does not
             # contain all the expected top-level members (debian-binary,
             # control.tar.gz, and data.tar.*).
-            yield UploadError(error)
+            yield UploadError(str(error))
+            return
         try:
             deb_file.control.go(tar_checker.callback)
             deb_file.data.go(tar_checker.callback)

=== modified file 'lib/lp/archiveuploader/tests/nascentuploadfile.txt'
--- lib/lp/archiveuploader/tests/nascentuploadfile.txt	2013-06-20 07:31:01 +0000
+++ lib/lp/archiveuploader/tests/nascentuploadfile.txt	2015-01-28 17:38:58 +0000
@@ -523,3 +523,21 @@
     [UploadError('ed_0.2-20_i386.deb:
     has 26 file(s) with a time stamp too far in the past
     (e.g. control [Thu Jan  3 19:29:01 2008]).',)]
+
+And we get a reasonable error if we provoke a SystemError from
+apt_inst.DebFile.
+
+    >>> import tempfile
+    >>> temp_path = tempfile.mkdtemp('nascentuploadfile')
+    >>> empty_ar_path = os.path.join(temp_path, 'empty_0.1_all.deb')
+    >>> with open(empty_ar_path, 'w') as f:
+    ...     f.write("!<arch>\n")
+
+    >>> empty_ar = DebBinaryUploadFile(
+    ...     empty_ar_path, dict(MD5='d41d8cd98f00b204e9800998ecf8427e'), 0,
+    ...     'main/admin', 'extra', 'foo', '1.2', ed_binary_changes,
+    ...     modified_insecure_policy, DevNullLogger())
+    >>> list(empty_ar.verifyDebTimestamp())
+    [UploadError('No debian archive, missing control.tar.gz',)]
+
+    >>> shutil.rmtree(temp_path)