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

login
register
mail settings
Submitter Ezequiel Garcia
Date March 9, 2013, 9:04 p.m.
Message ID <1362863052-8781-1-git-send-email-ezequiel.garcia@free-electrons.com>
Download mbox | patch
Permalink /patch/226377/
State Superseded
Headers show

Comments

Ezequiel Garcia - March 9, 2013, 9:04 p.m.
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.

 fs/common.mk |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
Ezequiel Garcia - March 12, 2013, 8:30 a.m.
On Sat, Mar 09, 2013 at 06:04:12PM -0300, Ezequiel Garcia 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.
> 
>  fs/common.mk |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 

Any comments? How does this look like?
Thomas De Schampheleire - March 12, 2013, 9:05 a.m.
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.

You mention a 'stall', can you elaborate more? What happens exactly?
Can you debug this further?

Best regards,
Thomas
Ezequiel Garcia - March 12, 2013, 10:06 a.m.
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.

Thanks,

Patch

diff --git a/fs/common.mk b/fs/common.mk
index 8b5b2f2..700dc00 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -47,9 +47,9 @@  $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
 	rm -f $$(FAKEROOT_SCRIPT)
 	rm -f $$(TARGET_DIR_WARNING_FILE)
 	echo "chown -R 0:0 $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
-ifneq ($$(ROOTFS_DEVICE_TABLES),)
+ifneq ($(ROOTFS_DEVICE_TABLES),)
 	cat $$(ROOTFS_DEVICE_TABLES) > $$(FULL_DEVICE_TABLE)
-ifeq ($$(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
+ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
 	printf '$$(subst $$(sep),\n,$$(PACKAGES_DEVICES_TABLE))' >> $$(FULL_DEVICE_TABLE)
 endif
 	printf '$$(subst $$(sep),\n,$$(PACKAGES_PERMISSIONS_TABLE))' >> $$(FULL_DEVICE_TABLE)
@@ -61,13 +61,13 @@  endif
 	cp support/misc/target-dir-warning.txt $$(TARGET_DIR_WARNING_FILE)
 	-@rm -f $$(FAKEROOT_SCRIPT) $$(FULL_DEVICE_TABLE)
 	$$(foreach hook,$$(ROOTFS_$(2)_POST_GEN_HOOKS),$$(call $$(hook))$$(sep))
-ifeq ($$(BR2_TARGET_ROOTFS_$(2)_GZIP),y)
+ifeq ($(BR2_TARGET_ROOTFS_$(2)_GZIP),y)
 	gzip -9 -c $$@ > $$@.gz
 endif
-ifeq ($$(BR2_TARGET_ROOTFS_$(2)_BZIP2),y)
+ifeq ($(BR2_TARGET_ROOTFS_$(2)_BZIP2),y)
 	bzip2 -9 -c $$@ > $$@.bz2
 endif
-ifeq ($$(BR2_TARGET_ROOTFS_$(2)_LZMA),y)
+ifeq ($(BR2_TARGET_ROOTFS_$(2)_LZMA),y)
 	$$(LZMA) -9 -c $$@ > $$@.lzma
 endif
 
@@ -76,7 +76,7 @@  rootfs-$(1)-show-depends:
 
 rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1) $$(ROOTFS_$(2)_POST_TARGETS)
 
-ifeq ($$(BR2_TARGET_ROOTFS_$(2)),y)
+ifeq ($(BR2_TARGET_ROOTFS_$(2)),y)
 TARGETS += rootfs-$(1)
 endif
 endef