[Merge] ~gjolly/livecd-rootfs:ubuntu-cpc/boot_partition into livecd-rootfs:ubuntu/master
Steve Langasek
mp+448592 at code.launchpad.net
Wed Sep 6 18:06:39 UTC 2023
Review: Needs Fixing
Minor point, but I don't think you should regress the comprehensibility of the code around setting image size limits
Diff comments:
> diff --git a/live-build/ubuntu-cpc/hooks.d/base/disk-image-uefi.binary b/live-build/ubuntu-cpc/hooks.d/base/disk-image-uefi.binary
> index 020dd33..c687b0b 100755
> --- a/live-build/ubuntu-cpc/hooks.d/base/disk-image-uefi.binary
> +++ b/live-build/ubuntu-cpc/hooks.d/base/disk-image-uefi.binary
> @@ -12,17 +12,14 @@ esac
> IMAGE_STR="# CLOUD_IMG: This file was created/modified by the Cloud Image build process"
> FS_LABEL="cloudimg-rootfs"
>
> -if [ "$ARCH" = "amd64" ]; then
> - IMAGE_SIZE=3758096384 # bump to 3.5G (3584*1024**2); Since Kinetic amd64 need more then the default 2.2G
> -fi
> -
> -if [ "$ARCH" = "armhf" ]; then
> - IMAGE_SIZE=3758096384 # bump to 3.5G (3584*1024**2); Since Jammy armhf need more then the default 2.2G
> -fi
> -
> -if [ "$ARCH" = "riscv64" ]; then
> - IMAGE_SIZE=4831838208 # bump to 4.5G (4608*1024**2); initrd creation fails with "No space left" with 3.5G
> -fi
> +case "$ARCH" in
> + amd64|arm64|armhf)
> + IMAGE_SIZE=3758096384
some sort of annotation here about the meaning of this number would be helpful, for those of us that don't map such numbers from decimal to binary easily in our heads
or even just doing the math inline with $(( )) instead? $((3*1024*1024*1024+512*1024*1024))
> + ;;
> + riscv64)
> + IMAGE_SIZE=4831838208
> + ;;
> +esac
>
> . config/binary
>
> @@ -60,15 +63,26 @@ create_partitions() {
> --print
> }
>
> -create_and_mount_uefi_partition() {
> +create_and_mount_boot_partitions() {
> uefi_dev="${loop_device}p15"
> + boot_dev="${loop_device}p16"
> mountpoint="$1"
> +
> mkfs.vfat -F 32 -n UEFI "${uefi_dev}"
> + mkfs.ext4 -L BOOT "${boot_dev}"
> +
> + # copying what was on the rootfs to the new boot partition
> + mount "${boot_dev}" "${mountpoint}"/mnt
> + mv "${mountpoint}"/boot/* "${mountpoint}"/mnt
> + umount "${boot_dev}"
> +
> + mount "${boot_dev}" "${mountpoint}"/boot
>
> mkdir -p "${mountpoint}"/boot/efi
> mount "${uefi_dev}" "$mountpoint"/boot/efi
>
> cat << EOF >> "mountpoint/etc/fstab"
> +LABEL=BOOT /boot ext4 defaults 0 2
I'm vaguely surprised to see mount by LABEL here, both for the ESP as well as the new /boot partition. In non-cloud installs, we mount by UUID. That gives more reliably-correct results for the case that the disk for one instance gets attached to another instance for debugging/recovery?
> LABEL=UEFI /boot/efi vfat umask=0077 0 1
> EOF
> }
--
https://code.launchpad.net/~gjolly/livecd-rootfs/+git/livecd-rootfs/+merge/448592
Your team Ubuntu Core Development Team is subscribed to branch livecd-rootfs:ubuntu/master.
More information about the Ubuntu-reviews
mailing list