launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> === 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'),

I completely agree. The majority of the columns on SPR should really just be JSON fields, as they're not useful to query on. But there's a lot of migration work.

> +            [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()."""
> 


-- 
https://code.launchpad.net/~wgrant/launchpad/xbia/+merge/249296
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.