Merge lp:~wgrant/launchpad/bug-1350208 into lp:launchpad

[William Grant]

Fix createForSource to correctly handle unusual arch-indep cases (eg. changes in nominatedarchindep, or "Architecture: powerpc all").

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/soyuz/adapters/buildarch.py'
--- lib/lp/soyuz/adapters/buildarch.py	2013-05-31 04:27:26 +0000
+++ lib/lp/soyuz/adapters/buildarch.py	2014-11-06 05:30:00 +0000
@@ -37,7 +37,7 @@
 
 
 def determine_architectures_to_build(hintlist, archive, distroseries,
-                                     legal_archseries):
+                                     legal_archseries, need_arch_indep):
     """Return a list of architectures for which this publication should build.
 
     This function answers the question: given a list of architectures and
@@ -78,7 +78,7 @@
 
     # 'all' is only used as a last resort, to create an arch-indep build
     # where no builds would otherwise exist.
-    if len(build_tags) == 0 and 'all' in hint_archs:
+    if need_arch_indep and len(build_tags) == 0 and 'all' in hint_archs:
         nominated_arch = distroseries.nominatedarchindep
         if nominated_arch in legal_archseries:
             build_tags = set([nominated_arch.architecturetag])

=== modified file 'lib/lp/soyuz/adapters/tests/test_buildarch.py'
--- lib/lp/soyuz/adapters/tests/test_buildarch.py	2013-09-11 06:05:44 +0000
+++ lib/lp/soyuz/adapters/tests/test_buildarch.py	2014-11-06 05:30:00 +0000
@@ -34,7 +34,7 @@
                 if arch.architecturetag in allowed_arch_tags]
         architectures = determine_architectures_to_build(
             pub.sourcepackagerelease.architecturehintlist, pub.archive,
-            self.publisher.breezy_autotest, allowed_archs)
+            self.publisher.breezy_autotest, allowed_archs, True)
         self.assertContentEqual(
             expected_arch_tags, [a.architecturetag for a in architectures])
 

=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2014-11-06 00:33:31 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2014-11-06 05:30:00 +0000
@@ -13,7 +13,10 @@
     ]
 
 import datetime
-from operator import itemgetter
+from operator import (
+    attrgetter,
+    itemgetter,
+    )
 
 import apt_pkg
 import pytz
@@ -1383,58 +1386,83 @@
             if not distroarch.processor.restricted or
                distroarch.processor in archive.enabled_restricted_processors]
 
-    def _createMissingBuildForArchitecture(self, sourcepackagerelease,
-                                           archive, arch, pocket,
-                                           logger=None):
-        """Create a build for a given architecture if it doesn't exist yet.
-
-        Return the just-created `IBinaryPackageBuild` record already
-        scored or None if a suitable build is already present.
-        """
-        exact_build = self.getBySourceAndLocation(
-            sourcepackagerelease, archive, arch)
-        if exact_build is not None:
-            return None
-
-        build_candidate = self.findBuiltOrPublishedBySourceAndArchive(
-            sourcepackagerelease, archive).get(arch.architecturetag)
-        if build_candidate is not None:
-            return None
-
-        build = self.new(
-            source_package_release=sourcepackagerelease,
-            distro_arch_series=arch, archive=archive, pocket=pocket)
-        # Create the builds in suspended mode for disabled archives.
-        build_queue = build.queueBuild(suspended=not archive.enabled)
-        Store.of(build).flush()
-
-        if logger is not None:
-            logger.debug(
-                "Created %s [%d] in %s (%d)"
-                % (build.title, build.id, build.archive.displayname,
-                   build_queue.lastscore))
-
-        return build
-
     def createForSource(self, sourcepackagerelease, archive, distroseries,
                         pocket, architectures_available=None, logger=None):
         """See `ISourcePackagePublishingHistory`."""
+
+        # Exclude any architectures which already have built or copied
+        # binaries. A new build with the same archtag could never
+        # succeed; its files would conflict during upload.
+        relevant_builds = self.findBuiltOrPublishedBySourceAndArchive(
+            sourcepackagerelease, archive).values()
+
+        # Find any architectures that already have a build that exactly
+        # matches, regardless of status. We can't create a second build
+        # with the same (SPR, Archive, DAS).
+        # XXX wgrant 2014-11-06: Should use a bulk query.
+        for das in distroseries.architectures:
+            build = self.getBySourceAndLocation(
+                sourcepackagerelease, archive, das)
+            if build is not None:
+                relevant_builds.append(build)
+
+        skip_archtags = set(
+            bpb.distro_arch_series.architecturetag for bpb in relevant_builds)
+        # We need to assign the arch-indep role to a build unless an
+        # arch-indep build has already succeeded, or another build in
+        # this series already has it set.
+        need_arch_indep = not any(bpb.arch_indep for bpb in relevant_builds)
+
+        # Find the architectures for which the source should end up with
+        # binaries, parsing architecturehintlist as you'd expect.
+        # For an architecturehintlist of just 'all', this will
+        # be the current nominatedarchindep if need_arch_indep,
+        # otherwise nothing.
         if architectures_available is None:
             architectures_available = list(
                 distroseries.buildable_architectures)
-
         architectures_available = self._getAllowedArchitectures(
             archive, architectures_available)
-
-        build_architectures = determine_architectures_to_build(
+        candidate_architectures = determine_architectures_to_build(
             sourcepackagerelease.architecturehintlist, archive, distroseries,
-            architectures_available)
-
-        builds = []
-        for arch in build_architectures:
-            build_candidate = self._createMissingBuildForArchitecture(
-                sourcepackagerelease, archive, arch, pocket, logger=logger)
-            if build_candidate is not None:
-                builds.append(build_candidate)
-
-        return builds
+            architectures_available, need_arch_indep)
+
+        # Filter out any architectures for which we earlier found sufficient
+        # builds.
+        needed_architectures = [
+            das for das in candidate_architectures
+            if das.architecturetag not in skip_archtags]
+        if not needed_architectures:
+            return []
+
+        arch_indep_das = None
+        if need_arch_indep:
+            # The ideal arch_indep build is nominatedarchindep. But if
+            # that isn't a build we would otherwise create, use the DAS
+            # with the lowest Processor.id.
+            # XXX wgrant 2014-11-06: The fact that production's
+            # Processor 1 is i386, a good arch-indep candidate, is a
+            # total coincidence and this isn't a hack. I promise.
+            if distroseries.nominatedarchindep in needed_architectures:
+                arch_indep_das = distroseries.nominatedarchindep
+            else:
+                arch_indep_das = sorted(
+                    needed_architectures, key=attrgetter('processor.id'))[0]
+
+        # Create builds for the remaining architectures.
+        new_builds = []
+        for das in needed_architectures:
+            build = self.new(
+                source_package_release=sourcepackagerelease,
+                distro_arch_series=das, archive=archive, pocket=pocket,
+                arch_indep=das == arch_indep_das)
+            new_builds.append(build)
+            # Create the builds in suspended mode for disabled archives.
+            build_queue = build.queueBuild(suspended=not archive.enabled)
+            Store.of(build).flush()
+            if logger is not None:
+                logger.debug(
+                    "Created %s [%d] in %s (%d)"
+                    % (build.title, build.id, build.archive.displayname,
+                    build_queue.lastscore))
+        return new_builds

=== modified file 'lib/lp/soyuz/tests/test_build_set.py'
--- lib/lp/soyuz/tests/test_build_set.py	2014-11-06 03:33:36 +0000
+++ lib/lp/soyuz/tests/test_build_set.py	2014-11-06 05:30:00 +0000
@@ -256,6 +256,16 @@
         self.distroseries.nominatedarchindep = self.distroseries['sparc']
         self.addFakeChroots(self.distroseries)
 
+        self.distroseries2 = self.factory.makeDistroSeries(
+            distribution=self.distro, name="dumb")
+        for name, arch in (('avr', self.avr), ('sparc', self.sparc),
+                           ('x32', self.x32)):
+            self.factory.makeDistroArchSeries(
+                architecturetag=name, processor=arch,
+                distroseries=self.distroseries2, supports_virtualized=True)
+        self.distroseries2.nominatedarchindep = self.distroseries2['x32']
+        self.addFakeChroots(self.distroseries2)
+
     def getPubSource(self, architecturehintlist):
         """Return a mock source package publishing record for the archive
         and architecture used in this testcase.
@@ -281,6 +291,17 @@
         self.assertContentEqual(expected.items(), actual.items())
         self.assertEqual(len(actual), len(builds))
 
+    def completeBuilds(self, builds, success_map):
+        for build in builds:
+            success_or_failure = success_map.get(
+                build.distro_arch_series.architecturetag, None)
+            if success_or_failure is not None:
+                build.updateStatus(
+                    BuildStatus.FULLYBUILT if success_or_failure
+                    else BuildStatus.FAILEDTOBUILD)
+                del success_map[build.distro_arch_series.architecturetag]
+        self.assertContentEqual([], success_map)
+
     def test__getAllowedArchitectures_restricted(self):
         """Test _getAllowedArchitectures doesn't return unrestricted
         archs.
@@ -353,6 +374,88 @@
         builds = self.createBuilds(spr, self.distroseries)
         self.assertBuildsMatch({'sparc': True, 'avr': False}, builds)
 
+    def test_createForSource_arch_indep_from_scratch(self):
+        """createForSource() sets arch_indep=True on builds for the
+        nominatedarchindep architecture when no builds already exist.
+        """
+        spr = self.factory.makeSourcePackageRelease(architecturehintlist='any')
+        builds = self.createBuilds(spr, self.distroseries)
+        self.assertBuildsMatch({'sparc': True, 'avr': False}, builds)
+
+    def test_createForSource_any_with_nai_change(self):
+        # A new non-arch-indep build is created for a new
+        # nominatedarchindep architecture if arch-indep has already
+        # built elsewhere.
+        #
+        # This is most important when copying with binaries between
+        # series with different nominatedarchdep (bug #1350208).
+        spr = self.factory.makeSourcePackageRelease(architecturehintlist='any')
+        builds = self.createBuilds(spr, self.distroseries)
+        self.assertBuildsMatch({'sparc': True, 'avr': False}, builds)
+        self.completeBuilds(builds, {'sparc': True, 'avr': True})
+        # The new nominatedarchindep needs to be built, but we already
+        # have arch-indep binaries so arch_indep is False.
+        new_builds = self.createBuilds(spr, self.distroseries2)
+        self.assertBuildsMatch({'x32': False}, new_builds)
+
+    def test_createForSource_any_with_nai_change_and_fail(self):
+        # When the previous arch-indep build has failed, and
+        # nominatedarchindep has changed in the new series, the new
+        # nominatedarchindep has arch_indep=True while the other arch
+        # has arch_indep=False.
+        spr = self.factory.makeSourcePackageRelease(architecturehintlist='any')
+        builds = self.createBuilds(spr, self.distroseries)
+        self.assertBuildsMatch({'sparc': True, 'avr': False}, builds)
+        self.completeBuilds(builds, {'sparc': False, 'avr': True})
+        # The new nominatedarchindep needs to be built, and the previous
+        # nominatedarchindep build failed. We end up with two new
+        # builds, and arch_indep on nominatedarchindep.
+        new_builds = self.createBuilds(spr, self.distroseries2)
+        self.assertBuildsMatch({'x32': True, 'sparc': False}, new_builds)
+
+    def test_createForSource_all_with_nai_change(self):
+        # If we only need arch-indep binaries and they've already built
+        # successfully, no build is created for the new series, even if
+        # nominatedarchindep has changed.
+        spr = self.factory.makeSourcePackageRelease(architecturehintlist='all')
+        builds = self.createBuilds(spr, self.distroseries)
+        self.assertBuildsMatch({'sparc': True}, builds)
+        self.completeBuilds(builds, {'sparc': True})
+        # Despite there being no build for the new nominatedarchindep,
+        # the old arch-indep build is sufficient and no new record is
+        # created.
+        new_builds = self.createBuilds(spr, self.distroseries2)
+        self.assertBuildsMatch({}, new_builds)
+
+    def test_createForSource_all_with_nai_change_and_fail(self):
+        # If the previous arch-indep sole build failed, a new arch-indep
+        # build is created for nominatedarchindep.
+        spr = self.factory.makeSourcePackageRelease(architecturehintlist='all')
+        builds = self.createBuilds(spr, self.distroseries)
+        self.assertBuildsMatch({'sparc': True}, builds)
+        self.completeBuilds(builds, {'sparc': False})
+        # Despite there being no build for the new nominatedarchindep,
+        # the old arch-indep build is sufficient and no new record is
+        # created.
+        new_builds = self.createBuilds(spr, self.distroseries2)
+        self.assertBuildsMatch({'x32': True}, new_builds)
+
+    def test_createForSource_all_and_other_archs(self):
+        # If a source package specifies both 'all' and a set of
+        # architectures that doesn't include nominatedarchindep,
+        # arch_indep is set on the available DistroArchSeries with the
+        # oldest Processor.
+        # This is mostly a hack to avoid hardcoding a preference for
+        # the faster x86-family architectures, so we don't accidentally
+        # build documentation on hppa.
+        spr = self.factory.makeSourcePackageRelease(
+            architecturehintlist='all sparc avr')
+        builds = self.createBuilds(spr, self.distroseries2)
+        self.assertBuildsMatch({'sparc': False, 'avr': True}, builds)
+        self.completeBuilds(builds, {'sparc': True, 'avr': True})
+        new_builds = self.createBuilds(spr, self.distroseries)
+        self.assertBuildsMatch({}, new_builds)
+
 
 class TestFindBuiltOrPublishedBySourceAndArchive(TestCaseWithFactory):
     """Tests for findBuiltOrPublishedBySourceAndArchive()."""