launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/deal-with-badly-formed-section into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #797678 in Launchpad itself: "Recipe build upload fails"
  https://bugs.launchpad.net/launchpad/+bug/797678

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/deal-with-badly-formed-section/+merge/125908

Deal with an inability to parse the file lines from the changes file. This also means we can't give any context to the user since it's far too early. Given the fields present it is only going to be component, section that are going to trip this up.

dpkg-genchanges also has a bug due to this due to accepting a Section that has a space in it.

I have tried to get this branch loc negative, but will take the ten line hit, I've clawed some of it back.
-- 
https://code.launchpad.net/~stevenk/launchpad/deal-with-badly-formed-section/+merge/125908
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/deal-with-badly-formed-section into lp:launchpad.

=== modified file 'lib/lp/archiveuploader/changesfile.py'
--- lib/lp/archiveuploader/changesfile.py	2012-06-29 08:40:05 +0000
+++ lib/lp/archiveuploader/changesfile.py	2012-09-24 01:16:20 +0000
@@ -177,8 +177,12 @@
         for fileline in self._dict['Files'].strip().split("\n"):
             # files lines from a changes file are always of the form:
             # CHECKSUM SIZE [COMPONENT/]SECTION PRIORITY FILENAME
-            digest, size, component_and_section, priority_name, filename = (
-                fileline.strip().split())
+            try:
+                digest, size, component_and_section, priority_name, filename = (
+                    fileline.strip().split())
+            except ValueError as e:
+                yield UploadError("Unable to parse file line, check section.")
+                continue
             filepath = os.path.join(self.dirname, filename)
             try:
                 if self.isCustom(component_and_section):

=== modified file 'lib/lp/archiveuploader/nascentuploadfile.py'
--- lib/lp/archiveuploader/nascentuploadfile.py	2012-07-24 06:39:54 +0000
+++ lib/lp/archiveuploader/nascentuploadfile.py	2012-09-24 01:16:20 +0000
@@ -335,9 +335,9 @@
         SourcePackageRelease creation, so component and section need to exist.
         Even if they might be overridden in the future.
         """
-        NascentUploadFile.__init__(
-            self, filepath, digest, size, component_and_section,
-            priority_name, policy, logger)
+        super(PackageUploadFile, self).__init__(
+            filepath, digest, size, component_and_section, priority_name,
+            policy, logger)
         self.package = package
         self.version = version
         self.changes = changes

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2012-07-03 08:04:35 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2012-09-24 01:16:20 +0000
@@ -221,9 +221,9 @@
     def assertLogContains(self, line):
         """Assert if a given line is present in the log messages."""
         log_lines = self.log.getLogBuffer()
-        self.assertTrue(line in log_lines,
-                        "'%s' is not in logged output\n\n%s"
-                        % (line, log_lines))
+        self.assertTrue(
+            line in log_lines, "'%s' is not in logged output\n\n%s" % (
+                line, log_lines))
 
     def assertRaisesAndReturnError(self, excClass, callableObj, *args,
                                    **kwargs):
@@ -1264,29 +1264,39 @@
         self.layer.txn.commit()
         self._uploadPartnerToNonReleasePocketAndCheckFail()
 
+    def assertRejectionMessage(self, uploadprocessor, msg, with_file=True):
+        expected = ''
+        for part in ('-1.dsc', '.orig.tar.gz', '-1.diff.gz'):
+            if with_file:
+                expected += 'bar_1.0%s: %s\n' % (part, msg)
+            else:
+                expected += '%s\n' % msg
+        expected += ('Further error processing not possible because of a '
+            'critical previous error.')
+        self.assertEqual(
+            expected, uploadprocessor.last_processed_upload.rejection_message)
+
     def testUploadWithUnknownSectionIsRejected(self):
         uploadprocessor = self.setupBreezyAndGetUploadProcessor()
         upload_dir = self.queueUpload("bar_1.0-1_bad_section")
         self.processUpload(uploadprocessor, upload_dir)
-        self.assertEqual(
-            uploadprocessor.last_processed_upload.rejection_message,
-            "bar_1.0-1.dsc: Unknown section 'badsection'\n"
-            "bar_1.0.orig.tar.gz: Unknown section 'badsection'\n"
-            "bar_1.0-1.diff.gz: Unknown section 'badsection'\n"
-            "Further error processing not possible because of a "
-            "critical previous error.")
+        self.assertRejectionMessage(
+            uploadprocessor, "Unknown section 'badsection'")
+
+    def testUploadWithMalformedSectionIsRejected(self):
+        uploadprocessor = self.setupBreezyAndGetUploadProcessor()
+        upload_dir = self.queueUpload("bar_1.0-1_malformed_section")
+        self.processUpload(uploadprocessor, upload_dir)
+        self.assertRejectionMessage(
+            uploadprocessor, 'Unable to parse file line, check section.',
+            with_file=False)
 
     def testUploadWithUnknownComponentIsRejected(self):
         uploadprocessor = self.setupBreezyAndGetUploadProcessor()
         upload_dir = self.queueUpload("bar_1.0-1_contrib_component")
         self.processUpload(uploadprocessor, upload_dir)
-        self.assertEqual(
-            uploadprocessor.last_processed_upload.rejection_message,
-            "bar_1.0-1.dsc: Unknown component 'contrib'\n"
-            "bar_1.0.orig.tar.gz: Unknown component 'contrib'\n"
-            "bar_1.0-1.diff.gz: Unknown component 'contrib'\n"
-            "Further error processing not possible because of a "
-            "critical previous error.")
+        self.assertRejectionMessage(
+            uploadprocessor, "Unknown component 'contrib'")
 
     def testSourceUploadToBuilddPath(self):
         """Source uploads to buildd upload paths are not permitted."""
@@ -1340,8 +1350,6 @@
         # The component contrib does not exist in the sample data, so
         # add it here.
         Component(name='contrib')
-
-        # Test it.
         self.checkComponentOverride(
             "bar_1.0-1_contrib_component", "multiverse")
 
@@ -1350,8 +1358,6 @@
         # The component non-free does not exist in the sample data, so
         # add it here.
         Component(name='non-free')
-
-        # Test it.
         self.checkComponentOverride(
             "bar_1.0-1_nonfree_component", "multiverse")
 
@@ -1902,7 +1908,7 @@
         should both have the PGP signature removed.
         """
         uploadprocessor = self.setupBreezyAndGetUploadProcessor(
-        policy='insecure')
+            policy='insecure')
         upload_dir = self.queueUpload("bar_1.0-1")
         self.processUpload(uploadprocessor, upload_dir)
         # ACCEPT the upload