[Merge] ~davidkrauser/livecd-rootfs/+git/livecd-rootfs:hyperv-gallery-images into livecd-rootfs:ubuntu/master
Robert C Jennings
robert.jennings at canonical.com
Thu May 2 18:28:08 UTC 2019
Review: Needs Fixing
I need to drop in the comments that I have so far. I'll come back to review the first part of the code that requires me to look at commits with greater context than in the MP view and review the later hooks that I haven't gotten to yet.
Diff comments:
> diff --git a/live-build/auto/build b/live-build/auto/build
> index 8238b8b..1521665 100755
> --- a/live-build/auto/build
> +++ b/live-build/auto/build
> @@ -109,7 +109,7 @@ Expire-Date: 0
> lb bootstrap "$@"
>
> case $PROJECT in
> - ubuntu-server|ubuntu-cpc)
> + ubuntu-server|ubuntu-cpc|ubuntu)
This is going to generate conversation (or should) as we should do as the message says and apply this for everyone. That wouldn't be something I'd consider in the scope of your change, but your change here effects a set of images beyond desktop-preinstalled so it will need to be resolved. Either it's acceptable for all images (or at least all images under the Ubuntu project) or you need to alter the scope to filter on $PROJECT:$SUBPROJECT and restrict to desktop-preinstalled (which may still be too broad).
> # Set locale to C.UTF-8 by default. We should
> # probably do this for all images early in the
> # 18.10 cycle but for now just do it for
> diff --git a/live-build/ubuntu/hooks/033-disk-image-uefi.binary b/live-build/ubuntu/hooks/033-disk-image-uefi.binary
> new file mode 100644
> index 0000000..3aad890
> --- /dev/null
> +++ b/live-build/ubuntu/hooks/033-disk-image-uefi.binary
> @@ -0,0 +1,186 @@
> +#!/bin/bash -eux
> +
> +case $ARCH in
> + amd64)
> + ;;
> + *)
> + echo "We don't create EFI images for $ARCH."
> + exit 0
> + ;;
> +esac
> +
> +IMAGE_STR="# Ubuntu Desktop"
> +FS_LABEL="desktop-rootfs"
> +IMAGE_SIZE=11806965760
I don't like *magic* numbers like these without explanation why that size is required/desired. Otherwise it makes it difficult to know if we can later change it. That comment should mention if any consideration is taken for alignment/rounding to a particular size which would need to be maintained. Also, units.
And just to pile on as many comments as I can for a single line of code... I see the same value is used in 040-hyperv-desktop-images.binary. Can it be common? Does it need to be the same in both places?
> +
> +. config/binary
> +
> +. config/functions
> +
> +create_partitions() {
A lot of what we have here in this function, and the few that follow, feels familiar w.r.t. live-build/functions. I haven't looked yet to see if it's duplicate (remove it), slightly different (enhance common code to deal with differences), or unique (think about putting it in live-build/functions for others) but it doesn't feel special enough to have in a single binary hook.
> + disk_image="$1"
> + sgdisk "${disk_image}" --zap-all
> + case $ARCH in
> + arm64|armhf)
> + sgdisk "${disk_image}" \
> + --new=15:0:204800 \
> + --typecode=15:ef00 \
> + --new=1:
> + ;;
> + amd64)
> + sgdisk "${disk_image}" \
> + --new=14::+4M \
> + --new=15::+106M \
> + --new=1::
> + sgdisk "${disk_image}" \
> + -t 14:ef02 \
> + -t 15:ef00
Same comment here on magic numbers. If there's anything important here in the size/alignment they need to be documented.
> + ;;
> + esac
> + sgdisk "${disk_image}" \
> + --print
> +}
> +
> +create_and_mount_uefi_partition() {
> + uefi_dev="/dev/mapper${loop_device///dev/}p15"
> + mountpoint="$1"
> + mkfs.vfat -F 32 -n UEFI "${uefi_dev}"
For a small partition like the UEFI partition why is an override for a 32-bit FAT size needed?
> +
> + mkdir -p "${mountpoint}"/boot/efi
> + mount "${uefi_dev}" "$mountpoint"/boot/efi
> + #efipartuuid=$(blkid -s PARTUUID -o value "$uefi_dev")
This comment can be dropped if you really don't need the UUID. Have you tested boot of a minimized image? You have code in install_grub() below to handle the minimized subproject and there we need to boot with UUIDs rather than labels due to the lack of an initramfs, otherwise the user will see a boot, panic, and reboot with initrd on each boot of the system.
> +
> + cat << EOF >> "mountpoint/etc/fstab"
> +LABEL=UEFI /boot/efi vfat defaults 0 0
> +EOF
> +}
> +
> +install_grub() {
> + mkdir mountpoint
> + mount_partition "${rootfs_dev_mapper}" mountpoint
> +
> + create_and_mount_uefi_partition mountpoint
> +
> + echo "(hd0) ${loop_device}" > mountpoint/tmp/device.map
> + mkdir -p mountpoint/etc/default/grub.d
> + efi_boot_dir="/boot/efi/EFI/BOOT"
> + chroot mountpoint mkdir -p "${efi_boot_dir}"
> +
> + if [ "${SUBPROJECT:-}" = minimized ] && [ -n "$partuuid" ]; then
> + echo "partuuid found for root device; omitting initrd"
> + echo "GRUB_FORCE_PARTUUID=$partuuid" >> mountpoint/etc/default/grub.d/40-force-partuuid.cfg
> + fi
> +
> + chroot mountpoint apt-get -y update
> +
> + # UEFI GRUB modules are meant to be used equally by Secure Boot and
> + # non-Secure Boot systems. If you need an extra module not already
> + # provided or run into "Secure Boot policy forbids loading X" problems,
> + # please file a bug against grub2 to include the affected module.
> + case $ARCH in
> + arm64)
> + chroot mountpoint apt-get -qqy install --no-install-recommends grub-efi-arm64 grub-efi-arm64-bin
--no-install-recommends is counter to policy for image builds. If it's required then there should be a comment to call out the rationale and maybe a bug if appropriate to changed recommends to suggests.
> + efi_target=arm64-efi
> + ;;
> + armhf)
> + chroot mountpoint apt-get -qqy install --no-install-recommends grub-efi-arm grub-efi-arm-bin
> + efi_target=arm-efi
> + ;;
> + amd64)
> + chroot mountpoint apt-get install -qqy grub-efi-amd64-signed grub-efi-amd64 shim-signed
> + efi_target=x86_64-efi
> + ;;
> + esac
> +
> + chroot mountpoint grub-install "${loop_device}" \
> + --boot-directory=/boot \
> + --efi-directory=/boot/efi \
> + --target=${efi_target} \
> + --removable \
> + --uefi-secure-boot \
> + --no-nvram
> +
> + if [ -f mountpoint/boot/efi/EFI/BOOT/grub.cfg ]; then
> + sed -i "s| root| root hd0,gpt1|" mountpoint/boot/efi/EFI/BOOT/grub.cfg
> + sed -i "1i${IMAGE_STR}" mountpoint/boot/efi/EFI/BOOT/grub.cfg
> + # For some reason the grub disk is looking for /boot/grub/grub.cfg on
> + # part 15....
This sounds like a bug that needs to be filed (and then documented in the commend with an "(LP: #xxxxxxx)"
> + chroot mountpoint mkdir -p /boot/efi/boot/grub
> + chroot mountpoint cp /boot/efi/EFI/BOOT/grub.cfg /boot/efi/boot/grub
> + fi
> +
> + if [ "$ARCH" = "amd64" ]; then
> + # Install the BIOS/GPT bits. Since GPT boots from the ESP partition,
> + # it means that we just run this simple command and we're done
> + chroot mountpoint grub-install --target=i386-pc "${loop_device}"
> + fi
> +
> + divert_grub mountpoint
> +
> + chroot mountpoint rm /usr/sbin/grub-probe
> + chroot mountpoint dpkg-divert --remove --local \
> + --rename /usr/sbin/grub-probe
> +
> + # update grub.cfg again, make sure this image has fs-uuid.
> + chroot mountpoint update-grub
> +
> + chroot mountpoint dpkg-divert --local \
> + --rename /usr/sbin/grub-probe
> + chroot mountpoint touch /usr/sbin/grub-probe
> + chroot mountpoint chmod +x /usr/sbin/grub-probe
> +
> + replace_grub_root_with_label mountpoint
> +
> + undivert_grub mountpoint
> +
> + chroot mountpoint apt-get -y clean
> +
> + rm mountpoint/tmp/device.map
> + umount mountpoint/boot/efi
> + umount_partition mountpoint
> + rmdir mountpoint
> +}
> +
> +disk_image=binary/boot/disk-uefi.ext4
> +
> +create_empty_disk_image "${disk_image}"
> +create_partitions "${disk_image}"
> +mount_image "${disk_image}" 1
> +
> +partuuid=$(blkid -s PARTUUID -o value "$rootfs_dev_mapper")
> +
> +# Copy the chroot in to the disk
> +make_ext4_partition "${rootfs_dev_mapper}"
> +mkdir mountpoint
> +mount "${rootfs_dev_mapper}" mountpoint
> +cp -a chroot/* mountpoint/
> +setup_mountpoint mountpoint
> +
> +# Disable UUID so we find root by label
> +sed -i "s|#GRUB_DISABLE_LINUX_UUID|GRUB_DISABLE_LINUX_UUID|" mountpoint/etc/default/grub
> +
> +# Add a swap file
Generally we don't have swap files in virtual images because it performs very poorly, so a comment here why we want it here and a comment on why we'd want a 1GiB swap file on disk. Or maybe we don't really want it, what is the effect of removing it?
> +dd if=/dev/zero of=mountpoint/swapfile bs=1024 count=1048576
> +chmod 0600 mountpoint/swapfile
> +mkswap mountpoint/swapfile
> +
> +# Edit fstab in the mounted disk
> +cat > "mountpoint/etc/fstab" << EOF
> +# <file system> <mount point> <type> <options> <dump> <pass>
If we "edit" a file (not the right word in the comment either, we're replacing it) we would drop the ${IMG_STR} in the first line to comment that this was altered by the image build process. It's helpful for users and support as this is a delta from what you get in normal installation.
> +LABEL=${fs_label} / ext4 errors=remount-ro 0 1
> +/swapfile none swap sw 0 0
> +EOF
> +
> +# Don't run gnome-initial-setup from gdm
> +sed -i "s|#WaylandEnable=false|#WaylandEnable=false\nInitialSetupEnable=false|" mountpoint/etc/gdm3/custom.conf
> +
> +chroot mountpoint /usr/sbin/useradd -d /home/oem -m -N -u 29999 oem
> +chroot mountpoint /usr/sbin/oem-config-prepare --quiet
> +touch mountpoint/var/lib/oem-config/run
> +umount_partition mountpoint
> +rmdir mountpoint
> +
> +install_grub
> +
> +clean_loops
> +trap - EXIT
--
https://code.launchpad.net/~davidkrauser/livecd-rootfs/+git/livecd-rootfs/+merge/366849
Your team Ubuntu Core Development Team is subscribed to branch livecd-rootfs:ubuntu/master.
More information about the Ubuntu-reviews
mailing list