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

Re: Gfxboot theme for booting



Juanje Ojeda Croissier wrote:
> Hi guys! :-)

Hi,

> We hope you find some of those useful and if you prefer any way we give
> to you our patches or features, just let us know.

sending them to the mailinglist is just fine, especially if you want to
discuss them. if you also mention where they can be fetched trough git,
that's nice as well and makes merging faster. for that one, i already
know that it's on github (i guess) from where i've got the last patch.

> Well, the last feature we needed and Roberto has implemented (trying to
> doing similar to the rest of the code and being as less intrusive we
> could) was a gfxboot support.

first of all, gfxboot is a bad idea since it makes you being locked into
an upstream-wise unsupported version of syslinux, and gfx patches tend
to be updated not very regularly, means you also don't profit from new
syslinux versions immediately.

i think that the debian approach by using vesamenu is saner. also note,
that there are experimental patches arround (which should be iirc in a
branch in upstreams syslinux.git by now) that reimplemented gfxboot as a
com32 module for syslinux, and they will eventually be merged into
mainline syslinux, which is nice.

and finally, gfxboot is not available in debian :)

but you probably already knew all of this anyway.

> We hope you like it and if you have any though, advise or comment we
> love to read it :-)

generally, i love to apply patches and to add stuff that make other
peoples live easier. however, i've three basic things why i'm reluctant
here. please read on.

first, i think that gfxboot is just an option to syslinux, so it should,
if at all, being incorporated into lh_binary_syslinux directly and
handled be there. imho, it should be handled the similar as the vesamenu
and therefore doesn't warrant a new helper (we have already a lot of
them :).

second, given that the /current/ gfxboot is more like a temporary hack
until gfxboot.c32 was mainlined, i think that the legacy gfxboot
handling would be better suited as a local-hook in binary stage
(lh_binary_local-hooks). once it's available as com32 module, it can be
very efficiently handled as alternative to vesamenu.c32.

third, by assigning LH_* variables to it, it becomes part of the
interface that frontends based on live-helper should implement. assumed
that gfxboot gets mainlined in weeks or months, this may be tedious to
add it and then revoke or change it again. also, i feel bad about having
'official' options in live-helper which are not actually
usable/resolvable at all within debian itself.

what do you think? other opinions are of course welcome.

> I attach the patch file.

some comments regarding coding style:

+lh_binary_enable_gfxboot ${*}

helpers names are generally short, so lh_binary_gfxboot would be better
(if it would be in an own helper).

+# Copyright (C) 2009 Roberto C. Morano <rcmorano@emergya.es>

for the moment, i'd like to keep copyright on all helpers consistently
assigned to me. is this a problem for you?

+if [ ! -z ${LH_GFXBOOT_THEME} ]

use the short test '-n' here (also on all places below this).

+GFXBOOT_TGZ="$GFXBOOT_THEME/bootlogo.tar.gz"

do use curly brackets for variables (also on all places below this).

+	Echo_error "gfxboot-theme-$GFXBOOT_THEME not installed"
+	exit 1

i think, the build should not fail here, but just give a warning that
although gfxboot was specified, it was not found and therefore ignored.
it would be consistend on how win32-loader is handled and imho sucks
less for the user :))

+	cp -a $GFXBOOT_LANG binary/isolinux/

why 'cp -a' here, isn't 'cp' enough?

rest looks good.

> Thanks for your time :-)

Thank *you*, you did all the work ;)

Regards,
Daniel

-- 
Address:        Daniel Baumann, Burgunderstrasse 3, CH-4562 Biberist
Email:          daniel.baumann@panthera-systems.net
Internet:       http://people.panthera-systems.net/~daniel-baumann/


Reply to: