Re: [Libguestfs] [PATCH] builder: Rearrange how template-building scripts work.

On Monday, 28 November 2016 10:40:51 CET Richard W.M. Jones wrote: > Create a new directory (builder/template). Integrate all of the > scripts into a single program, so that templates are generated more > consistently. > > This also changes how the index file is generated. The script now > generates the index file fragment and saves it under version control, > and then generates the final index file by concatenating these. > (Previously the index was written by hand which was tedious and > error-prone.) > > The new script also saves the generated kickstart under version > control so it can be referenced later. > --- Note that the fragments for Ubuntu guests don't seem to match what the new make-template.ml produces -- in particular, the ones in this patch have osinfo=ubuntu$codename instead of osinfo=ubuntu$version as the script should produce. Ah I see, the current index is wrong already...

Also, this patch includes changes in the website index but not in the index.asc -- I'd just leave them unchanged in this patch (simple git move), and then regenerate them in a followup commit.

> diff --git a/builder/website/Makefile.am b/builder/templates/Makefile.am > similarity index 66% > rename from builder/website/Makefile.am > rename to builder/templates/Makefile.am > index d79b9a0..d1b89f9 100644 > --- a/builder/website/Makefile.am > +++ b/builder/templates/Makefile.am > @@ -1,5 +1,5 @@ > # libguestfs virt-builder tool > -# Copyright (C) 2013 Red Hat Inc. > +# Copyright (C) 2013-2016 Red Hat Inc. > # > # This program is free software; you can redistribute it and/or modify > # it under the terms of the GNU General Public License as published by > @@ -17,33 +17,25 @@ > > include $(top_srcdir)/subdir-rules.mk > > +index_fragments = $(wildcard *.index-fragment) > + > EXTRA_DIST = \ > - .gitignore \ > - compress.sh \ > - test-guest.sh \ > - validate.sh \ > - README \ > - index \ > - index.asc \ > - centos.sh \ > - centos-aarch64.sh \ > + $(index_fragments) \ > + *.ks \ Considering $(wildcard) above is already a GNU extension, I'd use it for *.ks as well. > debian.preseed \ > - debian.sh \ > - fedora.sh \ > - fedora-aarch64.sh \ > - fedora-armv7l.sh \ > - fedora-i686.sh \ > - fedora-ppc64.sh \ > - fedora-ppc64le.sh \ > - fedora-s390x.sh \ > - rhel.sh \ > - rhel-aarch64.sh \ > - rhel-ppc64.sh \ > - rhel-ppc64le.sh \ > - scientificlinux.sh \ > + make-template.ml \ > ubuntu.preseed \ > - ubuntu.sh \ > - ubuntu-ppc64le.sh > + validate.sh > + > +# Create the index file under the top level website/ directory. > +noinst_DATA = $(top_builddir)/website/download/builder/index > + > +$(top_builddir)/website/download/builder/index: $(index_fragments) > + rm -f $@ $@-t > + cat *.index-fragment > $@-t Should $^ be used instead of *index-fragment? Also, the order of the files could be made stable with sorting, so the index does not change just because of different results of the wildcard expansion.

> + (* Set the path to use locally built copies of the virt-* tools. *) > + let () = > + let orig_path = Unix.getenv "PATH" in > + let paths = ["builder"; "customize"; "sparsify"; "sysprep"] in > + let prefix = Sys.getcwd () // ".." // ".." in > + let paths = List.map ((//) prefix) paths in > + let new_path = String.concat ":" (paths @ [orig_path]) in > + Unix.putenv "PATH" new_path in Wouldn't be better just rely on the libguestfs tools in $PATH, i.e. using ./run for uninstalled versions? Just use a check similar to the one below (inspired from virt-builder): (* Check that we have libguestfs tools. *) let cmd = "virt-sparsify --help >/dev/null 2>&1" in if shell_command cmd <> 0 then ( eprintf "%s: libguestfs tools not available (use ./run for uninstalled versions)\n" prog; exit 1 );

> +and os_of_string os ver = > + match os, ver with > + | "centos", ver -> let maj, min = parse_major_minor ver in CentOS (maj, min) > + | "rhel", ver -> let maj, min = parse_major_minor ver in RHEL (maj, min) > + | "debian", "6" -> Debian (6, "squeeze") > + | "debian", "7" -> Debian (7, "wheezy") > + | "debian", "8" -> Debian (8, "jessie") > + | "ubuntu", "10.04" -> Ubuntu (ver, "lucid") > + | "ubuntu", "12.04" -> Ubuntu (ver, "precise") > + | "ubuntu", "14.04" -> Ubuntu (ver, "trusty") > + | "ubuntu", "16.04" -> Ubuntu (ver, "xenial") > + | "fedora", ver -> Fedora (int_of_string ver) > + | _ -> > + eprintf "%s: unknown or unsupported OS\n" prog; exit 1 Can the error message also print the distro name and version?

> +and make_kickstart_common ks_filename os arch = > + let buf = Buffer.create 4096 in > + let bpf fs = bprintf buf fs in > + > + bpf "\ > +# Kickstart file for %s > +# Generated by libguestfs.git/builder/templates/make-template.ml > + > +install > +text > +reboot > +lang en_US.UTF-8 > +keyboard us > +network --bootproto dhcp > +rootpw builder > +firewall --enabled --ssh > +timezone --utc America/New_York > +" (string_of_os os arch); > + > + (match os with > + | RHEL (3, _) -> > + bpf "\ > +langsupport en_US > +mouse generic > +"; > + | _ -> () > + ); rhel.sh has: if [ $major -le 4 ]; then so I think the check should rather be: | RHEL (ver, _) when ver <= 4 ->

> + > + (match os with > + | RHEL (3, _) -> () > + | _ -> bpf "selinux --enforcing\n" > + ); rhel.sh has: if [ $major -ge 4 ]; then so I think the check should rather be: | RHEL (ver, _) when ver >= 4 -> (OK, the current code does the same considering only RHEL >= 3 is supported, but could be better to convert it straight from the script.)

> + > + (match os with > + | RHEL (5, _) -> bpf "key --skip\n" > + | _ -> () > + ); > + bpf "\n"; > + > + bpf "bootloader --location=mbr --append=\"%s\"\n" > + (kernel_cmdline_of_os os arch); > + bpf "\n"; > + > + (match os with > + | CentOS _ -> > + bpf "\ > +zerombr > +clearpart --all --initlabel > +part /boot --fstype=ext4 --size=512 --asprimary > +part swap --size=1024 --asprimary > +part / --fstype=ext4 --size=1024 --grow --asprimary > +"; > + | RHEL ((3|4), _) -> > + bpf "\ > +zerombr > +clearpart --all --initlabel > +part /boot --fstype=ext2 --size=512 --asprimary > +part swap --size=1024 --asprimary > +part / --fstype=ext3 --size=1024 --grow --asprimary > +"; > + | RHEL (5, _) -> > + bpf "\ > +zerombr > +clearpart --all --initlabel > +part /boot --fstype=ext2 --size=512 --asprimary > +part swap --size=1024 --asprimary > +part / --fstype=ext4 --size=1024 --grow --asprimary > +"; > + | RHEL (6, _) -> > + bpf "\ > +zerombr > +clearpart --all --initlabel > +part /boot --fstype=ext4 --size=512 --asprimary > +part swap --size=1024 --asprimary > +part / --fstype=ext4 --size=1024 --grow --asprimary > +"; > + | RHEL (7, _) -> > + bpf "\ > +zerombr > +clearpart --all --initlabel > +part /boot --fstype=ext4 --size=512 --asprimary > +part swap --size=1024 --asprimary > +part / --fstype=xfs --size=1024 --grow --asprimary > +"; IMHO the RHEL cases could be merged together, leaving the bootfs and rootfs selection by version.

> + | _ -> > + bpf "\ > +zerombr > +clearpart --all --initlabel > +autopart --type=plain > +"; > + ); > + bpf "\n"; > + > + (match os with > + | RHEL (3, _) -> () > + | _ -> > + bpf "\ > +# Halt the system once configuration has finished. > +poweroff > +"; > + ); rhel.sh has: if [ $major -ge 4 ]; then so I think the check should rather be: | RHEL (ver, _) when ver >= 4 -> (Same note above.)

> + bpf "\n"; > + > + bpf "\ > +%%packages > +@core > +"; > + > + (match os with > + | RHEL ((3|4|5), _) -> () > + | _ -> > + bpf "%%end\n" > + ); rhel.sh has: if [ $major -ge 6 ]; then so I think the check should rather be: | RHEL (ver, _) when ver >= 6 -> (Same note above.)

> +and copy_preseed_to_temporary source = > + (* d-i only works if the file is literally called "/preseed.cfg" *) > + let d = "/tmp" // random8 () ^ ".tmp" in Filename.temp_dir_name instead of /tmp here.

> +and make_location os arch = > + match os, arch with > + | CentOS (major, _), Aarch64 -> > + (* XXX This always points to the latest CentOS, so > + * effectively the minor number is always ignored. > + *) > + sprintf "http://mirror.centos.org/altarch/%d/os/aarch64/" major > + > + | CentOS (major, _), X86_64 -> > + (* For 6.x we rebuild this every time there is a new 6.x release, and bump > + * the revision in the index. > + * For 7.x this always points to the latest CentOS, so > + * effectively the minor number is always ignored. > + *) > + sprintf "http://mirror.centos.org/centos-7/%d/os/x86_64/" major > + > + | Debian (_, dist), X86_64 -> > + sprintf "http://ftp.uk.debian.org/debian/dists/%s/main/installer-amd64" > + dist While we are here, should we use either httpredir.debian.org (but IIRC it's deprecated), or deb.debian.org (works best with current testing, but should still work also with current stable and older). > + | _ -> > + eprintf "%s: don't know how to calculate the --location for this OS and architecture\n" prog; > + exit 1 Ditto as above (i.e. print details of unknown distro/version).

> +and make_index_fragment os arch index_fragment output nvram revision expandfs = > + let chan = open_out (index_fragment ^ ".new") in > + let fpf fs = fprintf chan fs in > + > + fpf "[%s]\n" (string_of_os_noarch os); > + fpf "name=%s\n" (long_name_of_os os arch); > + fpf "osinfo=%s\n" (true_os_variant_of_os os); > + fpf "arch=%s\n" (string_of_arch arch); > + fpf "file=%s\n" output; > + (match revision with > + | None -> () > + | Some i -> fpf "revision=%d\n" i > + ); > + fpf "checksum[sha512]=%s\n" (sha512sum_of_file output); > + fpf "format=raw\n"; > + fpf "size=%d\n" (virtual_size_gb * 1024 * 1024 * 1024); Shouldn't this be Int64?

> +and open_guest filename = > + let g = new Guestfs.guestfs () in > + g#add_drive_opts ~format:"raw" filename; > + g#launch (); > + > + let roots = g#inspect_os () in > + if Array.length roots = 0 then ( > + eprintf "%s: cannot inspect this guest\n" prog; > + exit 1 > + ); > + let root = roots.(0) in > + > + let mps = g#inspect_get_mountpoints root in > + let cmp (a,_) (b,_) = compare (String.length a) (String.length b) in > + let mps = List.sort cmp mps in > + List.iter ( > + fun (mp, dev) -> > + try g#mount_ro dev mp > + with Guestfs.Error msg -> eprintf "%s (ignored)\n" msg Should mount errors be ignored in this case?