[arch-releng] aif... syslinux

[Dieter Plaetinck]

On Fri, 11 Feb 2011 23:55:31 -0500
Matthew Gyurgyik <pyther at pyther.net> wrote:

> Hi Dieter,
> 
> https://github.com/pyther/aif/compare/master...syslinux-final
> 
> Would you be able to look over that code. I still have to test it and 
> probably fix up a few things. Figure you could at least let me know how 
> it looks and what suggestions you have.
> 
> Matthew

I see syslinux is still in extra.
- what did you discuss with the package maintainer? will it go to core? I
  don't see much response on https://bugs.archlinux.org/task/22234
- does the package come with all your requirements, like the script you wrote,
  etc?
- how will you make sure the user installs the right package? give him a hint
  during package selection?
  Right now, grub is in base, I think it should go out of base, as long as we
  have a clear message "be sure to select the bootloader you want: one of
  grub, syslinux"
  In fact, if you've ever have seen the list of warnings/errors after manually configuring filesystems, I would do the same for package selection.
For example we would make a rule that one (and not more) of grub/syslinux must be enabled during package selection, if that's not correct, we present the user the warning, strongly recommend him to go back and correct his mistake (optionally let him continue, maybe he really knows what he's doing)
Then, in the "install bootloader" we can just do a pacman query to see which bootloader the user installed. If we don't get one of grub/syslinux we give the user a warning "you have no bootloader installed, so make sure you do the right thing"
we shoud also mention that syslinux does not support all filesystems, and when user selects an FS for /boot or / that doesn't work with syslinux, we should warn him. (same thing for grub actually)


 grubmenu="/boot/grub/menu.lst" # be sure to override this if you have it somewhere else
+syslinuxmenu="/boot/syslinux/syslinux.cfg"

we should put a "you can override the following vars" that applies to both variables.


+  "Syslinux" "Use the Syslinux bootloader (ext2/3/4, btrfs, and vfat)" \

you can just use @{syslinux_supported_fs[@]} there


+interactive_bootloader_menu() {
+  # $1 - Bootloader Name
+  # $2 - Bootloader Configuration Files

why use capital letters for its arguments?
there's also a bunch of not very useful empty lines there.

+  if ! "$var_TARGET_DIR/usr/lib/syslinux/syslinux-install_update" -i -c /mnt;
  then
you should make sure the user can see the output of this command
something like: >$LOG 2>&1


some install_syslinux_mbr  calls have no || return 1.

in interactive_install_bootloader you should check the return code of
interactive_grub / interactive_syslinux and show the user a warning when it failed.
then we can probably remove the GRUB_OK variables also.

get_kernel_parameters should get an '|| return 1'
same thing for generate_syslinux_menu and generate_grub_menulst in
interactive_bootloader_menu()

other then this, I think it looks ok ;)

Dieter