[Buildroot] [PATCH 1/1] new package - generate iso with isolinux bootloader
Arnout Vandecappelle
arnout at mind.be
Tue Aug 20 22:24:36 UTC 2013
On 16/08/13 11:22, jean wrote:
>
> Signed-off-by: jean <jean.sorgemoel at laposte.net>
Hi Jean,
Thanks for your patch. We will comment on it, and hopefully you can
send an updated patch to fix the issues we report. When you do, please
send the updated patch with the
--in-reply-to=1376644934-4302-1-git-send-email-jean.sorgemoel at laposte.net
and
-v 2 (or --subject-prefix='PATCH v2' for older git)
options, so that we can easily follow the updated patch in patchwork.
First a generic comment. Is there any reason not to simply replace the
iso9660 filesystem? That one is currently using grub as a bootloader, but
it doesn't work very well. So I think this patch is very valuable to get
the iso9660 filesystem working properly again. But as it is now, it is
too complex to be included in buildroot. See my comments below.
A second generic comment is about the choice of booting with an
initramfs. Why not boot with a (rockridge) iso9660 rootfs? Clearly it
puts a bit more strain on the kernel config since iso9660 as well as the
bus drivers (sata, usb) have to be linked in, but I think that would be a
much nicer solution. This type of image containing the actual rootfs in a
different format should really be generated by a post-image script
instead of a filesystem target. Can the rest of the list give their opinion?
> ---
> fs/Config.in | 1 +
> fs/isolinux/Config.in | 182 ++++++++++++++++++++++++++++++++++++++
> fs/isolinux/isolinux.mk | 226 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 409 insertions(+)
> create mode 100644 fs/isolinux/Config.in
> create mode 100644 fs/isolinux/isolinux.mk
>
> diff --git a/fs/Config.in b/fs/Config.in
> index da4c5ff..02294a9 100644
> --- a/fs/Config.in
> +++ b/fs/Config.in
> @@ -11,5 +11,6 @@ source "fs/romfs/Config.in"
> source "fs/squashfs/Config.in"
> source "fs/tar/Config.in"
> source "fs/ubifs/Config.in"
> +source "fs/isolinux/Config.in"
>
> endmenu
> diff --git a/fs/isolinux/Config.in b/fs/isolinux/Config.in
> new file mode 100644
> index 0000000..8abe409
> --- /dev/null
> +++ b/fs/isolinux/Config.in
> @@ -0,0 +1,182 @@
> +## Menu ISO image with syslinux
This comment is redundant: the line below says exactly that.
> +menu "iso image (isolinux bootloader - with initramfs)"
We normally don't have a menu in this case. The sub-options will be
hidden unless the ISOLINUX option is selected, and normally you only have
one filesystem so the total menu doesn't become too large.
In some cases we do add a menuconfig (which is a combination of a menu
and a boolean config), but I don't think it is warranted here.
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX
The name of the config symbols should correspond to the directory, i.e.
BR2_TARGET_ROOTFS_ISOLINUX (or BR2_TARGET_ROOTFS_ISO9660 if you do decide
to replace that one). And of course the suboptions should also be renamed
to BR2_TARGET_ROOTFS_ISOLINUX_FOO.
> + bool "iso image (isolinux bootloader)"
> + depends on (BR2_i386 || BR2_x86_64)
> + depends on BR2_LINUX_KERNEL
If you have a dependency on the kernel, you should also add a comment
at the file explaining that it is needed. Cfr. iso9660.
> + select BR2_TARGET_ROOTFS_INITRAMFS
> + select BR2_TARGET_SYSLINUX
> + select BR2_TARGET_SYSLINUX_ISOLINUX
> + help
> + Build a bootable iso9660 image (with isolinux bootloader and initramfs)
I think it would be nice if the bootable aspect would be optional. But
that can be done in a follow-up patch.
Help text should be indented with 1 tab + 2 spaces, and wrap at 70
characters (= 80 columns if you have 8-char tabs).
> + You can launch this iso with :
> + qemu -boot d -cdrom rootfs.isolinux
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_JOLIET
> + bool "create iso with Joliet format"
> + depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
Instead of defining a 'depends on' for every suboption, just wrap the
whole thing in a 'if ... endif' construct.
> + help
> + Create iso image with Joliet format (long file)
long filenames
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_ROCK_RIDGE
> + bool "Generate Rock Ridge directory information"
> + depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> + help
> + Generate Rock Ridge directory information
That help text isn't very useful... How about 'Generate Rock Ridge
directory information, which includes symbolic links, file mode, uid,
gid, etc.'
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_MENU_LINUX
> + string "Name to start linux"
All Config.in stuff (except help text) should be indented with 1 tab,
not with spaces.
> + depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> + default "buildroot"
> + help
> + define message see to select linux
I don't think generating the boot menu is a good idea. Instead, add an
option to let the user point to the menu file, like iso9660 does it. Or
actually, a list of files to include in the root dir.
There should be a default 'menu' that boots immediately. You can just
put this menu in fs/isolinux, and set the default for this option to that
path.
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_NAME
> + string "Name iso"
"Volume label" or "Volume ID"
And maybe the config symbol should be named the same.
> + depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> + default "buildroot iso"
I think the default label should be just "buildroot".
> + help
> + cdrom name
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_INPUT_CHARSET
> + string "define parameter input charset"
> + depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> + default "iso8859-15"
> + help
> + define parameter input charset
> + (see program genisoimage : genisoimage -input-charset help)
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_BOOT_MESSAGE
> + string "Message boot for isolinux"
I guess this one is part of the menu file so can be removed.
> + depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> + default "Buildroot isolinux boot"
> + help
> + define first message see in prompt boot
> + after that, you see all options
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_BOOT_TIMEOUT
> + int "timeout"
Ditto.
> + depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> + default "20"
> + help
> + define timeout (second)
> + after this time, boot on default config
> +
> +config BR2_TARGET_ISO_ISOLINUX_TOOLS_HARDWARE_INFO
> + bool "add tools hardware info"
Ditto.
> + depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> + help
> + isolinux menu, you can see hardware info
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_TOOLS_REBOOT
> + bool "add isolinux option reboot"
Ditto.
> + depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> + help
> + isolinux menu, you can reboot
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_TOOLS_POWEROFF
> + bool "add isolinux option poweroff"
Ditto.
> + depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> + help
> + isolinux menu, you can power off
> +
> +config BR2_TARGET_ROOTFS_ISO_ISOLINUX_TOOLS_FIRSTDISKBOOT
> + bool "add isolinux option to boot on first disk"
Ditto.
> + depends on BR2_TARGET_ROOTFS_ISO_ISOLINUX
> + help
> + isolinux menu, you can boot on disk
> +
> +## Keyboard menu
> +menu "keyboard"
This whole keyboard thing is way too complex. Buildroot wants to be
_simple_. The only relevant thing would be a space-separate list of
keyboard layouts to include in the image (cfr. the manual option, but
with the possibility to specify several keyboards). However, I think for
the time being it is best to only support a single keyboard (US).
[snip the 100 lines for the keyboard definition]
> diff --git a/fs/isolinux/isolinux.mk b/fs/isolinux/isolinux.mk
> new file mode 100644
> index 0000000..0d9464d
> --- /dev/null
> +++ b/fs/isolinux/isolinux.mk
> @@ -0,0 +1,226 @@
> +################################################################################
> +#
> +# Build the iso96600 with isolinux bootloader (and initramfs)
> +#
> +################################################################################
> +
> +BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_BOOT=boot
Variables defined in a .mk file should never start with BR2_. The BR2_
prefix indicates that it is a Kconfig variable. Also the rest of the
prefixes can be removed, so you would simply have ISOLINUX_ROOT_DIR_BOOT
Assignments in .mk files should have a space before and after the =
(like you do below).
This variable and the three below, however, are redundant. They are not
an abbreviation, and they are not variable. So they're not useful, and
they make the file harder to read.
> +BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_ISOLINUX=isolinux
> +BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_TOOLS=tools
> +BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_KEYBOARD=keyboard
> +
> +BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR = $(BUILD_DIR)/isolinux
This one could still be useful, since we have a similar variable for
the generic packages. However, it should then be called ISOLINUX_DIR.
> +BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_BOOT = $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/$(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_BOOT)
> +BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_ISOLINUX = $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/$(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_ISOLINUX)
> +BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_TOOLS = $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/$(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_TOOLS)
> +BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_KEYBOARD = $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/$(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_KEYBOARD)
These four are redundant again.
> +
> +BR2_TARGET_ISO_ISOLINUX_BOOT_MESSAGE := $(call qstrip,$(BR2_TARGET_ROOTFS_ISO_ISOLINUX_BOOT_MESSAGE))
> +BR2_TARGET_ISO_ISOLINUX_BOOT_TIMEOUT := $(call qstrip,$(BR2_TARGET_ROOTFS_ISO_ISOLINUX_BOOT_TIMEOUT))
Redundant.
> +
> +BR2_TARGET_ISO_ISOLINUX_KEYBOARD_US = $(KBD_BUILDDIR)data/keymaps/i386/qwerty/us.map
Redundant.
Also, refering to KBD_BUILDDIR is not very nice. As I said before, I
would leave the whole keyboard handling thing out for the first
submission, and maybe add it in a follow-up patch.
> +
> +BR2_TARGET_ISO_ISOLINUX_CONFIG = ""
> +BR2_TARGET_ISO_BR2_TARGET_ISO_ISOLINUX_BOOT_MSG = ""
> +
> +define copy_files_isolinux_image_tools
> + cp $(1) $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_TOOLS);
> +endef
Redundant
> +
> +define copy_files_isolinux_image_keyboard
> + $(SYSLINUX_BUILDDIR)utils/keytab-lilo \
> + $(BR2_TARGET_ISO_ISOLINUX_KEYBOARD_US) \
> + $1 \
> + > $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_KEYBOARD)/$(subst -,_,$(notdir $(basename $1))).ktl;
> +endef
> +
> +$(BINARIES_DIR)/rootfs.isolinux: host-cdrkit linux syslinux rootfs-initramfs
Dependency on linux is redundant, since it is implied by the initramfs.
You should probably collect the dependencies in a ISOLINUX_DEPENDENCIES
variable, for consistency with the generic infrastructure.
> + @$(call MESSAGE,"Generating root filesystem image rootfs.isolinux")
> + @mkdir -p $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)
> + @mkdir -p $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_BOOT)
> + @mkdir -p $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_ISOLINUX)
> + @mkdir -p $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_TOOLS)
> + @mkdir -p $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR_KEYBOARD)
> +
> + @echo -e $(BR2_TARGET_ISO_BR2_TARGET_ISO_ISOLINUX_BOOT_MSG) > $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/bootmsg.txt
> +
> + @cp $(BINARIES_DIR)/isolinux.bin $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/$(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_ISOLINUX)/
We use '$(INSTALL) -D' instead of cp, which also makes the mkdir's
above redundant. Remember, however, that the target file name has to be
given if the -D option is used.
> + @cp $(LINUX_IMAGE_PATH) $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/$(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_BOOT)/
> +
> + @echo -e $(BR2_TARGET_ISO_ISOLINUX_CONFIG) > $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)/isolinux.cfg
> +
> + @$(foreach file, $(BR2_TARGET_ISO_ISOLINUX_LIST_TOOLS), $(call copy_files_isolinux_image_tools, $(file) ) )
> + @$(foreach file, $(BR2_TARGET_ISO_ISOLINUX_LIST_KEYBOARD), $(call copy_files_isolinux_image_keyboard, $(file) ) )
> +
> + $(HOST_DIR)/usr/bin/genisoimage \
> + $(BR2_TARGET_ISO_ISOLINUX_GENISOIMAGE_OPTION) \
> + -o $(BINARIES_DIR)/rootfs.isolinux \
This should be $@
> + -b $(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_ISOLINUX)/isolinux.bin \
> + -no-emul-boot \
> + -c $(BR2_TARGET_ISO_ISOLINUX_ROOT_DIR_ISOLINUX)/boot.cat \
> + -boot-load-size 4 \
> + -boot-info-table \
> + -input-charset $(BR2_TARGET_ROOTFS_ISO_ISOLINUX_INPUT_CHARSET) \
> + -V $(BR2_TARGET_ROOTFS_ISO_ISOLINUX_NAME) \
> + $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)
> +
> + - at rm -rf $(BR2_TARGET_ISO_ISOLINUX_BUILDIMAGE_DIR)
> +
> +rootfs-isolinux: $(BINARIES_DIR)/rootfs.isolinux
I won't comment on the rest anymore since it will change too much after
my feedback.
I hope you're not scared off by the big change requests...
Regards,
Arnout
[snip]
--
Arnout Vandecappelle arnout at mind be
Senior Embedded Software Architect +32-16-286500
Essensium/Mind http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint: 7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
More information about the buildroot
mailing list