Launchpad itself

Merge lp:~julian-edwards/launchpad/copy-archive-fix-component into lp:launchpad

  1. copy-archive-fix-component
  2. Merge into devel
Proposed by Julian Edwards
Status: Merged
Approved by: Julian Edwards
Approved revision: no longer in the source branch.
Merged at revision: 11088
Proposed branch: lp:~julian-edwards/launchpad/copy-archive-fix-component
Merge into: lp:launchpad
Diff against target: 92 lines (+21/-17)
2 files modified
lib/lp/soyuz/model/packagecloner.py (+7/-2)
lib/lp/soyuz/scripts/tests/test_populatearchive.py (+14/-15)
To merge this branch: bzr merge lp:~julian-edwards/launchpad/copy-archive-fix-component
Reviewer Review Type Date Requested Status
Francis J. Lacoste (community) release-critical Approve
Jeroen T. Vermeulen (community) Approve
Review via email: mp+28871@code.launchpad.net

Description of the change

= Summary =
Creation of COPY (rebuild) archives doesn't respect the specified component.

This fix is required for an urgent cherry pick as the Linaro guys need a
rebuild for ARM set up.

== Pre-implementation notes ==
Chatted to Muharem who identified the culprit code which is an SQL query that
ignores the component specified for the copy operation.

== Implementation details ==
Add the component to the query if it's been specified. Also fix a bunch of
tests that relied on the broken behaviour.

== Tests ==
bin/test -cvv test_populatearchive

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/soyuz/model/packagecloner.py
  lib/lp/soyuz/scripts/tests/test_populatearchive.py

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Looks good.

review: Approve
Revision history for this message
Francis J. Lacoste (flacoste) :
review: Approve (release-critical)
Revision history for this message
James Westby (james-w) wrote :

Hi Julian,

You should fix the docstring of the test that asserts the behaviour
that we don't want.

57 When copying you specify a component, but that component doesn't
58 limit the packages copied. We create a source in main and one in
59 universe, and then copy with --component main, and expect to see
60 both sources in the copy.

Thanks,

James

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/soyuz/model/packagecloner.py'
--- lib/lp/soyuz/model/packagecloner.py 2010-01-12 08:36:58 +0000
+++ lib/lp/soyuz/model/packagecloner.py 2010-06-30 15:09:31 +0000
@@ -305,7 +305,7 @@
305 to be copied.305 to be copied.
306 """306 """
307 store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)307 store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
308 store.execute('''308 query = '''
309 INSERT INTO SourcePackagePublishingHistory (309 INSERT INTO SourcePackagePublishingHistory (
310 sourcepackagerelease, distroseries, status, component,310 sourcepackagerelease, distroseries, status, component,
311 section, archive, datecreated, datepublished, pocket)311 section, archive, datecreated, datepublished, pocket)
@@ -321,7 +321,12 @@
321 UTC_NOW, destination.pocket, origin.distroseries,321 UTC_NOW, destination.pocket, origin.distroseries,
322 PackagePublishingStatus.PENDING,322 PackagePublishingStatus.PENDING,
323 PackagePublishingStatus.PUBLISHED,323 PackagePublishingStatus.PUBLISHED,
324 origin.pocket, origin.archive))324 origin.pocket, origin.archive)
325
326 if origin.component:
327 query += "and spph.component = %s" % sqlvalues(origin.component)
328
329 store.execute(query)
325330
326 def packageSetDiff(self, origin, destination, logger=None):331 def packageSetDiff(self, origin, destination, logger=None):
327 """Please see `IPackageCloner`."""332 """Please see `IPackageCloner`."""
328333
=== modified file 'lib/lp/soyuz/scripts/tests/test_populatearchive.py'
--- lib/lp/soyuz/scripts/tests/test_populatearchive.py 2010-06-22 11:14:02 +0000
+++ lib/lp/soyuz/scripts/tests/test_populatearchive.py 2010-06-30 15:09:31 +0000
@@ -119,7 +119,6 @@
119 '--to-distribution', distro_name, '--to-suite', 'hoary',119 '--to-distribution', distro_name, '--to-suite', 'hoary',
120 '--to-archive', archive_name, '--to-user', 'salgado', '--reason',120 '--to-archive', archive_name, '--to-user', 'salgado', '--reason',
121 '"copy archive from %s"' % datetime.ctime(datetime.utcnow()),121 '"copy archive from %s"' % datetime.ctime(datetime.utcnow()),
122 '--component', 'main'
123 ]122 ]
124123
125 # Start archive population now!124 # Start archive population now!
@@ -206,7 +205,7 @@
206 def getScript(self, test_args=None):205 def getScript(self, test_args=None):
207 """Return an ArchivePopulator instance."""206 """Return an ArchivePopulator instance."""
208 if test_args is None:207 if test_args is None:
209 test_args = []208 test_args = []
210 script = ArchivePopulator("test copy archives", test_args=test_args)209 script = ArchivePopulator("test copy archives", test_args=test_args)
211 script.logger = QuietFakeLogger()210 script.logger = QuietFakeLogger()
212 script.txn = self.layer.txn211 script.txn = self.layer.txn
@@ -502,24 +501,24 @@
502 architectures=["386", "amd64"])501 architectures=["386", "amd64"])
503 self.checkBuilds(copy_archive, [package_info, package_info])502 self.checkBuilds(copy_archive, [package_info, package_info])
504503
505 def testCopyArchiveCopiesAllComponents(self):504 def testCopyArchiveCopiesRightComponents(self):
506 """Test that packages from all components are copied.505 """Test that packages from the right components are copied.
507506
508 When copying you specify a component, but that component doesn't507 When copying you specify a component, that component should
509 limit the packages copied. We create a source in main and one in508 limit the packages copied. We create a source in main and one in
510 universe, and then copy with --component main, and expect to see509 universe, and then copy with --component main, and expect to see
511 both sources in the copy.510 only main in the copy.
512 """511 """
513 package_infos = [512 package_info_universe = PackageInfo(
514 PackageInfo(
515 "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED,513 "bzr", "2.1", status=PackagePublishingStatus.PUBLISHED,
516 component="universe"),514 component="universe")
517 PackageInfo(515 package_info_main = PackageInfo(
518 "apt", "2.2", status=PackagePublishingStatus.PUBLISHED,516 "apt", "2.2", status=PackagePublishingStatus.PUBLISHED,
519 component="main")]517 component="main")
520 copy_archive, distroseries = self.makeCopyArchive(package_infos,518 package_infos_both = [package_info_universe, package_info_main]
521 component="main")519 copy_archive, distroseries = self.makeCopyArchive(
522 self.checkBuilds(copy_archive, package_infos)520 package_infos_both, component="main")
521 self.checkBuilds(copy_archive, [package_info_main])
523522
524 def testCopyFromPPA(self):523 def testCopyFromPPA(self):
525 """Test we can create a copy archive with a PPA as the source."""524 """Test we can create a copy archive with a PPA as the source."""
@@ -545,7 +544,7 @@
545 archive_name = self.getTargetArchiveName(distroseries.distribution)544 archive_name = self.getTargetArchiveName(distroseries.distribution)
546 copy_archive = self.copyArchive(545 copy_archive = self.copyArchive(
547 distroseries, archive_name, owner, from_user=ppa_owner_name,546 distroseries, archive_name, owner, from_user=ppa_owner_name,
548 from_archive=ppa_name)547 from_archive=ppa_name, component=package_info.component)
549 self.checkCopiedSources(548 self.checkCopiedSources(
550 copy_archive, distroseries, [package_info])549 copy_archive, distroseries, [package_info])
551550