launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/publishbinary-without-publications into lp:launchpad.

Commit message:
Do not attempt to publish binaries for disabled DistroArchSeries.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1077254 in Launchpad itself: "PublishingSet.publishBinaries OOPS if it results in no publications"
  https://bugs.launchpad.net/launchpad/+bug/1077254

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/publishbinary-without-publications/+merge/134527

As seen in OOPS-0255df70569a8fcc9fe45c645713e04c,
PublishingSet.publishBinaries OOPSes due to bad SQL if there are no
publications to create. This mostly happens when an architecture is
newly disabled.

--------------------------------------------------------------------

RULES

    Pre-implementation: wgrant
    * PublishingSet.publishBinaries must return a empty list of the
      binaries are for a disabled DistroArchSeries.
    * The publishBinaries method is the only callsite for
      expand_binary_requests, which promises to exclude unused
      architectures. The helper function failed to consider that
      an architecture was used, but was later disabled.


QA

    * Ask a webops to
      * Visit https://qastaging.launchpad.net/ubuntu/precise/armel/+admin
      * Disabled armel
      * For qastaging,
        ./cronscripts/publish-ftpmaster.py -v -d ubuntu 
      * Verify the log does not container oopses
      * Visit https://qastaging.launchpad.net/ubuntu/precise/armel/+admin
      * Enabled armel again.

LINT

    lib/lp/soyuz/model/publishing.py
    lib/lp/soyuz/tests/test_publishing.py


LoC

    I have a 3,000 line credit this week.


TEST

    ./bin/test -vv -t TestPublishBinaries lp.soyuz.tests.test_publishing


IMPLEMENTATION

I updated expand_binary_requests() to ignore disabled DistroArchSeries.
I then updated publishBinaries() to return early if the expanded
binaries is empty.
    lib/lp/soyuz/model/publishing.py
    lib/lp/soyuz/tests/test_publishing.py
-- 
https://code.launchpad.net/~sinzui/launchpad/publishbinary-without-publications/+merge/134527
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/publishbinary-without-publications into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2012-10-30 01:46:10 +0000
+++ lib/lp/soyuz/model/publishing.py	2012-11-15 17:26:27 +0000
@@ -1396,7 +1396,8 @@
         else:
             target_archs = archs
         for target_arch in target_archs:
-            expanded.append((target_arch, bpr, overrides))
+            if target_arch.enabled:
+                expanded.append((target_arch, bpr, overrides))
     return expanded
 
 
@@ -1456,6 +1457,10 @@
         # Expand the dict of binaries into a list of tuples including the
         # architecture.
         expanded = expand_binary_requests(distroseries, binaries)
+        if len(expanded) == 0:
+            # The binaries are for a disabled DistroArchSeries or for
+            # an unsupported architecture.
+            return []
 
         # Find existing publications.
         # We should really be able to just compare BPR.id, but

=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py	2012-06-19 17:09:47 +0000
+++ lib/lp/soyuz/tests/test_publishing.py	2012-11-15 17:26:27 +0000
@@ -1599,6 +1599,21 @@
             set((target_das_a, target_das_b)),
             set(bpph.distroarchseries for bpph in bpphs))
 
+    def test_architecture_disabled(self):
+        # An empty list is return if the DistroArchSeries was disabled.
+        arch_tag = self.factory.getUniqueString('arch-')
+        orig_das = self.factory.makeDistroArchSeries(
+            architecturetag=arch_tag)
+        target_das = self.factory.makeDistroArchSeries(
+            architecturetag=arch_tag)
+        build = self.factory.makeBinaryPackageBuild(distroarchseries=orig_das)
+        bpr = self.factory.makeBinaryPackageRelease(
+            build=build, architecturespecific=True)
+        target_das.enabled = False
+        args = self.makeArgs([bpr], target_das.distroseries)
+        results = getUtility(IPublishingSet).publishBinaries(**args)
+        self.assertEqual([], results)
+
     def test_does_not_duplicate(self):
         # An attempt to copy something for a second time is ignored.
         bpr = self.factory.makeBinaryPackageRelease()