Merge lp:~cjwatson/launchpad/remove-query-distro-pending-suites into lp:launchpad

[Colin Watson]

Remove now-pointless indirection through LpQueryDistro pending_suites, and remove several other obsolete actions.

== Summary ==

The "pending_suites" action in the lp-query-distro.py script (and the LpQueryDistro script class) made sense when the ftpmaster publisher script was a shell script; pending_suites is used in exactly one place, to discover whether any security suites are pending publication (see r6303.1.2). However, since it was rewritten in Python, it's just pointless indirection, not to mention inefficient because it means setting up and tearing down a LaunchpadScript instance. PublishFTPMaster can just talk to the database directly. We can refactor this.

I also noticed that several other LpQueryDistro actions are no longer used, and removed them; and likewise from a mock version of lp-query-distro.py in use in the cron.germinate tests. (It probably wouldn't be desperately hard to get rid of the entire script at this point, but that's a matter for another day and another branch.)

I think it's extremely unlikely that we'll need this code in future and regret removing it. lp-query-distro.py was always mainly for shell scripts that nobody had got round to rewriting in Python yet, and anything new that needs to know about this stuff is very likely to be in Python. There was a secondary goal at one point of helping out local archive administration scripts on ftpmaster, but we have nothing left that uses lp-query-distro.py and anything new we write will be based on the API.

== Tests ==

bin/test -vvct test_lpquerydistro -t test_publish_ftpmaster -t test_generate_contents_files -t test_cron_germinate

== lint ==

Just one false positive:

./scripts/ftpmaster-tools/lp-query-distro.py
      23: '_pythonpath' imported but unused

[Jeroen T. Vermeulen (jtv)]

This makes me very happy. It's one thing to say “we should look into cleaning this up some day,” but actually doing it is another. It's also a very well-written merge proposal and it very much looks like a good branch.

Just a small note, partly because it bothered me to see one piece of code growing a bit instead of shrinking:

=== modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
--- lib/lp/archivepublisher/scripts/publish_ftpmaster.py 2012-02-28 04:24:19 +0000
+++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py 2012-04-07 14:59:21 +0000

@@ -329,12 +324,20 @@
     def getDirtySuites(self, distribution):
         """Return list of suites that have packages pending publication."""
         self.logger.debug("Querying which suites are pending publication...")
- query_distro = LpQueryDistro(
- test_args=['-d', distribution.name, "pending_suites"],
- logger=self.logger)
- receiver = StoreArgument()
- query_distro.runAction(presenter=receiver)
- return receiver.argument.split()
+
+ pending_suites = set()
+ pending_sources = distribution.main_archive.getPublishedSources(
+ status=PackagePublishingStatus.PENDING)
+ for pub in pending_sources:
+ pending_suites.add((pub.distroseries, pub.pocket))
*
+ pending_binaries = distribution.main_archive.getAllPublishedBinaries(
+ status=PackagePublishingStatus.PENDING)
+ for pub in pending_binaries:
+ pending_suites.add((pub.distroarchseries.distroseries, pub.pocket))
+
+ return [distroseries.name + pocketsuffix[pocket]
+ for distroseries, pocket in pending_suites]

As far as I can make out, the return value will duplicate a suite name between architectures (where "source" is effectively also an architecture). BI'll assume that's not what you want.

If it weren't for performance, the code could have been written more simply as:

        archive = distribution.main_archive
        pending = PackagePublishingStatus.PENDING
        pending_sources = list(archive.getPublishedSources(pending))
        pending_binaries = list(archive.getAllPublishedBinaries(pending))
        return set(
            pub.distroseries.name + pocketsuffix[pub.pocket]
            for pub in pending_sources + pending_binaries)

If the pending_binaries bit could fetch a large number of distroarchseries from the database — I'm assuming the number of distroseries will be small and probably already in memory — you can bulk-load those using load_related using a single query:

        archive = distribution.main_archive
        pending = PackagePublishingStatus.PENDING
        pending_sources = list(archive.getPublishedSources(pending))
        pending_binaries = list(archive.getAllPublishedBinaries(pending))
        load_related(DistroArchSeries, pending_binaries, 'distroarchseries_id')
        return set(
            pub.distroseries.name + pocketsuffix[pub.pocket]
            for pub in pending_sources + pending_binaries)

This does return a set instead of a list. But I can't think of any problems that might cause.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Wow. It's a good thing I was substantially done with that review, because it submitted by accident as I was trying to turn off a caps lock that had gotten accidentally enabled. I have no idea what made Chromium submit the form. You can still tell where I was typing at “BI'll.”

[Colin Watson (cjwatson)]

On Mon, Apr 09, 2012 at 05:38:18AM -0000, Jeroen T. Vermeulen wrote:
> Just a small note, partly because it bothered me to see one piece of code growing a bit instead of shrinking:

I should point out for the record that I just moved this code around; I
didn't actually write it originally. (That said, obviously I don't
object to improving it.)

> As far as I can make out, the return value will duplicate a suite name between architectures (where "source" is effectively also an architecture). BI'll assume that's not what you want.

I can't see how that's the case. pending_suites is a set, so that deals
with deduplication.

Thanks for the suggestions about rewriting this section of code, though.
I hadn't previously known anything about the bulk loading interfaces
inside Launchpad, so that was a good thing to learn about. I can't
imagine that it loads a particularly large number of distroarchseries -
this script only touches Ubuntu, and there are only 32 active
distroarchseries in Ubuntu right now - but it doesn't hurt to load them
in one go anyway. I've pushed a modified version for your delectation
and delight.

[Jeroen T. Vermeulen (jtv)]

Thanks. I was mistaken about the duplications.

=== modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
--- lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2012-02-28 04:24:19 +0000
+++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2012-04-10 00:33:18 +0000
@@ -21,16 +21,20 @@
 from lp.registry.interfaces.pocket import pocketsuffix
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.config import config
+from lp.services.database.bulk import load_related
 from lp.services.scripts.base import (
     LaunchpadCronScript,
     LaunchpadScriptFailure,
     )
 from lp.services.utils import file_exists
-from lp.soyuz.enums import ArchivePurpose
+from lp.soyuz.enums import (
+    ArchivePurpose,
+    PackagePublishingStatus,
+    )
+from lp.soyuz.model.distroarchseries import DistroArchSeries
 from lp.soyuz.scripts.custom_uploads_copier import CustomUploadsCopier
 from lp.soyuz.scripts.processaccepted import ProcessAccepted
 from lp.soyuz.scripts.publishdistro import PublishDistro
-from lp.soyuz.scripts.querydistro import LpQueryDistro
 
 
 def get_publishable_archives(distribution):
@@ -121,13 +125,6 @@
     return {"PATH": '"$PATH":%s' % shell_quote(scripts_dir)}
 
 
-class StoreArgument:
-    """Helper class: receive argument and store it."""
-
-    def __call__(self, argument):
-        self.argument = argument
-
-
 def find_run_parts_dir(distro, parts):
     """Find the requested run-parts directory, if it exists."""
     run_parts_location = config.archivepublisher.run_parts_location
@@ -327,14 +324,18 @@
         script.main()
 
     def getDirtySuites(self, distribution):
-        """Return list of suites that have packages pending publication."""
+        """Return set of suites that have packages pending publication."""
         self.logger.debug("Querying which suites are pending publication...")
-        query_distro = LpQueryDistro(
-            test_args=['-d', distribution.name, "pending_suites"],
-            logger=self.logger)
-        receiver = StoreArgument()
-        query_distro.runAction(presenter=receiver)
-        return receiver.argument.split()
+
+        archive = distribution.main_archive
+        pending = PackagePublishingStatus.PENDING
+        pending_sources = list(archive.getPublishedSources(status=pending))
+        pending_binaries = list(archive.getAllPublishedBinaries(
+            status=pending))
+        load_related(DistroArchSeries, pending_binaries, 'distroarchseriesID')
+        return set(
+            pub.distroseries.name + pocketsuffix[pub.pocket]
+            for pub in pending_sources + pending_binaries)
 
     def getDirtySecuritySuites(self, distribution):
         """List security suites with pending publications."""

=== modified file 'lib/lp/archivepublisher/tests/test_publish_ftpmaster.py'
--- lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2012-01-01 02:58:52 +0000
+++ lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2012-04-10 00:33:18 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test publish-ftpmaster cron script."""
@@ -400,7 +400,7 @@
         distro = spph.distroseries.distribution
         script = self.makeScript(spph.distroseries.distribution)
         script.setUp()
-        self.assertEqual(
+        self.assertContentEqual(
             [name_spph_suite(spph)], script.getDirtySuites(distro))
 
     def test_getDirtySuites_returns_suites_with_pending_publications(self):
@@ -423,7 +423,7 @@
         distro = spph.distroseries.distribution
         script = self.makeScript(spph.distroseries.distribution)
         script.setUp()
-        self.assertEqual([], script.getDirtySuites(distro))
+        self.assertContentEqual([], script.getDirtySuites(distro))
 
     def test_getDirtySecuritySuites_returns_security_suites(self):
         distro = self.makeDistroWithPublishDirectory()

=== modified file 'lib/lp/soyuz/scripts/querydistro.py'
--- lib/lp/soyuz/scripts/querydistro.py	2012-01-19 03:06:04 +0000
+++ lib/lp/soyuz/scripts/querydistro.py	2012-04-10 00:33:18 +0000
@@ -7,7 +7,6 @@
 
 __all__ = ['LpQueryDistro']
 
-from lp.registry.interfaces.pocket import pocketsuffix
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.scripts.base import (
     LaunchpadScript,
@@ -17,7 +16,6 @@
     build_package_location,
     PackageLocationError,
     )
-from lp.soyuz.enums import PackagePublishingStatus
 
 
 class LpQueryDistro(LaunchpadScript):
@@ -28,9 +26,7 @@
 
         Also initialize the list 'allowed_arguments'.
         """
-        self.allowed_actions = [
-            'current', 'development', 'supported', 'pending_suites', 'archs',
-            'official_archs', 'nominated_arch_indep', 'pocket_suffixes']
+        self.allowed_actions = ['development', 'supported', 'archs']
         self.usage = '%%prog <%s>' % ' | '.join(self.allowed_actions)
         LaunchpadScript.__init__(self, *args, **kwargs)
 
@@ -122,23 +118,6 @@
                 "Action does not accept defined suite.")
 
     @property
-    def current(self):
-        """Return the name of the CURRENT distroseries.
-
-        It is restricted for the context distribution.
-
-        It may raise LaunchpadScriptFailure if a suite was passed on the
-        command-line or if not CURRENT distroseries was found.
-        """
-        self.checkNoSuiteDefined()
-        series = self.location.distribution.getSeriesByStatus(
-            SeriesStatus.CURRENT)
-        if not series:
-            raise LaunchpadScriptFailure("No CURRENT series.")
-
-        return series[0].name
-
-    @property
     def development(self):
         """Return the name of the DEVELOPMENT distroseries.
 
@@ -197,28 +176,6 @@
         return " ".join(supported_series)
 
     @property
-    def pending_suites(self):
-        """Return the suite names containing PENDING publication.
-
-        It check for sources and/or binary publications.
-        """
-        self.checkNoSuiteDefined()
-        pending_suites = set()
-        pending_sources = self.location.archive.getPublishedSources(
-            status=PackagePublishingStatus.PENDING)
-        for pub in pending_sources:
-            pending_suites.add((pub.distroseries, pub.pocket))
-
-        pending_binaries = self.location.archive.getAllPublishedBinaries(
-            status=PackagePublishingStatus.PENDING)
-        for pub in pending_binaries:
-            pending_suites.add(
-                (pub.distroarchseries.distroseries, pub.pocket))
-
-        return " ".join([distroseries.name + pocketsuffix[pocket]
-                         for distroseries, pocket in pending_suites])
-
-    @property
     def archs(self):
         """Return a space-separated list of architecture tags.
 
@@ -226,35 +183,3 @@
         """
         architectures = self.location.distroseries.architectures
         return " ".join(arch.architecturetag for arch in architectures)
-
-    @property
-    def official_archs(self):
-        """Return a space-separated list of official architecture tags.
-
-        It is restricted to the context distribution and suite.
-        """
-        architectures = self.location.distroseries.architectures
-        return " ".join(arch.architecturetag
-                        for arch in architectures
-                        if arch.official)
-
-    @property
-    def nominated_arch_indep(self):
-        """Return the nominated arch indep architecture tag.
-
-        It is restricted to the context distribution and suite.
-        """
-        series = self.location.distroseries
-        return series.nominatedarchindep.architecturetag
-
-    @property
-    def pocket_suffixes(self):
-        """Return a space-separated list of non-empty pocket suffixes.
-
-        The RELEASE pocket (whose suffix is the empty string) is omitted.
-
-        The returned space-separated string is ordered alphabetically.
-        """
-        sorted_non_empty_suffixes = sorted(
-            suffix for suffix in pocketsuffix.values() if suffix != '')
-        return " ".join(sorted_non_empty_suffixes)

=== modified file 'lib/lp/soyuz/scripts/tests/germinate-test-data/mock-lp-root/scripts/ftpmaster-tools/lp-query-distro.py'
--- lib/lp/soyuz/scripts/tests/germinate-test-data/mock-lp-root/scripts/ftpmaster-tools/lp-query-distro.py	2011-10-15 02:57:02 +0000
+++ lib/lp/soyuz/scripts/tests/germinate-test-data/mock-lp-root/scripts/ftpmaster-tools/lp-query-distro.py	2012-04-10 00:33:18 +0000
@@ -8,8 +8,8 @@
 
 
 def error_and_exit():
-    sys.stderr.write("ERROR: I'm a mock, I only support 'development' "
-                        "and 'supported' as argument\n")
+    sys.stderr.write("ERROR: I'm a mock, I only support 'supported' as "
+                     "argument\n")
     sys.exit(1)
 
 
@@ -18,12 +18,8 @@
     # test for it and error if it looks wrong
     if len(args) == 2:
         distro = args[1]
-        if distro == "development":
-                return "natty"
-        elif distro == "supported":
-                return "hardy jaunty karmic lucid maverick"
-    elif len(args) == 4 and args[1] == '-s' and args[3] == 'archs':
-        return "i386 amd64 powerpc armel"
+        if distro == "supported":
+            return "hardy jaunty karmic lucid maverick"
     error_and_exit()
 
 

=== modified file 'lib/lp/soyuz/scripts/tests/test_lpquerydistro.py'
--- lib/lp/soyuz/scripts/tests/test_lpquerydistro.py	2012-01-19 03:09:38 +0000
+++ lib/lp/soyuz/scripts/tests/test_lpquerydistro.py	2012-04-10 00:33:18 +0000
@@ -50,15 +50,15 @@
         Check that:
          * return code is ZERO,
          * standard error is empty
-         * standard output contains only the 'current distroseries' name
+         * standard output contains only the 'development distroseries' name
         """
         returncode, out, err = self.runLpQueryDistro(
-            extra_args=['current'])
+            extra_args=['development'])
 
         self.assertEqual(
             0, returncode, "\nScript Failed:%s\nStdout:\n%s\nStderr\n%s\n"
             % (returncode, out, err))
-        self.assertEqual(out.strip(), 'warty')
+        self.assertEqual(out.strip(), 'hoary')
         self.assertEqual(err.strip(), '')
 
     def testMissingAction(self):
@@ -108,7 +108,7 @@
          * standard error contains additional information about the failure.
         """
         returncode, out, err = self.runLpQueryDistro(
-            extra_args=['-s', 'hoary', 'current'])
+            extra_args=['-s', 'warty', 'development'])
 
         self.assertEqual(
             1, returncode,
@@ -140,15 +140,6 @@
         """
         self.test_output = '%s' % args
 
-    def testSuccessfullyAction(self):
-        """Check if the 'current' action is executed sucessfully."""
-        helper = self.getLpQueryDistro(test_args=['current'])
-        helper.runAction(presenter=self.presenter)
-        warty = self.ubuntu['warty']
-        self.assertEqual(warty.status.name, 'CURRENT')
-        self.assertEqual(helper.location.distribution.name, u'ubuntu')
-        self.assertEqual(self.test_output, u'warty')
-
     def testDevelopmentAndFrozenDistroSeries(self):
         """The 'development' action should cope with FROZEN distroseries."""
         helper = self.getLpQueryDistro(test_args=['development'])
@@ -185,7 +176,8 @@
         Some actions do not allow passing 'suite'.
         See testActionswithDefinedSuite for further information.
         """
-        helper = self.getLpQueryDistro(test_args=['-s', 'hoary', 'current'])
+        helper = self.getLpQueryDistro(
+            test_args=['-s', 'warty', 'development'])
         self.assertRaises(LaunchpadScriptFailure,
                           helper.runAction, self.presenter)
 
@@ -213,23 +205,16 @@
     def testActionsWithUndefinedSuite(self):
         """Check the actions supposed to work with undefined suite.
 
-        Only 'current', 'development' and 'supported' work with undefined
-        suite.
-        The other actions ('archs', 'official_arch', 'nominated_arch_indep')
-        will assume the CURRENT distroseries in context.
+        Only 'development' and 'supported' work with undefined suite.
+        The other actions ('archs') will assume the CURRENT distroseries in
+        context.
         """
         helper = self.getLpQueryDistro(test_args=[])
         helper._buildLocation()
 
-        self.assertEqual(helper.current, 'warty')
         self.assertEqual(helper.development, 'hoary')
         self.assertEqual(helper.supported, 'hoary warty')
-        self.assertEqual(helper.pending_suites, 'warty')
         self.assertEqual(helper.archs, 'hppa i386')
-        self.assertEqual(helper.official_archs, 'i386')
-        self.assertEqual(helper.nominated_arch_indep, 'i386')
-        self.assertEqual(helper.pocket_suffixes,
-                         '-backports -proposed -security -updates')
 
     def assertAttributeRaisesScriptFailure(self, obj, attr_name):
         """Asserts if accessing the given attribute name fails.
@@ -242,20 +227,13 @@
     def testActionsWithDefinedSuite(self):
         """Opposite of  testActionsWithUndefinedSuite.
 
-        Only some actions ('archs', 'official_arch', 'nominated_arch_indep',
-        and pocket_suffixes) work with defined suite, the other actions
-        ('current', 'development' and 'supported') will raise
+        Only some actions ('archs') work with defined suite, the other
+        actions ('development' and 'supported') will raise
         LaunchpadScriptError if the suite is defined.
         """
         helper = self.getLpQueryDistro(test_args=['-s', 'warty'])
         helper._buildLocation()
 
-        self.assertAttributeRaisesScriptFailure(helper, 'current')
         self.assertAttributeRaisesScriptFailure(helper, 'development')
         self.assertAttributeRaisesScriptFailure(helper, 'supported')
-        self.assertAttributeRaisesScriptFailure(helper, 'pending_suites')
         self.assertEqual(helper.archs, 'hppa i386')
-        self.assertEqual(helper.official_archs, 'i386')
-        self.assertEqual(helper.nominated_arch_indep, 'i386')
-        self.assertEqual(helper.pocket_suffixes,
-                         '-backports -proposed -security -updates')

=== modified file 'scripts/ftpmaster-tools/lp-query-distro.py'
--- scripts/ftpmaster-tools/lp-query-distro.py	2012-01-19 03:09:38 +0000
+++ scripts/ftpmaster-tools/lp-query-distro.py	2012-04-10 00:33:18 +0000
@@ -9,15 +9,10 @@
    It should provide an easy way to retrieve current information from
    Launchpad when using plain shell scripts, for example:
 
-   * CURRENT distroseries name: `./ubuntu-helper.py -d ubuntu current`
    * DEVELOPMENT distroseries name:
        `./ubuntu-helper.py -d ubuntu development`
    * Distroseries architectures:
        `./lp-query-distro.py -d ubuntu -s feisty archs`
-   * Distroseries official architectures:
-       `./lp-query-distro.py -d ubuntu -s feisty official_archs`
-   * Distroseries nominated-arch-indep:
-       `./lp-query-distro.py -d ubuntu -s feisty nominated_arch_indep`
 
    Standard Output will carry the successfully executed information and
    exit_code will be ZERO.