Merge lp:~wgrant/launchpad/bug-1350208 into lp:launchpad
- bug-1350208
- Merge into devel
Proposed by
William Grant
Status: | Merged |
---|---|
Approved by: | William Grant |
Approved revision: | no longer in the source branch. |
Merged at revision: | 17231 |
Proposed branch: | lp:~wgrant/launchpad/bug-1350208 |
Merge into: | lp:launchpad |
Diff against target: |
308 lines (+181/-50) 4 files modified
lib/lp/soyuz/adapters/buildarch.py (+2/-2) lib/lp/soyuz/adapters/tests/test_buildarch.py (+1/-1) lib/lp/soyuz/model/binarypackagebuild.py (+75/-47) lib/lp/soyuz/tests/test_build_set.py (+103/-0) |
To merge this branch: | bzr merge lp:~wgrant/launchpad/bug-1350208 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
William Grant | code | Approve | |
Review via email: mp+240808@code.launchpad.net |
Commit message
Fix createForSource to correctly handle unusual arch-indep cases (eg. changes in nominatedarchindep, or "Architecture: powerpc all").
Description of the change
To post a comment you must log in.
Revision history for this message
William Grant (wgrant) : | # |
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/soyuz/adapters/buildarch.py' | |||
2 | --- lib/lp/soyuz/adapters/buildarch.py 2013-05-31 04:27:26 +0000 | |||
3 | +++ lib/lp/soyuz/adapters/buildarch.py 2014-11-06 05:30:00 +0000 | |||
4 | @@ -37,7 +37,7 @@ | |||
5 | 37 | 37 | ||
6 | 38 | 38 | ||
7 | 39 | def determine_architectures_to_build(hintlist, archive, distroseries, | 39 | def determine_architectures_to_build(hintlist, archive, distroseries, |
9 | 40 | legal_archseries): | 40 | legal_archseries, need_arch_indep): |
10 | 41 | """Return a list of architectures for which this publication should build. | 41 | """Return a list of architectures for which this publication should build. |
11 | 42 | 42 | ||
12 | 43 | This function answers the question: given a list of architectures and | 43 | This function answers the question: given a list of architectures and |
13 | @@ -78,7 +78,7 @@ | |||
14 | 78 | 78 | ||
15 | 79 | # 'all' is only used as a last resort, to create an arch-indep build | 79 | # 'all' is only used as a last resort, to create an arch-indep build |
16 | 80 | # where no builds would otherwise exist. | 80 | # where no builds would otherwise exist. |
18 | 81 | if len(build_tags) == 0 and 'all' in hint_archs: | 81 | if need_arch_indep and len(build_tags) == 0 and 'all' in hint_archs: |
19 | 82 | nominated_arch = distroseries.nominatedarchindep | 82 | nominated_arch = distroseries.nominatedarchindep |
20 | 83 | if nominated_arch in legal_archseries: | 83 | if nominated_arch in legal_archseries: |
21 | 84 | build_tags = set([nominated_arch.architecturetag]) | 84 | build_tags = set([nominated_arch.architecturetag]) |
22 | 85 | 85 | ||
23 | === modified file 'lib/lp/soyuz/adapters/tests/test_buildarch.py' | |||
24 | --- lib/lp/soyuz/adapters/tests/test_buildarch.py 2013-09-11 06:05:44 +0000 | |||
25 | +++ lib/lp/soyuz/adapters/tests/test_buildarch.py 2014-11-06 05:30:00 +0000 | |||
26 | @@ -34,7 +34,7 @@ | |||
27 | 34 | if arch.architecturetag in allowed_arch_tags] | 34 | if arch.architecturetag in allowed_arch_tags] |
28 | 35 | architectures = determine_architectures_to_build( | 35 | architectures = determine_architectures_to_build( |
29 | 36 | pub.sourcepackagerelease.architecturehintlist, pub.archive, | 36 | pub.sourcepackagerelease.architecturehintlist, pub.archive, |
31 | 37 | self.publisher.breezy_autotest, allowed_archs) | 37 | self.publisher.breezy_autotest, allowed_archs, True) |
32 | 38 | self.assertContentEqual( | 38 | self.assertContentEqual( |
33 | 39 | expected_arch_tags, [a.architecturetag for a in architectures]) | 39 | expected_arch_tags, [a.architecturetag for a in architectures]) |
34 | 40 | 40 | ||
35 | 41 | 41 | ||
36 | === modified file 'lib/lp/soyuz/model/binarypackagebuild.py' | |||
37 | --- lib/lp/soyuz/model/binarypackagebuild.py 2014-11-06 00:33:31 +0000 | |||
38 | +++ lib/lp/soyuz/model/binarypackagebuild.py 2014-11-06 05:30:00 +0000 | |||
39 | @@ -13,7 +13,10 @@ | |||
40 | 13 | ] | 13 | ] |
41 | 14 | 14 | ||
42 | 15 | import datetime | 15 | import datetime |
44 | 16 | from operator import itemgetter | 16 | from operator import ( |
45 | 17 | attrgetter, | ||
46 | 18 | itemgetter, | ||
47 | 19 | ) | ||
48 | 17 | 20 | ||
49 | 18 | import apt_pkg | 21 | import apt_pkg |
50 | 19 | import pytz | 22 | import pytz |
51 | @@ -1383,58 +1386,83 @@ | |||
52 | 1383 | if not distroarch.processor.restricted or | 1386 | if not distroarch.processor.restricted or |
53 | 1384 | distroarch.processor in archive.enabled_restricted_processors] | 1387 | distroarch.processor in archive.enabled_restricted_processors] |
54 | 1385 | 1388 | ||
55 | 1386 | def _createMissingBuildForArchitecture(self, sourcepackagerelease, | ||
56 | 1387 | archive, arch, pocket, | ||
57 | 1388 | logger=None): | ||
58 | 1389 | """Create a build for a given architecture if it doesn't exist yet. | ||
59 | 1390 | |||
60 | 1391 | Return the just-created `IBinaryPackageBuild` record already | ||
61 | 1392 | scored or None if a suitable build is already present. | ||
62 | 1393 | """ | ||
63 | 1394 | exact_build = self.getBySourceAndLocation( | ||
64 | 1395 | sourcepackagerelease, archive, arch) | ||
65 | 1396 | if exact_build is not None: | ||
66 | 1397 | return None | ||
67 | 1398 | |||
68 | 1399 | build_candidate = self.findBuiltOrPublishedBySourceAndArchive( | ||
69 | 1400 | sourcepackagerelease, archive).get(arch.architecturetag) | ||
70 | 1401 | if build_candidate is not None: | ||
71 | 1402 | return None | ||
72 | 1403 | |||
73 | 1404 | build = self.new( | ||
74 | 1405 | source_package_release=sourcepackagerelease, | ||
75 | 1406 | distro_arch_series=arch, archive=archive, pocket=pocket) | ||
76 | 1407 | # Create the builds in suspended mode for disabled archives. | ||
77 | 1408 | build_queue = build.queueBuild(suspended=not archive.enabled) | ||
78 | 1409 | Store.of(build).flush() | ||
79 | 1410 | |||
80 | 1411 | if logger is not None: | ||
81 | 1412 | logger.debug( | ||
82 | 1413 | "Created %s [%d] in %s (%d)" | ||
83 | 1414 | % (build.title, build.id, build.archive.displayname, | ||
84 | 1415 | build_queue.lastscore)) | ||
85 | 1416 | |||
86 | 1417 | return build | ||
87 | 1418 | |||
88 | 1419 | def createForSource(self, sourcepackagerelease, archive, distroseries, | 1389 | def createForSource(self, sourcepackagerelease, archive, distroseries, |
89 | 1420 | pocket, architectures_available=None, logger=None): | 1390 | pocket, architectures_available=None, logger=None): |
90 | 1421 | """See `ISourcePackagePublishingHistory`.""" | 1391 | """See `ISourcePackagePublishingHistory`.""" |
91 | 1392 | |||
92 | 1393 | # Exclude any architectures which already have built or copied | ||
93 | 1394 | # binaries. A new build with the same archtag could never | ||
94 | 1395 | # succeed; its files would conflict during upload. | ||
95 | 1396 | relevant_builds = self.findBuiltOrPublishedBySourceAndArchive( | ||
96 | 1397 | sourcepackagerelease, archive).values() | ||
97 | 1398 | |||
98 | 1399 | # Find any architectures that already have a build that exactly | ||
99 | 1400 | # matches, regardless of status. We can't create a second build | ||
100 | 1401 | # with the same (SPR, Archive, DAS). | ||
101 | 1402 | # XXX wgrant 2014-11-06: Should use a bulk query. | ||
102 | 1403 | for das in distroseries.architectures: | ||
103 | 1404 | build = self.getBySourceAndLocation( | ||
104 | 1405 | sourcepackagerelease, archive, das) | ||
105 | 1406 | if build is not None: | ||
106 | 1407 | relevant_builds.append(build) | ||
107 | 1408 | |||
108 | 1409 | skip_archtags = set( | ||
109 | 1410 | bpb.distro_arch_series.architecturetag for bpb in relevant_builds) | ||
110 | 1411 | # We need to assign the arch-indep role to a build unless an | ||
111 | 1412 | # arch-indep build has already succeeded, or another build in | ||
112 | 1413 | # this series already has it set. | ||
113 | 1414 | need_arch_indep = not any(bpb.arch_indep for bpb in relevant_builds) | ||
114 | 1415 | |||
115 | 1416 | # Find the architectures for which the source should end up with | ||
116 | 1417 | # binaries, parsing architecturehintlist as you'd expect. | ||
117 | 1418 | # For an architecturehintlist of just 'all', this will | ||
118 | 1419 | # be the current nominatedarchindep if need_arch_indep, | ||
119 | 1420 | # otherwise nothing. | ||
120 | 1422 | if architectures_available is None: | 1421 | if architectures_available is None: |
121 | 1423 | architectures_available = list( | 1422 | architectures_available = list( |
122 | 1424 | distroseries.buildable_architectures) | 1423 | distroseries.buildable_architectures) |
123 | 1425 | |||
124 | 1426 | architectures_available = self._getAllowedArchitectures( | 1424 | architectures_available = self._getAllowedArchitectures( |
125 | 1427 | archive, architectures_available) | 1425 | archive, architectures_available) |
128 | 1428 | 1426 | candidate_architectures = determine_architectures_to_build( | |
127 | 1429 | build_architectures = determine_architectures_to_build( | ||
129 | 1430 | sourcepackagerelease.architecturehintlist, archive, distroseries, | 1427 | sourcepackagerelease.architecturehintlist, archive, distroseries, |
140 | 1431 | architectures_available) | 1428 | architectures_available, need_arch_indep) |
141 | 1432 | 1429 | ||
142 | 1433 | builds = [] | 1430 | # Filter out any architectures for which we earlier found sufficient |
143 | 1434 | for arch in build_architectures: | 1431 | # builds. |
144 | 1435 | build_candidate = self._createMissingBuildForArchitecture( | 1432 | needed_architectures = [ |
145 | 1436 | sourcepackagerelease, archive, arch, pocket, logger=logger) | 1433 | das for das in candidate_architectures |
146 | 1437 | if build_candidate is not None: | 1434 | if das.architecturetag not in skip_archtags] |
147 | 1438 | builds.append(build_candidate) | 1435 | if not needed_architectures: |
148 | 1439 | 1436 | return [] | |
149 | 1440 | return builds | 1437 | |
150 | 1438 | arch_indep_das = None | ||
151 | 1439 | if need_arch_indep: | ||
152 | 1440 | # The ideal arch_indep build is nominatedarchindep. But if | ||
153 | 1441 | # that isn't a build we would otherwise create, use the DAS | ||
154 | 1442 | # with the lowest Processor.id. | ||
155 | 1443 | # XXX wgrant 2014-11-06: The fact that production's | ||
156 | 1444 | # Processor 1 is i386, a good arch-indep candidate, is a | ||
157 | 1445 | # total coincidence and this isn't a hack. I promise. | ||
158 | 1446 | if distroseries.nominatedarchindep in needed_architectures: | ||
159 | 1447 | arch_indep_das = distroseries.nominatedarchindep | ||
160 | 1448 | else: | ||
161 | 1449 | arch_indep_das = sorted( | ||
162 | 1450 | needed_architectures, key=attrgetter('processor.id'))[0] | ||
163 | 1451 | |||
164 | 1452 | # Create builds for the remaining architectures. | ||
165 | 1453 | new_builds = [] | ||
166 | 1454 | for das in needed_architectures: | ||
167 | 1455 | build = self.new( | ||
168 | 1456 | source_package_release=sourcepackagerelease, | ||
169 | 1457 | distro_arch_series=das, archive=archive, pocket=pocket, | ||
170 | 1458 | arch_indep=das == arch_indep_das) | ||
171 | 1459 | new_builds.append(build) | ||
172 | 1460 | # Create the builds in suspended mode for disabled archives. | ||
173 | 1461 | build_queue = build.queueBuild(suspended=not archive.enabled) | ||
174 | 1462 | Store.of(build).flush() | ||
175 | 1463 | if logger is not None: | ||
176 | 1464 | logger.debug( | ||
177 | 1465 | "Created %s [%d] in %s (%d)" | ||
178 | 1466 | % (build.title, build.id, build.archive.displayname, | ||
179 | 1467 | build_queue.lastscore)) | ||
180 | 1468 | return new_builds | ||
181 | 1441 | 1469 | ||
182 | === modified file 'lib/lp/soyuz/tests/test_build_set.py' | |||
183 | --- lib/lp/soyuz/tests/test_build_set.py 2014-11-06 03:33:36 +0000 | |||
184 | +++ lib/lp/soyuz/tests/test_build_set.py 2014-11-06 05:30:00 +0000 | |||
185 | @@ -256,6 +256,16 @@ | |||
186 | 256 | self.distroseries.nominatedarchindep = self.distroseries['sparc'] | 256 | self.distroseries.nominatedarchindep = self.distroseries['sparc'] |
187 | 257 | self.addFakeChroots(self.distroseries) | 257 | self.addFakeChroots(self.distroseries) |
188 | 258 | 258 | ||
189 | 259 | self.distroseries2 = self.factory.makeDistroSeries( | ||
190 | 260 | distribution=self.distro, name="dumb") | ||
191 | 261 | for name, arch in (('avr', self.avr), ('sparc', self.sparc), | ||
192 | 262 | ('x32', self.x32)): | ||
193 | 263 | self.factory.makeDistroArchSeries( | ||
194 | 264 | architecturetag=name, processor=arch, | ||
195 | 265 | distroseries=self.distroseries2, supports_virtualized=True) | ||
196 | 266 | self.distroseries2.nominatedarchindep = self.distroseries2['x32'] | ||
197 | 267 | self.addFakeChroots(self.distroseries2) | ||
198 | 268 | |||
199 | 259 | def getPubSource(self, architecturehintlist): | 269 | def getPubSource(self, architecturehintlist): |
200 | 260 | """Return a mock source package publishing record for the archive | 270 | """Return a mock source package publishing record for the archive |
201 | 261 | and architecture used in this testcase. | 271 | and architecture used in this testcase. |
202 | @@ -281,6 +291,17 @@ | |||
203 | 281 | self.assertContentEqual(expected.items(), actual.items()) | 291 | self.assertContentEqual(expected.items(), actual.items()) |
204 | 282 | self.assertEqual(len(actual), len(builds)) | 292 | self.assertEqual(len(actual), len(builds)) |
205 | 283 | 293 | ||
206 | 294 | def completeBuilds(self, builds, success_map): | ||
207 | 295 | for build in builds: | ||
208 | 296 | success_or_failure = success_map.get( | ||
209 | 297 | build.distro_arch_series.architecturetag, None) | ||
210 | 298 | if success_or_failure is not None: | ||
211 | 299 | build.updateStatus( | ||
212 | 300 | BuildStatus.FULLYBUILT if success_or_failure | ||
213 | 301 | else BuildStatus.FAILEDTOBUILD) | ||
214 | 302 | del success_map[build.distro_arch_series.architecturetag] | ||
215 | 303 | self.assertContentEqual([], success_map) | ||
216 | 304 | |||
217 | 284 | def test__getAllowedArchitectures_restricted(self): | 305 | def test__getAllowedArchitectures_restricted(self): |
218 | 285 | """Test _getAllowedArchitectures doesn't return unrestricted | 306 | """Test _getAllowedArchitectures doesn't return unrestricted |
219 | 286 | archs. | 307 | archs. |
220 | @@ -353,6 +374,88 @@ | |||
221 | 353 | builds = self.createBuilds(spr, self.distroseries) | 374 | builds = self.createBuilds(spr, self.distroseries) |
222 | 354 | self.assertBuildsMatch({'sparc': True, 'avr': False}, builds) | 375 | self.assertBuildsMatch({'sparc': True, 'avr': False}, builds) |
223 | 355 | 376 | ||
224 | 377 | def test_createForSource_arch_indep_from_scratch(self): | ||
225 | 378 | """createForSource() sets arch_indep=True on builds for the | ||
226 | 379 | nominatedarchindep architecture when no builds already exist. | ||
227 | 380 | """ | ||
228 | 381 | spr = self.factory.makeSourcePackageRelease(architecturehintlist='any') | ||
229 | 382 | builds = self.createBuilds(spr, self.distroseries) | ||
230 | 383 | self.assertBuildsMatch({'sparc': True, 'avr': False}, builds) | ||
231 | 384 | |||
232 | 385 | def test_createForSource_any_with_nai_change(self): | ||
233 | 386 | # A new non-arch-indep build is created for a new | ||
234 | 387 | # nominatedarchindep architecture if arch-indep has already | ||
235 | 388 | # built elsewhere. | ||
236 | 389 | # | ||
237 | 390 | # This is most important when copying with binaries between | ||
238 | 391 | # series with different nominatedarchdep (bug #1350208). | ||
239 | 392 | spr = self.factory.makeSourcePackageRelease(architecturehintlist='any') | ||
240 | 393 | builds = self.createBuilds(spr, self.distroseries) | ||
241 | 394 | self.assertBuildsMatch({'sparc': True, 'avr': False}, builds) | ||
242 | 395 | self.completeBuilds(builds, {'sparc': True, 'avr': True}) | ||
243 | 396 | # The new nominatedarchindep needs to be built, but we already | ||
244 | 397 | # have arch-indep binaries so arch_indep is False. | ||
245 | 398 | new_builds = self.createBuilds(spr, self.distroseries2) | ||
246 | 399 | self.assertBuildsMatch({'x32': False}, new_builds) | ||
247 | 400 | |||
248 | 401 | def test_createForSource_any_with_nai_change_and_fail(self): | ||
249 | 402 | # When the previous arch-indep build has failed, and | ||
250 | 403 | # nominatedarchindep has changed in the new series, the new | ||
251 | 404 | # nominatedarchindep has arch_indep=True while the other arch | ||
252 | 405 | # has arch_indep=False. | ||
253 | 406 | spr = self.factory.makeSourcePackageRelease(architecturehintlist='any') | ||
254 | 407 | builds = self.createBuilds(spr, self.distroseries) | ||
255 | 408 | self.assertBuildsMatch({'sparc': True, 'avr': False}, builds) | ||
256 | 409 | self.completeBuilds(builds, {'sparc': False, 'avr': True}) | ||
257 | 410 | # The new nominatedarchindep needs to be built, and the previous | ||
258 | 411 | # nominatedarchindep build failed. We end up with two new | ||
259 | 412 | # builds, and arch_indep on nominatedarchindep. | ||
260 | 413 | new_builds = self.createBuilds(spr, self.distroseries2) | ||
261 | 414 | self.assertBuildsMatch({'x32': True, 'sparc': False}, new_builds) | ||
262 | 415 | |||
263 | 416 | def test_createForSource_all_with_nai_change(self): | ||
264 | 417 | # If we only need arch-indep binaries and they've already built | ||
265 | 418 | # successfully, no build is created for the new series, even if | ||
266 | 419 | # nominatedarchindep has changed. | ||
267 | 420 | spr = self.factory.makeSourcePackageRelease(architecturehintlist='all') | ||
268 | 421 | builds = self.createBuilds(spr, self.distroseries) | ||
269 | 422 | self.assertBuildsMatch({'sparc': True}, builds) | ||
270 | 423 | self.completeBuilds(builds, {'sparc': True}) | ||
271 | 424 | # Despite there being no build for the new nominatedarchindep, | ||
272 | 425 | # the old arch-indep build is sufficient and no new record is | ||
273 | 426 | # created. | ||
274 | 427 | new_builds = self.createBuilds(spr, self.distroseries2) | ||
275 | 428 | self.assertBuildsMatch({}, new_builds) | ||
276 | 429 | |||
277 | 430 | def test_createForSource_all_with_nai_change_and_fail(self): | ||
278 | 431 | # If the previous arch-indep sole build failed, a new arch-indep | ||
279 | 432 | # build is created for nominatedarchindep. | ||
280 | 433 | spr = self.factory.makeSourcePackageRelease(architecturehintlist='all') | ||
281 | 434 | builds = self.createBuilds(spr, self.distroseries) | ||
282 | 435 | self.assertBuildsMatch({'sparc': True}, builds) | ||
283 | 436 | self.completeBuilds(builds, {'sparc': False}) | ||
284 | 437 | # Despite there being no build for the new nominatedarchindep, | ||
285 | 438 | # the old arch-indep build is sufficient and no new record is | ||
286 | 439 | # created. | ||
287 | 440 | new_builds = self.createBuilds(spr, self.distroseries2) | ||
288 | 441 | self.assertBuildsMatch({'x32': True}, new_builds) | ||
289 | 442 | |||
290 | 443 | def test_createForSource_all_and_other_archs(self): | ||
291 | 444 | # If a source package specifies both 'all' and a set of | ||
292 | 445 | # architectures that doesn't include nominatedarchindep, | ||
293 | 446 | # arch_indep is set on the available DistroArchSeries with the | ||
294 | 447 | # oldest Processor. | ||
295 | 448 | # This is mostly a hack to avoid hardcoding a preference for | ||
296 | 449 | # the faster x86-family architectures, so we don't accidentally | ||
297 | 450 | # build documentation on hppa. | ||
298 | 451 | spr = self.factory.makeSourcePackageRelease( | ||
299 | 452 | architecturehintlist='all sparc avr') | ||
300 | 453 | builds = self.createBuilds(spr, self.distroseries2) | ||
301 | 454 | self.assertBuildsMatch({'sparc': False, 'avr': True}, builds) | ||
302 | 455 | self.completeBuilds(builds, {'sparc': True, 'avr': True}) | ||
303 | 456 | new_builds = self.createBuilds(spr, self.distroseries) | ||
304 | 457 | self.assertBuildsMatch({}, new_builds) | ||
305 | 458 | |||
306 | 356 | 459 | ||
307 | 357 | class TestFindBuiltOrPublishedBySourceAndArchive(TestCaseWithFactory): | 460 | class TestFindBuiltOrPublishedBySourceAndArchive(TestCaseWithFactory): |
308 | 358 | """Tests for findBuiltOrPublishedBySourceAndArchive().""" | 461 | """Tests for findBuiltOrPublishedBySourceAndArchive().""" |