[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#922251: live-build: support syslinux-efi as (additional) bootloader

Hi Adrian,

thanks again for your thorough review. I'm responding to both your
e-mails inline below.

> >>> 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).
> I'm revisiting this commit and I'm not sure this is right thing to do.
> If your pull request only affects syslinux-efi why do you even care
> about 32bit code?
True, this was just a cleanup I came across. It is sort of related due
to the next commit that moves all c32 modules into a subdirectory. I
wondered whether to also move ldlinux.c32 and whether leaving it would
cause problems with the CWD change in the EFI boot. I then concluded the
file was not used at all and just sidestepped the issue by removing it

I did one more test and noted that the existence of these files do not
cause problems for syslinux-efi. I'm not sure it it makes sense to keep
them or not, but dropping this commit from this MR for now seems fine
with me.

> > 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".
> Well, I think you should use something else.
> modules32 is 9 characters long (not 8.3 compatible). What about module32
> and module64? So that we can reuse the code in the iso side when
> isolinux is updated to support hybrid isos in efi mode ?
Good point, hadn't considered 8.3 compatibility. Singular "module32"
sounds a bit weird to me, but it is probably clearer than something like
"mods32" or even just "c32" (the latter seems reasonable, except that
the "c64" directory would then still contain ".c32" files since that's
what 64-bit syslinux-efi also uses...).

> >> 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 :-)
> Well, you could have a binary_syslinux_cfg file similar to the
> binary_loopback_cfg one or maybe binary_shared_cfg.
That would indeed make some sense. I had not really considered this
before, but did now. The problem with this approach is that both
binary_syslinux_cfg, binary_syslinux and binary_syslinux-efi would need
to duplicate code. At least they all need Install_Bootloader_Files,
which could be slightly generalized and moved into functions.

There is also a bunch of code which post-processes .cfg and splash.svg
files. This would be mostly needed in binary_syslinux_cfg only (since
the syslinux/syslinux-efi only contains a minimal config file that
includes the shared config file and needs minimal post-processing).

However, if binary_syslinux_cfg would install syslinux-shared and then
do all the post-processing, followed by binary_syslinux and/or
binary_syslinux-efi without any post-processing, you would:
 - lose the ability to override some the shared config files with
   bootloader-specific (e.g. syslinux/isolinux/extlinux)
 - lose backward-compatibility with existing live-build configs that do
   not have a syslinux-shared config directory but have all their config
   in the bootloader-specific directory (which is installed *after*
   configs are post-processed).

This can probably be fixed by further splitting the cfg step into an
install and post-processing step. You would then get:
 - binary_syslinux_cfg that installs config files verbatim (runs if any
   of syslinux/extlinux/isolinux/syslinux-efi is selected).
 - binary_syslinux that installs the modules and main config file for
   syslinux/isolinux/extlinux (runs if any of syslinux/extlinux/isolinux
   is selected, but not if only syslinux-efi is selected).
 - binary_syslinux-efi that installs the modules and main config file for
   syslinux-efi (runs if syslinux-efi is selected).
 - binary_syslinux_proces_cfg that post-processes all config files
   installed by previous steps (runs if any of
   syslinux/extlinux/isolinux/syslinux-efi is selected).

But I'm not so sure that this is really better than the current
single-script for everything approach (which essentially just does the
above four steps in a single script).

> 6.1)
> PATH syslinux command is still being written in capital letters in
> share/bootloaders/syslinux-efi/syslia32.cfg and
> share/bootloaders/syslinux-efi/syslx64.cfg while it should be written in
> non-capital letters.
Good catch, will fix that.

> 6.2) What you should learn about the multibootloader support is that it
> should let use as much as bootloaders as you want to.
> So, you could have something like:
> --bootloaders syslinux,syslinux-efi,grub-efi
> which, of course it does not make sense because either syslinux-efi and
> grub-efi would overwrite each ones code.
I see, and I think this still "works" with my MR.

> Anyway what I wanted to say here is that:
> --bootloaders syslinux,syslinux-efi
> and
> --bootloaders syslinux-efi,syslinux
> are not the same thing.
> The first one (regarding binary_syslinux) should install special MBR
> code so that BIOS can chainload into syslinx.
> The second one (regarding binary_syslinux) should NOT install special
> MBR code so that BIOS can chainload into syslinux. It should inhibit
> itself because it is not the first bootloader.
I'm not so convinved that this is how it should be. I guess the
complication comes from HDD images, as you mentioned. On ISOs you just
have multiple bootloader "slots" with one being the default. On a HDD
image, you sort of have 2 bootloader "slots": the BIOS MBR and the EFI
boot dir. As long as you select only one BIOS bootloader and one EFI
bootloader, they can (and probably should) nicely coexist. Once you
select more than one, they will likely overwrite each other, or perhaps
only the first one should take effect (the latter is how it works with
ISOs, I guess.).

I guess the interpretation of multiple bootloaders is currently
 - For iso images, all bootloaders are installed and the first one is
   made the default.
 - For hdd images before my MR, only the first bootloader is installed.
   Any extra bootloaders have their config files installed, but they are
   not installed into the MBR. EFI bootloaders are not supported for hdd
 - For hdd images with my MR applied, any BIOS bootloader is only
   installed if it is the first bootloader. Any EFI bootloader (which
   can only be syslinux-efi) is installed, regardless of its position.
   Multiple efi bootloaders would overwrite each other.

This might warrant an extra configuration check for hdd images perhaps,
that checks that no BIOS-bootloader appears in second or further
positions and only one EFI bootloader appears in the list? I did not add
anything like that, since there is no such check in place currently and
I did not want to further complicate the MR.

> Anyways... if we take a look at your untouched binary_hdd:
> https://salsa.debian.org/live-team/live-build/blob/00eab3a77f3da176f3f0aa807b886206f8f0f0f1/scripts/build/binary_hdd
> we can see that grub-pc is not supported. I guess I might add it in the
> future.
> But grub legacy is supported. (Well, it's not actually not supported.)
> Assuming grub legacy are available at buster which I'm not totally sure.
I actually think no grub is supported by hdd, the only grub code in
there is in a big FIXME:

But this is probably beside the point.

> Anyway... what I want to say is that you should be able to choose:
> --bootloaders grub,syslinux-efi
> or (if grub-pc was implemented there)
> --bootloaders grub-pc,syslinux-efi
> in a hdd target and 'syslinux' installation should be only triggered if
> it's the first bootloader.
This is what the current MR implements. In this case, the
"syslinux-shared" configuration is installed, which is referenced by the
"syslinux-efi" installation.

See https://salsa.debian.org/matthijs-guest/live-build/blob/syslinux-efi/scripts/build/binary_syslinux#L165-171

> Well, that's exactly how binary_hdd works right now... although the
> multi bootloader part should be improved to have something better than:
> https://salsa.debian.org/live-team/live-build/blob/00eab3a77f3da176f3f0aa807b886206f8f0f0f1/scripts/build/binary_hdd#L60-86
In what sense would you want to improve this? It's not very pretty that
binary_hdd has to know about each bootloader, but since bootloader
installation in most cases needs details about the image type and
bootloader, some crossover is going to be present anyway.

Or did you see something else to improve?

> All of this above is trying to improve multi bootloader support in the
> binary_hdd part of live-build. Not sure if you should deal with this
> prior to adding your code.
> But if you want to take a look and tell us if binary_hdd should be
> updated or not (in the end the efi installation is handled by
> binary_syslinux-efi or binary_grub-efi in the filesystem level and not
> by binary_hdd).
I do not think any updates to binary_hdd are needed for efi support (and
any improvements I can see to binary_hdd are mostly unrelated to efi
support as well). Would that answer your question?

> 6.3) Many of your commits seem to need a rebase into the current master
> branch. Well, that's to be expected.
Yeah, I usually rebase whenever I push an update.

> 9) Improve bootloader configuration checks (3e6645a2)
> Well, the way how defaults.sh checks if anything is valid is kind of
> antique.
> In my opinnion binary_syslinux, binary_syslinux-efi, binary_grub-pc and
> so on... should be able to be sourced and use sort of can_i_boot
> function where you input architectures, filesystem and if the bootloader
> is the first bootloader or not.
> Then the binary_syslinux (or whatever) replies.
That would certainly be better, but also out of scope for this PR (and
probably an intrusive change as well...).

I'll make the changes mentioned above later (no more time now), but feel
free to already respond to the above in the meanwhile :-).



Attachment: signature.asc
Description: PGP signature

Reply to: