[Buildroot] [PATCH] linux.mk: check for y explicitly

[Buildroot] [PATCH] linux.mk: check for y explicitly

[Thomas De Schampheleire]

LINUX_CONFIGURE_CMDS uses the construct
$(if $(BR2_x), ...
which don't work as expected when BR2_x=n.

This patch modifies linux.mk to use the construct
$(if $(filter y,$(BR2_x)), ...

See http://lists.busybox.net/pipermail/buildroot/2012-March/051523.html
for more information about a specific problem case.

Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com>

---
 linux/linux.mk |  8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

Note: the problematic construct seems to be present in many files in buildroot.
I'd say we need to fix them all, but I'm not sure if you agree. Feedback
welcome!

diff --git a/linux/linux.mk b/linux/linux.mk
--- a/linux/linux.mk
+++ b/linux/linux.mk
@@ -128,14 +128,14 @@ 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
 	rm $(KERNEL_ARCH_PATH)/configs/buildroot_defconfig
-	$(if $(BR2_ARM_EABI),
+	$(if $(filter y,$(BR2_ARM_EABI)),
 		$(call KCONFIG_ENABLE_OPT,CONFIG_AEABI,$(@D)/.config),
 		$(call KCONFIG_DISABLE_OPT,CONFIG_AEABI,$(@D)/.config))
 	# As the kernel gets compiled before root filesystems are
 	# built, we create a fake cpio file. It'll be
 	# replaced later by the real cpio archive, and the kernel will be
 	# rebuilt using the linux26-rebuild-with-initramfs target.
-	$(if $(BR2_TARGET_ROOTFS_INITRAMFS),
+	$(if $(filter y,$(BR2_TARGET_ROOTFS_INITRAMFS)),
 		touch $(BINARIES_DIR)/rootfs.cpio
 		$(call KCONFIG_ENABLE_OPT,CONFIG_BLK_DEV_INITRD,$(@D)/.config)
 		$(call KCONFIG_SET_OPT,CONFIG_INITRAMFS_SOURCE,\"$(BINARIES_DIR)/rootfs.cpio\",$(@D)/.config)
@@ -143,10 +143,10 @@ define LINUX_CONFIGURE_CMDS
 		$(call KCONFIG_SET_OPT,CONFIG_INITRAMFS_ROOT_GID,0,$(@D)/.config)
 		$(call KCONFIG_DISABLE_OPT,CONFIG_INITRAMFS_COMPRESSION_NONE,$(@D)/.config)
 		$(call KCONFIG_ENABLE_OPT,CONFIG_INITRAMFS_COMPRESSION_GZIP,$(@D)/.config))
-	$(if $(BR2_ROOTFS_DEVICE_CREATION_STATIC),,
+	$(if $(filter y,$(BR2_ROOTFS_DEVICE_CREATION_STATIC)),,
 		$(call KCONFIG_ENABLE_OPT,CONFIG_DEVTMPFS,$(@D)/.config)
 		$(call KCONFIG_ENABLE_OPT,CONFIG_DEVTMPFS_MOUNT,$(@D)/.config))
-	$(if $(BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV),
+	$(if $(filter y,$(BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV)),
 		$(call KCONFIG_SET_OPT,CONFIG_UEVENT_HELPER_PATH,\"/sbin/mdev\",$(@D)/.config))
 	yes '' | $(TARGET_MAKE_ENV) $(MAKE1) $(LINUX_MAKE_FLAGS) -C $(@D) oldconfig
 endef

[Buildroot] [PATCH] linux.mk: check for y explicitly

[Thomas Petazzoni]

Le Wed, 14 Mar 2012 17:36:49 +0100,
Thomas De Schampheleire <patrickdepinguin+buildroot@gmail.com> a ?crit :

> LINUX_CONFIGURE_CMDS uses the construct
> $(if $(BR2_x), ...
> which don't work as expected when BR2_x=n.

This is an invalid construct in my opinion. Unset options should look
like:

# BR2_SOMETHING is not set

in the .config file.

There are many other places in Buildroot which assumes that an
unselected option has an empty value:

xenomai/xenomai.mk:ifeq ($(XENOMAI_VERSION),)
xenomai/xenomai.mk:ifeq ($(BR2_PACKAGE_XENOMAI_SMP),y)
xenomai/xenomai.mk:ifeq ($(BR2_HAVE_DEVFILES),)
xenomai/xenomai.mk:ifeq ($(BR2_PACKAGE_XENOMAI_TESTSUITE),)
xenomai/xenomai.mk:ifeq ($(BR2_PACKAGE_XENOMAI_RTCAN),)
xenomai/xenomai.mk:ifeq ($(BR2_PACKAGE_XENOMAI_ANALOGY),)

connman/connman.mk:     $(if $(BR2_PACKAGE_CONNMAN_THREADS),--enable-threads,--disable-threads)         \
connman/connman.mk:     $(if $(BR2_PACKAGE_CONNMAN_DEBUG),--enable-debug,--disable-debug)               \
connman/connman.mk:     $(if $(BR2_PACKAGE_CONNMAN_ETHERNET),--enable-ethernet,--disable-ethernet)      \
connman/connman.mk:     $(if $(BR2_PACKAGE_CONNMAN_WIFI),--enable-wifi,--disable-wifi)                  \
connman/connman.mk:     $(if $(BR2_PACKAGE_CONNMAN_BLUETOOTH),--enable-bluetooth,--disable-bluetooth)   \
connman/connman.mk:     $(if $(BR2_PACKAGE_CONNMAN_LOOPBACK),--enable-loopback,--disable-loopback)      \
connman/connman.mk:     $(if $(BR2_PACKAGE_CONNMAN_NTPD),--enable-ntpd,--disable-ntpd)

And many, many, many more. Just grep for '(if' in the Buildroot code.
So definitely not my Ack on this patch.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[Buildroot] [PATCH] linux.mk: check for y explicitly

[Thomas De Schampheleire]

Hi Thomas, Arnout,

On Wed, Mar 14, 2012 at 8:21 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Le Wed, 14 Mar 2012 17:36:49 +0100,
> Thomas De Schampheleire <patrickdepinguin+buildroot@gmail.com> a ?crit :
>
>> LINUX_CONFIGURE_CMDS uses the construct
>> $(if $(BR2_x), ...
>> which don't work as expected when BR2_x=n.
>
> This is an invalid construct in my opinion. Unset options should look
> like:
>
> # BR2_SOMETHING is not set
>
> in the .config file.
>
> There are many other places in Buildroot which assumes that an
> unselected option has an empty value:
>
> xenomai/xenomai.mk:ifeq ($(XENOMAI_VERSION),)
> xenomai/xenomai.mk:ifeq ($(BR2_PACKAGE_XENOMAI_SMP),y)
> xenomai/xenomai.mk:ifeq ($(BR2_HAVE_DEVFILES),)
> xenomai/xenomai.mk:ifeq ($(BR2_PACKAGE_XENOMAI_TESTSUITE),)
> xenomai/xenomai.mk:ifeq ($(BR2_PACKAGE_XENOMAI_RTCAN),)
> xenomai/xenomai.mk:ifeq ($(BR2_PACKAGE_XENOMAI_ANALOGY),)
>
> connman/connman.mk: ? ? $(if $(BR2_PACKAGE_CONNMAN_THREADS),--enable-threads,--disable-threads) ? ? ? ? \
> connman/connman.mk: ? ? $(if $(BR2_PACKAGE_CONNMAN_DEBUG),--enable-debug,--disable-debug) ? ? ? ? ? ? ? \
> connman/connman.mk: ? ? $(if $(BR2_PACKAGE_CONNMAN_ETHERNET),--enable-ethernet,--disable-ethernet) ? ? ?\
> connman/connman.mk: ? ? $(if $(BR2_PACKAGE_CONNMAN_WIFI),--enable-wifi,--disable-wifi) ? ? ? ? ? ? ? ? ?\
> connman/connman.mk: ? ? $(if $(BR2_PACKAGE_CONNMAN_BLUETOOTH),--enable-bluetooth,--disable-bluetooth) ? \
> connman/connman.mk: ? ? $(if $(BR2_PACKAGE_CONNMAN_LOOPBACK),--enable-loopback,--disable-loopback) ? ? ?\
> connman/connman.mk: ? ? $(if $(BR2_PACKAGE_CONNMAN_NTPD),--enable-ntpd,--disable-ntpd)
>
> And many, many, many more. Just grep for '(if' in the Buildroot code.
> So definitely not my Ack on this patch.

I was giving this on the command-line, but as Arnout hinted I could
give BR2_x= instead.

Although I find this behavior counter-intuitive (if you can give =y
you should be able to give =n as well) I can understand that you don't
like such changes...

Best regards,
Thomas

[Buildroot] [PATCH] linux.mk: check for y explicitly

[Thomas Petazzoni]

Le Wed, 14 Mar 2012 20:30:58 +0100,
Thomas De Schampheleire <patrickdepinguin+buildroot@gmail.com> a ?crit :

> I was giving this on the command-line, but as Arnout hinted I could
> give BR2_x= instead.
> 
> Although I find this behavior counter-intuitive (if you can give =y
> you should be able to give =n as well) I can understand that you don't
> like such changes...

Well, kconfig is not done by us, so I think it's much better to stick
with what the kernel is doing (i.e, unselected options have an empty
value).

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com