launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



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

The comment doesn't appear to quite match the test here, or else the test is more vacuous than intended, as armel is the earliest architecture in the (indep) hint list.

> +
> +    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 tend to think that the point of user-defined fields in SPR isn't so much that they happen to be written in debian/control using XS- (which is removed before it reaches the .dsc anyway), but that Launchpad doesn't assign them any special meaning.  Once we start assigning meaning then I think we should be modelling them directly, or possibly radically simplifying SPR so that lots of other fields are handled in JSON rather than given their own columns.  However, either of those is a fair bit of database work, so this approach is OK for now.

> +            [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.