launchpad-reviewers team mailing list archive

[Colin Watson <mp+410320@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:ddtp-translations-en into launchpad:master.

Commit message:
Prevent Translation-en publishing collisions

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1900464 in Launchpad itself: "ddtp-translations publication of Translation-en breaks the publisher"
  https://bugs.launchpad.net/launchpad/+bug/1900464

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

If the series is configured to do so, the publisher may generate Translation-en files with long descriptions extracted from Packages files.  However, it's also possible for DDTP tarballs to include Translation-en files, which then collide with the publisher when extracted and produce either assertion failures or inconsistent Release files depending on which one happened to win.  Filter these out.

I fixed a ResourceWarning in passing.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:ddtp-translations-en into launchpad:master.

diff --git a/lib/lp/archivepublisher/ddtp_tarball.py b/lib/lp/archivepublisher/ddtp_tarball.py
index d0f4c60..78e4e3e 100644
--- a/lib/lp/archivepublisher/ddtp_tarball.py
+++ b/lib/lp/archivepublisher/ddtp_tarball.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The processing of translated packages descriptions (ddtp) tarballs.
@@ -16,8 +16,13 @@ __all__ = [
 
 import os
 
+from zope.component import getUtility
+
 from lp.archivepublisher.config import getPubConfig
 from lp.archivepublisher.customupload import CustomUpload
+from lp.registry.interfaces.distroseries import IDistroSeriesSet
+from lp.services.features import getFeatureFlag
+from lp.soyuz.enums import ArchivePurpose
 
 
 class DdtpTarballUpload(CustomUpload):
@@ -59,6 +64,9 @@ class DdtpTarballUpload(CustomUpload):
 
     def setTargetDirectory(self, archive, tarfile_path, suite):
         self.setComponents(tarfile_path)
+        self.archive = archive
+        self.distro_series, _ = getUtility(IDistroSeriesSet).fromSuite(
+            archive.distribution, suite)
         pubconf = getPubConfig(archive)
         self.targetdir = os.path.join(
             pubconf.archiveroot, 'dists', suite, self.component)
@@ -76,7 +84,28 @@ class DdtpTarballUpload(CustomUpload):
 
     def shouldInstall(self, filename):
         # Ignore files outside of the i18n subdirectory
-        return filename.startswith('i18n/')
+        if not filename.startswith("i18n/"):
+            return False
+        # apt-ftparchive or the PPA publisher (with slightly different
+        # conditions depending on the archive purpose) may be configured to
+        # create its own Translation-en files.  If so, we must take care not
+        # to allow ddtp-tarball custom uploads to collide with those.
+        if (filename == "i18n/Translation-en" or
+                filename.startswith("i18n/Translation-en.")):
+            # Compare with the step C condition in
+            # PublishDistro.publishArchive.
+            if self.archive.purpose in (
+                    ArchivePurpose.PRIMARY, ArchivePurpose.COPY):
+                # See FTPArchiveHandler.writeAptConfig.
+                if not self.distro_series.include_long_descriptions:
+                    return False
+            else:
+                # See Publisher._writeComponentIndexes.
+                if (not self.distro_series.include_long_descriptions and
+                        getFeatureFlag(
+                            "soyuz.ppa.separate_long_descriptions")):
+                    return False
+        return True
 
     def fixCurrentSymlink(self):
         # There is no symlink to fix up for DDTP uploads
diff --git a/lib/lp/archivepublisher/model/ftparchive.py b/lib/lp/archivepublisher/model/ftparchive.py
index d56b881..67ba9d5 100644
--- a/lib/lp/archivepublisher/model/ftparchive.py
+++ b/lib/lp/archivepublisher/model/ftparchive.py
@@ -855,6 +855,7 @@ class FTPArchiveHandler:
                          "CACHEINSERT": "",
                          "DISTS": os.path.basename(self._config.distsroot),
                          "HIDEEXTRA": "",
+                         # Must match DdtpTarballUpload.shouldInstall.
                          "LONGDESCRIPTION":
                              "true" if include_long_descriptions else "false",
                          })
diff --git a/lib/lp/archivepublisher/publishing.py b/lib/lp/archivepublisher/publishing.py
index 708e9de..86ebe1a 100644
--- a/lib/lp/archivepublisher/publishing.py
+++ b/lib/lp/archivepublisher/publishing.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __all__ = [
@@ -893,6 +893,7 @@ class Publisher(object):
         self.log.debug("Generating Sources")
 
         separate_long_descriptions = False
+        # Must match DdtpTarballUpload.shouldInstall.
         if (not distroseries.include_long_descriptions and
                 getFeatureFlag("soyuz.ppa.separate_long_descriptions")):
             # If include_long_descriptions is False and the feature flag is
diff --git a/lib/lp/archivepublisher/tests/test_ddtp_tarball.py b/lib/lp/archivepublisher/tests/test_ddtp_tarball.py
index e90e59b..1c31fa3 100644
--- a/lib/lp/archivepublisher/tests/test_ddtp_tarball.py
+++ b/lib/lp/archivepublisher/tests/test_ddtp_tarball.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test ddtp-tarball custom uploads.
@@ -15,6 +15,7 @@ from zope.component import getUtility
 from lp.archivepublisher.config import getPubConfig
 from lp.archivepublisher.ddtp_tarball import DdtpTarballUpload
 from lp.archivepublisher.interfaces.publisherconfig import IPublisherConfigSet
+from lp.services.features.testing import FeatureFixture
 from lp.services.tarfile_helpers import LaunchpadWriteTarFile
 from lp.soyuz.enums import ArchivePurpose
 from lp.testing import TestCaseWithFactory
@@ -34,7 +35,9 @@ class TestDdtpTarball(TestCaseWithFactory):
         db_pubconf.root_dir = six.ensure_text(self.temp_dir)
         self.archive = self.factory.makeArchive(
             distribution=self.distro, purpose=ArchivePurpose.PRIMARY)
-        self.suite = "distroseries"
+        self.distroseries = self.factory.makeDistroSeries(
+            distribution=self.distro)
+        self.suite = self.distroseries.name
         # CustomUpload.installFiles requires a umask of 0o022.
         old_umask = os.umask(0o022)
         self.addCleanup(os.umask, old_umask)
@@ -58,10 +61,14 @@ class TestDdtpTarball(TestCaseWithFactory):
     def test_basic(self):
         # Processing a simple correct tar file works.
         self.openArchive("20060728")
-        self.tarfile.add_file("i18n/Translation-de", b"")
+        names = (
+            "Translation-en", "Translation-en.xz",
+            "Translation-de", "Translation-de.xz")
+        for name in names:
+            self.tarfile.add_file(os.path.join("i18n", name), b"")
         self.process()
-        self.assertTrue(os.path.exists(
-            self.getTranslationsPath("Translation-de")))
+        for name in names:
+            self.assertTrue(os.path.exists(self.getTranslationsPath(name)))
 
     def test_ignores_empty_directories(self):
         # Empty directories in the tarball are not extracted.
@@ -92,6 +99,43 @@ class TestDdtpTarball(TestCaseWithFactory):
         with open(self.getTranslationsPath("Translation-ca")) as ca_file:
             self.assertEqual("ca", ca_file.read())
 
+    def test_does_not_collide_with_publisher_primary(self):
+        # If the publisher is configured to generate its own Translations-en
+        # file (for the apt-ftparchive case), then colliding entries in DDTP
+        # tarballs are not extracted.
+        self.distroseries.include_long_descriptions = False
+        self.openArchive("20060728")
+        en_names = ("Translation-en", "Translation-en.xz")
+        de_names = ("Translation-de", "Translation-de.xz")
+        for name in en_names + de_names:
+            self.tarfile.add_file(os.path.join("i18n", name), b"")
+        self.process()
+        for name in en_names:
+            self.assertFalse(os.path.exists(self.getTranslationsPath(name)))
+        for name in de_names:
+            self.assertTrue(os.path.exists(self.getTranslationsPath(name)))
+
+    def test_does_not_collide_with_publisher_ppa(self):
+        # If the publisher is configured to generate its own Translations-en
+        # file (for the PPA case), then colliding entries in DDTP
+        # tarballs are not extracted.
+        self.archive = self.factory.makeArchive(
+            distribution=self.distro, purpose=ArchivePurpose.PPA)
+        self.useFixture(FeatureFixture({
+            "soyuz.ppa.separate_long_descriptions": "on",
+            }))
+        self.distroseries.include_long_descriptions = False
+        self.openArchive("20060728")
+        en_names = ("Translation-en", "Translation-en.xz")
+        de_names = ("Translation-de", "Translation-de.xz")
+        for name in en_names + de_names:
+            self.tarfile.add_file(os.path.join("i18n", name), b"")
+        self.process()
+        for name in en_names:
+            self.assertFalse(os.path.exists(self.getTranslationsPath(name)))
+        for name in de_names:
+            self.assertTrue(os.path.exists(self.getTranslationsPath(name)))
+
     def test_breaks_hard_links(self):
         # Our archive uses dsync to replace identical files with hard links
         # in order to save some space.  tarfile.extract overwrites
@@ -133,6 +177,8 @@ class TestDdtpTarball(TestCaseWithFactory):
         # getSeriesKey extracts the component from an upload's filename.
         self.openArchive("20060728")
         self.assertEqual("main", DdtpTarballUpload.getSeriesKey(self.path))
+        self.tarfile.close()
+        self.buffer.close()
 
     def test_getSeriesKey_returns_None_on_mismatch(self):
         # getSeriesKey returns None if the filename does not match the