Merge lp:~maxb/udd/fix-import-ordering into lp:udd

[Max Bowsher]

This branch tidies up and then alters the logic for determining in which order several import operations are performed when there are multiple to do for a package.

This is of particular importance when an import which has been broken for some time, and is then fixed. The old code can produce some spectacularly contorted history, doing things such as importing versions in parallel into the testing and unstable Debian branches.

Even with this change, there's still one other quirk that persists, in terms of history looking different because the importer was temporarily broken: Take qbzr as an example, which has been broken since lucid. As a result, once fixed (by inserting a tag which has gone missing), the imported history has an odd shape, with the maverick, natty and oneiric branches all separately taking the lucid branch as their direct left-hand parent.

[Jelmer Vernooij (jelmer)]

Thanks, I think these changes make a lot of sense.

What about, in addition to testing coming after experimental/sid, things entering testing before stable/lenny/etch/etc ?

This seems like fairly trivial code; it would be nice to have a few more tests (and perhaps factor out compare_versions) ?

review: Needs Fixing

[James Westby (james-w)]

Hi,

Importing all Debian before any Ubuntu would prevent recording merges
if Debian were to merge from Ubuntu. It's a rare case, so ok to sacrifice
if this is needed to make other things work, but if we can preserve
it it would be good.

Thanks,

James

[Max Bowsher (maxb)]

@ james_w:

> Importing all Debian before any Ubuntu would prevent recording merges
> if Debian were to merge from Ubuntu. It's a rare case, so ok to sacrifice
> if this is needed to make other things work, but if we can preserve
> it it would be good.

That particular element of the ordering is pre-existing before my changes (though this branch adds the comment explaining it). I don't think it will be possible to remove it without significantly rewriting the ordering algorithm to either be based on a topological sort involving introspection of each version's debian/changelog, or to be a simple sort on the timestamp of the publishing records fetched from Launchpad.

[Max Bowsher (maxb)]

@ jelmer:

> What about, in addition to testing coming after experimental/sid, things
> entering testing before stable/lenny/etch/etc ?

We talked IRL on this and decided there is probably no issue here - because usually nothing should be entering testing other than through unstable, so will be imported first there. Other possibilities are packages entering via *-proposed-updates, but that's OK too, because the UDD importer does not track *-proposed-updates, and versions entering a series via *-proposed-updates do not migrate to other series.

> This seems like fairly trivial code; it would be nice to have a few more tests
> (and perhaps factor out compare_versions) ?

A few more tests wouldn't be hard, I'll make some.

MP marked WIP pending addition of more tests.

[Max Bowsher (maxb)]

Additional revisions 429-434 pushed with miscellaneous cleanups, one small fix, and then some tests.

[Jelmer Vernooij (jelmer)]

On Tue, 2011-05-17 at 13:35 +0000, Max Bowsher wrote:
> Additional revisions 429-434 pushed with miscellaneous cleanups, one small fix, and then some tests.

  review approve

Thanks for fixing this.

As discussed IRL, it might be possible to get away with actually
comparing versions here first, distros later (if the versions are equal)
and just having a special case for -backports (always sorting them
later, I guess).

Cheers,

Jelmer

review: Approve

=== modified file 'icommon.py'
--- icommon.py	2011-04-28 11:41:36 +0000
+++ icommon.py	2011-05-17 13:32:07 +0000
@@ -230,6 +230,8 @@
 
 
 class PackageToImport(object):
+    """This object is analogous to (in Soyuz terminology) a source package
+    publication."""
 
     def __init__(self, name, version, distro, release, pocket, component="",
             on_lp=True, url=None):
@@ -248,6 +250,11 @@
                    self.distro, self.release, self.pocket, self.component,
                    self.url))
 
+    def __str__(self):
+        return "PackageToImport(%s, %s, %s, %s, %s, %s)" % (self.name,
+                self.version, self.distro, self.release, self.pocket,
+                self.component)
+
     def same_as(self, other):
         return (self.name == other.name
                 and self.version == other.version
@@ -282,10 +289,69 @@
         self.url = files_url + "%s_%s.dsc" % (self.name, version_str)
         return self.url
 
-    def __str__(self):
-        return "PackageToImport(%s, %s, %s, %s, %s, %s)" % (self.name,
-                self.version, self.distro, self.release, self.pocket,
-                self.component)
+    @staticmethod
+    def import_order_comparator(a, b):
+        """This comparator aims to sort prospective import jobs such that
+        package versions are imported after versions they were derived from,
+        and publications that are likely to be copied into other archives,
+        series or pockets are imported before imports to the likely
+        destinations, such that the source imported version is available to be
+        pulled."""
+        # Debian versions should be imported before Ubuntu versions, so
+        # that any Debian version an Ubuntu version may be based on is
+        # already imported when the Ubuntu version is imported.
+        if a.distro != b.distro:
+            if a.distro == "debian":
+                return -1
+            return 1
+        # From here on, a.distro == b.distro
+        if a.distro == "ubuntu":
+            # Ubuntu backports should be imported after all other Ubuntu
+            # pockets, so that the version the backport is based on is
+            # imported before the backport itself.
+            if a.pocket == "backports" and b.pocket != "backports":
+                return 1
+            if a.pocket != "backports" and b.pocket == "backports":
+                return -1
+            # Within Ubuntu, release series should be imported in ascending
+            # order, so that a package version is imported into the first
+            # series in which it is present.
+            if a.release != b.release:
+                a_release = distro_releases[a.distro].index(a.release)
+                b_release = distro_releases[b.distro].index(b.release)
+                if a_release < b_release:
+                    return -1
+                return 1
+            # Within a single Ubuntu release series, import pockets such
+            # that any pocket that can contain versions based on versions
+            # in another pocket, is imported after that pocket.
+            if a.pocket != b.pocket:
+                a_pocket = distro_pockets[a.distro].index(a.pocket)
+                b_pocket = distro_pockets[b.distro].index(b.pocket)
+                if a_pocket < b_pocket:
+                    return -1
+                return 1
+        if a.distro == "debian":
+            # Debian sid/experimental should be imported before any other
+            # codename, to get the proper ancestry.
+            if a.release in ("sid", "experimental") and b.release not in ("sid", "experimental"):
+                return -1
+            if a.release not in ("sid", "experimental") and b.release in ("sid", "experimental"):
+                return 1
+            # Other than that special case, import in ascending order.
+            if a.release != b.release:
+                a_release = distro_releases[a.distro].index(a.release)
+                b_release = distro_releases[b.distro].index(b.release)
+                if a_release < b_release:
+                    return -1
+                return 1
+        # Within a single distro, release, and pocket, import in version
+        # order.
+        if a.version < b.version:
+            return -1
+        if a.version > b.version:
+            return 1
+        return 0
 
 
 class ImportList(object):
@@ -297,37 +363,7 @@
         return '%s(%s)' % (self.__class__.__name__, self.plist)
 
     def sort(self):
-        def compare_versions(a, b):
-            if a.distro == "ubuntu" and a.distro == b.distro:
-                if a.pocket == "backports" and b.pocket != "backports":
-                    return -1
-                if a.pocket != "backports" and b.pocket == "backports":
-                    return 1
-            if a.distro != b.distro:
-                if a.distro == "debian":
-                    return 1
-                return -1
-            if a.release != b.release:
-                a_release = distro_releases[a.distro].index(a.release)
-                b_release = distro_releases[b.distro].index(b.release)
-                if a_release < b_release:
-                    return 1
-                return -1
-            if a.pocket != b.pocket:
-                a_pocket = distro_pockets[a.distro].index(a.pocket)
-                b_pocket = distro_pockets[b.distro].index(b.pocket)
-                if a_pocket < b_pocket:
-                    return 1
-                return -1
-            if a.version < b.version:
-                return 1
-            if a.version > b.version:
-                return -1
-            return 0
-        self.plist.sort(cmp=compare_versions)
-
-    def reverse(self):
-        self.plist.reverse()
+        self.plist.sort(cmp=PackageToImport.import_order_comparator)
 
     def add_if_needed(self, newp):
         for oldp in self.plist:

=== modified file 'import_package.py'
--- import_package.py	2011-03-28 09:33:45 +0000
+++ import_package.py	2011-05-17 13:32:07 +0000
@@ -1014,7 +1014,6 @@
             trace.mutter("finding all versions of %s" % (package,))
             versions = get_versions(package, extra_debian=extra_debian)
             assert len(versions.plist) > 0
-            versions.reverse()
             trace.mutter("found %d versions: %s"
                          % (len(versions.plist), versions))
             possible_transports = []

=== modified file 'tests/test_import_list.py'
--- tests/test_import_list.py	2011-03-24 18:52:48 +0000
+++ tests/test_import_list.py	2011-05-17 13:32:07 +0000
@@ -22,17 +22,14 @@
                 "jaunty, Release, ), PackageToImport(pkg, 1.0-1, "
                 "ubuntu, karmic, Release, )]", str(list))
 
-    def test_reverse(self):
-        list = self.make_simple()
-        self.assertEqual([self.pkg1, self.pkg2], list.plist)
-        list.reverse()
-        self.assertEqual([self.pkg2, self.pkg1], list.plist)
-
     def test_sort(self):
         list = self.make_simple()
-        self.assertEqual([self.pkg1, self.pkg2], list.plist)
+        # make_simple's list is already sorted, reverse it to prove that
+        # sorting will restore the original order:
+        list.plist.reverse()
+        self.assertEqual([self.pkg2, self.pkg1], list.plist)
         list.sort()
-        self.assertEqual([self.pkg2, self.pkg1], list.plist)
+        self.assertEqual([self.pkg1, self.pkg2], list.plist)
 
     def test_add_if_needed(self):
         list = self.make_simple()

=== modified file 'tests/test_package_to_import.py'
--- tests/test_package_to_import.py	2011-03-24 18:39:55 +0000
+++ tests/test_package_to_import.py	2011-05-17 13:32:07 +0000
@@ -7,38 +7,30 @@
 
 
 import icommon
+from icommon import PackageToImport as PTI
+V = changelog.Version
 
 
 class PackageToImportTests(tests.TestCase):
 
     def test_same_as_same(self):
-        a = icommon.PackageToImport("a", changelog.Version("1"),
-                "debian", "sid", "release", component="main",
-                on_lp=True, url=None)
-        b = icommon.PackageToImport("a", changelog.Version("1"),
-                "debian", "sid", "release", component="contrib")
-        c = icommon.PackageToImport("a", changelog.Version("1"),
-                "debian", "sid", "release", on_lp=False)
-        d = icommon.PackageToImport("a", changelog.Version("1"),
-                "debian", "sid", "release", url="a")
+        a = PTI("a", V("1"), "debian", "sid", "release",
+                component="main", on_lp=True, url=None)
+        b = PTI("a", V("1"), "debian", "sid", "release", component="contrib")
+        c = PTI("a", V("1"), "debian", "sid", "release", on_lp=False)
+        d = PTI("a", V("1"), "debian", "sid", "release", url="a")
         self.assertTrue(a.same_as(b))
         self.assertTrue(a.same_as(c))
         self.assertTrue(a.same_as(d))
 
     def test_same_as_different(self):
-        a = icommon.PackageToImport("a", changelog.Version("1"),
-                "debian", "sid", "release", component="main",
-                on_lp=True, url=None)
-        b = icommon.PackageToImport("b", changelog.Version("1"),
-                "debian", "sid", "release")
-        c = icommon.PackageToImport("a", changelog.Version("1.1"),
-                "debian", "sid", "release")
-        d = icommon.PackageToImport("a", changelog.Version("1"),
-                "ubuntu", "sid", "release")
-        e = icommon.PackageToImport("a", changelog.Version("1"),
-                "debian", "squeeze", "release")
-        f = icommon.PackageToImport("a", changelog.Version("1"),
-                "debian", "sid", "security")
+        a = PTI("a", V("1"), "debian", "sid", "release",
+                component="main", on_lp=True, url=None)
+        b = PTI("b", V("1"), "debian", "sid", "release")
+        c = PTI("a", V("1.1"), "debian", "sid", "release")
+        d = PTI("a", V("1"), "ubuntu", "sid", "release")
+        e = PTI("a", V("1"), "debian", "squeeze", "release")
+        f = PTI("a", V("1"), "debian", "sid", "security")
         self.assertFalse(a.same_as(b))
         self.assertFalse(a.same_as(c))
         self.assertFalse(a.same_as(d))
@@ -46,34 +38,57 @@
         self.assertFalse(a.same_as(f))
 
     def test_get_url(self):
-        a = icommon.PackageToImport("a", changelog.Version("1"),
-                "debian", "sid", "release", url="url")
+        a = PTI("a", V("1"), "debian", "sid", "release", url="url")
         self.assertEqual("url", a.get_url())
-        b = icommon.PackageToImport("a", changelog.Version("1"),
-                "debian", "sid", "release", component="non-free", on_lp=False)
+        b = PTI("a", V("1"), "debian", "sid", "release",
+                component="non-free", on_lp=False)
         self.assertEqual(icommon.urllib_debian_base_url
                 + "pool/non-free/a/a/a_1.dsc", b.get_url())
-        c = icommon.PackageToImport("libfoo", changelog.Version("1:1.1-1"),
-                "debian", "squeeze", "release", component="contrib", on_lp=False)
+        c = PTI("libfoo", V("1:1.1-1"), "debian", "squeeze", "release",
+                component="contrib", on_lp=False)
         self.assertEqual(icommon.urllib_debian_base_url
                 + "pool/contrib/libf/libfoo/libfoo_1.1-1.dsc", c.get_url())
-        c = icommon.PackageToImport("libfoo", changelog.Version("1:1.1-1"),
-                "debian", "squeeze", "release", component="contrib",
-                on_lp=True)
+        c = PTI("libfoo", V("1:1.1-1"), "debian", "squeeze", "release",
+                component="contrib", on_lp=True)
         self.assertEqual(icommon.urllib_launchpad_base_url
                 + "debian/squeeze/+source/libfoo/1:1.1-1/+files/libfoo_1.1-1.dsc",
                 c.get_url())
-        d = icommon.PackageToImport("libfoo", changelog.Version("1:1.1-1"),
-                "ubuntu", "lucid", "security")
+        d = PTI("libfoo", V("1:1.1-1"), "ubuntu", "lucid", "security")
         self.assertEqual(icommon.urllib_launchpad_base_url
                 + "ubuntu/lucid/+source/libfoo/1:1.1-1/+files/libfoo_1.1-1.dsc",
                 d.get_url())
 
     def test_str(self):
-        a = icommon.PackageToImport("a", changelog.Version("1"),
-                "debian", "sid", "release", component="main",
-                on_lp=True, url=None)
+        a = PTI("a", V("1"), "debian", "sid", "release",
+                component="main", on_lp=True, url=None)
         self.assertEqual("PackageToImport(a, 1, debian, sid, release, main)",
                 str(a))
 
-
+    def assertOrdering(self, a, b):
+        self.assertTrue(PTI.import_order_comparator(a, b) < 0)
+
+    def test_comparison(self):
+        # Distro is the most-significant factor, with all of Ubuntu coming
+        # after all of Debian, irregardless of releases and versions:
+        self.assertOrdering(PTI("a", V("999999"), "debian", "sid", "release"),
+                            PTI("a", V("1"), "ubuntu", "warty", "release"))
+
+        # Within a distro, release is the next most significant:
+        self.assertOrdering(PTI("a", V("1"), "debian", "lenny", "release"),
+                            PTI("a", V("1"), "debian", "squeeze", "release"))
+
+        # Except for Ubuntu backports, which are a special case, imported after
+        # all other Ubuntu imports in the same per-package run:
+        self.assertOrdering(PTI("a", V("1"), "ubuntu", "natty", "release"),
+                            PTI("a", V("1~hardy1"), "ubuntu", "hardy",
+                                "backports"))
+
+        # Within Ubuntu, pockets matter:
+        self.assertOrdering(PTI("a", V("1"), "ubuntu", "natty", "release"),
+                            PTI("a", V("1"), "ubuntu", "natty", "security"))
+        self.assertOrdering(PTI("a", V("1"), "ubuntu", "natty", "security"),
+                            PTI("a", V("1"), "ubuntu", "natty", "proposed"))
+        self.assertOrdering(PTI("a", V("1"), "ubuntu", "natty", "proposed"),
+                            PTI("a", V("1"), "ubuntu", "natty", "updates"))
+        self.assertOrdering(PTI("a", V("1"), "ubuntu", "natty", "updates"),
+                            PTI("a", V("1"), "ubuntu", "natty", "backports"))