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 entirely. 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 image-type-specific: - 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 images. - 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: https://salsa.debian.org/live-team/live-build/blob/00eab3a77f3da176f3f0aa807b886206f8f0f0f1/scripts/build/binary_hdd#L282-311 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 :-). Gr. Matthijs
Attachment:
signature.asc
Description: PGP signature