launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/xbia-refactor into lp:launchpad.

Commit message:
Refactor build creation so buildarch and its tests can avoid the DB.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/xbia-refactor/+merge/249295

Refactor build creation so buildarch and its tests can avoid the DB. Makes things a bit less painfully slow and paves the way for the implementation of Build-Indep-Architecture.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/xbia-refactor into lp:launchpad.

=== modified file 'lib/lp/soyuz/adapters/buildarch.py'
--- lib/lp/soyuz/adapters/buildarch.py	2014-11-06 05:01:05 +0000
+++ lib/lp/soyuz/adapters/buildarch.py	2015-02-11 10:10:26 +0000
@@ -8,7 +8,6 @@
     ]
 
 
-from operator import attrgetter
 import os
 import subprocess
 
@@ -29,63 +28,37 @@
         return self._matches[(arch, wildcard)]
 
     def findAllMatches(self, arches, wildcards):
-        return [arch for arch in arches for wildcard in wildcards
-                if self.match(arch, wildcard)]
+        return list(sorted(set(
+            arch for arch in arches for wildcard in wildcards
+            if self.match(arch, wildcard))))
 
 
 dpkg_architecture = DpkgArchitectureCache()
 
 
-def determine_architectures_to_build(hintlist, archive, distroseries,
-                                     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
-    an archive, what architectures should we build it for? It takes a set of
-    legal distroarchseries and the distribution series for which we are
-    building.
-
-    For PPA publications we only consider architectures supported by PPA
-    subsystem (`DistroArchSeries`.supports_virtualized flag).
-
-    :param: hintlist: A string of the architectures this source package
+def determine_architectures_to_build(hintlist, valid_archs,
+                                     nominated_arch_indep, need_arch_indep):
+    """Return a set of architectures to build.
+
+    :param hintlist: A string of the architectures this source package
         specifies it builds for.
-    :param: archive: The `IArchive` we are building into.
-    :param: distroseries: the context `DistroSeries`.
-    :param: legal_archseries: a list of all initialized `DistroArchSeries`
-        to be considered.
-    :return: a list of `DistroArchSeries` for which the source publication in
+    :param valid_archs: a list of all architecture tags that we can
+        create builds for.
+    :param nominated_arch_indep: a preferred architecture tag for
+        architecture-independent builds. May be None.
+    :return: a set of architecture tags for which the source publication in
         question should be built.
     """
-    # The 'PPA supported' flag only applies to virtualized archives
-    if archive.require_virtualized:
-        legal_archseries = [
-            arch for arch in legal_archseries if arch.supports_virtualized]
-        # Cope with no virtualization support at all. It usually happens when
-        # a distroseries is created and initialized, by default no
-        # architecture supports its. Distro-team might take some time to
-        # decide which architecture will be allowed for PPAs and queue-builder
-        # will continue to work meanwhile.
-        if not legal_archseries:
-            return []
-
-    legal_arch_tags = set(
-        arch.architecturetag for arch in legal_archseries if arch.enabled)
-
     hint_archs = set(hintlist.split())
-    build_tags = set(dpkg_architecture.findAllMatches(
-        legal_arch_tags, hint_archs))
+    build_archs = set(
+        dpkg_architecture.findAllMatches(valid_archs, hint_archs))
 
     # 'all' is only used as a last resort, to create an arch-indep build
     # where no builds would otherwise exist.
-    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])
+    if need_arch_indep and len(build_archs) == 0 and 'all' in hint_archs:
+        if nominated_arch_indep in valid_archs:
+            return set([nominated_arch_indep])
         else:
-            build_tags = set()
+            return set()
 
-    sorted_archseries = sorted(
-        legal_archseries, key=attrgetter('architecturetag'))
-    return [arch for arch in sorted_archseries
-            if arch.architecturetag in build_tags]
+    return build_archs

=== modified file 'lib/lp/soyuz/adapters/tests/test_buildarch.py'
--- lib/lp/soyuz/adapters/tests/test_buildarch.py	2014-11-06 05:01:05 +0000
+++ lib/lp/soyuz/adapters/tests/test_buildarch.py	2015-02-11 10:10:26 +0000
@@ -3,48 +3,52 @@
 
 __metaclass__ = type
 
-from lp.soyuz.adapters.buildarch import determine_architectures_to_build
-from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
-from lp.testing import TestCaseWithFactory
-from lp.testing.layers import LaunchpadZopelessLayer
-
-
-class TestDetermineArchitecturesToBuild(TestCaseWithFactory):
+from lp.soyuz.adapters.buildarch import (
+    determine_architectures_to_build,
+    DpkgArchitectureCache,
+    )
+from lp.testing import TestCase
+
+
+class TestDpkgArchitectureCache(TestCase):
+
+    def test_multiple(self):
+        self.assertContentEqual(
+            ['amd64', 'armhf'],
+            DpkgArchitectureCache().findAllMatches(
+                ['amd64', 'i386', 'armhf'], ['amd64', 'armhf']))
+
+    def test_any(self):
+        self.assertContentEqual(
+            ['amd64', 'i386', 'kfreebsd-amd64'],
+            DpkgArchitectureCache().findAllMatches(
+                ['amd64', 'i386', 'kfreebsd-amd64'], ['any']))
+
+    def test_all(self):
+        self.assertContentEqual(
+            [],
+            DpkgArchitectureCache().findAllMatches(
+                ['amd64', 'i386', 'kfreebsd-amd64'], ['all']))
+
+    def test_partial_wildcards(self):
+        self.assertContentEqual(
+            ['amd64', 'i386', 'kfreebsd-amd64'],
+            DpkgArchitectureCache().findAllMatches(
+                ['amd64', 'i386', 'kfreebsd-amd64', 'kfreebsd-i386'],
+                ['linux-any', 'any-amd64']))
+
+
+class TestDetermineArchitecturesToBuild(TestCase):
     """Test that determine_architectures_to_build correctly interprets hints.
     """
 
-    layer = LaunchpadZopelessLayer
-
-    def setUp(self):
-        super(TestDetermineArchitecturesToBuild, self).setUp()
-        self.publisher = SoyuzTestPublisher()
-        self.publisher.prepareBreezyAutotest()
-        armel = self.factory.makeProcessor('armel', 'armel', 'armel')
-        self.publisher.breezy_autotest.newArch(
-            'armel', armel, False, self.publisher.person)
-        self.publisher.addFakeChroots()
-
-    def assertArchitecturesToBuild(self, expected_arch_tags, pub,
-                                   allowed_arch_tags=None):
+    def assertArchsForHint(self, hint_string, expected_arch_tags,
+                           allowed_arch_tags=None):
         if allowed_arch_tags is None:
-            allowed_archs = self.publisher.breezy_autotest.architectures
-        else:
-            allowed_archs = [
-                arch for arch in self.publisher.breezy_autotest.architectures
-                if arch.architecturetag in allowed_arch_tags]
-        architectures = determine_architectures_to_build(
-            pub.sourcepackagerelease.architecturehintlist, pub.archive,
-            self.publisher.breezy_autotest, allowed_archs, True)
-        self.assertContentEqual(
-            expected_arch_tags, [a.architecturetag for a in architectures])
-
-    def assertArchsForHint(self, hint_string, expected_arch_tags,
-                           allowed_arch_tags=None, sourcename=None):
-        """Assert that the given hint resolves to the expected archtags."""
-        pub = self.publisher.getPubSource(
-            sourcename=sourcename, architecturehintlist=hint_string)
-        self.assertArchitecturesToBuild(
-            expected_arch_tags, pub, allowed_arch_tags=allowed_arch_tags)
+            allowed_arch_tags = ['armel', 'hppa', 'i386']
+        arch_tags = determine_architectures_to_build(
+            hint_string, allowed_arch_tags, 'i386', True)
+        self.assertContentEqual(expected_arch_tags, arch_tags)
 
     def test_single_architecture(self):
         # A hint string with a single arch resolves to just that arch.
@@ -112,17 +116,6 @@
         # it anyway.
         self.assertArchsForHint('any-any', ['armel', 'hppa', 'i386'])
 
-    def test_disabled_architectures_omitted(self):
-        # Disabled architectures are not buildable, so are excluded.
-        self.publisher.breezy_autotest['hppa'].enabled = False
-        self.assertArchsForHint('any', ['armel', 'i386'])
-
-    def test_virtualized_archives_have_only_virtualized_archs(self):
-        # For archives which must build on virtual builders, only
-        # virtual archs are returned.
-        self.publisher.breezy_autotest.main_archive.require_virtualized = True
-        self.assertArchsForHint('any', ['i386'])
-
     def test_no_all_builds_when_nominatedarchindep_not_permitted(self):
         # Some archives (eg. armel rebuilds) don't want arch-indep
         # builds. If the nominatedarchindep architecture (normally

=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2014-11-07 12:05:37 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2015-02-11 10:10:26 +0000
@@ -1385,9 +1385,16 @@
         """
         # Return all distroarches with unrestricted processors or with
         # processors the archive is explicitly associated with.
-        return [distroarch for distroarch in available_archs
-            if not distroarch.processor.restricted or
-               distroarch.processor in archive.enabled_restricted_processors]
+        return [
+            das for das in available_archs
+            if (
+                das.enabled
+                and (
+                    not das.processor.restricted
+                    or das.processor in archive.enabled_restricted_processors)
+                and (
+                    das.supports_virtualized
+                    or not archive.require_virtualized))]
 
     def createForSource(self, sourcepackagerelease, archive, distroseries,
                         pocket, architectures_available=None, logger=None):
@@ -1421,14 +1428,21 @@
         # 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)
-        candidate_architectures = determine_architectures_to_build(
-            sourcepackagerelease.architecturehintlist, archive, distroseries,
-            architectures_available, need_arch_indep)
+        valid_archs = list(
+            architectures_available or distroseries.buildable_architectures)
+        valid_archs = self._getAllowedArchitectures(archive, valid_archs)
+        nominated_arch_indep_tag = (
+            distroseries.nominatedarchindep.architecturetag
+            if distroseries.nominatedarchindep else None)
+
+        # Filter the valid archs against the hint list.
+        candidate_arch_tags = determine_architectures_to_build(
+            sourcepackagerelease.architecturehintlist,
+            [das.architecturetag for das in valid_archs],
+            nominated_arch_indep_tag, need_arch_indep)
+        candidate_architectures = set(
+            das for das in valid_archs
+            if das.architecturetag in candidate_arch_tags)
 
         # Filter out any architectures for which we earlier found sufficient
         # builds.

=== modified file 'lib/lp/soyuz/tests/test_build_set.py'
--- lib/lp/soyuz/tests/test_build_set.py	2014-11-06 05:20:01 +0000
+++ lib/lp/soyuz/tests/test_build_set.py	2015-02-11 10:10:26 +0000
@@ -236,6 +236,73 @@
                 self.builds[1].distro_arch_series))
 
 
+class TestGetAllowedArchitectures(TestCaseWithFactory):
+    """Tests for _getAllowedArchitectures."""
+
+    layer = ZopelessDatabaseLayer
+
+    def setUp(self):
+        super(TestGetAllowedArchitectures, self).setUp()
+        self.avr = self.factory.makeProcessor(name="avr2001")
+        self.sparc = self.factory.makeProcessor(name="sparc64")
+        self.distroseries = self.factory.makeDistroSeries()
+        for name, arch in (('avr', self.avr), ('sparc', self.sparc)):
+            self.factory.makeDistroArchSeries(
+                architecturetag=name, processor=arch,
+                distroseries=self.distroseries, supports_virtualized=True)
+        self.archive = self.factory.makeArchive(
+            distribution=self.distroseries.distribution)
+
+    def test_normal(self):
+        self.assertContentEqual(
+            [self.distroseries['sparc'], self.distroseries['avr']],
+            BinaryPackageBuildSet()._getAllowedArchitectures(
+                self.archive, self.distroseries.architectures))
+
+    def test_restricted(self):
+        # Restricted architectures aren't returned by default.
+        self.avr.restricted = True
+        self.assertContentEqual(
+            [self.distroseries['sparc']],
+            BinaryPackageBuildSet()._getAllowedArchitectures(
+                self.archive, self.distroseries.architectures))
+
+    def test_restricted_override(self):
+        # Restricted architectures are returned if allowed by the archive.
+        self.avr.restricted = True
+        getUtility(IArchiveArchSet).new(self.archive, self.avr)
+        self.assertContentEqual(
+            [self.distroseries['sparc'], self.distroseries['avr']],
+            BinaryPackageBuildSet()._getAllowedArchitectures(
+                self.archive, self.distroseries.architectures))
+
+    def test_disabled_architectures_omitted(self):
+        # Disabled architectures are not buildable, so are excluded.
+        self.distroseries['sparc'].enabled = False
+        self.assertContentEqual(
+            [self.distroseries['avr']],
+            BinaryPackageBuildSet()._getAllowedArchitectures(
+                self.archive, self.distroseries.architectures))
+
+    def test_virt_archives_have_only_virt_archs(self):
+        # For archives which must build on virtual builders, only
+        # virtual archs are returned.
+        self.distroseries['sparc'].supports_virtualized = False
+        self.assertContentEqual(
+            [self.distroseries['avr']],
+            BinaryPackageBuildSet()._getAllowedArchitectures(
+                self.archive, self.distroseries.architectures))
+
+    def test_nonvirt_archives_have_only_all_archs(self):
+        # Non-virtual archives can build on all unrestricted architectures.
+        self.distroseries['sparc'].supports_virtualized = False
+        self.archive.require_virtualized = False
+        self.assertContentEqual(
+            [self.distroseries['sparc'], self.distroseries['avr']],
+            BinaryPackageBuildSet()._getAllowedArchitectures(
+                self.archive, self.distroseries.architectures))
+
+
 class BuildRecordCreationTests(TestNativePublishingBase):
     """Test the creation of build records."""
 
@@ -302,38 +369,6 @@
                 del success_map[build.distro_arch_series.architecturetag]
         self.assertContentEqual([], success_map)
 
-    def test__getAllowedArchitectures_restricted(self):
-        """Test _getAllowedArchitectures doesn't return unrestricted
-        archs.
-
-        For a normal archive, only unrestricted architectures should
-        be used.
-        """
-        self.avr.restricted = True
-        available_archs = [
-            self.distroseries['sparc'], self.distroseries['avr']]
-        pubrec = self.getPubSource(architecturehintlist='any')
-        self.assertEqual(
-            [self.distroseries['sparc']],
-            BinaryPackageBuildSet()._getAllowedArchitectures(
-                pubrec.archive, available_archs))
-
-    def test__getAllowedArchitectures_restricted_override(self):
-        """Test _getAllowedArchitectures honors overrides of restricted archs.
-
-        Restricted architectures should only be allowed if there is
-        an explicit ArchiveArch association with the archive.
-        """
-        self.avr.restricted = True
-        available_archs = [
-            self.distroseries['sparc'], self.distroseries['avr']]
-        getUtility(IArchiveArchSet).new(self.archive, self.avr)
-        pubrec = self.getPubSource(architecturehintlist='any')
-        self.assertEqual(
-            [self.distroseries['sparc'], self.distroseries['avr']],
-            BinaryPackageBuildSet()._getAllowedArchitectures(
-                pubrec.archive, available_archs))
-
     def test_createForSource_restricts_any(self):
         """createForSource() should limit builds targeted at 'any'
         architecture to those allowed for the archive.