diff mbox

[1/1] Adding extlinux to sysliux binaries copied into target/images.

Message ID 20140105122039.GA4201@zelow.no
State Rejected
Delegated to: Yann E. MORIN
Headers show

Commit Message

Thomas Lundquist Jan. 5, 2014, 12:20 p.m. UTC
Using the mail address I have on Github, which is the reason for the difference
in addresses.


Signed-off-by: Thomas Lundquist <thomasez@redpill-linpro.com>
---
 boot/syslinux/Config.in   | 4 ++++
 boot/syslinux/syslinux.mk | 7 +++++++
 2 files changed, 11 insertions(+)

Comments

Yann E. MORIN Jan. 5, 2014, 3:16 p.m. UTC | #1
Thomas, All,

On 2014-01-05 13:20 +0100, Thomas Lundquist spake thusly:

Subject should be something like:
    syslinux: add option to install extlinux

> Using the mail address I have on Github, which is the reason for the difference
> in addresses.

This should not be part of the commit log. If you want to add somthing
like that, do it after the three-dash line, below...

> Signed-off-by: Thomas Lundquist <thomasez@redpill-linpro.com>
> ---

...here. That '---' line above tells git to ignore it and all that
follows, as not being part of the commit message itself.

>  boot/syslinux/Config.in   | 4 ++++
>  boot/syslinux/syslinux.mk | 7 +++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/boot/syslinux/Config.in b/boot/syslinux/Config.in
> index 2c39e65..5773abb 100644
> --- a/boot/syslinux/Config.in
> +++ b/boot/syslinux/Config.in
> @@ -17,4 +17,8 @@ config BR2_TARGET_SYSLINUX_PXELINUX
>  	bool "Install pxelinux"
>  	default y
>  
> +config BR2_TARGET_SYSLINUX_EXTLINUX
> +	bool "Install extlinux"
> +	default y
> +
>  endif
> diff --git a/boot/syslinux/syslinux.mk b/boot/syslinux/syslinux.mk
> index eedc364..bfc2694 100644
> --- a/boot/syslinux/syslinux.mk
> +++ b/boot/syslinux/syslinux.mk
> @@ -26,10 +26,17 @@ endef
>  SYSLINUX_IMAGES-$(BR2_TARGET_SYSLINUX_ISOLINUX) += isolinux.bin
>  SYSLINUX_IMAGES-$(BR2_TARGET_SYSLINUX_PXELINUX) += pxelinux.bin
>  
> +ifeq ($(BR2_TARGET_SYSLINUX_EXTLINUX),y)
> +define SYSLINUX_INSTALL_EXTLINUX
> +	$(INSTALL) -D -m 0755 $(@D)/extlinux/extlinux $(BINARIES_DIR)/extlinux;
> +endef
> +endif
> +
>  define SYSLINUX_INSTALL_IMAGES_CMDS
>  	for i in $(SYSLINUX_IMAGES-y); do \
>  		$(INSTALL) -D -m 0755 $(@D)/core/$$i $(BINARIES_DIR)/$$i; \
>  	done
> +    $(SYSLINUX_INSTALL_EXTLINUX)

Use leading tabs, not spaces.

However, I'd rather we use a generic command, such as:

SYSLINUX_IMAGES-$(BR2_TARGET_SYSLINUX_ISOLINUX) += core/isolinux.bin
SYSLINUX_IMAGES-$(BR2_TARGET_SYSLINUX_PXELINUX) += core/pxelinux.bin
SYSLINUX_IMAGES-$(BR2_TARGET_SYSLINUX_EXTLINUX) += extlinux/extlinux

define SYSLINUX_INSTALL_IMAGES_CMDS
    for i in $(SYSLINUX_IMAGES-y); do \
        $(INSTALL) -D -m 0755 $(@D)/$$i $(BINARIES_DIR)/$${i##*/}; \
    done
endef

Regards,
Yann E. MORIN.
Yann E. MORIN Jan. 5, 2014, 5:19 p.m. UTC | #2
Thomas, All,

On 2014-01-05 13:20 +0100, Thomas Lundquist spake thusly:
[--SNIP--]
> diff --git a/boot/syslinux/Config.in b/boot/syslinux/Config.in
> index 2c39e65..5773abb 100644
> --- a/boot/syslinux/Config.in
> +++ b/boot/syslinux/Config.in
> @@ -17,4 +17,8 @@ config BR2_TARGET_SYSLINUX_PXELINUX
>  	bool "Install pxelinux"
>  	default y
>  
> +config BR2_TARGET_SYSLINUX_EXTLINUX
> +	bool "Install extlinux"
> +	default y

While you are working on syslinux, would you also consider adding the
MBR bootcode as an option, please?

Regards,
Yann E. MORIN.
Yann E. MORIN April 23, 2014, 9:02 p.m. UTC | #3
Thomas, All,

On 2014-01-05 13:20 +0100, Thomas Lundquist spake thusly:
> diff --git a/boot/syslinux/syslinux.mk b/boot/syslinux/syslinux.mk
> index eedc364..bfc2694 100644
> --- a/boot/syslinux/syslinux.mk
> +++ b/boot/syslinux/syslinux.mk
> @@ -26,10 +26,17 @@ endef
>  SYSLINUX_IMAGES-$(BR2_TARGET_SYSLINUX_ISOLINUX) += isolinux.bin
>  SYSLINUX_IMAGES-$(BR2_TARGET_SYSLINUX_PXELINUX) += pxelinux.bin
>  
> +ifeq ($(BR2_TARGET_SYSLINUX_EXTLINUX),y)
> +define SYSLINUX_INSTALL_EXTLINUX
> +	$(INSTALL) -D -m 0755 $(@D)/extlinux/extlinux $(BINARIES_DIR)/extlinux;
> +endef
> +endif

This does not make sense at all:

  - extlinux is an executable, not a raw image. As such, it does not
    belong to $(O)/images/ but to $(TARGET_DIR)/sbin/

  - extlinux is anyway built as a host compiler, not the target
    compiler. As such, it is a host tool, not fitted for the target

The second point is a blocking point: if the host is a 64-bit machine,
then extlinux is built as a 64-bit executable. If the target is a 32-bit
machine, then extlinux will not run on it. Conversely, if the host is a
32-bit, and the target a pure 64-bit, the extlinux won't run either.

Without taking into account that some build machines might not even be
an x86 at all (eg. we have PowerPC-based autobuilders), so this is even
more wrong (but this is already the case without your patch anyway.)

So this patch does not make sense in the current state.

Note however that I'm about to submitted a (pretty large) series to bump
and enhance syslinux. Keep an eye open for it in the short future. ;-)

Regards,
Yann E. MORIN.
diff mbox

Patch

diff --git a/boot/syslinux/Config.in b/boot/syslinux/Config.in
index 2c39e65..5773abb 100644
--- a/boot/syslinux/Config.in
+++ b/boot/syslinux/Config.in
@@ -17,4 +17,8 @@  config BR2_TARGET_SYSLINUX_PXELINUX
 	bool "Install pxelinux"
 	default y
 
+config BR2_TARGET_SYSLINUX_EXTLINUX
+	bool "Install extlinux"
+	default y
+
 endif
diff --git a/boot/syslinux/syslinux.mk b/boot/syslinux/syslinux.mk
index eedc364..bfc2694 100644
--- a/boot/syslinux/syslinux.mk
+++ b/boot/syslinux/syslinux.mk
@@ -26,10 +26,17 @@  endef
 SYSLINUX_IMAGES-$(BR2_TARGET_SYSLINUX_ISOLINUX) += isolinux.bin
 SYSLINUX_IMAGES-$(BR2_TARGET_SYSLINUX_PXELINUX) += pxelinux.bin
 
+ifeq ($(BR2_TARGET_SYSLINUX_EXTLINUX),y)
+define SYSLINUX_INSTALL_EXTLINUX
+	$(INSTALL) -D -m 0755 $(@D)/extlinux/extlinux $(BINARIES_DIR)/extlinux;
+endef
+endif
+
 define SYSLINUX_INSTALL_IMAGES_CMDS
 	for i in $(SYSLINUX_IMAGES-y); do \
 		$(INSTALL) -D -m 0755 $(@D)/core/$$i $(BINARIES_DIR)/$$i; \
 	done
+    $(SYSLINUX_INSTALL_EXTLINUX)
 endef