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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
