Bug#757883: Patch for Loopback cfg support
I had a quick look over your patch. You're building it against
live-build 3.x, but since it's a feature improvement not a "critical
bug in core functionality", I believe I'm right in saying that you must
build against live-build 4.x, preferably the debian-next branch, if you
want to get it accepted (http://live.debian.net/project/lifespan/).
Some small issues I have with your binary_loopback_cfg script besides
being built against the wrong version:
- It would be nice to have a brief comment preceding the big if
structure beginning on line 165 to explain why you're branching based on
syslinux, so it's there for future people reading the code, saving them
a hunt through git commit logs or staring at the code trying to make
sense of it.
- Unless I've overlooked something, all the script is doing is
generating a grub config file, the template for which it retrieves from
live-build. It's not actually using or copying any grub binaries, unlike
the grub script. If that's correct, and I think it is, the dependency on
the grub-pc package is unnecessary and should be removed. You can strip
out the dependency itself in lines 48-49, along with the supporting code
in lines 51-55 and 298-302.
- Don't forget to update the copyright heading in your new script when
you rebase onto 4.x.
- I don't agree with the sequence you've inserted it in within the
binary script, I think that it should come after the bootloaders
(between syslinux and disk in 4.x). My primary reason being, mandatory
or otherwise, and although related to grub, loopback support is an
entirely separate component from the choice between grub, grub2 and
syslinux. The three lines of code executing the grub/grub2/syslinux
scripts (more in LB 3.x I believe) collectively represent the step of
implementing our chose (primary) bootloader in the overall sequence.
This should not be broken up unless absolutely necessary, otherwise
you're muddling things up.
On 07/12/2014 17:38, adrian15 wrote:
> Just in case it's easier for you to accept my patch you can also find
> it in this git repo commit based on live-build git repo:
>
> https://github.com/adrian15/live-build/commit/c6da5ff61bb46cb66b23fcb66daa83e23fc8a36b
>
>
> adrian15
Reply to: