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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
