launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/buildmaster/model/buildfarmjobbehaviour.py'
> --- lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2016-10-12 14:02:19 +0000
> +++ lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2018-05-02 13:37:24 +0000
> @@ -48,11 +49,39 @@
>          self.build = build
>          self._builder = None
>  
> +    @property
> +    def distro_arch_series(self):
> +        if self.build is not None:
> +            return self.build.distro_arch_series
> +        else:
> +            return None

I'm not sure None is really a valid value, nor why RecipeBuilder allows it today. dispatchBuildToSlave will always crash when composeBuildRequests returns das=None, so I'm not sure it's worth not crashing in the default implementation of distro_arch_series itself.

> +
>      def setBuilder(self, builder, slave):
>          """The builder should be set once and not changed."""
>          self._builder = builder
>          self._slave = slave
>  
> +    def determineFilesToSend(self):
> +        """The default behaviour is to send no files."""
> +        return {}
> +
> +    def extraBuildArgs(self, logger=None):
> +        """The default behaviour is to send only common extra arguments."""
> +        args = {}
> +        args["arch_tag"] = self.distro_arch_series.architecturetag
> +        args["archive_private"] = self.build.archive.private
> +        args["build_url"] = canonical_url(self.build)
> +        args["fast_cleanup"] = self._builder.virtualized
> +        args["series"] = self.distro_arch_series.distroseries.name
> +        return args
> +
> +    @defer.inlineCallbacks
> +    def composeBuildRequest(self, logger):
> +        args = yield self.extraBuildArgs(logger=logger)
> +        defer.returnValue(
> +            (self.builder_type, self.distro_arch_series,
> +             self.determineFilesToSend(), args))
> +
>      def verifyBuildRequest(self, logger):
>          """The default behaviour is a no-op."""
>          pass
> 
> === modified file 'lib/lp/soyuz/model/livefsbuildbehaviour.py'
> --- lib/lp/soyuz/model/livefsbuildbehaviour.py	2018-03-01 17:36:31 +0000
> +++ lib/lp/soyuz/model/livefsbuildbehaviour.py	2018-05-02 13:37:24 +0000
> @@ -76,33 +77,26 @@
>                  "Missing chroot for %s" % build.distro_arch_series.displayname)
>  
>      @defer.inlineCallbacks
> -    def _extraBuildArgs(self, logger=None):
> +    def extraBuildArgs(self, logger=None):
>          """
>          Return the extra arguments required by the slave for the given build.
>          """
>          build = self.build
> +        args = yield super(LiveFSBuildBehaviour, self).extraBuildArgs(
> +            logger=logger)
>          # Non-trivial metadata values may have been security-wrapped, which
>          # is pointless here and just gets in the way of xmlrpclib
>          # serialisation.
> -        args = dict(removeSecurityProxy(build.livefs.metadata))
> +        args.update(removeSecurityProxy(build.livefs.metadata))

It's probably slightly strange that livefs.metadata can clobber arch_tag etc., but hopefully only lp-buildd reads the args dict so there's no security issue.

>          if build.metadata_override is not None:
>              args.update(removeSecurityProxy(build.metadata_override))
> -        args["series"] = build.distro_series.name
>          args["pocket"] = build.pocket.name.lower()
> -        args["arch_tag"] = build.distro_arch_series.architecturetag
>          args["datestamp"] = build.version
>          args["archives"], args["trusted_keys"] = (
>              yield get_sources_list_for_building(
>                  build, build.distro_arch_series, None, logger=logger))
> -        args["archive_private"] = build.archive.private
> -        args["build_url"] = canonical_url(build)
>          defer.returnValue(args)
>  
> -    @defer.inlineCallbacks
> -    def composeBuildRequest(self, logger):
> -        args = yield self._extraBuildArgs(logger=logger)
> -        defer.returnValue(("livefs", self.build.distro_arch_series, {}, args))
> -
>      def verifySuccessfulBuild(self):
>          """See `IBuildFarmJobBehaviour`."""
>          # The implementation in BuildFarmJobBehaviourBase checks whether the


-- 
https://code.launchpad.net/~cjwatson/launchpad/build-fast-cleanup/+merge/344958
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.