Merge lp:~cjohnston/launchpad/proper-pkg-link-copying into lp:launchpad

[Chris Johnston]

Copy packaging links only for packages which are being copied from the parent.

[William Grant (wgrant)]

This is looking good, but it needs tests.

I'd also consider dropping the else block; the if block skips the rest of the loop body anyway, so having the SQL in an else block just adds another level of avoidable indentation and obfuscates the diff.

review: Needs Fixing (code)

[William Grant (wgrant)]

Most of the test lines seem to be identical setup code repeated twice. Please refactor that.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/soyuz/scripts/initialize_distroseries.py'
--- lib/lp/soyuz/scripts/initialize_distroseries.py	2014-07-21 22:45:01 +0000
+++ lib/lp/soyuz/scripts/initialize_distroseries.py	2014-07-25 00:24:39 +0000
@@ -510,10 +510,9 @@
         for parent in self.derivation_parents:
             spns = self.source_names_by_parent.get(parent.id, None)
             if spns is not None and len(spns) == 0:
-                # Some packagesets where selected but not a single
-                # source from this parent: we skip the copy since
-                # calling copy with spns=[] would copy all the packagesets
-                # from this parent.
+                # Some packagesets may have been selected but not a single
+                # source from this parent. We will not copy any records from
+                # this parent.
                 continue
             # spns=None means no packagesets selected so we need to consider
             # all sources.
@@ -612,7 +611,13 @@
         # We iterate over the parents and copy into the child in
         # sequence to avoid creating duplicates.
         for parent_id in self.derivation_parent_ids:
-            self._store.execute("""
+            spns = self.source_names_by_parent.get(parent_id, None)
+            if spns is not None and len(spns) == 0:
+                # Some packagesets may have been selected but not a single
+                # source from this parent. We will not copy any links for this
+                # parent
+                continue
+            sql = ("""
                 INSERT INTO
                     Packaging(
                         distroseries, sourcepackagename, productseries,
@@ -634,25 +639,40 @@
                     -- Select only the packaging links that are in the parent
                     -- that are not in the child.
                     ChildSeries.id = %s
-                    AND Packaging.sourcepackagename in (
-                        SELECT sourcepackagename
-                        FROM Packaging
-                        WHERE distroseries in (
-                            SELECT id
-                            FROM Distroseries
-                            WHERE id = %s
-                            )
-                        EXCEPT
-                        SELECT sourcepackagename
-                        FROM Packaging
-                        WHERE distroseries in (
-                            SELECT id
-                            FROM Distroseries
-                            WHERE id = ChildSeries.id
-                            )
-                        )
-                """ % sqlvalues(
-                    parent_id, self.distroseries.id, parent_id))
+                """ % sqlvalues(parent_id, self.distroseries.id))
+            sql_filter = ("""
+                AND Packaging.sourcepackagename in (
+                SELECT
+                    Sourcepackagename.id
+                FROM
+                    Sourcepackagename
+                WHERE
+                    Sourcepackagename.name IN %s
+                )
+                """ % sqlvalues(spns))
+            sql_end = ("""
+                AND Packaging.sourcepackagename in (
+                SELECT Packaging.sourcepackagename
+                FROM Packaging
+                WHERE distroseries in (
+                    SELECT id
+                    FROM Distroseries
+                    WHERE id = %s
+                    )
+                EXCEPT
+                SELECT Packaging.sourcepackagename
+                FROM Packaging
+                WHERE distroseries in (
+                    SELECT id
+                    FROM Distroseries
+                    WHERE id = ChildSeries.id
+                    )
+                )
+                """ % sqlvalues(parent_id))
+            if spns is not None:
+                self._store.execute(sql + sql_filter + sql_end)
+            else:
+                self._store.execute(sql + sql_end)
 
     def _copy_packagesets(self):
         """Copy packagesets from the parent distroseries."""

=== modified file 'lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py'
--- lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py	2014-07-23 01:37:37 +0000
+++ lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py	2014-07-25 00:24:39 +0000
@@ -172,6 +172,31 @@
             pocket=PackagePublishingPocket.RELEASE)
         return parent, parent_das, parent_das2, source
 
+    def setupPackagingTesting(self):
+        # Setup the environment for testing the packaging links
+        self.parent, self.parent_das = self.setupParent()
+        test1 = getUtility(IPackagesetSet).new(
+            u'test1', u'test 1 packageset', self.parent.owner,
+            distroseries=self.parent)
+        test2 = getUtility(IPackagesetSet).new(
+            u'test2', u'test 2 packageset', self.parent.owner,
+            distroseries=self.parent)
+        packages_test1 = ['udev', 'chromium', 'libc6']
+        packages_test2 = ['postgresql', 'vim']
+        for pkg in packages_test1:
+            test1.addSources(pkg)
+            sp = self.parent.getSourcePackage(pkg)
+            product_series = self.factory.makeProductSeries()
+            product_series.setPackaging(
+                self.parent, sp.sourcepackagename, self.parent.owner)
+        for pkg in packages_test2:
+            test2.addSources(pkg)
+            sp = self.parent.getSourcePackage(pkg)
+            product_series = self.factory.makeProductSeries()
+            product_series.setPackaging(
+                self.parent, sp.sourcepackagename, self.parent.owner)
+        return packages_test1, packages_test2
+
 
 class TestInitializeDistroSeries(InitializationHelperTestCase):
 
@@ -910,6 +935,47 @@
             len(packages_test1) + len(packages_test2))
         self.assertEqual(child.binarycount, 4)  # Chromium is FTBFS
 
+    def test_copy_packaging_links_some(self):
+        # Test that when copying some packagesets from the parent, only
+        # the packaging links for the copied packages are copied.
+        packages_test1, packages_test2 = self.setupPackagingTesting()
+        packageset1 = getUtility(IPackagesetSet).getByName(
+            self.parent, u'test1')
+        child = self._fullInitialize(
+            [self.parent], packagesets=(str(packageset1.id),))
+        packagings = child.getMostRecentlyLinkedPackagings()
+        names = [
+            packaging.sourcepackagename.name for packaging in packagings]
+        self.assertEqual(
+            0, child.getPrioritizedUnlinkedSourcePackages().count())
+        self.assertEqual(set(packages_test1), set(names))
+
+    def test_copy_packaging_links_empty(self):
+        # Test that when copying no packagesets from the parent, none of
+        # the packaging links for the packages are copied.
+        self.setupPackagingTesting()
+        child = self._fullInitialize(
+            [self.parent], packagesets=[])
+        packagings = child.getMostRecentlyLinkedPackagings()
+        names = [
+            packaging.sourcepackagename.name for packaging in packagings]
+        self.assertEqual(
+            0, child.getPrioritizedUnlinkedSourcePackages().count())
+        self.assertEqual([], names)
+
+    def test_copy_packaging_links_none(self):
+        # Test that when copying all packagesets from the parent, all of
+        # the packaging links are copied.
+        packages_test1, packages_test2 = self.setupPackagingTesting()
+        child = self._fullInitialize(
+            [self.parent], packagesets=None)
+        packagings = child.getMostRecentlyLinkedPackagings()
+        names = [
+            packaging.sourcepackagename.name for packaging in packagings]
+        self.assertEqual(
+            0, child.getPrioritizedUnlinkedSourcePackages().count())
+        self.assertEqual(set(packages_test1 + packages_test2), set(names))
+
     def test_rebuild_flag(self):
         # No binaries will get copied if we specify rebuild=True.
         self.parent, self.parent_das = self.setupParent()