[Merge] lp:~tobijk/livecd-rootfs/image-sets into lp:livecd-rootfs
Dan Watkins
daniel.watkins at canonical.com
Fri Oct 19 16:21:34 UTC 2018
Review: Needs Fixing
Overall, I really like this. The code is well laid out, and the series concept should serve us well in the future due to its flexibility. Thanks!
(I have some inline comments on the Python script, but none of these relate to the overall behaviour of the script or this MP.)
Diff comments:
>
> === added directory 'live-build/ubuntu-cpc/hooks.d/chroot'
> === renamed file 'live-build/ubuntu-cpc/hooks/099-cleanup.chroot' => 'live-build/ubuntu-cpc/hooks.d/chroot/cleanup.chroot'
> === renamed file 'live-build/ubuntu-cpc/hooks/999-cpc-fixes.chroot' => 'live-build/ubuntu-cpc/hooks.d/chroot/cpc-fixes.chroot'
> === renamed file 'live-build/ubuntu-cpc/hooks/025-create-groups.chroot' => 'live-build/ubuntu-cpc/hooks.d/chroot/create-groups.chroot'
> === renamed file 'live-build/ubuntu-cpc/hooks/001-divert-sync.chroot_early' => 'live-build/ubuntu-cpc/hooks.d/chroot/divert-sync.chroot_early'
> === renamed file 'live-build/ubuntu-cpc/hooks/060-ipv6.chroot' => 'live-build/ubuntu-cpc/hooks.d/chroot/ipv6.chroot'
> === renamed file 'live-build/ubuntu-cpc/hooks/061-open-iscsi.chroot' => 'live-build/ubuntu-cpc/hooks.d/chroot/open-iscsi.chroot'
> === renamed file 'live-build/ubuntu-cpc/hooks/020-pkg-configure.chroot' => 'live-build/ubuntu-cpc/hooks.d/chroot/pkg-configure.chroot'
> === renamed file 'live-build/ubuntu-cpc/hooks/052-ssh_authentication.chroot' => 'live-build/ubuntu-cpc/hooks.d/chroot/ssh_authentication.chroot'
> === renamed file 'live-build/ubuntu-cpc/hooks/999-undivert-sync.chroot' => 'live-build/ubuntu-cpc/hooks.d/chroot/undivert-sync.chroot'
> === renamed file 'live-build/ubuntu-cpc/hooks/010-write-etc-ec2-version.chroot' => 'live-build/ubuntu-cpc/hooks.d/chroot/write-etc-ec2-version.chroot'
> === added file 'live-build/ubuntu-cpc/hooks.d/make-hooks'
> --- live-build/ubuntu-cpc/hooks.d/make-hooks 1970-01-01 00:00:00 +0000
> +++ live-build/ubuntu-cpc/hooks.d/make-hooks 2018-10-16 12:54:50 +0000
> @@ -0,0 +1,180 @@
> +#!/usr/bin/env python
This should be Python 3 in at least the devel release, cosmic and bionic. (I think it can/should probably also be Python 3 in xenial; we may need to ensure that Python 3 is in the build environment.)
If it can be Python 3-only, then there are some Python 2-isms that can be removed from this file; let's confirm that it can be 3-only before I call any of them out.
> +#-*- encoding: utf-8 -*-
> +
> +import codecs
> +import getopt
> +import os
> +import re
> +import shutil
> +import sys
> +
> +SCRIPT_DIR = os.path.normpath(os.path.dirname(os.path.realpath(sys.argv[0])))
> +HOOKS_DIR = os.path.normpath(os.path.join(SCRIPT_DIR, "..", "hooks"))
> +EXIT_OK = 0
> +EXIT_ERR = 1
> +
> +class MakeHooksError(RuntimeError):
RuntimeError generally indicates an issue internal to Python; could this just inherit from Exception instead?
> + pass
> +
> +class MakeHooks(object):
> +
> + RESOURCES = {
> + # source in hooks.d | target in hooks
> + "base/ovf": "ovf",
> + "base/raspi2": "raspi2",
> + "extra/functions": "extra/functions",
> + "extra/gce-uefi-keys": "extra/gce-uefi-keys",
> + "extra/ovf": "extra/ovf"
> + }
> +
> + def __init__(self, hooks_dir=None, quiet=False):
> + self._script_dir = SCRIPT_DIR
> + self._hooks_dir = hooks_dir or HOOKS_DIR
> + self._quiet = quiet
> +
> + self.reset()
> + #end function
> +
> + def reset(self):
> + self._hooks_list = []
> + self._hooks_index = {}
> + self._included = {}
Could this be a set object instead of a dict? It looks like we only ever use the uniqueness-of-keys property of dicts, which we'd also get from sets.
(It looks to me like the ordering of includes isn't important, as they are processed as soon as they are found, but be aware that the iteration order of both dict keys[0] and sets is not guaranteed to be in any particular order.)
[0] This is less true in the latest version(s?) of Python 3; dict order is stable. Regardless, it's true in versions we're still going to be supporting for ~4 years. :p
> + #end function
> +
> + def print_usage(self):
> + print(
> + "CPC live build hook generator script \n"
> + " \n"
> + "Usage: ./make-hooks.sh [OPTIONS] <image_set> \n"
> + " \n"
> + "Options: \n"
> + " \n"
> + " --help, -h Show this message and exit. \n"
> + " --quiet, -q Only show warnings and error messages. \n"
> + " --hooks-dir, -d <dir> The directory where to write the symlinks.\n"
> + )
> + #end function
> +
> + def find_series_file(self, image_set):
> + for subdir in ["extra", "base"]:
> + series_file = os.path.join(self._script_dir, subdir, "series", image_set)
> +
> + if os.path.isfile(series_file):
> + return series_file
> + #end for
> +
> + return None
> + #end function
> +
> + def create_symlinks(self, image_sets, level=0):
> + for image_set in image_sets:
> + series_file = self.find_series_file(image_set)
> +
> + if not series_file:
> + raise MakeHooksError(
> + "Series file for image set '%s' not found." % image_set)
> +
> + with codecs.open(series_file, "r", encoding="utf-8") as fp:
This is the one Python 2/3 thing I'm going to note because I'm not convinced I'll remember it on a re-review: Python 3's builtin open takes the encoding kwarg, so if this script can be Py3-only then I think we can drop the use of the codecs library.
> + for line in fp:
> + line = line.strip()
> +
> + if not line:
> + continue
> +
> + m = re.match(r"^\s*depends\s+(\S+.*)$", line)
> + if m:
> + include_set = m.group(1)
> + if include_set in self._included:
> + continue
> + self._included[include_set] = 1
> + self.create_symlinks([include_set,], level=level+1)
> + continue
> + #end if
> +
> + if not line in self._hooks_index:
Do we need a separate _hooks_index? It looks like this could just be `if not line in self._hooks_list` and that would remove the need for separate collections.
> + self._hooks_list.append(line)
> + self._hooks_index[line] = 1
> + #end if
> + #end for
> + #end with
> + #end for
> +
> + if level == 0:
This check suggests that perhaps this method could be split in to two? Maybe collect_hooks (which would do everything before this line) and create_symlinks (which would do everything after)? (I think that would also let us drop the level argument?)
> + for counter, hook in enumerate(self._hooks_list, start=1):
> + linkname = ("%03d-" % counter) + os.path.basename(hook)
> + linksrc = os.path.join(self._hooks_dir, linkname)
> + linkdest = os.path.join(self._script_dir, hook)
> +
> + if not self._quiet:
> + print("[HOOK] %s => %s" % (linkname, hook))
> +
> + os.symlink(linkdest, linksrc)
> + #end for
> + #end if
> + #end function
> +
> + def copy_resources(self):
> + for source, target in MakeHooks.RESOURCES.items():
It's not likely to matter in this script, but this would be better as self.RESOURCES.items() so that sub-classes can override RESOURCES if necessary.
> + source_abs = os.path.join(self._script_dir, source)
> + target_abs = os.path.join(self._hooks_dir, target)
> +
> + if not os.path.exists(source_abs):
> + continue
> +
> + if os.path.isdir(source_abs):
> + shutil.copytree(source_abs, target_abs)
> + else:
> + target_dir = os.path.dirname(target_abs)
> +
> + if not os.path.exists(target_dir):
> + os.makedirs(target_dir)
> +
> + shutil.copy2(source_abs, target_abs)
> + #end if
> + #end for
> + #end function
> +
> + def cli(self):
> + self.reset()
> +
> + try:
> + opts, args = getopt.getopt(sys.argv[1:], "hqd:", ["help", "quiet", "hooks-dir="])
> + except getopt.GetoptError as e:
> + raise MakeHooksError(str(e))
> +
> + for o, v in opts:
> + if o in ["-h", "--help"]:
> + self.print_usage()
> + sys.exit(EXIT_OK)
> + elif o in ["-q", "--quiet"]:
> + self._quiet = True
> + elif o in ["-d", "--hooks-dir"]:
> + self._hooks_dir = v.strip()
> + #end for
> +
> + if len(args) < 1:
> + self.print_usage()
> + sys.exit(EXIT_ERR)
> + #end if
> +
> + # Take remaining command line arguments, sanitize and turn into list.
> + image_sets = re.sub(r";|,", " ", " ".join(args)).split()
> +
> + if os.path.isdir(self._hooks_dir):
Would we ever expect this to be the case? (Or, put another way, if we hit this case, have we run in to some error state that we should indicate in some way?)
> + shutil.rmtree(self._hooks_dir)
> + os.makedirs(self._hooks_dir)
> +
> + self.create_symlinks(image_sets)
> + self.copy_resources()
> + #end function
> +
> +#end class
> +
> +if __name__ == "__main__":
> + try:
> + MakeHooks().cli()
> + except MakeHooksError as e:
> + sys.stderr.write("%s: %s\n" % (os.path.basename(sys.argv[0]), str(e)))
> + sys.exit(EXIT_ERR)
> + #end try
> +#end __main__
--
https://code.launchpad.net/~tobijk/livecd-rootfs/image-sets/+merge/356246
Your team Ubuntu Core Development Team is requested to review the proposed merge of lp:~tobijk/livecd-rootfs/image-sets into lp:livecd-rootfs.
More information about the Ubuntu-reviews
mailing list