launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/more-extra-override-filenames into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #966092 in Launchpad itself: "more-extra.override.* generation doesn't match use"
  https://bugs.launchpad.net/launchpad/+bug/966092

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/more-extra-override-filenames/+merge/99504

== Summary ==

Remove another step from NewReleaseCycleProcess and remove one of the remaining reasons why Ubuntu archive administrators still routinely make use of access to lp_publish, by fixing a glitch in the handling of more-extra.override.* files (bug 966092).

== Proposed fix ==

Make ftparchive.py match the generation code and use "more-extra.override.%s.main".  (The bug has grep output showing why I think this makes sense.)

== Implementation details ==

I had to refactor the ftparchive tests somewhat to avoid falling foul of the LoC policy.

== Tests ==

bin/test -vvct test_ftparchive

== Demo and Q/A ==

mawson has never had the symlink hacks, so we can test this easily there: do a publisher run (with careful-apt, or with something to publish to precise), and check that the "Package: caps" stanza in /srv/launchpad.net/ubuntu-archive/ubuntu/dists/precise/universe/binary-i386/Packages.gz regains its missing "Task: ubuntustudio-audio-plugins" field.

After production deployment, we should remove the symlink hacks from cocoplum, and remove the corresponding instruction from NewReleaseCycleProcess.

== lint ==

None.
-- 
https://code.launchpad.net/~cjwatson/launchpad/more-extra-override-filenames/+merge/99504
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/more-extra-override-filenames into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/model/ftparchive.py'
--- lib/lp/archivepublisher/model/ftparchive.py	2012-02-07 14:53:38 +0000
+++ lib/lp/archivepublisher/model/ftparchive.py	2012-03-27 12:21:35 +0000
@@ -443,11 +443,11 @@
 
         # Set up filepaths for the overrides we read
         extra_extra_overrides = os.path.join(self._config.miscroot,
-            "more-extra.override.%s.%s" % (distroseries, component))
+            "more-extra.override.%s.main" % distroseries)
         if not os.path.exists(extra_extra_overrides):
             unpocketed_series = "-".join(distroseries.split('-')[:-1])
             extra_extra_overrides = os.path.join(self._config.miscroot,
-                "more-extra.override.%s.%s" % (unpocketed_series, component))
+                "more-extra.override.%s.main" % unpocketed_series)
         # And for the overrides we write out
         main_override = os.path.join(self._config.overrideroot,
                                      "override.%s.%s" %

=== modified file 'lib/lp/archivepublisher/tests/test_ftparchive.py'
--- lib/lp/archivepublisher/tests/test_ftparchive.py	2012-02-03 18:31:21 +0000
+++ lib/lp/archivepublisher/tests/test_ftparchive.py	2012-03-27 12:21:35 +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).
 
 """Tests for ftparchive.py"""
@@ -9,8 +9,6 @@
 import os
 import re
 import shutil
-from tempfile import mkdtemp
-import unittest
 
 from zope.component import getUtility
 
@@ -29,7 +27,10 @@
     BufferLogger,
     DevNullLogger,
     )
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    TestCase,
+    TestCaseWithFactory,
+    )
 from lp.testing.dbuser import switch_dbuser
 from lp.testing.layers import (
     LaunchpadZopelessLayer,
@@ -135,10 +136,24 @@
         file(fullpath, "w").write(leafcontent)
 
     def _setUpFTPArchiveHandler(self):
-        fa = FTPArchiveHandler(
+        return FTPArchiveHandler(
             self._logger, self._config, self._dp, self._distribution,
             self._publisher)
-        return fa
+
+    def _publishDefaultOverrides(self, fa, component):
+        source_overrides = FakeSelectResult(
+            [('foo', 'hoary-test', component, 'misc')])
+        binary_overrides = FakeSelectResult(
+            [('foo', 'hoary-test', component, 'misc', 'extra')])
+        fa.publishOverrides(source_overrides, binary_overrides)
+
+    def _publishDefaultFileLists(self, fa, component):
+        source_files = FakeSelectResult(
+            [('foo', 'hoary-test', 'foo_1.dsc', component)])
+        binary_files = FakeSelectResult(
+            [('foo', 'hoary-test', 'foo_1_i386.deb', component,
+             'binary-i386')])
+        fa.publishFileLists(source_files, binary_files)
 
     def test_getSourcesForOverrides(self):
         # getSourcesForOverrides returns a list of tuples containing:
@@ -190,7 +205,7 @@
         fa = self._setUpFTPArchiveHandler()
 
         breezy_autotest = self._distribution.getSeries('breezy-autotest')
-        self.assertEquals([], list(breezy_autotest.architectures))
+        self.assertEqual([], list(breezy_autotest.architectures))
 
         published_binaries = fa.getBinariesForOverrides(
             breezy_autotest, PackagePublishingPocket.RELEASE)
@@ -199,12 +214,7 @@
     def test_publishOverrides(self):
         # publishOverrides write the expected files on disk.
         fa = self._setUpFTPArchiveHandler()
-
-        source_overrides = FakeSelectResult(
-            [('foo', 'hoary-test', 'main', 'misc')])
-        binary_overrides = FakeSelectResult(
-            [('foo', 'hoary-test', 'main', 'misc', 'extra')])
-        fa.publishOverrides(source_overrides, binary_overrides)
+        self._publishDefaultOverrides(fa, 'main')
 
         # Check that the overrides lists generated by LP exist and have the
         # expected contents.
@@ -212,6 +222,22 @@
         self._verifyFile("override.hoary-test.main.src", self._overdir)
         self._verifyFile("override.hoary-test.extra.main", self._overdir)
 
+    def test_publishOverrides_more_extra_components(self):
+        # more-extra.override.%s.main is used regardless of component.
+        fa = self._setUpFTPArchiveHandler()
+
+        sentinel = ("hello/i386", "Task", "minimal")
+        extra_overrides = os.path.join(
+            self._confdir, "more-extra.override.hoary-test.main")
+        with open(extra_overrides, "w") as extra_override_file:
+            print >>extra_override_file, "  ".join(sentinel)
+        self._publishDefaultOverrides(fa, 'universe')
+
+        result_path = os.path.join(
+            self._overdir, "override.hoary-test.extra.universe")
+        with open(result_path) as result_file:
+            self.assertIn("\t".join(sentinel), result_file.read().splitlines())
+
     def test_getSourceFiles(self):
         # getSourceFiles returns a list of tuples containing:
         # (sourcename, suite, filename, component)
@@ -245,24 +271,15 @@
 
         binary_files = fa.getBinaryFiles(
             hoary, PackagePublishingPocket.RELEASE)
-        expected_files = [(
-            'pmount',
-            'hoary',
-            'pmount_1.9-1_all.deb',
-            'main',
-            'binary-hppa',
-            )]
+        expected_files = [
+            ('pmount', 'hoary', 'pmount_1.9-1_all.deb', 'main', 'binary-hppa'),
+            ]
         self.assertEqual(expected_files, list(binary_files))
 
     def test_publishFileLists(self):
         # publishFileLists writes the expected files on disk.
         fa = self._setUpFTPArchiveHandler()
-
-        source_files = FakeSelectResult(
-            [('foo', 'hoary-test', 'foo_1.dsc', 'main')])
-        binary_files = FakeSelectResult(
-            [('foo', 'hoary-test', 'foo_1_i386.deb', 'main', 'binary-i386')])
-        fa.publishFileLists(source_files, binary_files)
+        self._publishDefaultFileLists(fa, 'main')
 
         # Check that the file lists generated by LP exist and have the
         # expected contents.
@@ -279,19 +296,9 @@
                                self._distribution, publisher)
         fa.createEmptyPocketRequests(fullpublish=True)
 
-        # Calculate overrides.
-        source_overrides = FakeSelectResult(
-            [('foo', 'hoary-test', 'main', 'misc'), ])
-        binary_overrides = FakeSelectResult(
-            [('foo', 'hoary-test', 'main', 'misc', 'extra')])
-        fa.publishOverrides(source_overrides, binary_overrides)
-
-        # Calculate filelists.
-        source_files = FakeSelectResult(
-            [('foo', 'hoary-test', 'foo_1.dsc', 'main')])
-        binary_files = FakeSelectResult(
-            [('foo', 'hoary-test', 'foo_1_i386.deb', 'main', 'binary-i386')])
-        fa.publishFileLists(source_files, binary_files)
+        # Calculate overrides and filelists.
+        self._publishDefaultOverrides(fa, 'main')
+        self._publishDefaultFileLists(fa, 'main')
 
         # Add mentioned files in the repository pool/.
         self._addRepositoryFile('main', 'foo', 'foo_1.dsc')
@@ -375,8 +382,7 @@
 
         for listname in lists:
             path = os.path.join(self._config.overrideroot, listname)
-            self.assertTrue(os.path.exists(path))
-            self.assertEquals("", open(path).read())
+            self._verifyEmpty(path)
 
         # XXX cprov 2007-03-21: see above, byte-to-byte configuration
         # comparing is weak.
@@ -489,14 +495,12 @@
         self.assertRaises(AptFTPArchiveFailure, fa.runApt, "bogus-config")
 
 
-class TestFTouch(unittest.TestCase):
+class TestFTouch(TestCase):
     """Tests for f_touch function."""
 
     def setUp(self):
-        self.test_folder = mkdtemp()
-
-    def tearDown(self):
-        shutil.rmtree(self.test_folder)
+        TestCase.setUp(self)
+        self.test_folder = self.useTempDir()
 
     def test_f_touch_new_file(self):
         # Test f_touch correctly creates a new file.
@@ -505,15 +509,10 @@
 
     def test_f_touch_existing_file(self):
         # Test f_touch truncates existing files.
-        f = open("%s/file_to_truncate" % self.test_folder, "w")
-        test_contents = "I'm some test contents"
-        f.write(test_contents)
-        f.close()
+        with open("%s/file_to_truncate" % self.test_folder, "w") as f:
+            f.write("I'm some test contents")
 
         f_touch(self.test_folder, "file_to_leave_alone")
 
-        f = open("%s/file_to_leave_alone" % self.test_folder, "r")
-        contents = f.read()
-        f.close()
-
-        self.assertEqual("", contents)
+        with open("%s/file_to_leave_alone" % self.test_folder, "r") as f:
+            self.assertEqual("", f.read())