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

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



El 08/04/19 a las 21:29, Matthijs Kooijman escribió:
> Hi Luca & others,
> 
> I've been working on syslinux-efi support in the past weeks and just
> submitted a MR with a working implementation, along with some small
> bootloader-related cleanups and refactors:
> 
> 	https://salsa.debian.org/live-team/live-build/merge_requests/19
Commenting below.
> 
> In the end, I opted to implement syslinux-efi rather than make grub-efi
> work on hdd images, since that seemed easier and allows preserving the
> existing bootloader config files in our project. Getting grub-efi to
> work on hdd images might still be an interesting project that could be
> implemented alongside syslinux-efi support, though I do not have any
> specific purpose for it as of yet.
I implemented multi bootloader support back in the day and maybe
grub-efi, not sure about that last one.

I didn't implement the hdd part because I didn't know what was the
purpose of an hdd image and how I was supposed to test it.

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.

What's your use case? What do you need to build live-build based hdd
imgs? I'm curious.

> 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.

> Gr.
> 
> Matthijs

> 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?

> 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.

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.

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.

> 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.

> 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.

3.2) BTW Do you support both EFI IA32 and EFI X64 or only EFI X64 ?

> 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.

>  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.

Else you are just echoing a warning but not stopping the build.

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
.

I know it's difficult to work on a new binary file because of the way
binary_syslinux handles kernel images and renames them.
But I was able to handle that on the loopback.cfg binary file.

Probably the common code for generating cfg files should be on a common
binary file which it's only triggered if either binary_syslinux or
binary_syslinux-efi is being used as a bootloader.

5.3) I see you add a lot of c32 and efi as symbolic links to paths on
the system.
I'm not sure this is the right way of doing this.
Are other equivalent files already implemented like you are doing?
Or is it me finding it out for the first time.

5.4)
In "Move syslinux modules into subdirectories (53901001)" you are using:
"path modules"
here you are using:
"PATH /@EFI_DIR@/modules32"
.

This should be using either "path" in both files or "PATH" in both
files. As the rest of the syslinux commands are in non capital letters I
suggest using "path".

6) Only default to grub-efi for iso* images (118de99a)

Seems good to me.

7) Improve bootloader configuration checks (5fb9ab31)

I need to read through these changes another day.
Was live-build to be based on python given a compatibility matrix based
on nested arrays... this should be easier to implement.

8) Remove --templates from lb_config manpage (371892af)

As I said in another mail this option makes more sense in another pull
request but I guess you can leave it in this pull request for now.

9) Use syslinux-efi for hdd images by default (4e292ed0)

Seems good to me.

10) I think buxy implemented syslinux-efi support back in the day. I
guess it wasn't added to live-build because of reasons? Because grub-efi
enabled Secure Boot to work?

Anyways... Did you ever check buxy's work just in case it has something
that you can recycle?



I'll try to comment on "Improve bootloader configuration checks
(5fb9ab31)" on the next days. I need to be focused on this one :) .


Waiting for your feedback on the rest of the points. Hopefully we can
agree on the binary_syslinux-efi creation and other points :) .




adrian15
-- 
Support free software. Donate to Super Grub Disk. Apoya el software
libre. Dona a Super Grub Disk. http://www.supergrubdisk.org/donate/


Reply to: