Hi Adrian,
thanks for your extensive review. I'll respond inline.
> I might take a look into your notes to implement grub-efi + secure boot
> in hdd img but... this might be in 2021 XD . Too busy at the moment.
Familiar sentiment. Would be nice to have it, though.
> What's your use case? What do you need to build live-build based hdd
> imgs? I'm curious.
I'm using live-build to build Webconverger (a kiosk OS booting into a
full-screen browser) and an image derived for that for Bizplay (a
digital signage system).
We need hdd images rather than isohybrid images, so the resulting
USB-disk can be written to. This is needed firstly to update the
bootloader configuration to change the kernel commandline (we use this
to customize the image, e.g. change the default homepage shown).
Secondly, the image uses git to manage the root filesystem and a running
image can update itself, but that again needs a writable filesystem.
> > As predicted by others in this bug report, my work does not enable
> > secure boot (which syslinux simply does not support), nor enable
> > syslinux-efi support in iso/isohybrid images (since syslinux-efi does
> > not support this, or at least it apparently does not work).
>
> Nice. What you need to make sure is that you cannot choose syslinux-efi
> when trying to build iso/isohybrid images.
Yeah, that's checked by the "Improve bootloader configuration checks"
commit.
> > 0) backward incompatible change
> In the pull request there is a mention about a backward incompatible
> change. Can you please describe in what that change consists?
I've added one commit with a NEWS file that describes this issue in a
bit more detail. Does that clarify sufficiently?
Note that while writing the NEWS entry, it seemed more confusing to have
a bootloaders/syslinux-common directory as well as a syslinux-common
Debian package, so I ended up modifying my commits to use
"syslinux-shared" rather than "syslinux-common" (I'll also use the
former below).
> > 1) Refactor some bootloader scripts (969cdf4e)
>
> 1.1) Nice idea the Get_Bootloader_Config_Dir function.
> Have you made sure that $BOOTLOADER variable is not used anywhere else
> in the code?
> Maybe you should make that variable local.
Good point. I guess I was following the style of other functions (that
dod not use local variables either), but that's no reason not to do it
here. I updated the PR with this change.
> 1.2) The function Install_Bootloader_Files does not seem to be right.
> If you are not going to reuse that function in more than one place (I'll
> check your other commits later) you might consider not having that
> function in the first place.
I am indeed planning to reuse the function. I've clarified this in the
commit message.
> Even if the Install_Bootloader_Files function was a good idea it's not
> well implemented commit-wise. You are performing two changes. One about
> moving the functionality to a function. Another one is chaning the
> comment about "two steps" to "three steps". It's hard to notice if you
> changed something other than that comment on that commit.
>
> I mean... this part should be splitted in two commits.
I'm usually a nitpick in this area, but here I decided for a bit more
simplicity. You're right, though. I ended up splitting this into three
commits, one for Get_Bootloader_Config_Dir, one for
Install_Bootloader_Files and one for the comment.
> > 2) Reduce config duplication in syslinux variants (bccd127b)
>
> I know it's better the way you merge files there but I think the
> original idea was about having independent configuration files for each
> different media.
>
> I mean there were not meant to be merged together in the first place.
The reason to do this, is that I want configuration files to be shared
between syslinux and syslinux-efi (which can co-exist in the same
image). If these would be duplicated everywhere, modifying the
bootloader config after an image is written to disk requires modifying
it in two places.
Note that syslinux-efi could still not have its own config files, but
refer to files contained in bootloaders/syslinux (without the need for
bootloaders/syslinux-shared), but then you can never have an image that
*only* contains syslinux-efi (which you can with the syslinux-shared
approach, since then you use syslinux-shared for the config and
syslinux-efi for the efi images).
> > 3) Remove ldlinux.c32 for extlinux and syslinux (6fc68c1d)
>
> 3.1) Nice. I didn't know about those new syslinux architecture dependent
> files. As per the wiki you link (
> https://wiki.syslinux.org/wiki/index.php?title=Library_modules#All_Syslinux_variants_need_an_additional_ldlinux_module
> ) in the commit description I guess that even ldlinux.c32 wouldn't be
> used but ldlinux.e64 instead.
Actually, ldlinux.e64 is only for EFI. This commit only touches
BIOS-related stuff. I'm not sure how "architecture dependent files" are
relevant here, since this commit just removes some files which are
really superfluous (since the 'syslinux' and 'extlinux' commands used to
install the bootloader in binary_hdd make sure to copy their own version
of these files into the image).
> 3.2) BTW Do you support both EFI IA32 and EFI X64 or only EFI X64 ?
Yes, but that's a later commit.
> > 4) Move syslinux modules into subdirectories (53901001)
>
> Shouldn't those c32 modules be moved to a folder named c32-modules (or
> c32mods for short name) instead?
>
> Let's see how you name the UEFI modules folder later.
>
> In "Add syslinux-efi bootloader support (a271f8f9)" you use modules32
> folder name for some c32. Shouldn't you use modules32 as a folder name
> in this "Remove ldlinux.c32 for extlinux and syslinux (6fc68c1d)" commit
> in the first place?
>
> I'm not sure about this one I might be missing something.
I think the missing bit might be that
bootloaders/{syslinux,syslinux-shared} is installed into binary/boot,
while bootloaders/syslinux-efi is installed into binary/EFI/boot, so
these are distinct locations.
Also, EFI supports 32 and 64 bit (hence modules32 and modules64), but
BIOS is (by definition, I think) only 32-bit, so I just used "modules".
> > 5) Add syslinux-efi bootloader support (a271f8f9)
> 5.1) You have got a nice bug on that code XD . Just after the Echo_error
> statements about syslinux-efi not being supported there should be an
> exit 1 statement.
Thanks, fixed.
> 5.2) Anyway I don't think I like this code at all. I mean... you are
> supposed to create a new file named:
> scripts/build/binary_syslinux-efi
> and not hack around the existant one: binary_syslinux
That would make sense if I wanted syslinux-efi to be really indepenent,
but as I noted above, I wanted to make them share a single configuration
(and also allow syslinux-efi to be installed by itself). This seemed
like best way to achieve that, though alternative suggestions are
welcome :-)
I pushed a new version with the above changes to
https://salsa.debian.org/live-team/live-build/merge_requests/19
Gr.
Matthijs
Attachment:
signature.asc
Description: PGP signature