Merge lp:~stevenk/launchpad/derive-common-ancestor into lp:launchpad

Proposed by Steve Kowalik
Status: Merged
Approved by: Steve Kowalik
Approved revision: no longer in the source branch.
Merged at revision: 12580
Proposed branch: lp:~stevenk/launchpad/derive-common-ancestor
Merge into: lp:launchpad
Diff against target: 396 lines (+128/-67)
5 files modified
lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py (+9/-1)
lib/lp/registry/model/distroseriesdifference.py (+20/-23)
lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py (+11/-2)
lib/lp/registry/tests/test_distroseriesdifference.py (+49/-27)
lib/lp/testing/factory.py (+39/-14)
To merge this branch: bzr merge lp:~stevenk/launchpad/derive-common-ancestor
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
Review via email: mp+52796@code.launchpad.net

Commit message

[r=allenap][bug=732392] Use changelog parsing to calculate base versions for DistroSeriesDifferences.

Description of the change

Change how base versions are calculated for DistroSeriesDifferences.

When DSDs were first implemented, they based base version calculation on the assumption that the highest common version is published in both series. Unfortunately, this assumption is false.

Based on the work from a Soyuz mini-sprint (the last one ever, in fact) it was determined that the safest and most correct way to determine the base version is parsing the changelog of the two packages. See https://dev.launchpad.net/Soyuz/BaseVersionDetermination for more information.

I have implemented a method into the object factory that creates a changelog with as many entries as required, which is then stored in the Librarian, and the LFA returned. I have extended makeSourcePackageRelease to accept a changelog argument, which is set on creation, and finally I have extended makeDistroSeriesDifference to set changelog if they are passed in.

It also does a little bit of drive-by in terms of lint.

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :

Lovely jubbly :)

[1]

+ entry = dedent('''
+ %s (%s) unstable; urgency=low
+
+ * %s.
+
+ -- Foo Bar <email address hidden> Tue, 01 Jan 1970 01:50:41 +0000
+
+ ''' % (spn, version, version))

This trailing whitespace will probably get culled by someone's editor
one day. If it's needed then I think you should find a more robust way
of ensuring it stays there. Fwiw, dedent will do the right thing
without it.

There's some trailing whitespace in test_distroseriesdifference.py
too.

[2]

There are no tests for the new factory methods. I'm not sure of how
needed they are, but there are a fair few in test_factory. Add some if
you think there's some value to it.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py'
2--- lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py 2010-10-29 19:28:14 +0000
3+++ lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py 2011-03-11 00:32:02 +0000
4@@ -103,11 +103,19 @@
5
6 def test_requestDiffs(self):
7 # The generation of package diffs can be requested via the API.
8+ derived_changelog = self.factory.makeChangelog(
9+ versions=['1.0', '1.2'])
10+ parent_changelog = self.factory.makeChangelog(
11+ versions=['1.0', '1.3'])
12+ transaction.commit() # Yay, librarian.
13 ds_diff = self.factory.makeDistroSeriesDifference(
14 source_package_name_str='foo', versions={
15 'derived': '1.2',
16 'parent': '1.3',
17- 'base': '1.0',
18+ 'base': '1.0'},
19+ changelogs={
20+ 'derived': derived_changelog,
21+ 'parent': parent_changelog,
22 })
23 ws_diff = ws_object(self.factory.makeLaunchpadService(
24 ds_diff.owner), ds_diff)
25
26=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
27--- lib/lp/registry/model/distroseriesdifference.py 2011-03-04 05:54:40 +0000
28+++ lib/lp/registry/model/distroseriesdifference.py 2011-03-11 00:32:02 +0000
29@@ -9,6 +9,7 @@
30 'DistroSeriesDifference',
31 ]
32
33+from debian.changelog import Changelog
34 from lazr.enum import DBItem
35 from storm.expr import Desc
36 from storm.locals import (
37@@ -182,6 +183,12 @@
38 'source_version': self.source_version,
39 })
40
41+ def getAncestry(self, spr):
42+ """Return the version ancestry for the given SPR, or None."""
43+ if spr.changelog is None:
44+ return None
45+ return set(Changelog(spr.changelog.read()).versions)
46+
47 def _getPackageDiffURL(self, package_diff):
48 """Check status and return URL if appropriate."""
49 if package_diff is None or (
50@@ -299,29 +306,19 @@
51 DistroSeriesDifferenceType.DIFFERENT_VERSIONS):
52 return False
53
54- # Find all source package releases for the derived and parent
55- # series and get the most recent common version.
56- derived_sprs = self.source_pub.meta_sourcepackage.distinctreleases
57- derived_versions = [
58- Version(spr.version) for spr in derived_sprs]
59-
60- parent_sourcepkg = self.parent_source_pub.meta_sourcepackage
61- parent_sprs = parent_sourcepkg.distinctreleases
62- parent_versions = [
63- Version(spr.version) for spr in parent_sprs]
64-
65- common_versions = list(
66- set(derived_versions).intersection(parent_versions))
67- if common_versions:
68- common_versions.sort()
69- self.base_version = unicode(common_versions.pop())
70- return True
71-
72- if self.base_version is None:
73- return False
74- else:
75- self.base_version = None
76- return True
77+ ancestry = self.getAncestry(self.source_pub.sourcepackagerelease)
78+ parent_ancestry = self.getAncestry(
79+ self.parent_source_pub.sourcepackagerelease)
80+
81+ # If the ancestry for the parent and the descendant is available, we
82+ # can reliably work out the most recent common ancestor using set
83+ # arithmetic.
84+ if ancestry is not None and parent_ancestry is not None:
85+ intersection = ancestry.intersection(parent_ancestry)
86+ if len(intersection) > 0:
87+ self.base_version = unicode(max(intersection))
88+ return True
89+ return False
90
91 def addComment(self, commenter, comment):
92 """See `IDistroSeriesDifference`."""
93
94=== modified file 'lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py'
95--- lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py 2011-03-04 05:34:46 +0000
96+++ lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py 2011-03-11 00:32:02 +0000
97@@ -14,6 +14,7 @@
98 )
99 from canonical.testing.layers import (
100 DatabaseFunctionalLayer,
101+ LaunchpadFunctionalLayer,
102 ZopelessDatabaseLayer,
103 )
104 from lp.registry.enum import (
105@@ -398,7 +399,7 @@
106 class TestPopulateDistroSeriesDiff(TestCaseWithFactory, FactoryHelper):
107 """Test `populate_distroseriesdiff`."""
108
109- layer = ZopelessDatabaseLayer
110+ layer = LaunchpadFunctionalLayer
111
112 def test_baseline(self):
113 distroseries = self.factory.makeDistroSeries()
114@@ -418,17 +419,25 @@
115
116 def test_does_not_overwrite_distroseriesdifference(self):
117 distroseries = self.makeDerivedDistroSeries()
118+ changelog = self.factory.makeChangelog(versions=['3.1', '3.141'])
119+ parent_changelog = self.factory.makeChangelog(
120+ versions=['3.1', '3.14'])
121+ transaction.commit() # Yay, librarian.
122 existing_versions = {
123 'base': '3.1',
124 'parent': '3.14',
125 'derived': '3.141',
126 }
127+ changelogs = {
128+ 'derived': changelog,
129+ 'parent': parent_changelog,
130+ }
131 spph = self.makeSPPH(distroseries=distroseries)
132 spr = spph.sourcepackagerelease
133 self.factory.makeDistroSeriesDifference(
134 derived_series=distroseries,
135 source_package_name_str=spr.sourcepackagename.name,
136- versions=existing_versions)
137+ versions=existing_versions, changelogs=changelogs)
138 dsd = self.getDistroSeriesDiff(distroseries).one()
139 self.assertEqual(existing_versions['base'], dsd.base_version)
140 self.assertEqual(
141
142=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
143--- lib/lp/registry/tests/test_distroseriesdifference.py 2010-12-22 14:50:08 +0000
144+++ lib/lp/registry/tests/test_distroseriesdifference.py 2011-03-11 00:32:02 +0000
145@@ -5,10 +5,9 @@
146
147 __metaclass__ = type
148
149-import unittest
150-
151 from storm.exceptions import IntegrityError
152 from storm.store import Store
153+import transaction
154 from zope.component import getUtility
155 from zope.security.interfaces import Unauthorized
156 from zope.security.proxy import removeSecurityProxy
157@@ -39,7 +38,7 @@
158
159 class DistroSeriesDifferenceTestCase(TestCaseWithFactory):
160
161- layer = DatabaseFunctionalLayer
162+ layer = LaunchpadFunctionalLayer
163
164 def test_implements_interface(self):
165 # The implementation implements the interface correctly.
166@@ -445,37 +444,25 @@
167
168 self.assertIs(None, ds_diff.base_version)
169
170- def test_base_version_common(self):
171- # The common base version is set when the difference is created.
172- # Publish v1.0 of foo in both series.
173- ds_diff = self.factory.makeDistroSeriesDifference(versions={
174- 'derived': '1.2',
175- 'parent': '1.3',
176- 'base': '1.0',
177- })
178-
179- self.assertEqual('1.0', ds_diff.base_version)
180-
181 def test_base_version_multiple(self):
182 # The latest common base version is set as the base-version.
183- # Publish v1.0 and 1.1 of foo in both series.
184 derived_series = self.factory.makeDistroSeries(
185 parent_series=self.factory.makeDistroSeries())
186 source_package_name = self.factory.getOrMakeSourcePackageName('foo')
187- for series in [derived_series, derived_series.parent_series]:
188- for version in ['1.0', '1.1']:
189- self.factory.makeSourcePackagePublishingHistory(
190- distroseries=series,
191- version=version,
192- sourcepackagename=source_package_name,
193- status=PackagePublishingStatus.SUPERSEDED)
194+ # Create changelogs for both.
195+ changelog_lfa = self.factory.makeChangelog('foo', ['1.2', '1.1'])
196+ parent_changelog_lfa = self.factory.makeChangelog('foo', ['1.1'])
197+ transaction.commit() # Yay, librarian.
198
199 ds_diff = self.factory.makeDistroSeriesDifference(
200 derived_series=derived_series, source_package_name_str='foo',
201 versions={
202 'derived': '1.2',
203 'parent': '1.3',
204- })
205+ },
206+ changelogs={
207+ 'derived': changelog_lfa,
208+ 'parent': parent_changelog_lfa})
209
210 self.assertEqual('1.1', ds_diff.base_version)
211
212@@ -488,22 +475,61 @@
213
214 def test_base_source_pub(self):
215 # The publishing in the derived series with base_version is returned.
216+ derived_changelog = self.factory.makeChangelog(
217+ versions=['1.0', '1.2'])
218+ parent_changelog = self.factory.makeChangelog(
219+ versions=['1.0', '1.3'])
220+ transaction.commit() # Yay, librarian.
221+
222 ds_diff = self.factory.makeDistroSeriesDifference(versions={
223 'derived': '1.2',
224 'parent': '1.3',
225 'base': '1.0',
226+ },
227+ changelogs={
228+ 'derived': derived_changelog,
229+ 'parent': parent_changelog,
230 })
231
232 base_pub = ds_diff.base_source_pub
233 self.assertEqual('1.0', base_pub.source_package_version)
234 self.assertEqual(ds_diff.derived_series, base_pub.distroseries)
235
236+ def test_base_source_pub_not_published(self):
237+ # If the base version isn't published, the base version is
238+ # calculated, but the source publication isn't set.
239+ derived_changelog = self.factory.makeChangelog(
240+ versions=['1.0', '1.2'])
241+ parent_changelog = self.factory.makeChangelog(
242+ versions=['1.0', '1.3'])
243+ transaction.commit() # Yay, librarian.
244+
245+ ds_diff = self.factory.makeDistroSeriesDifference(versions={
246+ 'derived': '1.2',
247+ 'parent': '1.3',
248+ },
249+ changelogs={
250+ 'derived': derived_changelog,
251+ 'parent': parent_changelog,
252+ })
253+ self.assertEqual('1.0', ds_diff.base_version)
254+ self.assertIs(None, ds_diff.base_source_pub)
255+
256 def test_requestPackageDiffs(self):
257 # IPackageDiffs are created for the corresponding versions.
258+ dervied_changelog = self.factory.makeChangelog(
259+ versions=['1.0', '1.2'])
260+ parent_changelog = self.factory.makeChangelog(
261+ versions=['1.0', '1.3'])
262+ transaction.commit() # Yay, librarian.
263 ds_diff = self.factory.makeDistroSeriesDifference(versions={
264 'derived': '1.2',
265 'parent': '1.3',
266 'base': '1.0',
267+ },
268+ changelogs={
269+ 'derived': dervied_changelog,
270+ 'parent': parent_changelog,
271 })
272
273 with person_logged_in(ds_diff.owner):
274@@ -647,7 +673,3 @@
275 ds_diff.derived_series, 'fooname')
276
277 self.assertEqual(ds_diff, result)
278-
279-
280-def test_suite():
281- return unittest.TestLoader().loadTestsFromName(__name__)
282
283=== modified file 'lib/lp/testing/factory.py'
284--- lib/lp/testing/factory.py 2011-03-10 07:16:43 +0000
285+++ lib/lp/testing/factory.py 2011-03-11 00:32:02 +0000
286@@ -2051,6 +2051,23 @@
287 removeSecurityProxy(code_import).review_status = review_status
288 return code_import
289
290+ def makeChangelog(self, spn=None, versions=[]):
291+ """Create and return a LFA of a valid Debian-style changelog."""
292+ if spn is None:
293+ spn = self.getUniqueString()
294+ changelog = ''
295+ for version in versions:
296+ entry = dedent('''
297+ %s (%s) unstable; urgency=low
298+
299+ * %s.
300+
301+ -- Foo Bar <foo@example.com> Tue, 01 Jan 1970 01:50:41 +0000
302+
303+ ''' % (spn, version, version))
304+ changelog += entry
305+ return self.makeLibraryFileAlias(content=changelog)
306+
307 def makeCodeImportEvent(self):
308 """Create and return a CodeImportEvent."""
309 code_import = self.makeCodeImport()
310@@ -2250,7 +2267,8 @@
311 self, derived_series=None, source_package_name_str=None,
312 versions=None,
313 difference_type=DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
314- status=DistroSeriesDifferenceStatus.NEEDS_ATTENTION):
315+ status=DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
316+ changelogs=None):
317 """Create a new distro series source package difference."""
318 if derived_series is None:
319 parent_series = self.makeDistroSeries()
320@@ -2265,32 +2283,38 @@
321
322 if versions is None:
323 versions = {}
324+ if changelogs is None:
325+ changelogs = {}
326
327 base_version = versions.get('base')
328 if base_version is not None:
329 for series in [derived_series, derived_series.parent_series]:
330- self.makeSourcePackagePublishingHistory(
331- distroseries=series,
332- version=base_version,
333+ spr = self.makeSourcePackageRelease(
334 sourcepackagename=source_package_name,
335+ version=base_version)
336+ self.makeSourcePackagePublishingHistory(
337+ distroseries=series, sourcepackagerelease=spr,
338 status=PackagePublishingStatus.SUPERSEDED)
339
340 if difference_type is not (
341 DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES):
342-
343- self.makeSourcePackagePublishingHistory(
344- distroseries=derived_series,
345+ spr = self.makeSourcePackageRelease(
346+ sourcepackagename=source_package_name,
347 version=versions.get('derived'),
348- sourcepackagename=source_package_name,
349+ changelog=changelogs.get('derived'))
350+ self.makeSourcePackagePublishingHistory(
351+ distroseries=derived_series, sourcepackagerelease=spr,
352 status = PackagePublishingStatus.PUBLISHED)
353
354 if difference_type is not (
355 DistroSeriesDifferenceType.UNIQUE_TO_DERIVED_SERIES):
356-
357+ spr = self.makeSourcePackageRelease(
358+ sourcepackagename=source_package_name,
359+ version=versions.get('parent'),
360+ changelog=changelogs.get('parent'))
361 self.makeSourcePackagePublishingHistory(
362 distroseries=derived_series.parent_series,
363- version=versions.get('parent'),
364- sourcepackagename=source_package_name,
365+ sourcepackagerelease=spr,
366 status = PackagePublishingStatus.PUBLISHED)
367
368 diff = getUtility(IDistroSeriesDifferenceSource).new(
369@@ -2594,7 +2618,7 @@
370 source_package_recipe_build=sprb,
371 sourcepackagename=sourcepackagename,
372 distroseries=distroseries, archive=archive)
373- spph = self.makeSourcePackagePublishingHistory(
374+ self.makeSourcePackagePublishingHistory(
375 sourcepackagerelease=spr, archive=archive,
376 distroseries=distroseries)
377
378@@ -3198,7 +3222,8 @@
379 dscsigningkey=None,
380 user_defined_fields=None,
381 changelog_entry=None,
382- homepage=None):
383+ homepage=None,
384+ changelog=None):
385 """Make a `SourcePackageRelease`."""
386 if distroseries is None:
387 if source_package_recipe_build is not None:
388@@ -3254,7 +3279,7 @@
389 build_conflicts=build_conflicts,
390 build_conflicts_indep=build_conflicts_indep,
391 architecturehintlist=architecturehintlist,
392- changelog=None,
393+ changelog=changelog,
394 changelog_entry=changelog_entry,
395 dsc=None,
396 copyright=self.getUniqueString(),