Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactoring --vmefi --arch arm64 #258

Open
adrelanos opened this issue Dec 8, 2023 · 3 comments
Open

refactoring --vmefi --arch arm64 #258

adrelanos opened this issue Dec 8, 2023 · 3 comments

Comments

@adrelanos
Copy link
Contributor

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.

    1. Use the same labels. EFI and ESP are currently labels.
    1. Use the same sizes.
    1. Use 'mkpart bios_grub 101MiB 102MiB' and 'set 2 bios_grub on' also for --arm64 if that works. Not sure if there are any ARM64 legacy BIOS booting systems,

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
Copy link
Contributor Author

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

  • grml-debootstrap currently relies on packages(-arm64) file to install package grub2-common.
  • packages-arm64 file currently contains grub2-common, grub-efi-arm64 but not grub-efi-arm64-signed.
  • grml-debootstrap installs grub-efi-arm64-bin, grub-efi-arm64-signed but not grub2-common. This breaks builds with a custom packages-arm64 not including grub2-common.
  • grml-debootstrap currently only checks if grub-efi-arm64-signed needs installation and then installs grub-efi-arm64-bin and grub-efi-arm64-signed. However, if a packages file had only grub-efi-arm64-signed but not grub-efi-arm64-bin, that would also not work. Might see as a nitpick but I want to find a nice, clean design because that might allow to support all architectures supported by grub. (At least theoretically. Testing these might need later CI enhancements adding additional tests if there is any interest.)

These inconsistencies should be fixed.

What should it be? Options:

  • A) each architecture needs its own distinct packages file which contains architecture specific packages such as bootloader packages. Or,
  • B) packages does not need to list these but then grml-debootstrap will auto-detect and auto-install all that is required.
  • C) Either. No preference.
  • D) Other?

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 #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
Copy link
Contributor Author

  • Package grub-efi-arm64-bin is also being installed too late by grml-debootstrap. The following...
  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.

  • I also wonder why if [ -n "$ARM_EFI_TARGET" ]; then needs a special case.

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.

  • And then also chroot-script also conditionally does grub-install.

@mika
Copy link
Member

mika commented Dec 22, 2023

@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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants