Launchpad itself

Merge lp:~wgrant/launchpad/publisher-release-cleanup into lp:launchpad

  1. publisher-release-cleanup
  2. Merge into devel
Proposed by William Grant
Status: Merged
Approved by: Abel Deuring
Approved revision: no longer in the source branch.
Merged at revision: 11730
Proposed branch: lp:~wgrant/launchpad/publisher-release-cleanup
Merge into: lp:launchpad
Prerequisite: lp:~wgrant/launchpad/better-publisher-index-tests
Diff against target: 509 lines (+95/-202)
3 files modified
lib/lp/archivepublisher/ftparchive.py (+12/-71)
lib/lp/archivepublisher/publishing.py (+76/-82)
lib/lp/archivepublisher/tests/test_publisher.py (+7/-49)
To merge this branch: bzr merge lp:~wgrant/launchpad/publisher-release-cleanup
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Review via email: mp+38530@code.launchpad.net

Commit message

Replace the messy FTPArchiveHandler.release_files_needed dict with a simple Publisher.release_files_needed set of suites.

Description of the change

= Summary =
lp.archivepublisher has two index generation mechanisms: native DB publication for partner, debug and PPA archives; and apt-ftparchive publication for primary and copy archives. The apt-ftparchive handler is deprecated and to be phased out, but its release_files_needed attribute is still used by the native publisher (bug #655690).

== Proposed fix ==
Move release_files_needed from FTPArchiveHandler to Publisher, and only instantiate FTPArchiveHandler when it's actually needed.

== Implementation details ==
While moving release_files_needed I realised that it wasn't very sensible: it consisted of a map from suite to component to a list of architectures. While the set of suites changes depending on what needs regeneration, the component and architecture lists must contain the full set each time -- calculating them in a roundabout manner and storing them in the dict is pointless.

So I changed release_files_needed into a set of suites, and adjusted its users to retrieve the component and architecture lists directly from the publisher configuration. This mainly involved the removal of hacks from _writeDistroArchSeries, which I've split and renamed.

I also removed some duplicated code from lib/lp/archivepublisher/ftparchive.py. It was XXX'd by kiko four years ago as probably being redundant, and investigation confirms that proposition.

== Tests ==
test_publisher.py has had complex release_files_needed verification removed. The new simpler data structure removes what they are testing, and their role is fulfilled by testAllIndicesArePublished as added in the prereq.

== Demo and Q/A ==
This is just a refactoring. It's fine as long as the tests work.

= Launchpad lint =
There is plenty of inherited lint that could be fixed, but this branch is heavy enough, and there are five following it fixing most of it.

To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/archivepublisher/ftparchive.py'
2--- lib/lp/archivepublisher/ftparchive.py 2010-10-06 13:50:50 +0000
3+++ lib/lp/archivepublisher/ftparchive.py 2010-10-15 11:41:18 +0000
4@@ -157,7 +157,6 @@
5 else:
6 self.distroseries.append(distroseries)
7 self.publisher = publisher
8- self.release_files_needed = {}
9
10 # We need somewhere to note down where the debian-installer
11 # components came from. in _di_release_components we store
12@@ -199,13 +198,9 @@
13 We do this to have Packages or Sources for them even if we lack
14 anything in them currently.
15 """
16- # XXX: kiko 2006-08-24: suffix is completely unnecessary here. Just
17- # iterate over the pockets, and do the suffix check inside
18- # createEmptyPocketRequest; that would also allow us to replace
19- # the == "" check we do there by a RELEASE match
20 for distroseries in self.distroseries:
21 components = self._config.componentsForSeries(distroseries.name)
22- for pocket, suffix in pocketsuffix.items():
23+ for pocket in PackagePublishingPocket.items:
24 if not fullpublish:
25 if not self.publisher.isDirty(distroseries, pocket):
26 continue
27@@ -213,29 +208,15 @@
28 if not self.publisher.isAllowed(distroseries, pocket):
29 continue
30
31+ self.publisher.release_files_needed.add(
32+ (distroseries.name, pocket))
33+
34 for comp in components:
35- self.createEmptyPocketRequest(distroseries, suffix, comp)
36-
37- def requestReleaseFile(self, suite_name, component_name, arch_name):
38- """Request Release file generation for given context.
39-
40- 'suite_name', 'component_name' and 'arch_name' will be organised as
41- a dictionary (self.release_files_needed) keyed by 'suite_name' which
42- value will be another dictionary keyed by 'component_name' and
43- containing a set of 'arch_name's as value.
44- """
45- suite_special = self.release_files_needed.setdefault(
46- suite_name, {})
47- suite_component_special = suite_special.setdefault(
48- component_name, set())
49- suite_component_special.add(arch_name)
50-
51- def createEmptyPocketRequest(self, distroseries, suffix, comp):
52+ self.createEmptyPocketRequest(distroseries, pocket, comp)
53+
54+ def createEmptyPocketRequest(self, distroseries, pocket, comp):
55 """Creates empty files for a release pocket and distroseries"""
56- full_distroseries_name = distroseries.name + suffix
57- arch_tags = self._config.archTagsForSeries(distroseries.name)
58-
59- if suffix == "":
60+ if pocket == PackagePublishingPocket.RELEASE:
61 # organize distroseries and component pair as
62 # debian-installer -> distroseries_component
63 # internal map. Only the main pocket actually
64@@ -246,6 +227,8 @@
65 ".".join(["override", distroseries.name, comp,
66 "debian-installer"]))
67
68+ full_distroseries_name = distroseries.name + pocketsuffix[pocket]
69+
70 # Touch the source file lists and override files
71 f_touch(self._config.overrideroot,
72 ".".join(["override", full_distroseries_name, comp]))
73@@ -254,20 +237,10 @@
74 f_touch(self._config.overrideroot,
75 ".".join(["override", full_distroseries_name, comp, "src"]))
76
77- dr_comps = self.release_files_needed.setdefault(
78- full_distroseries_name, {})
79-
80 f_touch(self._config.overrideroot,
81 "_".join([full_distroseries_name, comp, "source"]))
82- dr_comps.setdefault(comp, set()).add("source")
83-
84- for arch in arch_tags:
85- # organize dr/comp/arch into temporary binary
86- # archive map for the architecture in question.
87- dr_special = self.release_files_needed.setdefault(
88- full_distroseries_name, {})
89- dr_special.setdefault(comp, set()).add("binary-"+arch)
90-
91+
92+ for arch in self._config.archTagsForSeries(distroseries.name):
93 # Touch more file lists for the archs.
94 f_touch(self._config.overrideroot,
95 "_".join([full_distroseries_name, comp, "binary-"+arch]))
96@@ -714,9 +687,6 @@
97
98 Also outputs a debian-installer file list if necessary.
99 """
100- self.release_files_needed.setdefault(
101- dr_pocketed, {}).setdefault(component, set()).add(arch)
102-
103 files = []
104 di_files = []
105 f_path = os.path.join(self._config.overrideroot,
106@@ -807,37 +777,8 @@
107 """Generates the config stanza for an individual pocket."""
108 dr_pocketed = distroseries_name + pocketsuffix[pocket]
109
110- # XXX kiko 2006-08-24: I have no idea what the code below is meant
111- # to do -- it appears to be a rehash of createEmptyPocketRequests.
112 archs = self._config.archTagsForSeries(distroseries_name)
113 comps = self._config.componentsForSeries(distroseries_name)
114- for comp in comps:
115- comp_path = os.path.join(self._config.overrideroot,
116- "_".join([dr_pocketed, comp, "source"]))
117- if not os.path.exists(comp_path):
118- # Create empty files so that even if we don't output
119- # anything here apt-ftparchive will DTRT
120- f_touch(comp_path)
121- f_touch(self._config.overrideroot,
122- ".".join(["override", dr_pocketed, comp]))
123- f_touch(self._config.overrideroot,
124- ".".join(["override", dr_pocketed, comp, "src"]))
125-
126- if len(comps) == 0:
127- self.log.debug("Did not find any components to create config "
128- "for %s" % dr_pocketed)
129- return
130-
131- # Second up, pare archs down as appropriate
132- for arch in archs:
133- # XXX: kiko 2006-08-24: why is it comps[0] here?
134- arch_path = os.path.join(self._config.overrideroot,
135- "_".join([dr_pocketed, comps[0], "binary-"+arch]))
136- if not os.path.exists(arch_path):
137- # Create an empty file if we don't have one so that
138- # apt-ftparchive will dtrt.
139- f_touch(arch_path)
140- # XXX kiko 2006-08-24: End uncomprehensible code.
141
142 self.log.debug("Generating apt config for %s" % dr_pocketed)
143 apt_config.write(STANZA_TEMPLATE % {
144
145=== modified file 'lib/lp/archivepublisher/publishing.py'
146--- lib/lp/archivepublisher/publishing.py 2010-10-06 13:49:38 +0000
147+++ lib/lp/archivepublisher/publishing.py 2010-10-15 11:41:18 +0000
148@@ -58,13 +58,19 @@
149 Over time this method needs to be removed and replaced by having
150 component ordering codified in the database.
151 """
152- ret = []
153+ remaining = list(components)
154+ ordered = []
155 for comp in HARDCODED_COMPONENT_ORDER:
156- if comp in components:
157- ret.append(comp)
158- components.remove(comp)
159- ret.extend(components)
160- return ret
161+ if comp in remaining:
162+ ordered.append(comp)
163+ remaining.remove(comp)
164+ ordered.extend(remaining)
165+ return ordered
166+
167+
168+def get_suffixed_indices(path):
169+ """Return a set of paths to compressed copies of the given index."""
170+ return set([path + suffix for suffix in ('', '.gz', '.bz2')])
171
172
173 def _getDiskPool(pubconf, log):
174@@ -147,21 +153,19 @@
175 else:
176 self._library = library
177
178- # Grab a reference to an apt_handler as we use it later to
179- # probe which components need releases files generated.
180- self.apt_handler = FTPArchiveHandler(self.log, self._config,
181- self._diskpool, self.distro,
182- self)
183 # Track which distroseries pockets have been dirtied by a
184 # change, and therefore need domination/apt-ftparchive work.
185 # This is a set of tuples in the form (distroseries.name, pocket)
186 self.dirty_pockets = set()
187
188+ # Track which pockets need release files. This will contain more
189+ # than dirty_pockets in the case of a careful index run.
190+ # This is a set of tuples in the form (distroseries.name, pocket)
191+ self.release_files_needed = set()
192+
193 def isDirty(self, distroseries, pocket):
194 """True if a publication has happened in this release and pocket."""
195- if not (distroseries.name, pocket) in self.dirty_pockets:
196- return False
197- return True
198+ return (distroseries.name, pocket) in self.dirty_pockets
199
200 def markPocketDirty(self, distroseries, pocket):
201 """Mark a pocket dirty only if it's allowed."""
202@@ -176,10 +180,8 @@
203
204 Otherwise, return False.
205 """
206- if (self.allowed_suites and
207- (distroseries.name, pocket) not in self.allowed_suites):
208- return False
209- return True
210+ return (not self.allowed_suites or
211+ (distroseries.name, pocket) in self.allowed_suites)
212
213 def A_publish(self, force_publishing):
214 """First step in publishing: actual package publishing.
215@@ -281,7 +283,10 @@
216 def C_doFTPArchive(self, is_careful):
217 """Does the ftp-archive step: generates Sources and Packages."""
218 self.log.debug("* Step C: Set apt-ftparchive up and run it")
219- self.apt_handler.run(is_careful)
220+ apt_handler = FTPArchiveHandler(self.log, self._config,
221+ self._diskpool, self.distro,
222+ self)
223+ apt_handler.run(is_careful)
224
225 def C_writeIndexes(self, is_careful):
226 """Write Index files (Packages & Sources) using LP information.
227@@ -297,6 +302,9 @@
228 (distroseries.name, pocket.name))
229 continue
230 self.checkDirtySuiteBeforePublishing(distroseries, pocket)
231+
232+ self.release_files_needed.add((distroseries.name, pocket))
233+
234 # Retrieve components from the publisher config because
235 # it gets overridden in getPubConfig to set the
236 # correct components for the archive being used.
237@@ -323,7 +331,7 @@
238 (distroseries.name, pocket.name))
239 continue
240 self.checkDirtySuiteBeforePublishing(distroseries, pocket)
241- self._writeDistroSeries(distroseries, pocket)
242+ self._writeSuite(distroseries, pocket)
243
244 def _writeComponentIndexes(self, distroseries, pocket, component):
245 """Write Index files for single distroseries + pocket + component.
246@@ -358,11 +366,6 @@
247
248 arch_path = 'binary-%s' % arch.architecturetag
249
250- # XXX wgrant 2010-10-06 bug=655690: Using FTPArchiveHandler
251- # for NMAF is wrong.
252- self.apt_handler.requestReleaseFile(
253- suite_name, component.name, arch_path)
254-
255 self.log.debug("Generating Packages for %s" % arch_path)
256
257 package_index_root = os.path.join(
258@@ -394,11 +397,6 @@
259 package_index.close()
260 di_index.close()
261
262- # XXX wgrant 2010-10-06 bug=655690: Using FTPArchiveHandler
263- # is wrong here too.
264- self.apt_handler.requestReleaseFile(
265- suite_name, component.name, 'source')
266-
267 def cannotModifySuite(self, distroseries, pocket):
268 """Return True if the distroseries is stable and pocket is release."""
269 return (not distroseries.isUnstable() and
270@@ -447,34 +445,28 @@
271 return self.distro.displayname
272 return "LP-PPA-%s" % get_ppa_reference(self.archive)
273
274- def _writeDistroSeries(self, distroseries, pocket):
275- """Write out the Release files for the provided distroseries."""
276+ def _writeSuite(self, distroseries, pocket):
277+ """Write out the Release files for the provided suite."""
278 # XXX: kiko 2006-08-24: Untested method.
279
280 # As we generate file lists for apt-ftparchive we record which
281 # distroseriess and so on we need to generate Release files for.
282 # We store this in release_files_needed and consume the information
283 # when writeReleaseFiles is called.
284- full_name = distroseries.name + pocketsuffix[pocket]
285- release_files_needed = self.apt_handler.release_files_needed
286- if full_name not in release_files_needed:
287+ if (distroseries.name, pocket) not in self.release_files_needed:
288 # If we don't need to generate a release for this release
289 # and pocket, don't!
290 return
291
292- all_components = set()
293- all_architectures = set()
294+ all_components = self._config.componentsForSeries(distroseries.name)
295+ all_architectures = self._config.archTagsForSeries(distroseries.name)
296 all_files = set()
297- release_files_needed_items = release_files_needed[full_name].items()
298- for component, architectures in release_files_needed_items:
299- all_components.add(component)
300- for architecture in architectures:
301- # XXX malcc 2006-09-20: We don't like the way we build this
302- # all_architectures list. Make this better code.
303- clean_architecture = self._writeDistroArchSeries(
304+ for component in all_components:
305+ self._writeSuiteSource(
306+ distroseries, pocket, component, all_files)
307+ for architecture in all_architectures:
308+ self._writeSuiteArch(
309 distroseries, pocket, component, architecture, all_files)
310- if clean_architecture != "source":
311- all_architectures.add(clean_architecture)
312
313 drsummary = "%s %s " % (self.distro.displayname,
314 distroseries.displayname)
315@@ -483,6 +475,7 @@
316 else:
317 drsummary += pocket.name.capitalize()
318
319+ full_name = distroseries.getSuite(pocket)
320 release_file = Release()
321 release_file["Origin"] = self._getOrigin()
322 release_file["Label"] = self._getLabel()
323@@ -530,60 +523,61 @@
324 archive_signer = IArchiveSigningKey(self.archive)
325 archive_signer.signRepository(full_name)
326
327- def _writeDistroArchSeries(self, distroseries, pocket, component,
328- architecture, all_files):
329- """Write out a Release file for a DAR."""
330+ def _writeSuiteArchOrSource(self, distroseries, pocket, component,
331+ file_stub, arch_name, arch_path,
332+ all_series_files):
333+ """Write out a Release file for an architecture or source."""
334 # XXX kiko 2006-08-24: Untested method.
335
336- full_name = distroseries.name + pocketsuffix[pocket]
337- index_suffixes = ('', '.gz', '.bz2')
338-
339+ suite = distroseries.getSuite(pocket)
340 self.log.debug("Writing Release file for %s/%s/%s" % (
341- full_name, component, architecture))
342-
343- if architecture != "source":
344- # Strip "binary-" off the front of the architecture
345- clean_architecture = architecture[7:]
346- file_stub = "Packages"
347-
348- # Only the primary and PPA archives have debian-installer.
349- if self.archive.purpose != ArchivePurpose.PARTNER:
350- # Set up the debian-installer paths for main_archive.
351- # d-i paths are nested inside the component.
352- di_path = os.path.join(
353- component, "debian-installer", architecture)
354- di_file_stub = os.path.join(di_path, file_stub)
355- for suffix in index_suffixes:
356- all_files.add(di_file_stub + suffix)
357- else:
358- file_stub = "Sources"
359- clean_architecture = architecture
360+ suite, component, arch_path))
361
362 # Now, grab the actual (non-di) files inside each of
363 # the suite's architectures
364- file_stub = os.path.join(component, architecture, file_stub)
365-
366- for suffix in index_suffixes:
367- all_files.add(file_stub + suffix)
368-
369- all_files.add(os.path.join(component, architecture, "Release"))
370+ file_stub = os.path.join(component, arch_path, file_stub)
371+
372+ all_series_files.update(get_suffixed_indices(file_stub))
373+ all_series_files.add(os.path.join(component, arch_path, "Release"))
374
375 release_file = Release()
376- release_file["Archive"] = full_name
377+ release_file["Archive"] = suite
378 release_file["Version"] = distroseries.version
379 release_file["Component"] = component
380 release_file["Origin"] = self._getOrigin()
381 release_file["Label"] = self._getLabel()
382- release_file["Architecture"] = clean_architecture
383+ release_file["Architecture"] = arch_name
384
385- f = open(os.path.join(self._config.distsroot, full_name,
386- component, architecture, "Release"), "w")
387+ f = open(os.path.join(self._config.distsroot, suite,
388+ component, arch_path, "Release"), "w")
389 try:
390 release_file.dump(f, "utf-8")
391 finally:
392 f.close()
393
394- return clean_architecture
395+ def _writeSuiteSource(self, distroseries, pocket, component,
396+ all_series_files):
397+ """Write out a Release file for a suite's sources."""
398+ self._writeSuiteArchOrSource(
399+ distroseries, pocket, component, 'Sources', 'source', 'source',
400+ all_series_files)
401+
402+ def _writeSuiteArch(self, distroseries, pocket, component,
403+ arch_name, all_series_files):
404+ """Write out a Release file for an architecture in a suite."""
405+ file_stub = 'Packages'
406+ arch_path = 'binary-' + arch_name
407+ # Only the primary and PPA archives have debian-installer.
408+ if self.archive.purpose != ArchivePurpose.PARTNER:
409+ # Set up the debian-installer paths for main_archive.
410+ # d-i paths are nested inside the component.
411+ di_path = os.path.join(
412+ component, "debian-installer", arch_path)
413+ di_file_stub = os.path.join(di_path, file_stub)
414+ all_series_files.update(get_suffixed_indices(di_file_stub))
415+ self._writeSuiteArchOrSource(
416+ distroseries, pocket, component, 'Packages', arch_name, arch_path,
417+ all_series_files)
418
419 def _readIndexFileContents(self, distroseries_name, file_name):
420 """Read an index files' contents.
421
422=== modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
423--- lib/lp/archivepublisher/tests/test_publisher.py 2010-10-15 11:41:16 +0000
424+++ lib/lp/archivepublisher/tests/test_publisher.py 2010-10-15 11:41:18 +0000
425@@ -639,13 +639,10 @@
426 ''],
427 index_contents)
428
429- # Check if apt_handler.release_files_needed has the right requests.
430- # 'source' & 'binary-i386' Release files should be regenerated
431- # for all breezy-autotest components. Note that 'debian-installer'
432- # indexes do not need Release files.
433-
434 # We always regenerate all Releases file for a given suite.
435- self.checkAllRequestedReleaseFiles(archive_publisher)
436+ self.assertTrue(
437+ ('breezy-autotest', PackagePublishingPocket.RELEASE) in
438+ archive_publisher.release_files_needed)
439
440 # remove PPA root
441 shutil.rmtree(config.personalpackagearchive.root)
442@@ -769,45 +766,6 @@
443 # are marked as dirty.
444 self.checkDirtyPockets(publisher, expected=allowed_suites)
445
446- def assertReleaseFileRequested(self, publisher, suite_name,
447- component_name, archs):
448- """Assert the given context will have it's Release file regenerated.
449-
450- Check if a request for the given context is correctly stored in:
451- publisher.apt_handler.release_files_needed
452- """
453- suite = publisher.apt_handler.release_files_needed.get(suite_name)
454- self.assertTrue(
455- suite is not None, 'Suite %s not requested' % suite_name)
456- self.assertTrue(
457- component_name in suite,
458- 'Component %s/%s not requested' % (suite_name, component_name))
459- self.assertEquals(archs, suite[component_name])
460-
461- def checkAllRequestedReleaseFiles(self, publisher,
462- architecturetags=('hppa', 'i386')):
463- """Check if all expected Release files are going to be regenerated.
464-
465- 'source', 'binary-i386' and 'binary-hppa' Release Files should be
466- requested for regeneration in all breezy-autotest components.
467- """
468- available_components = sorted([
469- c.name for c in self.breezy_autotest.components])
470- self.assertEqual(available_components,
471- ['main', 'multiverse', 'restricted', 'universe'])
472-
473- available_archs = ['binary-%s' % arch for arch in architecturetags]
474-
475- # XXX cprov 20071210: Include the artificial component 'source' as a
476- # location to check for generated indexes. Ideally we should
477- # encapsulate this task in publishing.py and this common method
478- # in tests as well.
479- all_archs = set(available_archs)
480- all_archs.add('source')
481- for component in available_components:
482- self.assertReleaseFileRequested(
483- publisher, 'breezy-autotest', component, all_archs)
484-
485 def _getReleaseFileOrigin(self, contents):
486 origin_header = 'Origin: '
487 [origin_line] = [
488@@ -831,8 +789,9 @@
489 publisher.A_publish(False)
490 publisher.C_doFTPArchive(False)
491
492- # We always regenerate all Releases file for a given suite.
493- self.checkAllRequestedReleaseFiles(publisher)
494+ self.assertTrue(
495+ ('breezy-autotest', PackagePublishingPocket.RELEASE) in
496+ publisher.release_files_needed)
497
498 publisher.D_writeReleaseFiles(False)
499
500@@ -1109,8 +1068,7 @@
501 """
502
503 self.assertTrue(
504- series.getSuite(pocket) in
505- publisher.apt_handler.release_files_needed)
506+ (series.name, pocket) in publisher.release_files_needed)
507
508 arch_template = os.path.join(
509 publisher._config.distsroot, series.getSuite(pocket), '%s/%s')