diff mbox series

fs: allow filesystems to set the name of their output file

Message ID 20181024010149.3970-1-casantos@datacom.com.br
State Superseded, archived
Headers show
Series fs: allow filesystems to set the name of their output file | expand

Commit Message

Carlos Santos Oct. 24, 2018, 1:01 a.m. UTC 
Some filesystems may want to tweak their output names, rather than using
the fixed "rootfs.foo" scheme. Add a ROOTFS_FOO_IMAGE_NAME variable for
this purpose and document it.

Signed-off-by: Carlos Santos <casantos@datacom.com.br>
---
NOTE: https://patchwork.ozlabs.org/patch/927116/ must be upated to
document this.
---
 fs/common.mk | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Yann E. MORIN Oct. 24, 2018, 2:36 p.m. UTC | #1 
On 2018-10-23 22:01 -0300, Carlos Santos spake thusly:
> Some filesystems may want to tweak their output names, rather than using
> the fixed "rootfs.foo" scheme. Add a ROOTFS_FOO_IMAGE_NAME variable for
> this purpose and document it.
> 
> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
> ---
> NOTE: https://patchwork.ozlabs.org/patch/927116/ must be upated to
> document this.
> ---
>  fs/common.mk | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/common.mk b/fs/common.mk
> index 453da6010a..22ca56a1ff 100644
> --- a/fs/common.mk
> +++ b/fs/common.mk
> @@ -106,6 +106,7 @@ rootfs-common-show-depends:
>  # all variable references except the arguments must be $$-quoted.
>  define inner-rootfs
>  
> +ROOTFS_$(2)_IMAGE_NAME ?= rootfs.$(1)
>  ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1)
>  ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target
>  
> @@ -140,10 +141,10 @@ ROOTFS_$(2)_COMPRESS_EXT = .xz
>  ROOTFS_$(2)_COMPRESS_CMD = xz -9 -C crc32 -c
>  endif
>  
> -$$(BINARIES_DIR)/rootfs.$(1): ROOTFS=$(2)
> -$$(BINARIES_DIR)/rootfs.$(1): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
> -$$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
> -	@$$(call MESSAGE,"Generating root filesystem image rootfs.$(1)")
> +$$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME): ROOTFS=$(2)

This unfortunately does not work when the filesystem gets 'imaginative'
when setting that variable.

For example, I tweaked the ext2 fs in Buildroot to remove the current
hook, and replacing it with a conditionally set name:

    diff --git a/fs/ext2/ext2.mk b/fs/ext2/ext2.mk
    index 6bb4b1c7f8..923ccdca2f 100644
    --- a/fs/ext2/ext2.mk
    +++ b/fs/ext2/ext2.mk
    @@ -40,7 +40,13 @@ ifneq ($(BR2_TARGET_ROOTFS_EXT2_GEN),2)
     define ROOTFS_EXT2_SYMLINK
     	ln -sf rootfs.ext2$(ROOTFS_EXT2_COMPRESS_EXT) $(BINARIES_DIR)/rootfs.ext$(BR2_TARGET_ROOTFS_EXT2_GEN)$(ROOTFS_EXT2_COMPRESS_EXT)
     endef
    -ROOTFS_EXT2_POST_GEN_HOOKS += ROOTFS_EXT2_SYMLINK
    +#ROOTFS_EXT2_POST_GEN_HOOKS += ROOTFS_EXT2_SYMLINK
     endif
     
    +ROOTFS_EXT2_IMAGE_NAME = \
    +	$(if $(BR2_TARGET_ROOTFS_EXT2_2r0),rootfs.ext2) \
    +	$(if $(BR2_TARGET_ROOTFS_EXT2_2r1),rootfs.ext2) \
    +	$(if $(BR2_TARGET_ROOTFS_EXT2_3),rootfs.ext3) \
    +	$(if $(BR2_TARGET_ROOTFS_EXT2_4),rootfs.ext4)
    +
     $(eval $(rootfs))

And then, the variable gets a leading sapce, unfortunately.

So, you need to qstrip the variable before using it, probably going with
an intermediate variable (in the fs infrastructure):

    ROOTFS_$(2)_IMAGE_NAME ?= rootfs.$(1)
    ROOTFS_$(2)_FINAL_IMAGE_NAME = $$(call qstrip,$$(ROOTFS_$()_IMAGE_NAME))

and then use ROOTFS_$(2)_FINAL_IMAGE_NAME to generate the rules...

Regards,
Yann E. MORIN.

> +$$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
> +$$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME): $$(ROOTFS_$(2)_DEPENDENCIES)
> +	@$$(call MESSAGE,"Generating filesystem image $$(ROOTFS_$(2)_IMAGE_NAME)")
>  	rm -rf $$(ROOTFS_$(2)_DIR)
>  	mkdir -p $$(ROOTFS_$(2)_DIR)
>  	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
> @@ -164,7 +165,7 @@ endif
>  rootfs-$(1)-show-depends:
>  	@echo $$(ROOTFS_$(2)_DEPENDENCIES)
>  
> -rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1)
> +rootfs-$(1): $$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME)
>  
>  .PHONY: rootfs-$(1) rootfs-$(1)-show-depends
>  
> -- 
> 2.17.1
>
Carlos Santos Oct. 25, 2018, 12:23 a.m. UTC | #2 
Superseded by https://patchwork.ozlabs.org/patch/988843/
Arnout Vandecappelle Nov. 3, 2018, 10:37 a.m. UTC | #3 
On 24/10/18 16:36, Yann E. MORIN wrote:
> On 2018-10-23 22:01 -0300, Carlos Santos spake thusly:
>> Some filesystems may want to tweak their output names, rather than using
>> the fixed "rootfs.foo" scheme. Add a ROOTFS_FOO_IMAGE_NAME variable for
>> this purpose and document it.
>>
>> Signed-off-by: Carlos Santos <casantos@datacom.com.br>
>> ---
>> NOTE: https://patchwork.ozlabs.org/patch/927116/ must be upated to
>> document this.
>> ---
>>  fs/common.mk | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/common.mk b/fs/common.mk
>> index 453da6010a..22ca56a1ff 100644
>> --- a/fs/common.mk
>> +++ b/fs/common.mk
>> @@ -106,6 +106,7 @@ rootfs-common-show-depends:
>>  # all variable references except the arguments must be $$-quoted.
>>  define inner-rootfs
>>  
>> +ROOTFS_$(2)_IMAGE_NAME ?= rootfs.$(1)
>>  ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1)
>>  ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target
>>  
>> @@ -140,10 +141,10 @@ ROOTFS_$(2)_COMPRESS_EXT = .xz
>>  ROOTFS_$(2)_COMPRESS_CMD = xz -9 -C crc32 -c
>>  endif
>>  
>> -$$(BINARIES_DIR)/rootfs.$(1): ROOTFS=$(2)
>> -$$(BINARIES_DIR)/rootfs.$(1): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
>> -$$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
>> -	@$$(call MESSAGE,"Generating root filesystem image rootfs.$(1)")
>> +$$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME): ROOTFS=$(2)
> 
> This unfortunately does not work when the filesystem gets 'imaginative'
> when setting that variable.
> 
> For example, I tweaked the ext2 fs in Buildroot to remove the current
> hook, and replacing it with a conditionally set name:
> 
>     diff --git a/fs/ext2/ext2.mk b/fs/ext2/ext2.mk
>     index 6bb4b1c7f8..923ccdca2f 100644
>     --- a/fs/ext2/ext2.mk
>     +++ b/fs/ext2/ext2.mk
>     @@ -40,7 +40,13 @@ ifneq ($(BR2_TARGET_ROOTFS_EXT2_GEN),2)
>      define ROOTFS_EXT2_SYMLINK
>      	ln -sf rootfs.ext2$(ROOTFS_EXT2_COMPRESS_EXT) $(BINARIES_DIR)/rootfs.ext$(BR2_TARGET_ROOTFS_EXT2_GEN)$(ROOTFS_EXT2_COMPRESS_EXT)
>      endef
>     -ROOTFS_EXT2_POST_GEN_HOOKS += ROOTFS_EXT2_SYMLINK
>     +#ROOTFS_EXT2_POST_GEN_HOOKS += ROOTFS_EXT2_SYMLINK
>      endif
>      
>     +ROOTFS_EXT2_IMAGE_NAME = \
>     +	$(if $(BR2_TARGET_ROOTFS_EXT2_2r0),rootfs.ext2) \
>     +	$(if $(BR2_TARGET_ROOTFS_EXT2_2r1),rootfs.ext2) \
>     +	$(if $(BR2_TARGET_ROOTFS_EXT2_3),rootfs.ext3) \
>     +	$(if $(BR2_TARGET_ROOTFS_EXT2_4),rootfs.ext4)

 NACK that. You're defining ROOTFS_EXT2_IMAGE_NAME to contain spaces, so I think
that's a bug on your side, not on the side where we use the variable. The proper
definition of this variable would be:

ifeq ($(BR2_TARGET_ROOTFS_EXT2_2r0),y)
ROOTFS_EXT2_IMAGE_NAME = rootfs.ext2
else ifeq (....)
...

 Because this is a bit verbose, we often handle the "switch" in Config.in rather
than *.mk.

 You should know this stuff :-)

 Bottom line: NACK against the stripping, Carlos' original patch was fine.

 Regards,
 Arnout

>     +
>      $(eval $(rootfs))
> 
> And then, the variable gets a leading sapce, unfortunately.
> 
> So, you need to qstrip the variable before using it, probably going with
> an intermediate variable (in the fs infrastructure):
> 
>     ROOTFS_$(2)_IMAGE_NAME ?= rootfs.$(1)
>     ROOTFS_$(2)_FINAL_IMAGE_NAME = $$(call qstrip,$$(ROOTFS_$()_IMAGE_NAME))
> 
> and then use ROOTFS_$(2)_FINAL_IMAGE_NAME to generate the rules...
> 
> Regards,
> Yann E. MORIN.
> 
>> +$$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
>> +$$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME): $$(ROOTFS_$(2)_DEPENDENCIES)
>> +	@$$(call MESSAGE,"Generating filesystem image $$(ROOTFS_$(2)_IMAGE_NAME)")
>>  	rm -rf $$(ROOTFS_$(2)_DIR)
>>  	mkdir -p $$(ROOTFS_$(2)_DIR)
>>  	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
>> @@ -164,7 +165,7 @@ endif
>>  rootfs-$(1)-show-depends:
>>  	@echo $$(ROOTFS_$(2)_DEPENDENCIES)
>>  
>> -rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1)
>> +rootfs-$(1): $$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME)
>>  
>>  .PHONY: rootfs-$(1) rootfs-$(1)-show-depends
>>  
>> -- 
>> 2.17.1
>>
>
Yann E. MORIN Nov. 3, 2018, 1:34 p.m. UTC | #4 
Arnout, All,

On 2018-11-03 11:37 +0100, Arnout Vandecappelle spake thusly:
> On 24/10/18 16:36, Yann E. MORIN wrote:
[--SNIP--]
> >     +ROOTFS_EXT2_IMAGE_NAME = \
> >     +	$(if $(BR2_TARGET_ROOTFS_EXT2_2r0),rootfs.ext2) \
> >     +	$(if $(BR2_TARGET_ROOTFS_EXT2_2r1),rootfs.ext2) \
> >     +	$(if $(BR2_TARGET_ROOTFS_EXT2_3),rootfs.ext3) \
> >     +	$(if $(BR2_TARGET_ROOTFS_EXT2_4),rootfs.ext4)
> 
>  NACK that. You're defining ROOTFS_EXT2_IMAGE_NAME to contain spaces, so I think
> that's a bug on your side, not on the side where we use the variable.

So, why do we call qstrip on other user-provided options, like FOO_SITE,
FOO_SOURCE, FOO_LICENSE_FILES for example, or others that we simply
strip, like FOO_VERSION? Those are even less prone to contain unexpected
spaces to begin with, yet we decided to sanitise them based on some
feedback from users who would write things like (see 2f074857816):

    FOO_VERSION = something # some comment

Note: in fact, here, when I suggested we call qstrip, I was wrong; I
should have said plain strip instead. Mea culpa.

> The proper
> definition of this variable would be:
> 
> ifeq ($(BR2_TARGET_ROOTFS_EXT2_2r0),y)
> ROOTFS_EXT2_IMAGE_NAME = rootfs.ext2
> else ifeq (....)

We already use the $(if...) construct in many places, so I don't see
what is wrong with it, and I find it in fact more readable than the
alternative... See the huge one in util-linux for an extreme example
(which would be barely readbale should we switch to the ifeq-else-endif
format), or aircrack-ng for a more reasonable case.

We even already use it in fs/btrfs too, which was added recently-ish.

>  Because this is a bit verbose, we often handle the "switch" in Config.in rather
> than *.mk.

Sorry, but the above is pretty trivial, so yes it becomes easy to do in
kconfig. But under more complex situations, where the extension can be
computed, it might be easier to do in the .mk file.

>  You should know this stuff :-)

Well, yeah, I know it, and I even tested it on version 1 of Carlos
patch.

>  Bottom line: NACK against the stripping, Carlos' original patch was fine.

Meh, I was dragged in just because that touches the fs infra, but I
don't even care about this feature; I was even pretty oppposed to it to
begin with. This is even only a corner case that we won't even be
subjected to in Buildroot itself, so I don't care about the outcome...

My position with that would have been to suggest that, should a user
want to name their generated image differently, they should do as we
currently do in the ext filesystem: add a post-gen hook that creates
a (sym|hard)link to the name of their heart's content.

Regards,
Yann E. MORIN.

>  Regards,
>  Arnout
> 
> >     +
> >      $(eval $(rootfs))
> > 
> > And then, the variable gets a leading sapce, unfortunately.
> > 
> > So, you need to qstrip the variable before using it, probably going with
> > an intermediate variable (in the fs infrastructure):
> > 
> >     ROOTFS_$(2)_IMAGE_NAME ?= rootfs.$(1)
> >     ROOTFS_$(2)_FINAL_IMAGE_NAME = $$(call qstrip,$$(ROOTFS_$()_IMAGE_NAME))
> > 
> > and then use ROOTFS_$(2)_FINAL_IMAGE_NAME to generate the rules...
> > 
> > Regards,
> > Yann E. MORIN.
> > 
> >> +$$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
> >> +$$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME): $$(ROOTFS_$(2)_DEPENDENCIES)
> >> +	@$$(call MESSAGE,"Generating filesystem image $$(ROOTFS_$(2)_IMAGE_NAME)")
> >>  	rm -rf $$(ROOTFS_$(2)_DIR)
> >>  	mkdir -p $$(ROOTFS_$(2)_DIR)
> >>  	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
> >> @@ -164,7 +165,7 @@ endif
> >>  rootfs-$(1)-show-depends:
> >>  	@echo $$(ROOTFS_$(2)_DEPENDENCIES)
> >>  
> >> -rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1)
> >> +rootfs-$(1): $$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME)
> >>  
> >>  .PHONY: rootfs-$(1) rootfs-$(1)-show-depends
> >>  
> >> -- 
> >> 2.17.1
> >>
> >
diff mbox series

Patch

diff --git a/fs/common.mk b/fs/common.mk
index 453da6010a..22ca56a1ff 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -106,6 +106,7 @@  rootfs-common-show-depends:
 # all variable references except the arguments must be $$-quoted.
 define inner-rootfs
 
+ROOTFS_$(2)_IMAGE_NAME ?= rootfs.$(1)
 ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1)
 ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target
 
@@ -140,10 +141,10 @@  ROOTFS_$(2)_COMPRESS_EXT = .xz
 ROOTFS_$(2)_COMPRESS_CMD = xz -9 -C crc32 -c
 endif
 
-$$(BINARIES_DIR)/rootfs.$(1): ROOTFS=$(2)
-$$(BINARIES_DIR)/rootfs.$(1): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
-$$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
-	@$$(call MESSAGE,"Generating root filesystem image rootfs.$(1)")
+$$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME): ROOTFS=$(2)
+$$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
+$$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME): $$(ROOTFS_$(2)_DEPENDENCIES)
+	@$$(call MESSAGE,"Generating filesystem image $$(ROOTFS_$(2)_IMAGE_NAME)")
 	rm -rf $$(ROOTFS_$(2)_DIR)
 	mkdir -p $$(ROOTFS_$(2)_DIR)
 	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
@@ -164,7 +165,7 @@  endif
 rootfs-$(1)-show-depends:
 	@echo $$(ROOTFS_$(2)_DEPENDENCIES)
 
-rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1)
+rootfs-$(1): $$(BINARIES_DIR)/$$(ROOTFS_$(2)_IMAGE_NAME)
 
 .PHONY: rootfs-$(1) rootfs-$(1)-show-depends