launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-ftparchive-avoid-empty-overrides into launchpad:master.

Commit message:
Generate empty override and file list files where necessary

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/399985

Commit 25ce0c1d87 stopped `FTPArchive.createEmptyPocketRequests` from writing empty override and file list files for all suites/components, instead merely making sure that they exist.  However, this broke in the case where a suite/component (or suite/component/architecture) was non-empty and then became empty, because the previous non-empty overrides and file lists were left in place.

To avoid this, ensure that `FTPArchive.publishOverrides` writes override files for all relevant components, and that `FTPArchive.publishFileLists` writes file list files for all relevant components/architectures; these may be empty if there are no publications in the corresponding contexts.  This doesn't suffer from the problem that the aforementioned commit was fixing, because these files are put in place atomically.

It's possible that this comes close to making `FTPArchive.createEmptyPocketRequests` entirely unnecessary, but I haven't yet tried to remove it.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-ftparchive-avoid-empty-overrides into launchpad:master.

diff --git a/lib/lp/archivepublisher/model/ftparchive.py b/lib/lp/archivepublisher/model/ftparchive.py
index a13269e..624aebf 100644
--- a/lib/lp/archivepublisher/model/ftparchive.py
+++ b/lib/lp/archivepublisher/model/ftparchive.py
@@ -382,10 +382,9 @@ class FTPArchiveHandler:
 
                 spphs = self.getSourcesForOverrides(distroseries, pocket)
                 bpphs = self.getBinariesForOverrides(distroseries, pocket)
-                self.publishOverrides(
-                    distroseries.getSuite(pocket), spphs, bpphs)
+                self.publishOverrides(distroseries, pocket, spphs, bpphs)
 
-    def publishOverrides(self, suite,
+    def publishOverrides(self, distroseries, pocket,
                          source_publications, binary_publications):
         """Output a set of override files for use in apt-ftparchive.
 
@@ -408,8 +407,15 @@ class FTPArchiveHandler:
         # test_ftparchive.
         from lp.archivepublisher.publishing import FORMAT_TO_SUBCOMPONENT
 
+        suite = distroseries.getSuite(pocket)
+
         # overrides[component][src/bin] = sets of tuples
         overrides = defaultdict(lambda: defaultdict(set))
+        # Ensure that we generate overrides for all the expected components,
+        # even if they're currently empty.
+        for component in self.publisher.archive.getComponentsForSeries(
+                distroseries):
+            overrides[component.name]
 
         def updateOverride(packagename, component, section, archtag=None,
                            priority=None, binpackageformat=None,
@@ -666,16 +672,25 @@ class FTPArchiveHandler:
                         continue
                 spps = self.getSourceFiles(distroseries, pocket)
                 pps = self.getBinaryFiles(distroseries, pocket)
-                self.publishFileLists(distroseries.getSuite(pocket), spps, pps)
+                self.publishFileLists(distroseries, pocket, spps, pps)
 
-    def publishFileLists(self, suite, sourcefiles, binaryfiles):
+    def publishFileLists(self, distroseries, pocket, sourcefiles, binaryfiles):
         """Collate the set of source files and binary files provided and
         write out all the file list files for them.
 
         listroot/distroseries_component_source
         listroot/distroseries_component_binary-archname
         """
+        suite = distroseries.getSuite(pocket)
+
         filelist = defaultdict(lambda: defaultdict(list))
+        # Ensure that we generate file lists for all the expected components
+        # and architectures, even if they're currently empty.
+        for component in self.publisher.archive.getComponentsForSeries(
+                distroseries):
+            filelist[component.name]["source"]
+            for das in distroseries.enabled_architectures:
+                filelist[component.name]["binary-%s" % das.architecturetag]
 
         def updateFileList(sourcepackagename, filename, component,
                            architecturetag=None):
@@ -698,7 +713,6 @@ class FTPArchiveHandler:
             updateFileList(*file_details)
 
         self.log.debug("Writing file lists for %s" % suite)
-        series, pocket = self.distro.getDistroSeriesAndPocket(suite)
         for component, architectures in six.iteritems(filelist):
             for architecture, file_names in six.iteritems(architectures):
                 # XXX wgrant 2010-10-06: There must be a better place to do
@@ -708,7 +722,7 @@ class FTPArchiveHandler:
                 else:
                     # The "[7:]" strips the "binary-" prefix off the
                     # architecture names we get here.
-                    das = series.getDistroArchSeries(architecture[7:])
+                    das = distroseries.getDistroArchSeries(architecture[7:])
                     enabled = das.enabled
                 if enabled:
                     self.writeFileList(
@@ -740,7 +754,8 @@ class FTPArchiveHandler:
             with open(new_path, 'w') as f:
                 files[subcomp].sort(key=package_name)
                 f.write("\n".join(files[subcomp]))
-                f.write("\n")
+                if files[subcomp]:
+                    f.write("\n")
             os.rename(new_path, final_path)
 
     #
diff --git a/lib/lp/archivepublisher/tests/test_ftparchive.py b/lib/lp/archivepublisher/tests/test_ftparchive.py
index 3a9665a..94b68fc 100755
--- a/lib/lp/archivepublisher/tests/test_ftparchive.py
+++ b/lib/lp/archivepublisher/tests/test_ftparchive.py
@@ -186,13 +186,17 @@ class TestFTPArchive(TestCaseWithFactory):
             'tiny', component, section, 'i386',
             PackagePublishingPriority.EXTRA, binpackageformat,
             phased_update_percentage)])
-        fa.publishOverrides('hoary-test', source_overrides, binary_overrides)
+        fa.publishOverrides(
+            self._distribution['hoary-test'], PackagePublishingPocket.RELEASE,
+            source_overrides, binary_overrides)
 
     def _publishDefaultFileLists(self, fa, component):
         source_files = FakeSelectResult([('tiny', 'tiny_0.1.dsc', component)])
         binary_files = FakeSelectResult(
             [('tiny', 'tiny_0.1_i386.deb', component, 'binary-i386')])
-        fa.publishFileLists('hoary-test', source_files, binary_files)
+        fa.publishFileLists(
+            self._distribution['hoary-test'], PackagePublishingPocket.RELEASE,
+            source_files, binary_files)
 
     def test_createEmptyPocketRequests_preserves_existing(self):
         # createEmptyPocketRequests leaves existing override and file list
@@ -362,6 +366,29 @@ class TestFTPArchive(TestCaseWithFactory):
             self.assertEqual(
                 ["tiny\textra\tdevel"], result_file.read().splitlines())
 
+    def test_publishOverrides_empties_missing_components(self):
+        # publishOverrides writes empty overrides files for components that
+        # have no publications.
+        fa = self._setUpFTPArchiveHandler()
+        empty_overrides = []
+        for component in ("restricted", "universe", "multiverse"):
+            empty_overrides.extend([
+                "override.hoary-test.%s" % component,
+                "override.hoary-test.%s.src" % component,
+                "override.hoary-test.extra.%s" % component,
+                ])
+        for override in empty_overrides:
+            write_file(
+                os.path.join(self._overdir, override), b"previous contents\n")
+
+        self._publishDefaultOverrides(fa, "main")
+
+        self._verifyFile("override.hoary-test.main", self._overdir)
+        self._verifyFile("override.hoary-test.main.src", self._overdir)
+        self._verifyFile("override.hoary-test.extra.main", self._overdir)
+        for override in empty_overrides:
+            self._verifyEmpty(os.path.join(self._overdir, override))
+
     def test_generateOverrides(self):
         # generateOverrides generates all the overrides from start to finish.
         self._distribution = getUtility(IDistributionSet).getByName('ubuntu')
@@ -459,6 +486,27 @@ class TestFTPArchive(TestCaseWithFactory):
         self._verifyFile("hoary-test_main_source", self._listdir)
         self._verifyFile("hoary-test_main_binary-i386", self._listdir)
 
+    def test_publishFileLists_empties_missing_components(self):
+        # publishFileLists writes empty file list files for components that
+        # have no publications.
+        fa = self._setUpFTPArchiveHandler()
+        empty_filelists = []
+        for component in ("restricted", "universe", "multiverse"):
+            empty_filelists.extend([
+                "hoary-test_%s_source" % component,
+                "hoary-test_%s_binary-i386" % component,
+                ])
+        for filelist in empty_filelists:
+            write_file(
+                os.path.join(self._listdir, filelist), b"previous contents\n")
+
+        self._publishDefaultFileLists(fa, "main")
+
+        self._verifyFile("hoary-test_main_source", self._listdir)
+        self._verifyFile("hoary-test_main_binary-i386", self._listdir)
+        for filelist in empty_filelists:
+            self._verifyEmpty(os.path.join(self._listdir, filelist))
+
     def test_generateConfig(self):
         # Generate apt-ftparchive configuration file and run it.
 
@@ -727,12 +775,16 @@ class TestFTPArchive(TestCaseWithFactory):
             "bin%d" % i, "main", "misc", "i386",
             PackagePublishingPriority.EXTRA, BinaryPackageFormat.DEB, None)
             for i in range(50)])
-        fa.publishOverrides("hoary-test", source_overrides, binary_overrides)
+        fa.publishOverrides(
+            self._distribution["hoary-test"], PackagePublishingPocket.RELEASE,
+            source_overrides, binary_overrides)
         source_files = FakeSelectResult([("tiny", "tiny_0.1.dsc", "main")])
         binary_files = FakeSelectResult([(
             "bin%d" % i, "bin%d_1_i386.deb" % i, "main", "binary-i386")
             for i in range(50)])
-        fa.publishFileLists("hoary-test", source_files, binary_files)
+        fa.publishFileLists(
+            self._distribution["hoary-test"], PackagePublishingPocket.RELEASE,
+            source_files, binary_files)
         self._addRepositoryFile("main", "tiny", "tiny_0.1.dsc")
         for i in range(50):
             self._addRepositoryFile(