Merge lp:~james-w/udd/unicode-everything into lp:udd

[James Westby]

Use unicode everywhere to make storm happy.

Hi,

Storm loves unicode. It really, really loves it. It loves it so much
that it won't let two strs be equal after round-tripping through the
database.

It's correct in a way, if a little annoying, so this branch changes
to use unicode rather than bytestrings where they may come in to
context with the database.

There are a lot of changes, but they are 99% boring.

There are three places that bytestrings might have come from:

  1. Literals. I changed most literals to be unicode. Exceptions were things
     related to urls, logging messages, and things that don't get anywhere
     near the database.

  2. Command line input. There are actually only a couple of places that
     a string supplied on the command line may hit the db (e.g. the
     package name passed to import-package). Coercing them to unicode was
     easy, and package names can only be ascii anyway, so we don't really
     have to worry about encodings there.

  3. External systems. launchpadlib happily returns unicode everywhere, so
     we are mostly safe from this, there were just a few extra spots to
     handle.

There are likely some places I missed, but this gets the core functionality
working again.

Thanks,

James

[lp:~james-w/udd/unicode-everything]

572. By James Westby on 2012-04-04

One more string literal. Thanks jml.

[Jonathan Lange (jml)]

Wow. Thanks. Makes it obvious how many duplicated string literals there are in the code base. As I mentioned on IRC, I think you missed converting one usage of 'release'

<jml> james_w: I think you missed one:
     except errors.NotBranchError:
 - ubuntu_b = bstore.get_branch_parts("ubuntu", ubuntu_current_series,
 + ubuntu_b = bstore.get_branch_parts(u"ubuntu", ubuntu_current_series,
                 "release", possible_transports=possible_transports)
 "release" → u"release"

Please fix that and land.

review: Approve

[Robert Collins (lifeless)]

A better way to do this might be barry's __future__ incantation, which
IIRC will make string literals unicode by default.IMBW.

-Rob

=== modified file 'selftest.py'
--- selftest.py	2012-03-23 16:56:38 +0000
+++ selftest.py	2012-04-04 17:35:29 +0000
@@ -47,6 +47,7 @@
                     'udd.tests.test_email_failures',
                     'udd.tests.test_graph_failures',
                     'udd.tests.test_helpers',
+                    'udd.tests.test_history_database',
                     'udd.tests.test_icommon',
                     'udd.tests.test_idb',
                     'udd.tests.test_import_list',

=== modified file 'udd/icommon.py'
--- udd/icommon.py	2012-03-23 16:56:38 +0000
+++ udd/icommon.py	2012-04-04 17:35:29 +0000
@@ -59,13 +59,13 @@
 # Distro releases, in chronological order, with continuing trunks like 'sid'
 # last, as used to set the order of branches in a DistributionBranchSet.
 db_set_distro_releases = {
-    "ubuntu": ["warty", "hoary", "breezy",
-               "dapper", "edgy", "feisty", "gutsy",
-               "hardy", "intrepid", "jaunty", "karmic",
-               "lucid", "maverick", "natty", "oneiric", "precise",],
-    "debian": ["woody", "sarge", "etch", "lenny",
-               "squeeze", "wheezy",
-               "sid", "experimental"],
+    u"ubuntu": [u"warty", u"hoary", u"breezy",
+               u"dapper", u"edgy", u"feisty", u"gutsy",
+               u"hardy", u"intrepid", u"jaunty", u"karmic",
+               u"lucid", u"maverick", u"natty", u"oneiric", u"precise",],
+    u"debian": [u"woody", u"sarge", u"etch", u"lenny",
+               u"squeeze", u"wheezy",
+               u"sid", u"experimental"],
 }
 
 # Distro releases, in an order such that whenever the _same_ version of a
@@ -78,47 +78,47 @@
 # and pulls from other branches when run from scratch as when run incrementally
 # after each publishing occurred.
 import_sequence_distro_releases = {
-    "ubuntu": ["warty", "hoary", "breezy",
-               "dapper", "edgy", "feisty", "gutsy",
-               "hardy", "intrepid", "jaunty", "karmic",
-               "lucid", "maverick", "natty", "oneiric", "precise",],
-    "debian": ["sid", "experimental",
-               "woody", "sarge", "etch", "lenny",
-               "squeeze", "wheezy"],
+    u"ubuntu": [u"warty", u"hoary", u"breezy",
+               u"dapper", u"edgy", u"feisty", u"gutsy",
+               u"hardy", u"intrepid", u"jaunty", u"karmic",
+               u"lucid", u"maverick", u"natty", u"oneiric", u"precise",],
+    u"debian": [u"sid", u"experimental",
+               u"woody", u"sarge", u"etch", u"lenny",
+               u"squeeze", u"wheezy"],
 }
 
 # Distro releases known to Launchpad, in the order they should be processed: do
 # the likely development focus first to fit better with stacking.  No etch or
 # earlier as they aren't on lp
 lp_distro_releases = {
-    "ubuntu": ["precise", "warty", "hoary", "breezy",
-               "dapper", "edgy", "feisty", "gutsy",
-               "hardy", "intrepid", "jaunty", "karmic",
-               "lucid", "maverick", "natty", "oneiric",],
-    "debian": ["sid", "experimental",
-               "wheezy", "squeeze", "lenny"],
+    u"ubuntu": [u"precise", u"warty", u"hoary", u"breezy",
+               u"dapper", u"edgy", u"feisty", u"gutsy",
+               u"hardy", u"intrepid", u"jaunty", u"karmic",
+               u"lucid", u"maverick", u"natty", u"oneiric",],
+    u"debian": [u"sid", u"experimental",
+               u"wheezy", u"squeeze", u"lenny"],
 }
 
 distro_pockets = {
-    "ubuntu": ["release", "security", "proposed", "updates", "backports"],
-    "debian": ["release",],
+    u"ubuntu": [u"release", u"security", u"proposed", u"updates", u"backports"],
+    u"debian": [u"release",],
 }
 
-default_debian_diff_release = "sid"
-default_ubuntu_merge_source = "squeeze"
+default_debian_diff_release = u"sid"
+default_ubuntu_merge_source = u"squeeze"
 
 
 def debian_base_url(release):
-    if release in ["hamm", "slink", "potato", "woody", "sarge", "etch"]:
+    if release in [u"hamm", u"slink", u"potato", u"woody", u"sarge", u"etch"]:
         return urllib_debian_archive_base_url
     return urllib_debian_base_url
 
 
 def make_suite(release, pocket):
-    if pocket == "release":
+    if pocket == u"release":
         suite = release
-    elif pocket in ("security", "proposed", "updates", "backports"):
-        suite = "%s-%s" % (release, pocket)
+    elif pocket in (u"security", u"proposed", u"updates", u"backports"):
+        suite = u"%s-%s" % (release, pocket)
     else:
         assert False, "Unknown pocket: %s" % pocket
     return suite
@@ -178,7 +178,7 @@
                     self.version.debian_version)
         else:
             version_str = self.version.upstream_version
-        if self.distro == "ubuntu":
+        if self.distro == u"ubuntu":
             files_url = (urllib_launchpad_base_url
                          + "ubuntu/%s/+source/%s/%s/+files/"
                          % (self.release, self.name, str(self.version)))
@@ -208,9 +208,9 @@
         # Core assumption: packages always have higher versions than their
         # ancestors... except for backports.  Therefore, sort backports after
         # all non-backports.
-        if a.pocket == "backports" and b.pocket != "backports":
+        if a.pocket == u"backports" and b.pocket != u"backports":
             return 1
-        if a.pocket != "backports" and b.pocket == "backports":
+        if a.pocket != u"backports" and b.pocket == u"backports":
             return -1
         # Next sort by version.
         if a.version < b.version:
@@ -221,7 +221,7 @@
         # Sort Debian before Ubuntu, so that synced packages are first imported
         # into Debian.
         if a.distro != b.distro:
-            if a.distro == "debian":
+            if a.distro == u"debian":
                 return -1
             return 1
         # From here on, a.distro == b.distro
@@ -838,7 +838,10 @@
 
 
 def make_datetime(strified):
-    return datetime.strptime(strified, "%Y-%m-%d %H:%M:%S.%f")
+    fmt = "%Y-%m-%d %H:%M:%S"
+    if "." in strified:
+        fmt += ".%f"
+    return datetime.strptime(strified, fmt)
 
 
 class CommitDatabase(object):
@@ -1002,11 +1005,11 @@
     def is_marked(self, version, suite):
         with Cursor(self.conn) as c:
             rows = list(c.execute(self.REVID_TABLE_FIND % self.REVID_TABLE,
-                    (str(version), self.package, suite)))
+                    (unicode(version), self.package, suite)))
             rows += list(c.execute(self.REVID_TABLE_FIND % self.REVID_WORKING_TABLE,
-                    (str(version), self.package, suite)))
+                    (unicode(version), self.package, suite)))
             rows += [foo for foo in self.outstanding_marks
-                        if foo == (str(version), suite)]
+                        if foo == (unicode(version), suite)]
             assert len(rows) < 2, "Multiple versions for package/suite?"
             if len(rows) > 0:
                 return True
@@ -1020,7 +1023,7 @@
             versions += [a[0] for a in c.execute(self.REVID_TABLE_FIND_SUITE
                     % self.REVID_TABLE,
                     (self.package, suite))]
-            versions += [str(foo[0]) for foo in self.outstanding_marks
+            versions += [unicode(foo[0]) for foo in self.outstanding_marks
                             if foo[1] == suite]
             if len(versions) > 0:
                 versions.sort(key=lambda x: changelog.Version(x))
@@ -1031,11 +1034,11 @@
         sha = self.get_testament_sha1(branch.repository, revid)
         with Cursor(self.conn) as c:
             rows = list(c.execute(self.REVID_TABLE_FIND % self.REVID_TABLE,
-                    (str(version), self.package, suite)))
+                    (unicode(version), self.package, suite)))
             rows += list(c.execute(self.REVID_TABLE_FIND % self.REVID_WORKING_TABLE,
-                    (str(version), self.package, suite)))
+                    (unicode(version), self.package, suite)))
             if (version, suite) in self.outstanding_marks:
-                revid, tment = self.outstanding_marks[(str(version), suite)]
+                revid, tment = self.outstanding_marks[(unicode(version), suite)]
                 rows += (None, None, None, revid, tment)
             if len(rows) < 1:
                 return False
@@ -1043,9 +1046,9 @@
             row = rows[0]
             if (revid, sha) != (row[3], row[4]):
                 assert False, ("%s != %s for %s %s in %s, something has changed"
-                     % (str((revid, sha)), str((row['revid'],
+                     % (unicode((revid, sha)), unicode((row['revid'],
                                  row['testament'])), self.package,
-                         str(version), suite))
+                         unicode(version), suite))
             return True
 
     def get_testament_sha1(self, repo, revid):
@@ -1091,20 +1094,20 @@
 
     def mark(self, version, revid, suite, branch):
         sha = self.get_testament_sha1(branch.repository, revid)
-        self.outstanding_marks[(str(version), suite)] = (revid, sha)
+        self.outstanding_marks[(unicode(version), suite)] = (revid, sha)
 
     def commit_outstanding(self):
         with Cursor(self.conn) as c:
             for version, suite in self.outstanding_marks:
                 revid, sha = self.outstanding_marks[(version, suite)]
                 rows = list(c.execute(self.REVID_TABLE_FIND % self.REVID_TABLE,
-                        (str(version), self.package, suite)))
+                        (unicode(version), self.package, suite)))
                 rows += list(c.execute(self.REVID_TABLE_FIND % self.REVID_WORKING_TABLE,
-                        (str(version), self.package, suite)))
+                        (unicode(version), self.package, suite)))
                 assert len(rows) < 1, "Trying to mark version %s in %s again" \
-                    % (str(version), suite)
+                    % (unicode(version), suite)
                 c.execute(self.REVID_TABLE_INSERT % self.REVID_WORKING_TABLE,
-                        (self.package, str(version), suite, revid, sha))
+                        (self.package, unicode(version), suite, revid, sha))
             self.outstanding_marks = {}
 
     def get_suffix(self):
@@ -1151,7 +1154,7 @@
         with Cursor(self.conn) as c:
             rows = c.execute("select * from %s"
                     % table)
-            return [(r[0], r[1], r[2]) for r in rows]
+            return [(make_datetime(r[0]), r[1], r[2]) for r in rows]
 
     def get_main_counts(self):
         return self._get_counts(self.MAIN_FAILED_TABLE)
@@ -1168,7 +1171,7 @@
 
     def is_marked(self, version, suite):
         revid_map = self._get_map(suite)
-        return str(version) in revid_map
+        return unicode(version) in revid_map
 
     def last_marked_version(self, suite):
         revid_map = self._get_map(suite)
@@ -1180,13 +1183,13 @@
 
     def check(self, version, revid, suite, branch):
         revid_map = self._get_map(suite)
-        if not str(version) in revid_map:
+        if not unicode(version) in revid_map:
             return False
         sha = self.get_testament_sha1(branch.repository, revid)
-        if (revid, sha) != revid_map[str(version)]:
+        if (revid, sha) != revid_map[unicode(version)]:
             mutter("%s != %s for %s %s, something has changed"
-                   % (str((revid, sha)), str(revid_map[str(version)]),
-                      self.package, str(version)))
+                   % (unicode((revid, sha)), unicode(revid_map[unicode(version)]),
+                      self.package, unicode(version)))
             return False
         return True
 
@@ -1208,11 +1211,11 @@
 
     def mark(self, version, revid, suite, branch):
         revid_map = self._get_map(suite)
-        assert str(version) not in revid_map, \
+        assert unicode(version) not in revid_map, \
               "Asked to mark %s %s %s, but it is already marked" \
                     % (self.package, version, suite)
         sha = self.get_testament_sha1(branch.repository, revid)
-        revid_map[str(version)] = (revid, sha)
+        revid_map[unicode(version)] = (revid, sha)
         self._save_map(suite, revid_map)
 
     def commit_outstanding(self):
@@ -1494,8 +1497,8 @@
         debian_b = branch.Branch.open(os.path.join(temp_dir,
                     default_debian_diff_release))
     except errors.NotBranchError:
-        debian_b = bstore.get_branch_parts("debian",
-                default_debian_diff_release, "release",
+        debian_b = bstore.get_branch_parts(u"debian",
+                default_debian_diff_release, u"release",
                 possible_transports=possible_transports)
     if debian_b is None:
         # Not in unstable
@@ -1507,8 +1510,8 @@
                     ubuntu_current_series))
         ubuntu_tree = ubuntu_b.bzrdir.open_workingtree()
     except errors.NotBranchError:
-        ubuntu_b = bstore.get_branch_parts("ubuntu", ubuntu_current_series,
-                "release", possible_transports=possible_transports)
+        ubuntu_b = bstore.get_branch_parts(u"ubuntu", ubuntu_current_series,
+                u"release", possible_transports=possible_transports)
         if ubuntu_b is not None and ensure_ubuntu_local:
             to_transport = transport.get_transport(os.path.join(temp_dir,
                         ubuntu_current_series))

=== modified file 'udd/scripts/import_package.py'
--- udd/scripts/import_package.py	2012-03-23 16:56:38 +0000
+++ udd/scripts/import_package.py	2012-04-04 17:35:29 +0000
@@ -52,18 +52,18 @@
 
 push_lock = threading.Lock()
 
-distros = ("debian", "ubuntu",)
-
-distro_list_base = {"ubuntu": "http://archive.ubuntu.com/ubuntu/dists/",
-    "debian": "http://ftp.debian.org/dists/",
-}
-
-distro_components = {"ubuntu": ["main", "restricted", "universe", "multiverse"],
-    "debian": ["main", "contrib", "non-free"],
-}
-
-distro_pool = {"ubuntu": "http://archive.ubuntu.com/ubuntu/",
-    "debian": "http://ftp.debian.org/",
+distros = (u"debian", u"ubuntu",)
+
+distro_list_base = {u"ubuntu": "http://archive.ubuntu.com/ubuntu/dists/",
+    u"debian": "http://ftp.debian.org/dists/",
+}
+
+distro_components = {u"ubuntu": [u"main", u"restricted", u"universe", u"multiverse"],
+    u"debian": [u"main", u"contrib", u"non-free"],
+}
+
+distro_pool = {u"ubuntu": "http://archive.ubuntu.com/ubuntu/",
+    u"debian": "http://ftp.debian.org/",
 }
 
 launchpad_base_url = "https://launchpad.net/"
@@ -141,8 +141,8 @@
     if icommon.lock_path(conf.get('pi.script_locks_dir'),
                          'update_lists') is None:
         return
-    for release in ("woody", "sarge", "etch",):
-        for component in ("main", "contrib", "non-free"):
+    for release in (u"woody", u"sarge", u"etch",):
+        for component in (u"main", u"contrib", u"non-free"):
             source_url = ''.join(['http://archive.debian.org/debian/dists/',
                                  release, '/', component, '/source/Sources.gz'])
             icommon.update_list(release, component, source_url)
@@ -160,12 +160,12 @@
     # - let's not.
     distro_series_link = publication.lp_get_parameter('distro_series_link')
     # eg u'https://api.launchpad.net/1.0/debian/sid'
-    return distro_series_link.rsplit('/', 2)[-1].lower()
+    return unicode(distro_series_link.rsplit('/', 2)[-1].lower())
 
 
 def get_debian_versions(lp, package, extra_debian=None):
     vlist = icommon.ImportList()
-    d_archive = lp._udd_distributions['debian'].main_archive
+    d_archive = lp._udd_distributions[u'debian'].main_archive
     publications = lpapi.lp_call(lpapi.call_with_limited_size,
                 d_archive.getPublishedSources,
                 source_name=package, exact_match=True)
@@ -177,7 +177,7 @@
             release = release_name_from_publication(publication)
             pocket = publication.pocket.lower()
             version = changelog.Version(publication.source_package_version)
-            newp = icommon.PackageToImport(package, version, "debian", release,
+            newp = icommon.PackageToImport(package, version, u"debian", release,
                     pocket)
             vlist.add_if_needed(newp)
             # if idx >= 20:
@@ -207,13 +207,13 @@
         line = line[line.index("|")+1:].strip()
         release_component = line[:line.index("|")].strip()
         if "/" in release_component:
-            release = release_component[:release_component.index("/")]
-            component = release_component[release_component.index("/")+1:]
+            release = unicode(release_component[:release_component.index("/")])
+            component = unicode(release_component[release_component.index("/")+1:])
         else:
-            release = release_component
-            component = "main"
-        newp = icommon.PackageToImport(package, version, "debian", release,
-                "release", component=component, on_lp=False)
+            release = unicode(release_component)
+            component = u"main"
+        newp = icommon.PackageToImport(package, version, u"debian", release,
+                u"release", component=component, on_lp=False)
         vlist.add_if_needed(newp)
     if extra_debian is not None:
         f = open(extra_debian)
@@ -221,11 +221,11 @@
             for line in f:
                 parts = line.split()
                 version = changelog.Version(parts[0])
-                release = parts[1]
-                pocket =  parts[2]
+                release = unicode(parts[1])
+                pocket =  unicode(parts[2])
                 base_url =  parts[3]
                 url = os.path.join(os.path.dirname(extra_debian), base_url)
-                newp = icommon.PackageToImport(package, version, "debian",
+                newp = icommon.PackageToImport(package, version, u"debian",
                         release, pocket, url=url)
                 vlist.add_if_needed(newp)
         finally:
@@ -235,7 +235,7 @@
 
 def get_versions(lp, package, extra_debian=None):
     vlist = get_debian_versions(lp, package, extra_debian=extra_debian)
-    u_archive = lp._udd_distributions['ubuntu'].main_archive
+    u_archive = lp._udd_distributions[u'ubuntu'].main_archive
     publications = lpapi.lp_call(lpapi.call_with_limited_size,
                 u_archive.getPublishedSources,
                 source_name=package, exact_match=True)
@@ -248,7 +248,7 @@
             release = release_name_from_publication(publication)
             pocket = publication.pocket.lower()
             version = changelog.Version(publication.source_package_version)
-            newp = icommon.PackageToImport(package, version, "ubuntu", release,
+            newp = icommon.PackageToImport(package, version, u"ubuntu", release,
                     pocket)
             vlist.add_if_needed(newp)
             # if idx >= 20:
@@ -305,11 +305,11 @@
 
 
 def other_distro(distro):
-    assert distro in ("debian", "ubuntu")
-    if distro == "debian":
-        return "ubuntu"
+    assert distro in (u"debian", u"ubuntu")
+    if distro == u"debian":
+        return u"ubuntu"
     else:
-        return "debian"
+        return u"debian"
 
 
 def grab_file(base_url, name, target_dir, possible_transports=None):
@@ -1170,7 +1170,7 @@
             sys.exit(1)
         try:
             sys.exit(_import_package(
-                    lp, args[0], push=not options.no_push,
+                    lp, unicode(args[0]), push=not options.no_push,
                     extra_debian=options.extra_debian, check=options.check,
                     no_existing=options.no_existing,
                     keep_temp=options.keep_temp,

=== modified file 'udd/scripts/requeue_package.py'
--- udd/scripts/requeue_package.py	2012-03-23 16:56:38 +0000
+++ udd/scripts/requeue_package.py	2012-04-04 17:35:29 +0000
@@ -44,6 +44,7 @@
     db = icommon.StatusDatabase(conn, db_type)
 
     for package in args:
+        pacakge = unicode(package)
         lock = icommon.lock_path(conf.get('pi.locks_dir'), package)
         if lock is None:
             print "Can't lock %s" % package

=== modified file 'udd/scripts/show_failure.py'
--- udd/scripts/show_failure.py	2012-03-23 16:56:38 +0000
+++ udd/scripts/show_failure.py	2012-04-04 17:35:29 +0000
@@ -19,4 +19,4 @@
 
 
 def main():
-    sys.exit(print_reason(sys.argv[1]))
+    sys.exit(print_reason(unicode(sys.argv[1])))

=== added file 'udd/tests/test_history_database.py'
--- udd/tests/test_history_database.py	1970-01-01 00:00:00 +0000
+++ udd/tests/test_history_database.py	2012-04-04 17:35:29 +0000
@@ -0,0 +1,33 @@
+from datetime import datetime
+
+from bzrlib import tests
+
+from udd import icommon, idb
+
+
+class TestHistoryDb(tests.TestCase):
+
+    def setUp(self):
+        super(TestHistoryDb, self).setUp()
+        self.db = icommon.HistoryDatabase(*idb.get_memory_db_connection())
+
+
+class GetMainCountsTests(TestHistoryDb):
+
+    def test_returns_queued(self):
+        self.db.set_main_counts(2, 3)
+        counts = self.db.get_main_counts()
+        self.assertEqual(1, len(counts))
+        self.assertEqual(2, counts[0][1])
+
+    def test_returns_failed(self):
+        self.db.set_main_counts(2, 3)
+        counts = self.db.get_main_counts()
+        self.assertEqual(1, len(counts))
+        self.assertEqual(3, counts[0][2])
+
+    def test_returns_datetimes(self):
+        self.db.set_main_counts(1, 2)
+        counts = self.db.get_main_counts()
+        self.assertEqual(1, len(counts))
+        self.assertIsInstance(counts[0][0], datetime)

=== modified file 'udd/tests/test_status_database.py'
--- udd/tests/test_status_database.py	2012-03-23 16:56:38 +0000
+++ udd/tests/test_status_database.py	2012-04-04 17:35:29 +0000
@@ -99,6 +99,15 @@
         self.db.add_import_jobs(['foo'], now)
         self.assertEqual(now, self.db.last_import_time())
 
+    def test_last_import_time_sqlite(self):
+        # if the last import time was inserted by sqlite3 then it has a
+        # different format from the one that storm would insert. Check
+        # that the migration is handled ok.
+        now = datetime.datetime.utcnow()
+        self.db.conn.execute('INSERT into %s values (?)' % self.db.IMPORT_TABLE,
+                (now.strftime("%Y-%m-%d %H:%M:%S"),))
+        self.assertEqual(now.replace(microsecond=0), self.db.last_import_time())
+
 
 class TestRetry(TestStatusDb):