launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'buildlivefs'
> --- buildlivefs	2014-06-24 14:59:08 +0000
> +++ buildlivefs	2014-07-24 07:36:30 +0000
> @@ -19,14 +19,14 @@
>  RETCODE_FAILURE_BUILD = 201
>  
>  
> -def get_build_path(build_id, *extra):
> +def get_build_path(build_home, build_id, *extra):
>      """Generate a path within the build directory.
>  
>      :param build_id: the build id to use.
>      :param extra: the extra path segments within the build directory.
>      :return: the generated path.
>      """
> -    return os.path.join(os.environ["HOME"], "build-" + build_id, *extra)
> +    return os.path.join(build_home, "build-" + build_id, *extra)
>  
>  
>  non_meta_re = re.compile(r'^[a-zA-Z0-9+,./:=@_-]+$')
> @@ -64,13 +64,34 @@
>      ]
>  
>  
> +class MissingOptionError(Exception):
> +    """A required option for the LiveFSBuilder is missing"""
> +    pass
> +
> +
>  class LiveFSBuilder:
>      """Builds a live file system."""
>  
>      def __init__(self, options):
> +        self._check_options(options)
>          self.options = options
>          self.chroot_path = get_build_path(
> -            self.options.build_id, 'chroot-autobuild')
> +            options.build_home, options.build_id, 'chroot-autobuild')
> +        # be gentle to people who run it outside the buildd environment
> +        if not os.path.exists(self.chroot_path):
> +            self._prepare_chroot(self.chroot_path, options.series)

An error in the event that the chroot is missing would be fine, but it really isn't the job of buildlivefs to prepare the chroot, and it shouldn't do this because it's preparing a chroot in a way that doesn't match the normal behaviour in Launchpad production.  I would recommend instead having whatever's calling this do an unpack-chroot / mount-chroot / update-debian-chroot / buildlivefs / umount-chroot / remove-build sequence, roughly matching how launchpad-buildd normally operates.

> +
> +    def _prepare_chroot(self, chroot_path, series):
> +        os.makedirs(os.path.join(chroot_path, "build"))
> +        subprocess.check_call(
> +            ["sudo", "debootstrap", "--variant=buildd",
> +             series, chroot_path])
> +
> +    def _check_options(self, options):
> +        for required_option in ["arch", "project", "series"]:
> +            if getattr(options, required_option) is None:
> +                raise MissingOptionError(
> +                    "Option '%s' is required" % required_option)

Maybe "Option '--%s' is required"?  I'd probably use a tuple rather than a list too, but minor detail.

>  
>      def chroot(self, args, echo=False):
>          """Run a command in the chroot.
> @@ -161,19 +182,23 @@
>  
>  def main():
>      parser = OptionParser()
> -    parser.add_option("--build-id", help="build identifier")
> +    parser.add_option("--build-id", default="buildlivefs",
> +                      help="build identifier")

I'm wary of this potentially causing accidents in production where we end up with multiple builds trying to use the same build directory, which might go unnoticed for a while since most (but not quite all) builds are fully cleaned up before the next one starts.  Again, I think it would be better for the caller to simply always pass a build ID.

>      parser.add_option(
> -        "--arch", metavar="ARCH", help="build for architecture ARCH")
> +        "--arch", metavar="ARCH",
> +        help="build for architecture ARCH (e.g. amd64)")
>      parser.add_option(
>          "--subarch", metavar="SUBARCH",
>          help="build for subarchitecture SUBARCH")
>      parser.add_option(
> -        "--project", metavar="PROJECT", help="build for project PROJECT")
> +        "--project", metavar="PROJECT",
> +        help="build for project PROJECT (e.g. ubuntu-desktop)")
>      parser.add_option(
>          "--subproject", metavar="SUBPROJECT",
>          help="build for subproject SUBPROJECT")
>      parser.add_option(
> -        "--series", metavar="SERIES", help="build for series SERIES")
> +        "--series", metavar="SERIES",
> +        help="build for series SERIES (e.g. trusty)")
>      parser.add_option("--datestamp", help="date stamp")
>      parser.add_option(
>          "--image-format", metavar="FORMAT", help="produce an image in FORMAT")
> @@ -186,6 +211,9 @@
>      parser.add_option(
>          "--extra-ppa", dest="extra_ppas", default=[], action="append",
>          help="use this additional PPA")
> +    parser.add_option(
> +        "--build-home", metavar="DIRECTORY", default=os.environ["HOME"],
> +        help="Root directory for the build results (default to $HOME)")
>      options, _ = parser.parse_args()
>  
>      builder = LiveFSBuilder(options)
> @@ -196,6 +224,8 @@
>          return RETCODE_FAILURE_INSTALL
>      try:
>          builder.build()
> +        print("Build sucessful, your build result is available in %s" % 

Typo for "successful", and a run-on sentence.

> +              os.path.join(builder.chroot_path, "build"))
>      except Exception:
>          traceback.print_exc()
>          return RETCODE_FAILURE_BUILD
> 


-- 
https://code.launchpad.net/~mvo/launchpad-buildd/mvo/+merge/228046
Your team Launchpad code reviewers is subscribed to branch lp:launchpad-buildd.