launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/unicode-logtail into lp:launchpad.

Commit message:
Always decode logtail as UTF-8 rather than guessing.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1585324 in Launchpad itself: "garbled display in build log preview"
  https://bugs.launchpad.net/launchpad/+bug/1585324

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/unicode-logtail/+merge/296550

Always decode logtail as UTF-8 rather than guessing.  We serve completed build logs as UTF-8 as of r17490, so we might as well do the same thing with partial ones.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/unicode-logtail into lp:launchpad.

=== modified file 'lib/lp/buildmaster/interactor.py'
--- lib/lp/buildmaster/interactor.py	2016-05-26 13:45:24 +0000
+++ lib/lp/buildmaster/interactor.py	2016-06-06 12:57:06 +0000
@@ -42,7 +42,6 @@
 from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
     IBuildFarmJobBehaviour,
     )
-from lp.services import encoding
 from lp.services.config import config
 from lp.services.twistedsupport import cancel_on_timeout
 from lp.services.twistedsupport.processmonitor import ProcessWithTimeout
@@ -539,8 +538,8 @@
         builder_status = slave_status['builder_status']
         if builder_status == 'BuilderStatus.BUILDING':
             # Build still building, collect the logtail.
-            vitals.build_queue.logtail = encoding.guess(
-                str(slave_status.get('logtail')))
+            vitals.build_queue.logtail = str(
+                slave_status.get('logtail')).decode('UTF-8', errors='replace')
             transaction.commit()
         elif builder_status == 'BuilderStatus.ABORTING':
             # Build is being aborted.

=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2015-10-05 17:21:45 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2016-06-06 12:57:06 +0000
@@ -1,3 +1,7 @@
+# -*- coding: utf-8 -*-
+# NOTE: The first line above must stay first; do not move the copyright
+# notice to the top.  See http://www.python.org/dev/peps/pep-0263/.
+#
 # Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
@@ -230,7 +234,8 @@
         self.assertIs(None, builder.currentjob)
         self._checkJobRescued(slave, builder, job)
 
-    def _checkJobUpdated(self, slave, builder, job):
+    def _checkJobUpdated(self, slave, builder, job,
+                         logtail='This is a build log: 0'):
         """`SlaveScanner.scan` updates legitimate jobs.
 
         Job is kept assigned to the active builder and its 'logtail' is
@@ -242,7 +247,7 @@
         self.assertTrue(builder.builderok)
 
         job = getUtility(IBuildQueueSet).get(job.id)
-        self.assertBuildingJob(job, builder, logtail='This is a build log: 0')
+        self.assertBuildingJob(job, builder, logtail=logtail)
 
     def testScanUpdatesBuildingJobs(self):
         # Enable sampledata builder attached to an appropriate testing
@@ -312,6 +317,35 @@
         d = scanner.scan()
         return assert_fails_with(d, xmlrpclib.Fault)
 
+    def test_scan_of_partial_utf8_logtail(self):
+        # The builder returns a fixed number of bytes from the tail of the
+        # log, so the first character can easily end up being invalid UTF-8.
+        class BrokenUTF8Slave(BuildingSlave):
+            @defer.inlineCallbacks
+            def status(self):
+                status = yield super(BrokenUTF8Slave, self).status()
+                status["logtail"] = xmlrpclib.Binary(
+                    u"───".encode("UTF-8")[1:])
+                defer.returnValue(status)
+
+        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
+        login('foo.bar@xxxxxxxxxxxxx')
+        builder.builderok = True
+        self.patch(
+            BuilderSlave, 'makeBuilderSlave',
+            FakeMethod(BrokenUTF8Slave(build_id='PACKAGEBUILD-8')))
+        transaction.commit()
+        login(ANONYMOUS)
+
+        job = builder.currentjob
+        self.assertBuildingJob(job, builder)
+
+        switch_dbuser(config.builddmaster.dbuser)
+        scanner = self._getScanner()
+        d = defer.maybeDeferred(scanner.scan)
+        d.addCallback(
+            self._checkJobUpdated, builder, job, logtail=u"\uFFFD\uFFFD──")
+
     @defer.inlineCallbacks
     def test_scan_calls_builder_factory_prescanUpdate(self):
         # SlaveScanner.scan() starts by calling