Merge lp:~cjwatson/launchpad/init-packageset-fixes into lp:launchpad

[Colin Watson]

Fix InitializeDistroSeries._copy_packagesets to copy from only the most recent series, not merge packagesets from all series.

== Summary ==

Bug 887185 reports that initialize-distroseries.py has been creating duplicate ArchivePermission rows. Relatedly, I've also noticed that, rather than copying the list of source packages in a given packageset from oneiric to precise when initialising precise (say), it's been merging the source packages in all instances of that packageset name in all distroseries. These look intuitively related, and indeed they turn out to be due to the same underlying bug.

== Proposed fix ==

        packagesets = self._store.find(
            Packageset, DistroSeries.id.is_in(self.derivation_parent_ids))

The intention is obvious here, but that code is missing anything relating the Packageset and Distroseries rows, so it ends up just loading all packagesets. Adding the obvious And(Packageset.distroseries == DistroSeries.id, ...) makes it work.

== Implementation details ==

+49 LoC, but I'd like to get this landed in time for opening Ubuntu Q and so don't want to spend lots of time refactoring if possible; could I use some credit from r15080 (-232) or r15103 (-93)?

== Tests ==

bin/test -vvct test_initialize_distroseries

== Demo and Q/A ==

I'm open to suggestions of something that can be done in practice in a reasonable time. My guess would be that we run initialize-distroseries.py on some suitable series on dogfood and then inspect the results, but I don't know what would be a suitable series and I don't know whether running i-d on dogfood is practical.

== lint ==

None.

[Colin Watson (cjwatson)]

Oh, incidentally, I don't think this entirely closes this bug, as we need to check for existing busted data and do something about it if any, so this is probably [incr]. I'm willing to work on the rest, although probably not immediately.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/soyuz/scripts/initialize_distroseries.py'
--- lib/lp/soyuz/scripts/initialize_distroseries.py	2011-12-30 06:14:56 +0000
+++ lib/lp/soyuz/scripts/initialize_distroseries.py	2012-04-23 01:10:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Initialize a distroseries from its parent distroseries."""
@@ -655,11 +655,9 @@
 
     def _copy_packagesets(self):
         """Copy packagesets from the parent distroseries."""
-        # Avoid circular imports.
-        from lp.registry.model.distroseries import DistroSeries
-
         packagesets = self._store.find(
-            Packageset, DistroSeries.id.is_in(self.derivation_parent_ids))
+            Packageset,
+            Packageset.distroseries_id.is_in(self.derivation_parent_ids))
         parent_to_child = {}
         # Create the packagesets and any archivepermissions if we're not
         # copying cross-distribution.

=== modified file 'lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py'
--- lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py	2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py	2012-04-23 01:10:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test the initialize_distroseries script machinery."""
@@ -156,9 +156,13 @@
             distroseries=distroseries,
             sourcepackagename=spn,
             pocket=PackagePublishingPocket.RELEASE)
-        packageset = getUtility(IPackagesetSet).new(
-            packageset_name, packageset_name, distroseries.owner,
-            distroseries=distroseries)
+        try:
+            packageset = getUtility(IPackagesetSet).getByName(
+                packageset_name, distroseries=distroseries)
+        except NoSuchPackageSet:
+            packageset = getUtility(IPackagesetSet).new(
+                packageset_name, packageset_name, distroseries.owner,
+                distroseries=distroseries)
         packageset.addSources(package_name)
         if create_build:
             source.createMissingBuilds()
@@ -686,6 +690,48 @@
             [(u'udev', u'0.1-1'), (u'firefox', u'2.1')],
             pub_sources)
 
+    def test_copying_packagesets_no_duplication(self):
+        # Copying packagesets only copies the packageset from the most
+        # recent series, rather than merging those from all series.
+        previous_parent, _ = self.setupParent()
+        parent = self._fullInitialize([previous_parent])
+        self.factory.makeSourcePackagePublishingHistory(distroseries=parent)
+        p1, parent_packageset, _ = self.createPackageInPackageset(
+            parent, u"p1", u"packageset")
+        uploader1 = self.factory.makePerson()
+        getUtility(IArchivePermissionSet).newPackagesetUploader(
+            parent.main_archive, uploader1, parent_packageset)
+        child = self._fullInitialize(
+            [previous_parent], previous_series=parent,
+            distribution=parent.distribution)
+        # Make sure the child's packageset has disjoint packages and
+        # permissions.
+        p2, child_packageset, _ = self.createPackageInPackageset(
+            child, u"p2", u"packageset")
+        child_packageset.removeSources([u"p1"])
+        uploader2 = self.factory.makePerson()
+        getUtility(IArchivePermissionSet).newPackagesetUploader(
+            child.main_archive, uploader2, child_packageset)
+        getUtility(IArchivePermissionSet).deletePackagesetUploader(
+            parent.main_archive, uploader1, child_packageset)
+        grandchild = self._fullInitialize(
+            [previous_parent], previous_series=child,
+            distribution=parent.distribution)
+        grandchild_packageset = getUtility(IPackagesetSet).getByName(
+            parent_packageset.name, distroseries=grandchild)
+        # The copied grandchild set has sources matching the child.
+        self.assertContentEqual(
+            child_packageset.getSourcesIncluded(),
+            grandchild_packageset.getSourcesIncluded())
+        # It also has permissions matching the child.
+        perms2 = getUtility(IArchivePermissionSet).uploadersForPackageset(
+            parent.main_archive, child_packageset)
+        perms3 = getUtility(IArchivePermissionSet).uploadersForPackageset(
+            parent.main_archive, grandchild_packageset)
+        self.assertContentEqual(
+            [perm.person.name for perm in perms2],
+            [perm.person.name for perm in perms3])
+
     def test_intra_distro_perm_copying(self):
         # If child.distribution equals parent.distribution, we also
         # copy the archivepermissions.