[Buildroot] [RFC/PATCH] fs/common.mk: Fix wrong double dollar usage

[Ezequiel Garcia <ezequiel.garcia@free-electrons.com>]

On Tue, Mar 12, 2013 at 12:28:15PM +0100, Thomas De Schampheleire wrote:
> Hi Ezequiel,
> 
> On Tue, Mar 12, 2013 at 11:06 AM, Ezequiel Garcia
> <ezequiel.garcia@free-electrons.com> wrote:
> > Hi Thomas,
> >
> > Thanks for your answer.
> >
> > On Tue, Mar 12, 2013 at 10:05:04AM +0100, Thomas De Schampheleire wrote:
> >> Hi,
> >>
> >> On Sat, Mar 9, 2013 at 10:04 PM, Ezequiel Garcia
> >> <ezequiel.garcia@free-electrons.com> wrote:
> >> > Double dollar sign is used to put an explicit dollar sign,
> >> > for instance, when writing a makefile rule.
> >> >
> >> > In this case, there are some makefile conditionals where
> >> > makefile variables are evaluated using double dollar signs
> >> > instead of single dollar, which is wrong.
> >> >
> >> > In particular, this fixes a buildroot 'make' stall
> >> > when building with an empty device table (empty BR2_ROOTFS_DEVICE_TABLE).
> >> >
> >> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> >> > ---
> >> > I'm sending this as RFC because I'm new to buildroot
> >> > and because I'm not a makefile wizard.
> >> > AFAIK, this fixes a real bug in my compilation,
> >> > as explained in the commit message.
> >>
> >> In fact you are reverting changes made by Arnout a while back, see git
> >> commit 847895d29524d81b64afb059b8649a77802a469b
> >> http://git.buildroot.org/buildroot/commit/?id=847895d29524d81b64afb059b8649a77802a469b
> >> There were specific reasons to make these changes, so I don't think we
> >> should revert them like this.
> >>
> >> Be aware that these make targets are inside a 'define
> >> ROOTFS_TARGET_INTERNAL' statement, and later in the file this
> >> 'function' is executed with 'call'. This makes the rules for $ or $$ a
> >> little different than in standard make recipes.
> >
> > Mmmm, I see. So it's not as easy as it seemed!
> > However, please note I'm *only* fixing the conditionals,
> > not the targets or the rules. See below.
> >
> >>
> >> You mention a 'stall', can you elaborate more? What happens exactly?
> >> Can you debug this further?
> >>
> >
> > Yes, I have. The problem is that this conditional
> >
> >   ifneq ($$(ROOTFS_DEVICE_TABLES),)
> >          cat $$(ROOTFS_DEVICE_TABLES) > $$(FULL_DEVICE_TABLE)
> >
> > is *never* false. However, if ROOTFS_DEVICE_TABLES is empty
> > the command will execute like this:
> >
> >   $ cat > some_file
> >
> > which simply stalls (well it doesn't stall, it's waiting for EOF at stdin).
> > I think this is easily reproducible, just set an empty value in your
> > BR2_ROOTFS_DEVICE_TABLE option.
> >
> > As I said in the patch I'm far from a makefile wizard, but it seems
> > to me that $$(ROOTFS_DEVICE_TABLES) (double dollar) is not equivalent
> > to $(ROOTFS_DEVICE_TABLES) (single dollar) from the perspective of the
> > conditional.
> 
> I can reproduce the problem you mention.
> The main problem is that the 'ifneq' never evaluates to false, as you
> mentioned, even if its contents are 'empty'. They are never really
> empty, because of this line:
> 
> ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE)) \
>         $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
> 
> Because it is a concatenation of two strings separated by spaces,
> there will always be a space in the final variable, which means it's
> not empty. We need to strip it.

You're right! (I actually verified this before sending the patch,
but I was doing something wrong because I missed the extra space).

> The following change fixes your problem, it runs the qstrip on the
> overal combination of the variables, causing the space to be removed
> if it's the only thing left.
> 
> diff --git a/fs/common.mk b/fs/common.mk
> --- a/fs/common.mk
> +++ b/fs/common.mk
> @@ -33,8 +33,8 @@
> 
>  FAKEROOT_SCRIPT = $(BUILD_DIR)/_fakeroot.fs
>  FULL_DEVICE_TABLE = $(BUILD_DIR)/_device_table.txt
> -ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE)) \
> -       $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
> +ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE) \
> +       $(BR2_ROOTFS_STATIC_DEVICE_TABLE))
> 
>  define ROOTFS_TARGET_INTERNAL
> 
> 

Indeed, this patch fixes my problem. Do you want me to prepare a patch?

Otherwise, if you plan to submit the patch yourself, feel free to add:
Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

Thanks,
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com