grml / grml-debootstrap

wrapper around debootstrap
59 stars 27 forks source link

Initial arm64 support #191

Closed GavinPacini closed 10 months ago

GavinPacini commented 2 years ago

Hi there, I finally decided to tackle #169, about time. :)

This is an initial implementation. I've tested it with:

sudo UPGRADE_SYSTEM='no' ./grml-debootstrap \
            --debopt "--verbose" \
            --arch arm64 \
            --filesystem ext4 \
            --hostname host \
            --nopassword \
            --nopackages \
            --release bullseye \
            --keep_src_list \
            --verbose \
            --vmfile \
            --vmsize 15G \
            --target ../debian-grml.raw

and I've been able to boot this image with qemu on an Apple Silicon based Mac (through UTM).

Trouble is that I cannot really make sure I didn't break the amd64 builds, albeit I made every effort not to.

I plan to test it a bit more but would appreciate an initial review at this stage. Thanks!

mika commented 2 years ago

Hi @GavinPacini oh wow, kudos! :partying_face: I'm currently quite busy and won't have time to review it right now, but on first glance it looks rather reasonable, will try to go through it and give it a try ASAP! :)

GavinPacini commented 2 years ago

Thanks @mika! No problem, appreciate any feedback once you have the chance.

mika commented 2 years ago

@GavinPacini finally had some time to take a look at it. Overall the PR looks good to me, there are some minor details we could improve, but let's polish this once we know it's working as expected. :)

I tried installing it into a VM, running:

CONFFILES=$(pwd) ./grml-debootstrap --vm --vmfile --target /dev/shm/debian-arm64.img -r bullseye --arch arm64 --password grml

But this failed for me with:

However the following packages replace it:
  grub2-common grub-common

E: Package 'grub-pc' has no installation candidate

This is related to the usage of --nopackages (adding that option then built the VM as expected), so I suppose we need to further adjust chroot-script to properly handle the grub situation also for arm64? :)

GavinPacini commented 2 years ago

Thanks for reviewing @mika!

I just reproduced that myself, thanks for bringing it up. After some digging, grub-pc seems to be the standard way to setup MBR / BIOS systems. There is no real support for MBR / BIOS on arm64 architectures (they use UEFI only). For this reason, we use grub2-common and grub-efi-arm64 packages (as shown in the diff here).

Basically, arm64 images should never have grub-pc installed. However grub-pc is in the packages file. Once I removed this from the packages file it created the VM fine. I see three options:

  1. Create a separate packages file for arm64 images (e.g. packages-arm64) which we can switch to based on the --arch flag.
  2. Add some flags into the packages file which allows us to denote if a package should be skipped for certain architectures.
  3. Hardcode in ignoring grub-pc for arm64 images.

I would probably pick number 1 as the most future proof, but there would be a lot of duplication for now.

What do you think?

adrelanos commented 2 years ago

1. seems also the most reliable, least surprising to me. 2. seems rather complex to implement, not very intuitive. 3. would be the most surprising.

So From the three options, I like 1. most.

fosple commented 1 year ago

@GavinPacini Thanks for looking into it. Are you still on it? 😊

adrelanos commented 1 year ago

Which of these options would be acceptable? @mika

GavinPacini commented 1 year ago

Hey @fosple, once @mika provides some direction on the preferred options mentioned above, I am happy to finish it out.

mika commented 1 year ago

Sorry for the delay, I also think that the first option (Create a separate packages file for arm64 images (e.g. packages-arm64) which we can switch to based on the --arch flag.) seems preferable given our current situation!

GavinPacini commented 1 year ago

Hi again, I've just pushed something that I think solves the outstanding issues. @mika I would appreciate another review when you have a chance. Thanks!

mika commented 1 year ago

@GavinPacini great, thanks! Your changes look good to me on a quick sight, though I need a more quiet time to give it a more detailed look. Also in Debian we're in freeze stage for bookworm (see https://release.debian.org/testing/freeze_policy.html), so it might make sense to apply those changes once we can upload towards Debian/unstable with changes like new features again. :)

GavinPacini commented 11 months ago

@mika Hey, just wondering what else is needed to get this merged in? I just want to make sure we don't miss the next release window. Thanks!

mika commented 11 months ago

@GavinPacini I'll try to look into this in the next few days to get this merged ASAP! Thanks for the ping

mika commented 11 months ago

@GavinPacini let's try to get this merged before more changes pile up and cause further merge conflicts :) I took your PR and tried to get it ready for submission, please see https://github.com/grml/grml-debootstrap/pull/214 - feel free to take it over and force-push it here or so - all credits should be toally yours of course! :) (When reviewing, ignore all the indention related changes, e.g. via git show --ignore-all-space on the cmdline)

Disclaimer: not yet tested/verified on my side. BTW, what's your environment to test this? An actual arm64 system, or a VM?

I've only two things in mind now:

1) why do we skip the Adjusting grub.cfg for successful boot sequence. in the arm64 situation? (Only trying to understand the situation we're in) 2) now also having EFI support for VMs (see f4d3906aae588d8dac1a146f05151211073cbb80), maybe there is something we could do to simplify code and unify code, see e.g.:

parted -s "${TARGET}" 'mkpart ESP fat32 1MiB 101MiB'

vs

parted -s "${TARGET}" 'mkpart EFI fat32 1MiB 10MiB'

But perfect is the enemy of good, I think we can further improve/iterate of anyway, though I'd appreciate your feedback before submitting this :)

adrelanos commented 11 months ago

I also think it's better to merge imperfect stuff fast for the purpose of avoiding merge conflicts.

You can withhold it from user documentation and/or label it an alpha/unsupported feature meanwhile if not (yet) tested. Since this is primarily a sysadmin / developer tool, the facts can just be documented and/or mentioned by the scripts. By facts I mean mentioning "community supported architecture, untested".

With increasing platform support it won't be possible for the main developer(s) to keep all tested. If github actions supported arm64 (not at time of writing but planned according to the tickets), we could use that. Perhaps in the future.

mika commented 11 months ago

FYI: if anyone of you can give https://github.com/grml/grml-debootstrap/pull/214 a try and let me know whether this works for you, I'd submit it ASAP.

adrelanos commented 11 months ago

I tested it but a bit too slow. Before today's rebase.

I am not an expert for qemu but this is my quick way to test boot it.

#!/bin/bash

set -x
set -e

## We might get away with --no-install-recommends, fewer packages.
sudo apt-get install qemu-system-arm qemu-system-aarch64 qemu-efi-aarch64 qemu-utils

qemu-system-aarch64 \
    -M virt \
    -cpu cortex-a57 \
    -m 1024 \
    -bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd \
    -drive format=raw,file=/home/user/grml-debootstraptestbin/test.img \
    -nographic

Happy to report that build + boot was success.

adrelanos commented 11 months ago

Continued testing here: https://github.com/grml/grml-debootstrap/pull/214#issuecomment-1771021365

mika commented 10 months ago

@GavinPacini we took care of your PR via https://github.com/grml/grml-debootstrap/pull/214, thanks for your work!