Merge ~cjwatson/launchpad:ddtp-translations-en into launchpad:master

Proposed by Colin Watson
Status: Merged
Approved by: Colin Watson
Approved revision: f506b902811acaee822c61e79387bc1b7907d1a7
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/launchpad:ddtp-translations-en
Merge into: launchpad:master
Diff against target: 194 lines (+85/-8)
4 files modified
lib/lp/archivepublisher/ddtp_tarball.py (+31/-2)
lib/lp/archivepublisher/model/ftparchive.py (+1/-0)
lib/lp/archivepublisher/publishing.py (+2/-1)
lib/lp/archivepublisher/tests/test_ddtp_tarball.py (+51/-5)
Reviewer Review Type Date Requested Status
Ioana Lasc Approve
Review via email: mp+410320@code.launchpad.net

Commit message

Prevent Translation-en publishing collisions

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Ioana Lasc (ilasc) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/archivepublisher/ddtp_tarball.py b/lib/lp/archivepublisher/ddtp_tarball.py
2index d0f4c60..78e4e3e 100644
3--- a/lib/lp/archivepublisher/ddtp_tarball.py
4+++ b/lib/lp/archivepublisher/ddtp_tarball.py
5@@ -1,4 +1,4 @@
6-# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
7+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
8 # GNU Affero General Public License version 3 (see the file LICENSE).
9
10 """The processing of translated packages descriptions (ddtp) tarballs.
11@@ -16,8 +16,13 @@ __all__ = [
12
13 import os
14
15+from zope.component import getUtility
16+
17 from lp.archivepublisher.config import getPubConfig
18 from lp.archivepublisher.customupload import CustomUpload
19+from lp.registry.interfaces.distroseries import IDistroSeriesSet
20+from lp.services.features import getFeatureFlag
21+from lp.soyuz.enums import ArchivePurpose
22
23
24 class DdtpTarballUpload(CustomUpload):
25@@ -59,6 +64,9 @@ class DdtpTarballUpload(CustomUpload):
26
27 def setTargetDirectory(self, archive, tarfile_path, suite):
28 self.setComponents(tarfile_path)
29+ self.archive = archive
30+ self.distro_series, _ = getUtility(IDistroSeriesSet).fromSuite(
31+ archive.distribution, suite)
32 pubconf = getPubConfig(archive)
33 self.targetdir = os.path.join(
34 pubconf.archiveroot, 'dists', suite, self.component)
35@@ -76,7 +84,28 @@ class DdtpTarballUpload(CustomUpload):
36
37 def shouldInstall(self, filename):
38 # Ignore files outside of the i18n subdirectory
39- return filename.startswith('i18n/')
40+ if not filename.startswith("i18n/"):
41+ return False
42+ # apt-ftparchive or the PPA publisher (with slightly different
43+ # conditions depending on the archive purpose) may be configured to
44+ # create its own Translation-en files. If so, we must take care not
45+ # to allow ddtp-tarball custom uploads to collide with those.
46+ if (filename == "i18n/Translation-en" or
47+ filename.startswith("i18n/Translation-en.")):
48+ # Compare with the step C condition in
49+ # PublishDistro.publishArchive.
50+ if self.archive.purpose in (
51+ ArchivePurpose.PRIMARY, ArchivePurpose.COPY):
52+ # See FTPArchiveHandler.writeAptConfig.
53+ if not self.distro_series.include_long_descriptions:
54+ return False
55+ else:
56+ # See Publisher._writeComponentIndexes.
57+ if (not self.distro_series.include_long_descriptions and
58+ getFeatureFlag(
59+ "soyuz.ppa.separate_long_descriptions")):
60+ return False
61+ return True
62
63 def fixCurrentSymlink(self):
64 # There is no symlink to fix up for DDTP uploads
65diff --git a/lib/lp/archivepublisher/model/ftparchive.py b/lib/lp/archivepublisher/model/ftparchive.py
66index d56b881..67ba9d5 100644
67--- a/lib/lp/archivepublisher/model/ftparchive.py
68+++ b/lib/lp/archivepublisher/model/ftparchive.py
69@@ -855,6 +855,7 @@ class FTPArchiveHandler:
70 "CACHEINSERT": "",
71 "DISTS": os.path.basename(self._config.distsroot),
72 "HIDEEXTRA": "",
73+ # Must match DdtpTarballUpload.shouldInstall.
74 "LONGDESCRIPTION":
75 "true" if include_long_descriptions else "false",
76 })
77diff --git a/lib/lp/archivepublisher/publishing.py b/lib/lp/archivepublisher/publishing.py
78index 708e9de..86ebe1a 100644
79--- a/lib/lp/archivepublisher/publishing.py
80+++ b/lib/lp/archivepublisher/publishing.py
81@@ -1,4 +1,4 @@
82-# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
83+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
84 # GNU Affero General Public License version 3 (see the file LICENSE).
85
86 __all__ = [
87@@ -893,6 +893,7 @@ class Publisher(object):
88 self.log.debug("Generating Sources")
89
90 separate_long_descriptions = False
91+ # Must match DdtpTarballUpload.shouldInstall.
92 if (not distroseries.include_long_descriptions and
93 getFeatureFlag("soyuz.ppa.separate_long_descriptions")):
94 # If include_long_descriptions is False and the feature flag is
95diff --git a/lib/lp/archivepublisher/tests/test_ddtp_tarball.py b/lib/lp/archivepublisher/tests/test_ddtp_tarball.py
96index e90e59b..1c31fa3 100644
97--- a/lib/lp/archivepublisher/tests/test_ddtp_tarball.py
98+++ b/lib/lp/archivepublisher/tests/test_ddtp_tarball.py
99@@ -1,4 +1,4 @@
100-# Copyright 2012-2018 Canonical Ltd. This software is licensed under the
101+# Copyright 2012-2021 Canonical Ltd. This software is licensed under the
102 # GNU Affero General Public License version 3 (see the file LICENSE).
103
104 """Test ddtp-tarball custom uploads.
105@@ -15,6 +15,7 @@ from zope.component import getUtility
106 from lp.archivepublisher.config import getPubConfig
107 from lp.archivepublisher.ddtp_tarball import DdtpTarballUpload
108 from lp.archivepublisher.interfaces.publisherconfig import IPublisherConfigSet
109+from lp.services.features.testing import FeatureFixture
110 from lp.services.tarfile_helpers import LaunchpadWriteTarFile
111 from lp.soyuz.enums import ArchivePurpose
112 from lp.testing import TestCaseWithFactory
113@@ -34,7 +35,9 @@ class TestDdtpTarball(TestCaseWithFactory):
114 db_pubconf.root_dir = six.ensure_text(self.temp_dir)
115 self.archive = self.factory.makeArchive(
116 distribution=self.distro, purpose=ArchivePurpose.PRIMARY)
117- self.suite = "distroseries"
118+ self.distroseries = self.factory.makeDistroSeries(
119+ distribution=self.distro)
120+ self.suite = self.distroseries.name
121 # CustomUpload.installFiles requires a umask of 0o022.
122 old_umask = os.umask(0o022)
123 self.addCleanup(os.umask, old_umask)
124@@ -58,10 +61,14 @@ class TestDdtpTarball(TestCaseWithFactory):
125 def test_basic(self):
126 # Processing a simple correct tar file works.
127 self.openArchive("20060728")
128- self.tarfile.add_file("i18n/Translation-de", b"")
129+ names = (
130+ "Translation-en", "Translation-en.xz",
131+ "Translation-de", "Translation-de.xz")
132+ for name in names:
133+ self.tarfile.add_file(os.path.join("i18n", name), b"")
134 self.process()
135- self.assertTrue(os.path.exists(
136- self.getTranslationsPath("Translation-de")))
137+ for name in names:
138+ self.assertTrue(os.path.exists(self.getTranslationsPath(name)))
139
140 def test_ignores_empty_directories(self):
141 # Empty directories in the tarball are not extracted.
142@@ -92,6 +99,43 @@ class TestDdtpTarball(TestCaseWithFactory):
143 with open(self.getTranslationsPath("Translation-ca")) as ca_file:
144 self.assertEqual("ca", ca_file.read())
145
146+ def test_does_not_collide_with_publisher_primary(self):
147+ # If the publisher is configured to generate its own Translations-en
148+ # file (for the apt-ftparchive case), then colliding entries in DDTP
149+ # tarballs are not extracted.
150+ self.distroseries.include_long_descriptions = False
151+ self.openArchive("20060728")
152+ en_names = ("Translation-en", "Translation-en.xz")
153+ de_names = ("Translation-de", "Translation-de.xz")
154+ for name in en_names + de_names:
155+ self.tarfile.add_file(os.path.join("i18n", name), b"")
156+ self.process()
157+ for name in en_names:
158+ self.assertFalse(os.path.exists(self.getTranslationsPath(name)))
159+ for name in de_names:
160+ self.assertTrue(os.path.exists(self.getTranslationsPath(name)))
161+
162+ def test_does_not_collide_with_publisher_ppa(self):
163+ # If the publisher is configured to generate its own Translations-en
164+ # file (for the PPA case), then colliding entries in DDTP
165+ # tarballs are not extracted.
166+ self.archive = self.factory.makeArchive(
167+ distribution=self.distro, purpose=ArchivePurpose.PPA)
168+ self.useFixture(FeatureFixture({
169+ "soyuz.ppa.separate_long_descriptions": "on",
170+ }))
171+ self.distroseries.include_long_descriptions = False
172+ self.openArchive("20060728")
173+ en_names = ("Translation-en", "Translation-en.xz")
174+ de_names = ("Translation-de", "Translation-de.xz")
175+ for name in en_names + de_names:
176+ self.tarfile.add_file(os.path.join("i18n", name), b"")
177+ self.process()
178+ for name in en_names:
179+ self.assertFalse(os.path.exists(self.getTranslationsPath(name)))
180+ for name in de_names:
181+ self.assertTrue(os.path.exists(self.getTranslationsPath(name)))
182+
183 def test_breaks_hard_links(self):
184 # Our archive uses dsync to replace identical files with hard links
185 # in order to save some space. tarfile.extract overwrites
186@@ -133,6 +177,8 @@ class TestDdtpTarball(TestCaseWithFactory):
187 # getSeriesKey extracts the component from an upload's filename.
188 self.openArchive("20060728")
189 self.assertEqual("main", DdtpTarballUpload.getSeriesKey(self.path))
190+ self.tarfile.close()
191+ self.buffer.close()
192
193 def test_getSeriesKey_returns_None_on_mismatch(self):
194 # getSeriesKey returns None if the filename does not match the

Subscribers

People subscribed via source and target branches

to status/vote changes: