launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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

Commit message:
Source packages can select their arch-indep build architecture using a new Build-Indep-Architecture field.

"all" in a hint list now forces a nominatedarchindep build only if it is the sole term, rather than if it was the only term we could resolve.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #217427 in Launchpad itself: "Please support arbitrary arch/buildd affinity for arch:all builds"
  https://bugs.launchpad.net/launchpad/+bug/217427
  Bug #1390409 in Launchpad itself: "BinaryPackageBuildSet.createForSource erroneously creates nominatedarchindep builds when arch-restricted"
  https://bugs.launchpad.net/launchpad/+bug/1390409

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

Source packages can select their arch-indep build architecture using a new Build-Indep-Architecture field.

"all" in a hint list now forces a nominatedarchindep build only if it is the sole term, rather than if it was the only term we could resolve.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/xbia into lp:launchpad.

=== modified file 'lib/lp/soyuz/adapters/buildarch.py'
--- lib/lp/soyuz/adapters/buildarch.py	2015-02-11 10:43:58 +0000
+++ lib/lp/soyuz/adapters/buildarch.py	2015-02-11 10:43:59 +0000
@@ -36,29 +36,62 @@
 dpkg_architecture = DpkgArchitectureCache()
 
 
-def determine_architectures_to_build(hintlist, valid_archs,
+def resolve_arch_spec(hintlist, valid_archs):
+    hint_archs = set(hintlist.split())
+    # 'all' is only used if it's a purely arch-indep package.
+    if hint_archs == set(["all"]):
+        return set(), True
+    return (
+        set(dpkg_architecture.findAllMatches(valid_archs, hint_archs)), False)
+
+
+def determine_architectures_to_build(hint_list, indep_hint_list, need_archs,
                                      nominated_arch_indep, need_arch_indep):
     """Return a set of architectures to build.
 
-    :param hintlist: A string of the architectures this source package
+    :param hint_list: a string of the architectures this source package
         specifies it builds for.
-    :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.
+    :param indep_hint_list: a string of the architectures this source package
+        specifies it can build architecture-independent packages on.
+    :param need_archs: an ordered list of all architecture tags that we can
+        create builds for. the first usable one gets the arch-indep flag.
+    :param nominated_arch_indep: the default architecture tag for
+        arch-indep-only packages. may be None.
+    :param need_arch_indep: should an arch-indep build be created if possible?
+    :return: a map of architecture tag to arch-indep flag for each build
+        that should be created.
     """
-    hint_archs = set(hintlist.split())
-    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_archs) == 0 and 'all' in hint_archs:
-        if nominated_arch_indep in valid_archs:
-            return set([nominated_arch_indep])
-        else:
-            return set()
-
-    return build_archs
+    build_archs, indep_only = resolve_arch_spec(hint_list, need_archs)
+
+    # Use the indep hint list if it's set, otherwise fall back to the
+    # main architecture list. If that's not set either (ie. it's just
+    # "all"), default to nominatedarchindep.
+    if indep_hint_list:
+        indep_archs, _ = resolve_arch_spec(indep_hint_list, need_archs)
+    elif not indep_only:
+        indep_archs = set(build_archs)
+    elif nominated_arch_indep in need_archs:
+        indep_archs = set([nominated_arch_indep])
+    else:
+        indep_archs = set()
+
+    indep_arch = None
+    if need_arch_indep:
+        # Try to avoid adding a new build if an existing one would work.
+        both_archs = set(build_archs) & set(indep_archs)
+        if both_archs:
+            indep_archs = both_archs
+
+        # The ideal arch_indep build is nominatedarchindep. But if we're
+        # not creating a build for it, use the first candidate DAS that
+        # made it this far.
+        for arch in [nominated_arch_indep] + need_archs:
+            if arch in indep_archs:
+                indep_arch = arch
+                break
+
+    # Ensure that we build the indep arch.
+    if indep_arch is not None and indep_arch not in build_archs:
+        build_archs.add(indep_arch)
+
+    return {arch: arch == indep_arch for arch in build_archs}

=== modified file 'lib/lp/soyuz/adapters/tests/test_buildarch.py'
--- lib/lp/soyuz/adapters/tests/test_buildarch.py	2015-02-11 10:43:58 +0000
+++ lib/lp/soyuz/adapters/tests/test_buildarch.py	2015-02-11 10:43:59 +0000
@@ -43,82 +43,140 @@
     """
 
     def assertArchsForHint(self, hint_string, expected_arch_tags,
-                           allowed_arch_tags=None):
+                           allowed_arch_tags=None, indep_hint_list=None,
+                           need_arch_indep=True):
         if allowed_arch_tags is None:
             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)
+            hint_string, indep_hint_list, allowed_arch_tags, 'i386',
+            need_arch_indep)
+        self.assertContentEqual(expected_arch_tags.items(), arch_tags.items())
 
     def test_single_architecture(self):
         # A hint string with a single arch resolves to just that arch.
-        self.assertArchsForHint('hppa', ['hppa'])
+        self.assertArchsForHint('hppa', {'hppa': True})
 
     def test_three_architectures(self):
         # A hint string with multiple archs resolves to just those
         # archs.
-        self.assertArchsForHint('amd64 i386 hppa', ['hppa', 'i386'])
+        self.assertArchsForHint(
+            'amd64 i386 hppa', {'hppa': False, 'i386': True})
 
     def test_independent(self):
         # 'all' is special, meaning just a single build. The
         # nominatedarchindep architecture is used -- in this case i386.
-        self.assertArchsForHint('all', ['i386'])
+        self.assertArchsForHint('all', {'i386': True})
 
     def test_one_and_independent(self):
         # 'all' is redundant if we have another build anyway.
-        self.assertArchsForHint('hppa all', ['hppa'])
+        self.assertArchsForHint('hppa all', {'hppa': True})
 
     def test_fictional_and_independent(self):
-        # But 'all' is useful if present with an arch that wouldn't
-        # generate a build.
-        self.assertArchsForHint('foo all', ['i386'])
+        # 'all' doesn't make an unbuildable string buildable.
+        self.assertArchsForHint('fiction all', {})
 
     def test_wildcard(self):
         # 'any' is a wildcard that matches all available archs.
-        self.assertArchsForHint('any', ['armel', 'hppa', 'i386'])
+        self.assertArchsForHint(
+            'any', {'armel': False, 'hppa': False, 'i386': True})
 
     def test_kernel_specific_architecture(self):
         # Since we only support Linux-based architectures, 'linux-foo'
         # is treated the same as 'foo'.
-        self.assertArchsForHint('linux-hppa', ['hppa'])
+        self.assertArchsForHint('linux-hppa', {'hppa': True})
 
     def test_unknown_kernel_specific_architecture(self):
         # Non-Linux architectures aren't supported.
-        self.assertArchsForHint('kfreebsd-hppa', [])
+        self.assertArchsForHint('kfreebsd-hppa', {})
 
     def test_kernel_wildcard_architecture(self):
         # Wildcards work for kernels: 'any-foo' is treated like 'foo'.
-        self.assertArchsForHint('any-hppa', ['hppa'])
+        self.assertArchsForHint('any-hppa', {'hppa': True})
 
     def test_kernel_wildcard_architecture_arm(self):
         # The second part of a wildcard matches the canonical CPU name, not
         # on the Debian architecture, so 'any-arm' matches 'armel'.
-        self.assertArchsForHint('any-arm', ['armel'])
+        self.assertArchsForHint('any-arm', {'armel': True})
 
     def test_kernel_specific_architecture_wildcard(self):
         # Wildcards work for archs too: 'linux-any' is treated like 'any'.
-        self.assertArchsForHint('linux-any', ['armel', 'hppa', 'i386'])
+        self.assertArchsForHint(
+            'linux-any', {'armel': False, 'hppa': False, 'i386': True})
 
     def test_unknown_kernel_specific_architecture_wildcard(self):
         # But unknown kernels continue to result in nothing.
-        self.assertArchsForHint('kfreebsd-any', [])
+        self.assertArchsForHint('kfreebsd-any', {})
 
     def test_wildcard_and_independent(self):
         # 'all' continues to be ignored alongside a valid wildcard.
-        self.assertArchsForHint('all linux-any', ['armel', 'hppa', 'i386'])
+        self.assertArchsForHint(
+            'all linux-any', {'armel': False, 'hppa': False, 'i386': True})
 
     def test_kernel_independent_is_invalid(self):
         # 'linux-all' isn't supported.
-        self.assertArchsForHint('linux-all', [])
+        self.assertArchsForHint('linux-all', {})
 
     def test_double_wildcard_is_same_as_single(self):
         # 'any-any' is redundant with 'any', but dpkg-architecture supports
         # it anyway.
-        self.assertArchsForHint('any-any', ['armel', 'hppa', 'i386'])
+        self.assertArchsForHint(
+            'any-any', {'armel': False, 'hppa': False, 'i386': True})
 
     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
         # i386) is omitted, no builds will be created for arch-indep
         # sources.
-        self.assertArchsForHint('all', [], allowed_arch_tags=['hppa'])
+        self.assertArchsForHint('all', {}, allowed_arch_tags=['hppa'])
+
+    def test_indep_hint_only(self):
+        # Some packages need to build arch-indep builds on a specific
+        # architecture, declared using XS-Build-Indep-Architecture.
+        self.assertArchsForHint('all', {'hppa': True}, indep_hint_list='hppa')
+
+    def test_indep_hint_only_multiple(self):
+        # The earliest available architecture in the available list (not
+        # the hint list) is chosen.
+        self.assertArchsForHint(
+            'all', {'armel': True}, indep_hint_list='armel hppa')
+
+    def test_indep_hint_only_unsatisfiable(self):
+        # An indep hint list that matches nothing results in no builds
+        self.assertArchsForHint('all', {}, indep_hint_list='fiction')
+
+    def test_indep_hint(self):
+        # Unlike nominatedarchindep, a hinted indep will cause an
+        # additional build to be created if necessary.
+        self.assertArchsForHint(
+            'armel all', {'armel': False, 'hppa': True},
+            indep_hint_list='hppa')
+
+    def test_indep_hint_wildcard(self):
+        # An indep hint list can include wildcards.
+        self.assertArchsForHint(
+            'armel all', {'armel': False, 'hppa': True},
+            indep_hint_list='any-hppa')
+
+    def test_indep_hint_coalesces(self):
+        # An indep hint list that matches an existing build will avoid
+        # creating another.
+        self.assertArchsForHint(
+            'hppa all', {'hppa': True}, indep_hint_list='linux-any')
+
+    def test_indep_hint_unsatisfiable(self):
+        # An indep hint list that matches nothing results in no
+        # additional builds
+        self.assertArchsForHint(
+            'armel all', {'armel': False}, indep_hint_list='fiction')
+
+    def test_no_need_arch_indep(self):
+        self.assertArchsForHint(
+            'armel all', {'armel': False}, need_arch_indep=False)
+
+    def test_no_need_arch_indep_hint(self):
+        self.assertArchsForHint(
+            'armel all', {'armel': False}, indep_hint_list='hppa',
+            need_arch_indep=False)
+
+    def test_no_need_arch_indep_only(self):
+        self.assertArchsForHint('all', {}, need_arch_indep=False)

=== modified file 'lib/lp/soyuz/interfaces/sourcepackagerelease.py'
--- lib/lp/soyuz/interfaces/sourcepackagerelease.py	2014-11-09 23:28:27 +0000
+++ lib/lp/soyuz/interfaces/sourcepackagerelease.py	2015-02-11 10:43:59 +0000
@@ -151,6 +151,9 @@
         title=_("Source package recipe build"),
         required=False, readonly=True)
 
+    def getUserDefinedField(name):
+        """Case-insensitively get a user-defined field."""
+
     def addFile(file, filetype=None):
         """Add the provided library file alias (file) to the list of files
         in this package.

=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2015-02-11 10:43:58 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2015-02-11 10:43:59 +0000
@@ -1423,56 +1423,44 @@
         # 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.
-        valid_archs = list(
-            architectures_available or distroseries.buildable_architectures)
-        valid_archs = self._getAllowedArchitectures(archive, valid_archs)
+        # Find the architectures for which the source chould end up with
+        # new binaries. Exclude architectures not allowed in this
+        # archive and architectures that have already built. Order by
+        # Processor.id so determine_architectures_to_build is
+        # deterministic.
+        # 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.
+        need_archs = sorted(
+            [das for das in
+             self._getAllowedArchitectures(
+                 archive,
+                 architectures_available
+                    or distroseries.buildable_architectures)
+             if das.architecturetag not in skip_archtags],
+            key=attrgetter('processor.id'))
         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(
+        # Filter the valid archs against the hint list and work out
+        # their arch-indepness.
+        create_tag_map = determine_architectures_to_build(
             sourcepackagerelease.architecturehintlist,
-            [das.architecturetag for das in valid_archs],
+            sourcepackagerelease.getUserDefinedField(
+                'Build-Indep-Architecture'),
+            [das.architecturetag for das in need_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.
-        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:
+        for das in need_archs:
+            if das.architecturetag not in create_tag_map:
+                continue
             build = self.new(
                 source_package_release=sourcepackagerelease,
                 distro_arch_series=das, archive=archive, pocket=pocket,
-                arch_indep=das == arch_indep_das)
+                arch_indep=create_tag_map[das.architecturetag])
             new_builds.append(build)
             # Create the builds in suspended mode for disabled archives.
             build_queue = build.queueBuild(suspended=not archive.enabled)

=== modified file 'lib/lp/soyuz/model/sourcepackagerelease.py'
--- lib/lp/soyuz/model/sourcepackagerelease.py	2014-11-09 23:28:27 +0000
+++ lib/lp/soyuz/model/sourcepackagerelease.py	2015-02-11 10:43:59 +0000
@@ -170,6 +170,12 @@
             return []
         return simplejson.loads(self._user_defined_fields)
 
+    def getUserDefinedField(self, name):
+        if self.user_defined_fields:
+            for k, v in self.user_defined_fields.iteritems():
+                if k.lower() == name.lower():
+                    return v
+
     @cachedproperty
     def package_diffs(self):
         return list(Store.of(self).find(

=== modified file 'lib/lp/soyuz/tests/test_build_set.py'
--- lib/lp/soyuz/tests/test_build_set.py	2015-02-11 10:43:58 +0000
+++ lib/lp/soyuz/tests/test_build_set.py	2015-02-11 10:43:59 +0000
@@ -491,6 +491,19 @@
         new_builds = self.createBuilds(spr, self.distroseries)
         self.assertBuildsMatch({}, new_builds)
 
+    def test_createForSource_build_indep_architecture(self):
+        # A user defined field of Build-Indep-Architecture provides a
+        # custom hint list to override Architecture and
+        # nominatedarchindep for arch-indep purposes.
+        spr = self.factory.makeSourcePackageRelease(
+            architecturehintlist='sparc all',
+            user_defined_fields={'build-indep-architecture': '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()."""