grml / grml-debootstrap

wrapper around debootstrap
59 stars 27 forks source link

refactoring `--vmefi` `--arch arm64` #258

Open adrelanos opened 9 months ago

adrelanos commented 9 months ago

The following code should be refactored, simplified.

  if [ -n "$VMFILE" ]; then
    qemu-img create -f raw "${TARGET}" "${VMSIZE}"
  fi
  if [ -n "$VMEFI" ]; then
    parted -s "${TARGET}" 'mklabel gpt'
    parted -s "${TARGET}" 'mkpart ESP fat32 1MiB 101MiB'
    parted -s "${TARGET}" 'set 1 boot on'
    parted -s "${TARGET}" 'mkpart bios_grub 101MiB 102MiB'
    parted -s "${TARGET}" 'set 2 bios_grub on'
    parted -s "${TARGET}" 'mkpart primary ext4 102MiB 100%'

  else
    # arm64 support largely only exists for GPT
    if [ "$ARCH" = 'arm64' ]; then
      einfo "Setting up GPT partitions for arm64"
      parted -s "${TARGET}" 'mklabel gpt'
      parted -s "${TARGET}" 'mkpart EFI fat32 1MiB 10MiB'
      parted -s "${TARGET}" 'set 1 boot on'
      parted -s "${TARGET}" 'mkpart LINUX ext4 10MiB 100%'
    else
      parted -s "${TARGET}" 'mklabel msdos'
      if [ "$FIXED_DISK_IDENTIFIERS" = "yes" ] ; then
        einfo "Adjusting disk signature to a fixed (non-random) value"
        MBRTMPFILE=$(mktemp)
        dd if="${TARGET}" of="${MBRTMPFILE}" bs=512 count=1
        echo -en "\\x41\\x41\\x41\\x41" | dd of="${MBRTMPFILE}" conv=notrunc seek=440 bs=1
        dd if="${MBRTMPFILE}" of="${TARGET}" conv=notrunc
        eend $?
      fi
      parted -s "${TARGET}" 'mkpart primary ext4 4MiB 100%'
      parted -s "${TARGET}" 'set 1 boot on'
    fi
  fi

Maybe the VMEFI and ARM64 code paths could be merged.

but all of these changes are for code simplification purposes so this code and related code that comes later can be simplified.

Also maybe the grub installation code can be refactored but perhaps that can be discussed separately.

Why? This would fix or simplify the following issues:

adrelanos commented 9 months ago

Also the grub installation need refactoring. Not sure I should discuss this here or in a separate issue.

These inconsistencies should be fixed.

What should it be? Options:

A) has the advantage that it simplifies grml-debootstrap's code but more messy with lots of files, one per architecture. Then we should think about a shared packages file supplemented by architecture specific packages files. B) Might be possible with a switch, case code block setting different package to install and command line options. I think that might be better because options will be different anyhow depending on the architecture.

After https://github.com/grml/grml-debootstrap/pull/255 was merged, I'll attempt to send a PR but I need your on guidance on how a PR would need to look like.

adrelanos commented 9 months ago
  if [ -n "$ARM_EFI_TARGET" ]; then
    einfo "Installing Grub as bootloader into EFI."

    chroot "${MNTPOINT}" grub-install --target=arm64-efi --efi-directory=/boot/efi --bootloader-id=debian --recheck --no-nvram --removable

...runs before any package installation. That code only comes later below in the same function.

elif [[ -z "${GRUB}" ]] || ! dd if="${GRUB}" bs=512 count=1 2>/dev/null | cat -v | grep -Fq GRUB; then

High mental load. I would like to simplify that.

mika commented 8 months ago

@adrelanos I fully agree that we should refactor this, to unify and simplify behavior as much as possible while keeping code minimal and simple as well. I'll try to take care of #255 so you're not blocked by that :)