launchpad-reviewers team mailing list archive

[Jelmer Vernooij <jelmer@xxxxxxxxx>]

Jelmer Vernooij has proposed merging lp:~jelmer/launchpad/production-cp-635591 into lp:~launchpad-pqm/launchpad/production-devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers): release-critical


In sync-source, fix the handling of changelog entries with UTF8-valid but non-ASCII characters. 

These changes have landed on devel in r11559 (MP at https://code.edge.launchpad.net/~jelmer/launchpad/635591-sync-source-unicode/+merge/35637
); a follow-up branch to deal with an issue I found during QA is in ec2 at the moment waiting to land on devel (MP here:
https://code.edge.launchpad.net/~jelmer/launchpad/635591-sync-source-unicode/+merge/35715)

I have already QA'ed the changes in this branch on dogfood. 

tests: ./bin/test lp.soyuz.scripts.tests.test_sync_source
-- 
https://code.launchpad.net/~jelmer/launchpad/production-cp-635591/+merge/35748
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jelmer/launchpad/production-cp-635591 into lp:~launchpad-pqm/launchpad/production-devel.

=== modified file 'lib/lp/soyuz/scripts/ftpmaster.py'
--- lib/lp/soyuz/scripts/ftpmaster.py	2010-08-24 15:29:01 +0000
+++ lib/lp/soyuz/scripts/ftpmaster.py	2010-09-16 21:51:24 +0000
@@ -20,11 +20,13 @@
     ]
 
 import commands
+from debian.deb822 import Changes
 import hashlib
 import os
 import stat
 import sys
 import tempfile
+import time
 
 import apt_pkg
 from zope.component import getUtility
@@ -470,7 +472,7 @@
                     pass
                 else:
                     version = bpph.binarypackagerelease.version
-                    self.logger.info ("Removed %s_%s from %s/%s ... "
+                    self.logger.info("Removed %s_%s from %s/%s ... "
                                       % (package, version,
                                          self.distroseries.name,
                                          distroarchseries.architecturetag))
@@ -1416,3 +1418,58 @@
             # Collect extra debug messages from chroot_manager.
             for debug_message in chroot_manager._messages:
                 self.logger.debug(debug_message)
+
+
+def generate_changes(dsc, dsc_files, suite, changelog, urgency, closes,
+                     lp_closes, section, priority, description,
+                     files_from_librarian, requested_by, origin):
+    """Generate a Changes object.
+
+    :param dsc: A `Dsc` instance for the related source package.
+    :param suite: Distribution name
+    :param changelog: Relevant changelog data
+    :param urgency: Urgency string (low, medium, high, etc)
+    :param closes: Sequence of Debian bug numbers (as strings) fixed by
+        this upload.
+    :param section: Debian section
+    :param priority: Package priority
+    """
+
+    # XXX cprov 2007-07-03:
+    # Changed-By can be extracted from most-recent changelog footer,
+    # but do we care?
+    # XXX James Troup 2006-01-30:
+    # 'Closes' but could be gotten from changelog, but we don't use them?
+
+    changes = Changes()
+    changes["Origin"] = "%s/%s" % (origin["name"], origin["suite"])
+    changes["Format"] = "1.7"
+    changes["Date"] = time.strftime("%a,  %d %b %Y %H:%M:%S %z")
+    changes["Source"] = dsc["source"]
+    changes["Binary"] = dsc["binary"]
+    changes["Architecture"] = "source"
+    changes["Version"] = dsc["version"]
+    changes["Distribution"] = suite
+    changes["Urgency"] = urgency
+    changes["Maintainer"] = dsc["maintainer"]
+    changes["Changed-By"] = requested_by
+    if description:
+        changes["Description"] = "\n %s" % description
+    if closes:
+        changes["Closes"] = " ".join(closes)
+    if lp_closes:
+        changes["Launchpad-bugs-fixed"] = " ".join(lp_closes)
+    files = []
+    for filename in dsc_files:
+        if filename in files_from_librarian:
+            continue
+        files.append({"md5sum": dsc_files[filename]["md5sum"],
+                      "size": dsc_files[filename]["size"],
+                      "section": section,
+                      "priority": priority,
+                      "name": filename,
+                     })
+
+    changes["Files"] = files
+    changes["Changes"] = "\n%s" % changelog
+    return changes

=== modified file 'lib/lp/soyuz/scripts/tests/test_sync_source.py'
--- lib/lp/soyuz/scripts/tests/test_sync_source.py	2010-08-27 12:21:25 +0000
+++ lib/lp/soyuz/scripts/tests/test_sync_source.py	2010-09-16 21:51:24 +0000
@@ -1,17 +1,22 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """SyncSource facilities tests."""
 
 __metaclass__ = type
 
+from cStringIO import StringIO
+from debian.deb822 import (
+    Changes,
+    Deb822Dict,
+    Dsc,
+    )
 import os
 import shutil
 import subprocess
 import sys
 import tempfile
 from unittest import (
-    TestCase,
     TestLoader,
     )
 
@@ -29,6 +34,10 @@
 from lp.soyuz.scripts.ftpmaster import (
     SyncSource,
     SyncSourceError,
+    generate_changes,
+    )
+from lp.testing import (
+    TestCase,
     )
 
 
@@ -42,6 +51,7 @@
         Setup and chdir into a temp directory, a jail, where we can
         control the file creation properly
         """
+        super(TestSyncSource, self).setUp()
         fillLibrarianFile(1, content='one')
         fillLibrarianFile(2, content='two')
         fillLibrarianFile(54, content='fifty-four')
@@ -57,6 +67,7 @@
         chdir back to the previous path (home) and remove the temp
         directory used as jail.
         """
+        super(TestSyncSource, self).tearDown()
         os.chdir(self._home)
         cleanupLibrarianFiles()
         shutil.rmtree(self._jail)
@@ -247,6 +258,7 @@
     dbuser = 'ro'
 
     def setUp(self):
+        super(TestSyncSourceScript, self).setUp()
         self._home = os.getcwd()
         self._jail = os.path.join(
             os.path.dirname(__file__), 'sync_source_home')
@@ -254,6 +266,7 @@
 
     def tearDown(self):
         """'chdir' back to the previous path (home)."""
+        super(TestSyncSourceScript, self).tearDown()
         os.chdir(self._home)
 
     def runSyncSource(self, extra_args=None):
@@ -416,5 +429,109 @@
         os.unlink(expected_changesfile)
 
 
+class TestGenerateChanges(TestCase):
+    """Test generate_changes()."""
+
+    def getBaseDsc(self):
+        """Create a basic Dsc object for use with generate_changes()."""
+        dsc = Dsc()
+        dsc["source"] = "mysrcpkg"
+        dsc["binary"] = "mybinpkg"
+        dsc["version"] = "4.2"
+        dsc["maintainer"] = "Maintainer <maintainer@xxxxxxxxxx>"
+        return dsc
+
+    def getBaseOrigin(self):
+        """Create a basic Origin dict for use with generate_changes()."""
+        origin = Deb822Dict()
+        origin["Name"] = "Debian"
+        origin["Suite"] = "sid"
+        return origin
+
+    def generateChanges(self, dsc=None, dsc_files=None, suite="maverick",
+                        changelog=None, urgency="low", closes=None,
+                        lp_closes=None, section="net", priority="extra",
+                        description=None, files_from_librarian=[],
+                        requested_by="Somebody <somebody@xxxxxxxxxx>",
+                        origin=None):
+        if dsc is None:
+            dsc = self.getBaseDsc()
+        if dsc_files is None:
+            dsc_files = []
+        if origin is None:
+            origin = self.getBaseOrigin()
+        if changelog is None:
+            changelog = 'changelog entry'
+        return generate_changes(
+            dsc=dsc, dsc_files=dsc_files, suite=suite, changelog=changelog,
+            urgency=urgency, closes=closes, lp_closes=lp_closes,
+            section=section, priority=priority, description=description,
+            files_from_librarian=files_from_librarian,
+            requested_by=requested_by, origin=origin)
+
+    def test_minimum_fields(self):
+        # The right (minimum) set of fields are set by generate_changes().
+        changes = self.generateChanges()
+        self.assertEquals("1.7", changes["Format"])
+        self.assertEquals("mysrcpkg", changes["Source"])
+        self.assertEquals("mybinpkg", changes["Binary"])
+        self.assertEquals("source", changes["Architecture"])
+        self.assertEquals("4.2", changes["Version"])
+        self.assertEquals("maverick", changes["Distribution"])
+        self.assertEquals("low", changes["Urgency"])
+        self.assertEquals("\nchangelog entry", changes["Changes"])
+        self.assertEquals(
+            "Maintainer <maintainer@xxxxxxxxxx>", changes["Maintainer"])
+        self.assertNotIn("Description", changes)
+        self.assertNotIn("Closes", changes)
+        self.assertNotIn("Launchpad-bugs-fixed", changes)
+        self.assertEquals([], changes["Files"])
+
+    def test_closes(self):
+        # Closes gets set if any Debian bugs to close were specified.
+        changes = self.generateChanges(closes=["1232", "4323"])
+        self.assertEquals("1232 4323", changes["Closes"])
+        self.assertNotIn("Launchpad-bugs-fixed", changes)
+
+    def test_lp_closes(self):
+        # Launchpad-Bugs-Fixed gets set if any Launchpad bugs to close were
+        # specified.
+        changes = self.generateChanges(lp_closes=["987987"])
+        self.assertEquals("987987", changes["Launchpad-Bugs-Fixed"])
+
+    def test_utf8_changelog(self):
+        # A changelog entry with non-ASCII UTF-8 characters is serialized in
+        # Changes properly.
+        changes = self.generateChanges(
+            changelog="* Updated French translation by J\xc3\xa9lmer.")
+        contents = changes.dump(encoding="utf-8").encode("utf-8")
+        self.assertIn(
+            "Updated French translation by J\xc3\xa9lmer.", contents)
+
+    def test_changelog_whitelines(self):
+        # The changelog entry can contain empty lines, and this should not
+        # mess up the parsing of the changes file.
+        changelog = "* Foo\n\n\n* Bar\n.\nEntries"
+        changes = self.generateChanges(changelog=changelog)
+        contents = changes.dump(encoding="utf-8").encode("utf-8")
+        # Read contents back
+        read_changes = Changes(contents)
+        self.assertEquals("\n%s" % changelog, changes['Changes'])
+        self.assertContentEqual([
+            'Architecture',
+            'Binary',
+            'Changed-By',
+            'Changes',
+            'Date',
+            'Distribution',
+            'Files',
+            'Format',
+            'Maintainer',
+            'Origin',
+            'Source',
+            'Urgency',
+            'Version',
+            ], read_changes.keys())
+
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)

=== modified file 'scripts/ftpmaster-tools/sync-source.py'
--- scripts/ftpmaster-tools/sync-source.py	2010-08-27 12:21:25 +0000
+++ scripts/ftpmaster-tools/sync-source.py	2010-09-16 21:51:24 +0000
@@ -36,7 +36,6 @@
 import stat
 import string
 import tempfile
-import time
 import urllib
 
 import dak_utils
@@ -45,16 +44,28 @@
 from zope.component import getUtility
 from contrib.glock import GlobalLock
 
-from canonical.database.sqlbase import sqlvalues, cursor
+from canonical.database.sqlbase import (
+    cursor,
+    sqlvalues,
+    )
 from canonical.launchpad.interfaces import (
-    IDistributionSet, IPersonSet)
+    IDistributionSet,
+    IPersonSet,
+    )
 from canonical.launchpad.scripts import (
-    execute_zcml_for_scripts, logger, logger_options)
+    execute_zcml_for_scripts,
+    logger,
+    logger_options,
+    )
 from canonical.librarian.client import LibrarianClient
 from canonical.lp import initZopeless
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.soyuz.enums import PackagePublishingStatus
-from lp.soyuz.scripts.ftpmaster import SyncSource, SyncSourceError
+from lp.soyuz.scripts.ftpmaster import (
+    generate_changes,
+    SyncSource,
+    SyncSourceError,
+    )
 
 
 reject_message = ""
@@ -139,53 +150,6 @@
     os.unlink(temp_filename)
 
 
-def generate_changes(dsc, dsc_files, suite, changelog, urgency, closes,
-                     lp_closes, section, priority, description,
-                     files_from_librarian, requested_by, origin):
-    """Generate a .changes as a string"""
-
-    # XXX cprov 2007-07-03:
-    # Changed-By can be extracted from most-recent changelog footer,
-    # but do we care?
-    # XXX James Troup 2006-01-30:
-    # 'Closes' but could be gotten from changelog, but we don't use them?
-
-    changes = ""
-    changes += "Origin: %s/%s\n" % (origin["name"], origin["suite"])
-    changes += "Format: 1.7\n"
-    changes += "Date: %s\n" % (time.strftime("%a,  %d %b %Y %H:%M:%S %z"))
-    changes += "Source: %s\n" % (dsc["source"])
-    changes += "Binary: %s\n" % (dsc["binary"])
-    changes += "Architecture: source\n"
-    changes += "Version: %s\n"% (dsc["version"])
-    # XXX: James Troup 2006-01-30:
-    # 'suite' forced to string to avoid unicode-vs-str grudge match
-    changes += "Distribution: %s\n" % (str(suite))
-    changes += "Urgency: %s\n" % (urgency)
-    changes += "Maintainer: %s\n" % (dsc["maintainer"])
-    changes += "Changed-By: %s\n" % (requested_by)
-    if description:
-        changes += "Description: \n"
-        changes += " %s\n" % (description)
-    if closes:
-        changes += "Closes: %s\n" % (" ".join(closes))
-    if lp_closes:
-        changes += "Launchpad-bugs-fixed: %s\n" % (" ".join(lp_closes))
-    changes += "Changes: \n"
-    changes += changelog
-    changes += "Files: \n"
-    for filename in dsc_files:
-        if filename in files_from_librarian:
-            continue
-        changes += " %s %s %s %s %s\n" % (dsc_files[filename]["md5sum"],
-                                          dsc_files[filename]["size"],
-                                          section, priority, filename)
-    # Strip trailing newline
-    changes = changes[:-1]
-
-    return changes
-
-
 def parse_changelog(changelog_filename, previous_version):
     if not os.path.exists(changelog_filename):
         dak_utils.fubar("debian/changelog not found in extracted source.")
@@ -500,9 +464,10 @@
         dsc["source"], dak_utils.re_no_epoch.sub('', dsc["version"]))
 
     filehandle = open(output_filename, 'w')
-    # XXX cprov 2007-07-03: The Soyuz .changes parser requires the extra '\n'
-    filehandle.write(changes+'\n')
-    filehandle.close()
+    try:
+        changes.dump(filehandle, encoding="utf-8")
+    finally:
+        filehandle.close()
 
 
 def read_current_source(distro_series, valid_component=None, arguments=None):