| Message ID | 1d22a1ff0f39a4b4e8fe4186cf9696a44a01e20f.1390744549.git.yann.morin.1998@free.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes: > From: "Yann E. MORIN" <yann.morin.1998@free.fr> > There is no reason to always depend on host-lzop, even when the kernel > compression is not LZO. > Since LZO is not the default compression option in the kernel (and there > is not sign that will change in the foreseeable future), it will always > appear in a condif file, whether it is a complete config file or it is > only a defconfig. > So, only depend on host-lzop if the LZO compression is enabled in the > kernel config file (either the defconfig or the custom config file). Nice, thanks - Just a few comments.. > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > --- > linux/linux.mk | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > diff --git a/linux/linux.mk b/linux/linux.mk > index ab25fe9..f34bea1 100644 > --- a/linux/linux.mk > +++ b/linux/linux.mk > @@ -38,7 +38,7 @@ endif > LINUX_PATCHES = $(call qstrip,$(BR2_LINUX_KERNEL_PATCH)) > LINUX_INSTALL_IMAGES = YES > -LINUX_DEPENDENCIES += host-kmod host-lzop > +LINUX_DEPENDENCIES += host-kmod > ifeq ($(BR2_LINUX_KERNEL_UBOOT_IMAGE),y) > LINUX_DEPENDENCIES += host-uboot-tools > @@ -163,6 +163,10 @@ else ifeq ($(BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG),y) > KERNEL_SOURCE_CONFIG = $(BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE) > endif > +ifeq ($(call KCONFIG_GET_OPT,CONFIG_KERNEL_LZO,$(call qstrip,$(KERNEL_SOURCE_CONFIG))),y) > +LINUX_DEPENDENCIES += host-lzop > +endif Another user of lzop seems to be the initramfs support, so we whould probably also check for RD_LZO (but that's auto enabled/prompt-less if !expert, so might not be in the .config, so perhaps we should just check for INITRAMFS_SOURCE): config RD_LZO bool "Support initial ramdisks compressed using LZO" if EXPERT default !EXPERT depends on BLK_DEV_INITRD select DECOMPRESS_LZO help Support loading of a LZO encoded initial ramdisk or cpio buffer If unsure, say N. Furthermore, this will probably give a pretty odd error message if people mistype the config file name. Not directly related to this change, but it would probably be a good thing if we would check and error out early if any of the needed config files aren't present (busybox/uclibc/uboot/kernel/..) with something like ifeq ($(wildcard $(BR2_PACKAGE_FOO_CONFIG)),) $(error Configuration file '$(BR2_PACKAGE_FOO_CONFIG)' not found. Check your BR2_PACKAGE_FOO_CONFIG settings)
On 26/01/14 14:56, Yann E. MORIN wrote: > From: "Yann E. MORIN" <yann.morin.1998@free.fr> > > There is no reason to always depend on host-lzop, even when the kernel > compression is not LZO. > > Since LZO is not the default compression option in the kernel (and there > is not sign that will change in the foreseeable future), it will always > appear in a condif file, whether it is a complete config file or it is > only a defconfig. > > So, only depend on host-lzop if the LZO compression is enabled in the > kernel config file (either the defconfig or the custom config file). > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > --- > linux/linux.mk | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/linux/linux.mk b/linux/linux.mk > index ab25fe9..f34bea1 100644 > --- a/linux/linux.mk > +++ b/linux/linux.mk > @@ -38,7 +38,7 @@ endif > LINUX_PATCHES = $(call qstrip,$(BR2_LINUX_KERNEL_PATCH)) > > LINUX_INSTALL_IMAGES = YES > -LINUX_DEPENDENCIES += host-kmod host-lzop > +LINUX_DEPENDENCIES += host-kmod > > ifeq ($(BR2_LINUX_KERNEL_UBOOT_IMAGE),y) > LINUX_DEPENDENCIES += host-uboot-tools > @@ -163,6 +163,10 @@ else ifeq ($(BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG),y) > KERNEL_SOURCE_CONFIG = $(BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE) > endif > > +ifeq ($(call KCONFIG_GET_OPT,CONFIG_KERNEL_LZO,$(call qstrip,$(KERNEL_SOURCE_CONFIG))),y) I would be nicer if the qstrip would move to the definition of KERNEL_SOURCE_CONFIG a few lines higher. It's already there in one condition, but not in the other. Regards, Arnout > +LINUX_DEPENDENCIES += host-lzop > +endif > + > define LINUX_CONFIGURE_CMDS > cp $(KERNEL_SOURCE_CONFIG) $(KERNEL_ARCH_PATH)/configs/buildroot_defconfig > $(TARGET_MAKE_ENV) $(MAKE1) $(LINUX_MAKE_FLAGS) -C $(@D) buildroot_defconfig >
Dear Yann E. MORIN, On Sun, 26 Jan 2014 14:56:27 +0100, Yann E. MORIN wrote: > +ifeq ($(call KCONFIG_GET_OPT,CONFIG_KERNEL_LZO,$(call qstrip,$(KERNEL_SOURCE_CONFIG))),y) > +LINUX_DEPENDENCIES += host-lzop > +endif Do this works reliably? KERNEL_SOURCE_CONFIG typically points to a minimal defconfig file (say arch/arm/configs/mvebu_defconfig). So if CONFIG_KERNEL_LZO is the default choice for the kernel, it will not appear in the defconfig, and therefore the piece of code above will not realize that we need host-lzop, because the test is done before "make mvebu_defconfig" is executed and turns it back into a full configuration file. Thomas
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: > Dear Yann E. MORIN, > On Sun, 26 Jan 2014 14:56:27 +0100, Yann E. MORIN wrote: >> +ifeq ($(call KCONFIG_GET_OPT,CONFIG_KERNEL_LZO,$(call qstrip,$(KERNEL_SOURCE_CONFIG))),y) >> +LINUX_DEPENDENCIES += host-lzop >> +endif > Do this works reliably? KERNEL_SOURCE_CONFIG typically points to a > minimal defconfig file (say arch/arm/configs/mvebu_defconfig). So if > CONFIG_KERNEL_LZO is the default choice for the kernel, it will not > appear in the defconfig, and therefore the piece of code above will not > realize that we need host-lzop, because the test is done before "make > mvebu_defconfig" is executed and turns it back into a full > configuration file. The commit message contained: Since LZO is not the default compression option in the kernel (and there is not sign that will change in the foreseeable future), it will always appear in a condif file, whether it is a complete config file or it is only a defconfig. So I believe the answer is yes, it will work.
Dear Peter Korsgaard, On Tue, 28 Jan 2014 23:17:32 +0100, Peter Korsgaard wrote: > > Do this works reliably? KERNEL_SOURCE_CONFIG typically points to a > > minimal defconfig file (say arch/arm/configs/mvebu_defconfig). So if > > CONFIG_KERNEL_LZO is the default choice for the kernel, it will not > > appear in the defconfig, and therefore the piece of code above will not > > realize that we need host-lzop, because the test is done before "make > > mvebu_defconfig" is executed and turns it back into a full > > configuration file. > > The commit message contained: > > Since LZO is not the default compression option in the kernel (and there > is not sign that will change in the foreseeable future), it will always > appear in a condif file, whether it is a complete config file or it is > only a defconfig. > > So I believe the answer is yes, it will work. Ok, thanks, I missed that. I believe it's a little bit fragile to rely on the assumption that it will not become the default, but since the breakage will be very clear if this ever changes, I think Yann's proposal is a good compromise. I was also annoyed by host-lzop always being built, even if useless. Seemed like Buildroot was turning into this other build system that builds gazillions of useless dependencies :) Thomas
Thomas, All, On 2014-01-28 23:14 +0100, Thomas Petazzoni spake thusly: > On Sun, 26 Jan 2014 14:56:27 +0100, Yann E. MORIN wrote: > > > +ifeq ($(call KCONFIG_GET_OPT,CONFIG_KERNEL_LZO,$(call qstrip,$(KERNEL_SOURCE_CONFIG))),y) > > +LINUX_DEPENDENCIES += host-lzop > > +endif > > Do this works reliably? KERNEL_SOURCE_CONFIG typically points to a > minimal defconfig file (say arch/arm/configs/mvebu_defconfig). So if > CONFIG_KERNEL_LZO is the default choice for the kernel, it will not > appear in the defconfig, and therefore the piece of code above will not > realize that we need host-lzop, because the test is done before "make > mvebu_defconfig" is executed and turns it back into a full > configuration file. I believe I've addresed your comments in the commit log itself: ---8<--- Since LZO is not the default compression option in the kernel (and there is not sign that will change in the foreseeable future), it will always appear in a condif file, whether it is a complete config file or it is only a defconfig. ---8<--- Of course, re-reading myself now, I can now spot a typo: s/condif/config/ Regards, Yann E. MORIN.
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: >> So I believe the answer is yes, it will work. > Ok, thanks, I missed that. I believe it's a little bit fragile to rely > on the assumption that it will not become the default, but since the > breakage will be very clear if this ever changes, I think Yann's > proposal is a good compromise. I was also annoyed by host-lzop always > being built, even if useless. Seemed like Buildroot was turning into > this other build system that builds gazillions of useless > dependencies :) Indeed. We do have a problem with the initramfs stuff though as I pointed out.
diff --git a/linux/linux.mk b/linux/linux.mk index ab25fe9..f34bea1 100644 --- a/linux/linux.mk +++ b/linux/linux.mk @@ -38,7 +38,7 @@ endif LINUX_PATCHES = $(call qstrip,$(BR2_LINUX_KERNEL_PATCH)) LINUX_INSTALL_IMAGES = YES -LINUX_DEPENDENCIES += host-kmod host-lzop +LINUX_DEPENDENCIES += host-kmod ifeq ($(BR2_LINUX_KERNEL_UBOOT_IMAGE),y) LINUX_DEPENDENCIES += host-uboot-tools @@ -163,6 +163,10 @@ else ifeq ($(BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG),y) KERNEL_SOURCE_CONFIG = $(BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE) endif +ifeq ($(call KCONFIG_GET_OPT,CONFIG_KERNEL_LZO,$(call qstrip,$(KERNEL_SOURCE_CONFIG))),y) +LINUX_DEPENDENCIES += host-lzop +endif + define LINUX_CONFIGURE_CMDS cp $(KERNEL_SOURCE_CONFIG) $(KERNEL_ARCH_PATH)/configs/buildroot_defconfig $(TARGET_MAKE_ENV) $(MAKE1) $(LINUX_MAKE_FLAGS) -C $(@D) buildroot_defconfig