launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/init-packageset-fixes into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #887185 in Launchpad itself: "ArchivePermission allows duplicated rows"
  https://bugs.launchpad.net/launchpad/+bug/887185

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/init-packageset-fixes/+merge/103036

== 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.
-- 
https://code.launchpad.net/~cjwatson/launchpad/init-packageset-fixes/+merge/103036
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/init-packageset-fixes into lp:launchpad.

=== 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 00:55:22 +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."""
@@ -11,6 +11,7 @@
 
 from operator import methodcaller
 
+from storm.expr import And
 import transaction
 from zope.component import getUtility
 
@@ -659,7 +660,9 @@
         from lp.registry.model.distroseries import DistroSeries
 
         packagesets = self._store.find(
-            Packageset, DistroSeries.id.is_in(self.derivation_parent_ids))
+            Packageset,
+            And(Packageset.distroseries == DistroSeries.id,
+                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 00:55:22 +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.