launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/germinate-disabled-das into lp:launchpad.

Commit message:
Make generate-extra-overrides and generate-contents-files consider only enabled architectures.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1077261 in Launchpad itself: "generate-extra-overrides processes disabled DistroArchSeries"
  https://bugs.launchpad.net/launchpad/+bug/1077261

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/germinate-disabled-das/+merge/133801

== Summary ==

We disabled the armel DAS in raring today.  Not everything handles this sensibly: bug 1077261.

== Proposed fix ==

William Grant suggested using enabled_architectures rather than architectures.

== Tests ==

bin/test -vvct test_generate_extra_overrides -t test_generate_contents_files

== Demo and Q/A ==

For generate-extra-overrides, disable raring/armel on dogfood (possibly temporarily?), run cronscripts/publishing/cron.germinate, and inspect the output to ensure that it doesn't mention armel.

For generate-contents-files, it's not clear how much effort is worth it given that it takes some time to run on production let alone mawson.  I suppose we could start it and Ctrl-C after it gets past armel in apt-ftparchive, since that's relatively close to the start.
-- 
https://code.launchpad.net/~cjwatson/launchpad/germinate-disabled-das/+merge/133801
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/germinate-disabled-das into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/scripts/generate_contents_files.py'
--- lib/lp/archivepublisher/scripts/generate_contents_files.py	2012-09-28 06:01:02 +0000
+++ lib/lp/archivepublisher/scripts/generate_contents_files.py	2012-11-10 18:00:33 +0000
@@ -94,16 +94,6 @@
             "Failure while running command: %s" % description)
 
 
-def move_file(old_path, new_path):
-    """Rename file `old_path` to `new_path`.
-
-    Mercilessly delete any file that may already exist at `new_path`.
-    """
-    if file_exists(new_path):
-        os.remove(new_path)
-    os.rename(old_path, new_path)
-
-
 class GenerateContentsFiles(LaunchpadCronScript):
 
     distribution = None
@@ -164,7 +154,7 @@
     def getArchs(self, suite):
         """Query architectures supported by the suite."""
         series, _ = self.distribution.getDistroSeriesAndPocket(suite)
-        return [arch.architecturetag for arch in series.architectures]
+        return [arch.architecturetag for arch in series.enabled_architectures]
 
     def getDirs(self, archs):
         """Subdirectories needed for each component."""
@@ -286,8 +276,8 @@
             contents_dest = os.path.join(
                 self.config.distsroot, suite, "%s.gz" % contents_filename)
 
-            move_file(current_contents, last_contents)
-            move_file(new_contents, contents_dest)
+            os.rename(current_contents, last_contents)
+            os.rename(new_contents, contents_dest)
             os.chmod(contents_dest, 0664)
         else:
             self.logger.debug(

=== modified file 'lib/lp/archivepublisher/scripts/generate_extra_overrides.py'
--- lib/lp/archivepublisher/scripts/generate_extra_overrides.py	2012-09-28 06:01:02 +0000
+++ lib/lp/archivepublisher/scripts/generate_extra_overrides.py	2012-11-10 18:00:33 +0000
@@ -367,7 +367,7 @@
             series_name = series.name
             components = self.getComponents(series)
             architectures = sorted(
-                [arch.architecturetag for arch in series.architectures])
+                arch.architecturetag for arch in series.enabled_architectures)
 
             # This takes a while.  Ensure that we do it without keeping a
             # database transaction open.

=== modified file 'lib/lp/archivepublisher/tests/test_generate_contents_files.py'
--- lib/lp/archivepublisher/tests/test_generate_contents_files.py	2012-08-17 11:15:35 +0000
+++ lib/lp/archivepublisher/tests/test_generate_contents_files.py	2012-11-10 18:00:33 +0000
@@ -14,10 +14,10 @@
     differ_in_content,
     execute,
     GenerateContentsFiles,
-    move_file,
     )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.log.logger import DevNullLogger
+from lp.services.osutils import write_file
 from lp.services.scripts.base import LaunchpadScriptFailure
 from lp.services.scripts.tests import run_script
 from lp.services.utils import file_exists
@@ -29,33 +29,24 @@
     )
 
 
-def write_file(filename, content=""):
-    """Write `content` to `filename`, and flush."""
-    output_file = file(filename, 'w')
-    output_file.write(content)
-    output_file.close()
-
-
 def fake_overrides(script, distroseries):
     """Fake overrides files so `script` can run `apt-ftparchive`."""
-    os.makedirs(script.config.overrideroot)
-
     components = ['main', 'restricted', 'universe', 'multiverse']
     architectures = script.getArchs(distroseries.name)
     suffixes = components + ['extra.' + component for component in components]
     for suffix in suffixes:
         write_file(os.path.join(
             script.config.overrideroot,
-            "override.%s.%s" % (distroseries.name, suffix)))
+            "override.%s.%s" % (distroseries.name, suffix)), "")
 
     for component in components:
         write_file(os.path.join(
             script.config.overrideroot,
-            "%s_%s_source" % (distroseries.name, component)))
+            "%s_%s_source" % (distroseries.name, component)), "")
         for arch in architectures:
             write_file(os.path.join(
                 script.config.overrideroot,
-                "%s_%s_binary-%s" % (distroseries.name, component, arch)))
+                "%s_%s_binary-%s" % (distroseries.name, component, arch)), "")
 
 
 class TestHelpers(TestCaseWithFactory):
@@ -105,24 +96,6 @@
         execute(logger, "touch", [filename])
         self.assertTrue(file_exists(filename))
 
-    def test_move_file_renames_file(self):
-        # move_file renames a file from its old name to its new name.
-        self.useTempDir()
-        text = self.factory.getUniqueString()
-        write_file("old_name", text)
-        move_file("old_name", "new_name")
-        self.assertEqual(text, file("new_name").read())
-
-    def test_move_file_overwrites_old_file(self):
-        # If move_file finds another file in the way, that file gets
-        # deleted.
-        self.useTempDir()
-        write_file("new_name", self.factory.getUniqueString())
-        new_text = self.factory.getUniqueString()
-        write_file("old_name", new_text)
-        move_file("old_name", "new_name")
-        self.assertEqual(new_text, file("new_name").read())
-
 
 class TestGenerateContentsFiles(TestCaseWithFactory):
     """Tests for the actual `GenerateContentsFiles` script."""
@@ -162,9 +135,6 @@
         :return: The arbitrary string that is also in the file.
         """
         marker_contents = self.factory.getUniqueString()
-        dir_name = os.path.dirname(file_path)
-        if not file_exists(dir_name):
-            os.makedirs(dir_name)
         write_file(file_path, marker_contents)
         return marker_contents
 
@@ -203,10 +173,12 @@
         self.assertEqual(distro, script.distribution)
 
     def test_getArchs(self):
-        # getArchs returns a list of architectures in the distroseries.
+        # getArchs returns a list of enabled architectures in the distroseries.
         distro = self.makeDistro()
         distroseries = self.factory.makeDistroSeries(distro)
         das = self.factory.makeDistroArchSeries(distroseries=distroseries)
+        self.factory.makeDistroArchSeries(
+            distroseries=distroseries, enabled=False)
         script = self.makeScript(das.distroseries.distribution)
         self.assertEqual(
             [das.architecturetag], script.getArchs(distroseries.name))

=== modified file 'lib/lp/archivepublisher/tests/test_generate_extra_overrides.py'
--- lib/lp/archivepublisher/tests/test_generate_extra_overrides.py	2012-09-07 05:13:57 +0000
+++ lib/lp/archivepublisher/tests/test_generate_extra_overrides.py	2012-11-10 18:00:33 +0000
@@ -36,6 +36,7 @@
 from lp.services.utils import file_exists
 from lp.soyuz.enums import PackagePublishingStatus
 from lp.testing import TestCaseWithFactory
+from lp.testing.fakemethod import FakeMethod
 from lp.testing.faketransaction import FakeTransaction
 from lp.testing.layers import (
     LaunchpadZopelessLayer,
@@ -613,6 +614,21 @@
         self.assertTrue(os.path.exists(output(seed_new)))
         self.assertFalse(os.path.exists(output(seed_old)))
 
+    def test_process_skips_disabled_distroarchseries(self):
+        # The script does not generate overrides for disabled DistroArchSeries.
+        flavour = self.factory.getUniqueString()
+        self.setUpDistroAndScript(extra_args=[flavour])
+        das = self.factory.makeDistroArchSeries(
+            distroseries=self.distroseries[0])
+        self.factory.makeDistroArchSeries(
+            distroseries=self.distroseries[0], enabled=False)
+        self.script.generateExtraOverrides = FakeMethod()
+        self.script.process()
+        self.assertEqual(1, self.script.generateExtraOverrides.call_count)
+        self.assertEqual(
+            [das.architecturetag],
+            self.script.generateExtraOverrides.calls[0][0][2])
+
     def test_main(self):
         # If run end-to-end, the script generates override files containing
         # output for all architectures, and sends germinate's log output to