launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxxxxxx>]

On Tue, Aug 24, 2010 at 11:48 AM, William Grant <me@xxxxxxxxxxxxxxxxxx> wrote:
> William Grant has proposed merging lp:~wgrant/launchpad/buildbase-must-die into lp:launchpad/devel.
>
> Requested reviews:
>  Launchpad code reviewers (launchpad-reviewers)
>
>
> Wellington saw the creation of BuildBase as a base class to sit under BinaryPackageBuild and SourcePackageRecipeBuild. It didn't have a table, so fields were duplicated in its subclasses. A couple of months ago, a new PackageBuild class and table were created to replace it, calling into BuildBase's methods to do its work. Now that both build types have been ported to PackageBuild, we can move BuildBase's functionality into PackageBuild, and remove BuildBase itself.
>
> This branch does a pretty straight move of BuildBase's methods to PackageBuild and PackageBuildDerived. I've de-static'd some methods where it was trivial, but other cleanup can wait for a subsequent branch.
>
> lp.buildmaster.model.buildbase is gone entirely, but lp.buildmaster.interfaces.buildbase retains two members (BUILDD_MANAGER_LOG_NAME and BuildStatus), since this branch is big enough. I'll move them afterwards.


Hey William,

Thanks for working on this branch, and for the helpful cover letter.

For ease of reviewing, I've commented on the additions below as if
they were genuine additions, rather than a move. I have, however,
restricted my comments to fairly shallow points, since this branch is
not the place for changing behaviour or large-scale refactoring.

Hope this helps,
jml

> === modified file 'lib/lp/buildmaster/model/packagebuild.py'
> --- lib/lp/buildmaster/model/packagebuild.py    2010-08-20 20:31:18 +0000
> +++ lib/lp/buildmaster/model/packagebuild.py    2010-08-24 10:48:48 +0000
...
>     def getUploadDirLeaf(self, build_cookie, now=None):
>         """See `IPackageBuild`."""
> -        return BuildBase.getUploadDirLeaf(build_cookie, now)
> +        # UPLOAD_LEAF: <TIMESTAMP>-<BUILD-COOKIE>

I'm not convinced this comment adds much. A temporary "timestamp"
local variable would say the same thing, but in Python.

If you'd rather keep the comment, it would be better to make it say
what "upload leaf" means and *why* its value is TIMESTAMP-BUILDCOOKIE.

>     @staticmethod
>     def getUploaderCommand(package_build, upload_leaf, upload_logfilename):
>         """See `IPackageBuild`."""
> -        return BuildBase.getUploaderCommand(
> -            package_build, upload_leaf, upload_logfilename)
> +        root = os.path.abspath(config.builddmaster.root)
> +        uploader_command = list(config.builddmaster.uploader.split())
> +
> +        # add extra arguments for processing a binary upload

Please write this as a sentence with a capital and a full stop.

> +        extra_args = [
> +            "--log-file", "%s" % upload_logfilename,
> +            "-d", "%s" % package_build.distribution.name,
> +            "-s", "%s" % (package_build.distro_series.name +
> +                          pocketsuffix[package_build.pocket]),

package_build.distro_series.getSuite(package_build.pocket) is slightly
nicer, imho.

>     @staticmethod
>     def getLogFromSlave(package_build):
>         """See `IPackageBuild`."""
> -        return BuildBase.getLogFromSlave(package_build)
> +        builder = package_build.buildqueue_record.builder
> +        return builder.transferSlaveFileToLibrarian(
> +            'buildlog', package_build.buildqueue_record.getLogFileName(),
> +            package_build.is_private)
>

Could you please define a constant for 'buildlog'?

>     @staticmethod
> -    def storeBuildInfo(package_build, librarian, slave_status):
> +    def storeBuildInfo(build, librarian, slave_status):
>         """See `IPackageBuild`."""
> -        return BuildBase.storeBuildInfo(package_build, librarian,
> -                                        slave_status)
> +        naked_build = removeSecurityProxy(build)

Please add a comment explaining why removing the security proxy here
is both necessary and safe.

...
> +        # ensure we have the correct build root as:
> +        # <BUILDMASTER_ROOT>/incoming/<UPLOAD_LEAF>/<TARGET_PATH>/[FILES]

Please capitalize.

> +        # create a single directory to store build result files

Capital 'C', full stop.

...
> === modified file 'lib/lp/buildmaster/tests/test_packagebuild.py'
> --- lib/lp/buildmaster/tests/test_packagebuild.py       2010-08-20 20:31:18 +0000
> +++ lib/lp/buildmaster/tests/test_packagebuild.py       2010-08-24 10:48:48 +0000
...
> @@ -221,3 +238,151 @@
...
> +    def test_getUploadLogContent_only_dir(self):
> +        """If there is a directory but no log file, expect the error string,
> +        not an exception."""
> +        self.useTempDir()
> +        os.makedirs("accepted/myleaf")
> +        self.assertEquals('Could not find upload log file',
> +            self.build.getUploadLogContent(os.getcwd(), "myleaf"))
> +

We normally indent multi-line function calls like:
  self.assertEquals(
      "Could not ...",
     self.build.getUploa...)

> +    def test_getUploadLogContent_readsfile(self):
> +        """If there is a log file, return its contents."""
> +        self.useTempDir()
> +        os.makedirs("accepted/myleaf")
> +        with open('accepted/myleaf/uploader.log', 'w') as f:
> +            f.write('foo')
> +        self.assertEquals('foo',
> +            self.build.getUploadLogContent(os.getcwd(), "myleaf"))
> +

As above.

Done. Thanks again. Let me know when you've pushed up the tweaks
suggested in the review and I'll land the branch.

jml
-- 
https://code.launchpad.net/~wgrant/launchpad/buildbase-must-die/+merge/33505
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/buildbase-must-die into lp:launchpad/devel.