Merge lp:~cjwatson/launchpad/archive-copy-packages-source-series into lp:launchpad

[Colin Watson]

[r=stevenk][bug=913722] Add an optional from_series parameter to Archive.copyPackages and Archive.syncSources.

== Summary ==

Archive.copyPackages doesn't allow specifying a source distroseries. This means that autosyncs from Debian using this method copy the newest version available no matter what - up to and including versions in experimental!

  https://lists.ubuntu.com/archives/ubuntu-release/2012-January/000672.html

== Proposed fix ==

Add an optional from_series parameter.

== Implementation details ==

I made from_series be a distroseries name rather than an object, matching to_series. This necessitated some mild tedium in Archive._text_to_series to fetch series from the source distribution.

== Tests ==

bin/test -vvct test_archive_webservice -t soyuz/doc/archive.txt -t soyuz.tests.test_archive

== Demo and Q/A ==

Find a package in dogfood that has dogfood/Ubuntu/oneiric (or precise) < dogfood/Debian/wheezy < dogfood/Debian/sid; hopefully there's at least one. Use Archive.copyPackages(from_series='wheezy') and make sure that the wheezy version gets copied and not the sid version.

== lint ==

./lib/lp/soyuz/model/archive.py
     348: redefinition of function 'private' from line 344

Just lint being confused about setter properties.

[Steve Kowalik (stevenk)]

This looks excellent, thank you.

review: Approve (code)

=== modified file 'lib/lp/soyuz/browser/tests/test_archive_webservice.py'
--- lib/lp/soyuz/browser/tests/test_archive_webservice.py	2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_webservice.py	2012-01-09 14:58:27 +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).
 
 __metaclass__ = type
@@ -338,13 +338,14 @@
         with person_logged_in(target_archive.owner):
             target_archive.newComponentUploader(uploader_dude, "universe")
         transaction.commit()
-        return (source_archive, source_name, target_archive, to_pocket,
-                to_series, uploader_dude, sponsored_dude, version)
+        return (source, source_archive, source_name, target_archive,
+                to_pocket, to_series, uploader_dude, sponsored_dude, version)
 
     def test_copyPackage(self):
         """Basic smoke test"""
-        (source_archive, source_name, target_archive, to_pocket, to_series,
-         uploader_dude, sponsored_dude, version) = self.setup_data()
+        (source, source_archive, source_name, target_archive, to_pocket,
+         to_series, uploader_dude, sponsored_dude,
+         version) = self.setup_data()
 
         ws_target_archive = self.wsObject(target_archive, user=uploader_dude)
         ws_source_archive = self.wsObject(source_archive)
@@ -363,8 +364,9 @@
 
     def test_copyPackages(self):
         """Basic smoke test"""
-        (source_archive, source_name, target_archive, to_pocket, to_series,
-         uploader_dude, sponsored_dude, version) = self.setup_data()
+        (source, source_archive, source_name, target_archive, to_pocket,
+         to_series, uploader_dude, sponsored_dude,
+         version) = self.setup_data()
 
         ws_target_archive = self.wsObject(target_archive, user=uploader_dude)
         ws_source_archive = self.wsObject(source_archive)
@@ -373,7 +375,8 @@
         ws_target_archive.copyPackages(
             source_names=[source_name], from_archive=ws_source_archive,
             to_pocket=to_pocket.name, to_series=to_series.name,
-            include_binaries=False, sponsored=ws_sponsored_dude)
+            from_series=source.distroseries.name, include_binaries=False,
+            sponsored=ws_sponsored_dude)
         transaction.commit()
 
         job_source = getUtility(IPlainPackageCopyJobSource)

=== modified file 'lib/lp/soyuz/doc/archive.txt'
--- lib/lp/soyuz/doc/archive.txt	2011-12-30 06:14:56 +0000
+++ lib/lp/soyuz/doc/archive.txt	2012-01-09 14:58:27 +0000
@@ -2342,6 +2342,25 @@
     ...
     NoSuchDistroSeries: No such distribution series: 'badseries'.
 
+If a package exists in multiple distroseries, we can use the `from_series`
+parameter to select the distroseries to synchronise from:
+
+    >>> test_publisher.addFakeChroots(breezy_autotest)
+    >>> discard = test_publisher.getPubSource(
+    ...     sourcename="package-multiseries", version="1.0",
+    ...     archive=cprov.archive, status=PackagePublishingStatus.PUBLISHED)
+    >>> discard = test_publisher.getPubSource(
+    ...     sourcename="package-multiseries", version="1.1",
+    ...     distroseries=breezy_autotest, archive=cprov.archive,
+    ...     status=PackagePublishingStatus.PUBLISHED)
+    >>> mark.archive.syncSources(
+    ...     ["package-multiseries"], cprov.archive, "release",
+    ...     from_series="hoary", person=mark)
+    >>> mark_multiseries = mark.archive.getPublishedSources(
+    ...     name="package-multiseries").one()
+    >>> print mark_multiseries.sourcepackagerelease.version
+    1.0
+
 We can also specify a single source to be copied with the `syncSource`
 call.  This allows a version to be specified so older versions can be
 pulled.

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2012-01-09 14:58:27 +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).
 
 # pylint: disable-msg=E0211,E0213
@@ -1268,7 +1268,14 @@
         from_archive=Reference(schema=Interface),
         #Really IArchive, see below
         to_pocket=TextLine(title=_("Pocket name")),
-        to_series=TextLine(title=_("Distroseries name"), required=False),
+        to_series=TextLine(
+            title=_("Distroseries name"),
+            description=_("The distro series to copy packages into."),
+            required=False),
+        from_series=TextLine(
+            title=_("Distroseries name"),
+            description=_("The distro series to copy packages from."),
+            required=False),
         include_binaries=Bool(
             title=_("Include Binaries"),
             description=_("Whether or not to copy binaries already built for"
@@ -1282,7 +1289,7 @@
     @export_write_operation()
     @operation_for_version('devel')
     def copyPackages(source_names, from_archive, to_pocket, person,
-                     to_series=None, include_binaries=False,
+                     to_series=None, from_series=None, include_binaries=False,
                      sponsored=None):
         """Copy multiple named sources into this archive from another.
 
@@ -1298,6 +1305,7 @@
         :param from_archive: the source archive from which to copy.
         :param to_pocket: the target pocket (as a string).
         :param to_series: the target distroseries (as a string).
+        :param from_series: the source distroseries (as a string).
         :param include_binaries: optional boolean, controls whether or not
             the published binaries for each given source should also be
             copied along with the source.
@@ -1325,7 +1333,14 @@
         from_archive=Reference(schema=Interface),
         #Really IArchive, see below
         to_pocket=TextLine(title=_("Pocket name")),
-        to_series=TextLine(title=_("Distroseries name"), required=False),
+        to_series=TextLine(
+            title=_("Distroseries name"),
+            description=_("The distro series to copy packages into."),
+            required=False),
+        from_series=TextLine(
+            title=_("Distroseries name"),
+            description=_("The distro series to copy packages from."),
+            required=False),
         include_binaries=Bool(
             title=_("Include Binaries"),
             description=_("Whether or not to copy binaries already built for"
@@ -1335,7 +1350,7 @@
     # Source_names is a string because exporting a SourcePackageName is
     # rather nonsensical as it only has id and name columns.
     def syncSources(source_names, from_archive, to_pocket, to_series=None,
-                    include_binaries=False, person=None):
+                    from_series=None, include_binaries=False, person=None):
         """Synchronise (copy) named sources into this archive from another.
 
         It will copy the most recent PUBLISHED versions of the named
@@ -1350,6 +1365,7 @@
         :param from_archive: the source archive from which to copy.
         :param to_pocket: the target pocket (as a string).
         :param to_series: the target distroseries (as a string).
+        :param from_series: the source distroseries (as a string).
         :param include_binaries: optional boolean, controls whether or not
             the published binaries for each given source should also be
             copied along with the source.

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-01-06 11:08:30 +0000
+++ lib/lp/soyuz/model/archive.py	2012-01-09 14:58:27 +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).
 
 # pylint: disable-msg=E0611,W0212
@@ -1526,11 +1526,12 @@
             reason)
 
     def syncSources(self, source_names, from_archive, to_pocket,
-                    to_series=None, include_binaries=False, person=None):
+                    to_series=None, from_series=None, include_binaries=False,
+                    person=None):
         """See `IArchive`."""
         # Find and validate the source package names in source_names.
         sources = self._collectLatestPublishedSources(
-            from_archive, source_names)
+            from_archive, from_series, source_names)
         self._copySources(
             sources, to_pocket, to_series, include_binaries,
             person=person)
@@ -1591,13 +1592,13 @@
             sponsored=sponsored)
 
     def copyPackages(self, source_names, from_archive, to_pocket,
-                     person, to_series=None, include_binaries=None,
-                     sponsored=None):
+                     person, to_series=None, from_series=None,
+                     include_binaries=None, sponsored=None):
         """See `IArchive`."""
         self._checkCopyPackageFeatureFlags()
 
         sources = self._collectLatestPublishedSources(
-            from_archive, source_names)
+            from_archive, from_series, source_names)
         if not sources:
             raise CannotCopy(
                 "None of the supplied package names are published")
@@ -1635,13 +1636,16 @@
             copy_policy=PackageCopyPolicy.MASS_SYNC,
             include_binaries=include_binaries, sponsored=sponsored)
 
-    def _collectLatestPublishedSources(self, from_archive, source_names):
+    def _collectLatestPublishedSources(self, from_archive, from_series,
+                                       source_names):
         """Private helper to collect the latest published sources for an
         archive.
 
         :raises NoSuchSourcePackageName: If any of the source_names do not
             exist.
         """
+        from_series_obj = self._text_to_series(
+            from_series, distribution=from_archive.distribution)
         # XXX bigjools bug=810421
         # This code is inefficient.  It should try to bulk load all the
         # sourcepackagenames and publications instead of iterating.
@@ -1654,7 +1658,7 @@
             # Grabbing the item at index 0 ensures it's the most recent
             # publication.
             published_sources = from_archive.getPublishedSources(
-                name=name, exact_match=True,
+                name=name, distroseries=from_series_obj, exact_match=True,
                 status=(PackagePublishingStatus.PUBLISHED,
                         PackagePublishingStatus.PENDING))
             first_source = published_sources.first()
@@ -1662,10 +1666,12 @@
                 sources.append(first_source)
         return sources
 
-    def _text_to_series(self, to_series):
+    def _text_to_series(self, to_series, distribution=None):
+        if distribution is None:
+            distribution = self.distribution
         if to_series is not None:
             result = getUtility(IDistroSeriesSet).queryByName(
-                self.distribution, to_series)
+                distribution, to_series)
             if result is None:
                 raise NoSuchDistroSeries(to_series)
             series = result

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2011-12-30 06:14:56 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2012-01-09 14:58:27 +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).
 
 """Test Archive features."""
@@ -443,7 +443,7 @@
             ["1.0", "1.1", "2.0"],
             [sourcepackagename, sourcepackagename, other_spn])
         pubs = removeSecurityProxy(archive)._collectLatestPublishedSources(
-            archive, ["foo"])
+            archive, None, ["foo"])
         self.assertEqual(1, len(pubs))
         self.assertEqual('1.1', pubs[0].source_package_version)
 
@@ -460,10 +460,32 @@
             ["1.0", "1.1", "2.0"],
             [sourcepackagename, sourcepackagename, other_spn])
         pubs = removeSecurityProxy(archive)._collectLatestPublishedSources(
-            archive, ["foo"])
+            archive, None, ["foo"])
         self.assertEqual(1, len(pubs))
         self.assertEqual('1.0', pubs[0].source_package_version)
 
+    def test_collectLatestPublishedSources_multiple_distroseries(self):
+        # The helper method selects the correct publication from multiple
+        # distroseries.
+        sourcepackagename = self.factory.makeSourcePackageName(name="foo")
+        archive = self.factory.makeArchive()
+        distroseries_one = self.factory.makeDistroSeries(
+            distribution=archive.distribution)
+        distroseries_two = self.factory.makeDistroSeries(
+            distribution=archive.distribution)
+        self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagename=sourcepackagename, archive=archive,
+            distroseries=distroseries_one, version="1.0",
+            status=PackagePublishingStatus.PUBLISHED)
+        self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagename=sourcepackagename, archive=archive,
+            distroseries=distroseries_two, version="1.1",
+            status=PackagePublishingStatus.PUBLISHED)
+        pubs = removeSecurityProxy(archive)._collectLatestPublishedSources(
+            archive, distroseries_one.name, ["foo"])
+        self.assertEqual(1, len(pubs))
+        self.assertEqual("1.0", pubs[0].source_package_version)
+
 
 class TestArchiveCanUpload(TestCaseWithFactory):
     """Test the various methods that verify whether uploads are allowed to
@@ -2302,6 +2324,45 @@
             to_pocket.name, to_series=to_series.name, include_binaries=False,
             person=person)
 
+    def test_copyPackages_with_multiple_distroseries(self):
+        # The from_series parameter selects a source distroseries.
+        (source, source_archive, source_name, target_archive, to_pocket,
+         to_series, version) = self._setup_copy_data()
+        new_distroseries = self.factory.makeDistroSeries(
+            distribution=source_archive.distribution)
+        new_version = "%s.1" % version
+        new_spr = self.factory.makeSourcePackageRelease(
+            archive=source_archive, distroseries=new_distroseries,
+            sourcepackagename=source_name, version=new_version)
+        self.factory.makeSourcePackagePublishingHistory(
+            archive=source_archive, distroseries=new_distroseries,
+            sourcepackagerelease=new_spr)
+
+        with person_logged_in(target_archive.owner):
+            target_archive.copyPackages(
+                [source_name], source_archive, to_pocket.name,
+                to_series=to_series.name,
+                from_series=source.distroseries.name, include_binaries=False,
+                person=target_archive.owner)
+
+        # There should be one copy job with the correct version.
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        copy_job = job_source.getActiveJobs(target_archive).one()
+        self.assertEqual(version, copy_job.package_version)
+
+        # If we now do another copy without the from_series parameter, it
+        # selects the newest version in the source archive.
+        with person_logged_in(target_archive.owner):
+            target_archive.copyPackages(
+                [source_name], source_archive, to_pocket.name,
+                to_series=to_series.name, include_binaries=False,
+                person=target_archive.owner)
+
+        copy_jobs = job_source.getActiveJobs(target_archive)
+        self.assertEqual(2, copy_jobs.count())
+        self.assertEqual(copy_job, copy_jobs[0])
+        self.assertEqual(new_version, copy_jobs[1].package_version)
+
 
 class TestgetAllPublishedBinaries(TestCaseWithFactory):